Skip to content

Additional X.509 Behavior Compatability Tests #2312

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

Merged
merged 2 commits into from
Apr 18, 2025

Conversation

skmcgrail
Copy link
Member

@skmcgrail skmcgrail commented Apr 3, 2025

  • CRL Distribution Points extension per RFC 5280 should be marked non-critical, but OpenSSL 1.1.1 does allow for it to be marked critical by a CA issuing a certificate. Thus we modify to allow for this condition and add an associated test-case.
  • Adds a test for a Root CA missing the basic constraints extension and a corresponding end-entity certificate signed by such a root.
  • Adds a test for a certificate containing a negative serial number. While this does not conform to RFC 5280, it is supported by OpenSSL 1.1.1 and many other implementations. This adds a test-case for this behavior which was not previously covered.

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.

@skmcgrail skmcgrail requested a review from a team as a code owner April 3, 2025 18:12
nid == NID_subject_alt_name ||
nid == NID_basic_constraints ||
nid == NID_certificate_policies ||
nid == NID_crl_distribution_points ||
Copy link
Member Author

@skmcgrail skmcgrail Apr 3, 2025

Choose a reason for hiding this comment

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

To help with this diff, NID_crl_distribution_points the item that was added with regards to being able to be set as critical :)

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.06%. Comparing base (fd8faa8) to head (7c9f002).

Additional details and impacted files
@@           Coverage Diff           @@
##             x509    #2312   +/-   ##
=======================================
  Coverage   79.05%   79.06%           
=======================================
  Files         614      614           
  Lines      106598   106611   +13     
  Branches    15083    15084    +1     
=======================================
+ Hits        84274    84288   +14     
- Misses      21673    21674    +1     
+ Partials      651      649    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

8c:02:20:21:22:ff:f5:9b:79:55:03:04:86:92:e1:c5:b2:11:
6d:7f:f8:77:23:e4:c0:09:53:c0:01:07:3d:f5:00:77:ec
*/
static char kValidEECertificateWithNegativeSerialNumber[] = R"(
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious how this file was generated, does OpenSSL support creating these type of certs via the cli?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is a bit more complicated as I wasn't sure how to do it with the OpenSSL CLI. What I did was generate a typical end-entity certificate using the CLI, then I used der2ascii to convert the certificate to ascii form. Extracted the tbsCertificate portion, modified the serial number, converted the tbsCertificate portion back to der, computed a new ECDSA signature using the over the uodate tbsCertificate bytes with the the OpenSSL CLI using the issuer key, then went back and formed the complete certificate with that modified tbsCertificate and new signature from the signing issuer.

02:20:3f:6a:d3:15:10:f6:38:fe:84:90:06:08:17:f7:cf:37:
b7:9a:a2:6e:b1:ba:38:ba:ca:0f:c0:52:06:10:a5:4c
*/
static char kInvalidEECertificateSignedByRootMissingBasicConstraintsExt[] = R"(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question about how this was generated here

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this can be generated via the CLI.

ROOT_SUBJECT="/C=US/ST=Washington/O=AWS Libcrypto/OU=Bad CA/CN=Pick a Name/"
ROOT_EXTENSIONS=(-addext "keyUsage=critical,keyCertSign,digitalSignature,cRLSign")
openssl req -new -key root.key -out root.csr -subj "${ROOT_SUBJECT}" -sha256 "${ROOT_EXTENSIONS[@]}"
faketime -m "2015-01-01 00:00:00 GMT" openssl x509 -req -in root.csr -days 31046 -key root.key -out root.pem -outform PEM -copy_extensions=copy

By not providing the basic constraints extension in these commands the self signed root woud not have the basic constraints extension. Where as also including -addext "basicConstraints=critical,CA:true" then it would satisfy it.

@torben-hansen torben-hansen self-requested a review April 17, 2025 19:32
@skmcgrail skmcgrail merged commit b92723f into aws:x509 Apr 18, 2025
103 of 107 checks passed
@skmcgrail skmcgrail deleted the rfc5280_additional_tests branch April 18, 2025 16:46
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.

None yet

4 participants