-
Notifications
You must be signed in to change notification settings - Fork 121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add support and tests for OCSP_basic_sign #1742
Conversation
b20691d
to
a3c114e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1742 +/- ##
=========================================
+ Coverage 0 78.32% +78.32%
=========================================
Files 0 580 +580
Lines 0 97115 +97115
Branches 0 13923 +13923
=========================================
+ Hits 0 76068 +76068
- Misses 0 20425 +20425
- Partials 0 622 +622 ☔ View full report in Codecov by Sentry. |
return 0; | ||
} | ||
respid->type = V_OCSP_RESPID_KEY; | ||
respid->value.byKey = byKey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What frees byKey later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't free this, it's memory is assigned to respid
to maintain. The free above only occurs on failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the existing OCSP_RESPID already cleanup byKey
if it is set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that also goes for any structures allocated by ASN.1. If we clean up byKey
here, OCSP_RESPID
can't access the memory here for use later either and we'd also be doing a double free. I was under the assumption that ASAN would also catch any memory leakages occurring here?
I've added a test in ocsp_test.cc
at around L1099-1105 if we're keen on checking this.
.c_str())); | ||
ASSERT_TRUE(additional_cert); | ||
bssl::UniquePtr<EVP_PKEY> pkey(EVP_PKEY_new()); | ||
if (std::string(t.private_key) == "server_key") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the server_key RSA, and !server_key is ECDSA? Can private_key be renamed to key_type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mostly legacy-wise since the original first implementation only tested against rsa certs. We only added ecdsa certs later.
I'm open to changing it, but I think it's a much larger task than this PR. server_key
is server_cert
's private key and server_ecdsa_key
is server_ecdsa_cert
's private key. I'd like to change both server_key
and server_cert
, but there are a bit too many mentions of both files in the tests and the runbook.
- https://github.com/aws/aws-lc/blob/c9f6b6df1c2776cb4ab23f654741dcc28df12b1f/crypto/ocsp/ocsp_test.cc
- https://github.com/aws/aws-lc/blob/c9f6b6df1c2776cb4ab23f654741dcc28df12b1f/crypto/ocsp/test/aws/OCSP-TEST.md
Renaming private_key
to key_type
also won't make much sense until we make that change, should we punt this to a later PR when we focus on OCSP testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine waiting for now.
crypto/ocsp/ocsp_test.cc
Outdated
EXPECT_EQ(OCSP_basic_sign(basic_response.get(), signer_cert.get(), pkey.get(), | ||
t.dgst, additional_cert.get(), 0), | ||
t.expected_sign_status); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we test anything about the resulting signed response? Would this test still pass if OCSP_basic_sign did nothing and just return 0/1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an OCSP_basic_verify
check right after on success, it does look much nicer now.
I'm not sure what you mean by OCSP_basic_sign
doing nothing? It'll only return 1 on success/0 on failure and there are success/error cases in the parameterized tests to verify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant does this test actually test any of the behavior of the function or just the return code. As an extreme example this test would pass if the function looked like:
int OCSP_basic_sign(OCSP_BASICRESP *resp, X509 *signer, EVP_PKEY *key,
const EVP_MD *dgst, STACK_OF(X509) *certs,
unsigned long flags) {
return resp != nullptr;
}
Using OCSP_basic_verify ensures it was actually signed, or that both sign and verify are broken equally which is less likely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but there's a rather high criteria for OCSP_basic_sign
to actually return 1
on success considering all the checks and hurdles it has to go through.
This parameterized test also has coverage with success cases when signing with the right certs and multiple error cases when attempting to sign with certificate key mismatches: https://github.com/aws/aws-lc/pull/1742/files#diff-b21542ac76bf0a11ad1ad8fdd01d9eab463a9fa814e94b5e494233732a8db602R962-R975
Here's a link to the coverage report if we want to see which operations have been properly checked for this test.
I've added OCSP_basic_verify
so we could check that the signature can be verified, but still I don't think this test would easily pass even without the additional verification check.
EXPECT_EQ(basic_response.get()->tbsResponseData->responderId->type, | ||
V_OCSP_RESPID_KEY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides just the type can we check that the right value is here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a sha1 hash, which means we'd have to do another hash here to verify the contents are the same. We could think about doing it, it just seems like overkill to do the same hashing here as what our code is already doing..
Do let me know if I should go for it anyways.
a3c114e
to
a0efbc9
Compare
Follow up from #1742 (comment) By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.
Issues:
Addresses
CryptoAlg-2420
Description of changes:
OCSP_basic_sign
is mandatory for OCSP responders as the RFC specifies that all OCSP response SHALL be digitally signed. This is different fromOCSP_request_sign
since signing the OCSP request was optional.OCSP_basic_sign
isn't too complex however, the API does a basic ASN.1 sign along with setting some fields. The slight complications involved are with the additional flag customizations exposed by OpenSSL. Ruby also exposes these flags, so I've taken them in and documented them. I also took the chance to rephrase the documentation aroundOCSP_request_sign
.Call-outs:
N/A
Testing:
Reused the certs and keys used for testing
OCSP_request_sign
. What's different fromOCSP_request_sign
is that we create our ownOCSP_BASICRESP
instead of loading it from a file. Creating one from scratch makes more sense as all OCSP response files already have an existing signature.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.