Skip to content
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

Merged
merged 3 commits into from
Aug 19, 2024

Conversation

samuel40791765
Copy link
Contributor

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 from OCSP_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 around OCSP_request_sign.

Call-outs:

N/A

Testing:

Reused the certs and keys used for testing OCSP_request_sign. What's different from OCSP_request_sign is that we create our own OCSP_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.

@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 86.44068% with 16 lines in your changes missing coverage. Please review.

Project coverage is 78.32%. Comparing base (d6caa99) to head (af6fda0).
Report is 15 commits behind head on main.

Files Patch % Lines
crypto/ocsp/ocsp_server.c 75.38% 16 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

include/openssl/ocsp.h Outdated Show resolved Hide resolved
include/openssl/ocsp.h Outdated Show resolved Hide resolved
return 0;
}
respid->type = V_OCSP_RESPID_KEY;
respid->value.byKey = byKey;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What frees byKey later?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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") {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine waiting for now.

Comment on lines 1021 to 1023
EXPECT_EQ(OCSP_basic_sign(basic_response.get(), signer_cert.get(), pkey.get(),
t.dgst, additional_cert.get(), 0),
t.expected_sign_status);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

crypto/ocsp/ocsp_test.cc Show resolved Hide resolved
crypto/ocsp/ocsp_test.cc Show resolved Hide resolved
Comment on lines +1078 to +1079
EXPECT_EQ(basic_response.get()->tbsResponseData->responderId->type,
V_OCSP_RESPID_KEY);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

crypto/ocsp/ocsp_test.cc Show resolved Hide resolved
@skmcgrail skmcgrail self-requested a review August 16, 2024 17:00
@samuel40791765 samuel40791765 merged commit 8080ce3 into aws:main Aug 19, 2024
105 of 106 checks passed
@samuel40791765 samuel40791765 deleted the ocsp-basic-sign branch August 19, 2024 18:42
samuel40791765 added a commit that referenced this pull request Aug 20, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants