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

Allow additionally parsing \r newlines in PEM files #30

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

complexspaces
Copy link
Contributor

@complexspaces complexspaces commented Nov 8, 2023

We (1Password) encountered some users on Linux who were encountering issues using custom certificate roots on their devices, which had been added to the "main" system CA bundle. The root cause of the issue turned out to be rustls-pemfile only supporting UNIX newline endings in parsed files (\n). The certificate bundles sent to us by users though contained one or more PEM objects with \r endings interspersed with ones using \n.

This PR proposes fixing the incompatibility described above by switching to a more lenient line parser function. BufReader::read_until was insufficient for this purpose because it only supports looking for a single needle. Instead, I ported the implementation of read_until out of the standard library and slightly tweaked it to look for more characters. While this uses the more low-level BufReader functions, its been well-tested already and has correct error handling. Thanks to @Ralith for helping point me in the right direction.

Additionally, I believe this is technically more correct based on RFC 7468 (assuming this is the right one 😓) as they call out support for multiple line endings:

Furthermore, parsers SHOULD ignore whitespace and other non-
base64 characters and MUST handle different newline conventions.

A full regression test is included as well. It can be confirmed by undoing the changes to src/pemfile.rs and running cargo test. It should panic with a bogus error. The data file consists of the same Amazon root CA repeated 4 times, but with 2 LF and 2 \r objects.

AFAICT, the only downside of this change is that it has a performance impact, even if almost imperceptible:

Version Performance
main 1,471 ns/iter
Slow custom read_until, no \r support 1,867 ns/iter (+/- 46)
This implementation 2,842 ns/iter (+/- 146)
memchr2 custom read_until, with \r support 1,491 ns/iter (+/- 12)
libc memchr custom read_until, with \r support 1,517 ns/iter (+/- 15)

A user would need to parse an absurd number of PEM objects to notice the difference, and the only no_std compatible way of regaining it is adding a dependency on the memchr crate, which does not seem to be worthwhile.

@ctz
Copy link
Member

ctz commented Nov 8, 2023

I was thinking this might be a regression from ff11ce1, but actually std's read_line is also obsessed with \n: https://doc.rust-lang.org/stable/src/std/io/mod.rs.html#2235-2240 🤨

@complexspaces
Copy link
Contributor Author

That was my conclusion as well. You have to go out of your way to support other line endings in most functions, with the exception of str::lines which supports CRLF as well.

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thank you! This is a really nice pull request. I appreciate the thorough description + testing methodology.

@djc djc merged commit 4cade3f into rustls:main Nov 9, 2023
8 checks passed
@complexspaces complexspaces deleted the parsing-other-line-endings branch November 9, 2023 18:27
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.

4 participants