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

fix: pem parsing should allow single dashes in comments #4787

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Sep 20, 2024

Description of changes:

Currently, s2n-tls can't parse certificates with comments containing '-' characters. Words and names containing '-' aren't uncommon-- "s2n-tls" has a '-'.

As an easy fix, this PR updates the parser to look for "--" instead of "-". "--" should be less common, and this fixes the problem without requiring a major rewrite of the parser.

This results in two changes to certificate format requirements:

  1. Files that contain comments with '-' were not valid before but now are valid
  2. Files that use single dashes in pem delimiters (like a copy/paste error resulting in "-BEGIN CERTIFICATE-----") were valid before but are now skipped.

Alternatively, if we want to continue accepting "-BEGIN CERTIFICATE-", I could skip to "-BEGIN" instead of '-' or "--". That would skip any extra preceding dashes and read the BEGIN token. The downsides are:

  1. I can see a valid name in a comment being something like "WE-BEGIN". Dashes in names are just standard.
  2. Malformed tokens like "-----BEGAN CERTIFICORT----" were not valid before but would be ignored.
  3. More than 64 dashes before the "BEGIN" was not valid before but would be valid.
  4. More generally, it significantly changes our parsing approach, possibly creating other errors I've missed.

Testing:

I added some easy to update unit tests, rather than relying on test pem files. They're not exhaustive, but they cover the cases I changed and a couple other important cases.

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

stuffer/s2n_stuffer_pem.c Show resolved Hide resolved
stuffer/s2n_stuffer_pem.c Show resolved Hide resolved
Copy link
Contributor

@alexw91 alexw91 left a comment

Choose a reason for hiding this comment

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

This change looks good. The only other test that I would like to see is a more "real-world" test of parsing some real PEM comments found in the wild.

I think it'd be worthwhile to check in that PEM file under /tests/pems/centos-default-ca-bundle-2019.crt and verify that s2n's PEM parser correctly skips all 133 of those comments.

Once that is done we can also close out: #1136

@lrstewart
Copy link
Contributor Author

I think it'd be worthwhile to check in that PEM file under /tests/pems/centos-default-ca-bundle-2019.crt and verify that s2n's PEM parser correctly skips all 133 of those comments.

We actually already have a trust store checked in for testing: https://github.com/aws/s2n-tls/tree/main/tests/pems/trust-store It's from an Amazon Linux 2012 instance. Is there anything particularly interesting about centos, or did you just want us to test with any copied trust store?

@lrstewart lrstewart enabled auto-merge (squash) September 26, 2024 04:08
@lrstewart lrstewart merged commit ff61063 into aws:main Sep 26, 2024
37 checks passed
@lrstewart lrstewart deleted the pem_fix branch September 26, 2024 16:03
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.

4 participants