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

feat: set certificate_authorities from trust store #4509

Merged
merged 7 commits into from
Apr 30, 2024

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Apr 18, 2024

Resolved issues:

Last chunk from #4502

Description of changes:

We need to parse every certificate in the trust store to collect a list of "Distinguished Names" (the "subject", like "C = US, ST = WA, L = Seattle, O = Amazon, OU = s2n, CN = s2nTestCert").

Call-outs:

  • I couldn't find a way to do this with openssl-1.0.2. If we think that's a blocker, I can go read through more very old documentation :(
  • I'm restricting the feature to client auth with custom trust stores with <10 CAs. I'm trying to avoid a misconfiguration leading to a very large extension, or an extension that "leaks" unintended information about the server's local environment.
  • I made the new API return "count". I'm not sure about this choice: it seems to improve usability to me, but it's also not strictly necessary.

Testing:

  • New unit tests
  • I generated the extension on Openssl and compare it byte-to-byte to what we generate
  • I tested that the extension causes Chrome to filter out certificates when prompting for a client auth certificate

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Apr 18, 2024
@lrstewart lrstewart marked this pull request as ready for review April 18, 2024 16:59
@lrstewart lrstewart requested review from goatgoose and jmayclin April 18, 2024 17:00
@lrstewart lrstewart requested a review from goatgoose April 18, 2024 22:49
tls/extensions/s2n_cert_authorities.c Show resolved Hide resolved
api/s2n.h Outdated Show resolved Hide resolved
tls/extensions/s2n_cert_authorities.h Outdated Show resolved Hide resolved
@lrstewart lrstewart requested review from goatgoose and jmayclin April 29, 2024 17:10
api/s2n.h Outdated Show resolved Hide resolved
tests/unit/s2n_cert_authorities_test.c Outdated Show resolved Hide resolved
Co-authored-by: Sam Clark <3758302+goatgoose@users.noreply.github.com>
@lrstewart lrstewart enabled auto-merge (squash) April 29, 2024 23:25
@lrstewart lrstewart merged commit d6baf1f into aws:main Apr 30, 2024
32 checks passed
@lrstewart lrstewart deleted the ca_ext_2 branch April 30, 2024 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants