Working towards nginx support #12
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cpu
reviewed
Apr 23, 2024
rustls-libssl/src/entry.rs
Outdated
Comment on lines
1084
to
1086
| OPENSSL_NPN_NO_OVERLAP | ||
| } else { | ||
| 0 |
There was a problem hiding this comment.
This feels like it should be NPN_NO_OVERLAP in both cases 🤔
There was a problem hiding this comment.
I have changed these all to NPN_NO_OVERLAP, and also cleared out and out_len for cases we can't write something sensible.
Entertainingly OpenSSL's versions explodes in the error cases we're dealing with here:
==281210== Invalid read of size 1
==281210== at 0x48A1C8D: SSL_select_next_proto (in /usr/lib/x86_64-linux-gnu/libssl.so.3)
cpu
approved these changes
Apr 24, 2024
There was a problem hiding this comment.
Finished a pass through the rest of the commits. Nice work! A lot of the intermediate commits were stubs that were quick reviews.
I'm a little fuzzy on the ex_data portions and feel least confident in my review of that area, but I didn't see anything that gave me particular pause: just want to call that out as an area that could use more scrutiny in the future.
Rework ALPN parsing to use an iterator.
Future callbacks will be necessary from rustls trait implementations (like certificate verifier & session store callbacks). For this reason, this commit introduces an indirect storage style for obtaining the original *SSL object pointer. This uses thread-local storage.
(NPN was abandoned in 2011 and replaced with ALPN.)
`SSL_CTX_set_tlsext_servername_callback` is a define to `SSL_CTX_callback_ctrl`. `SSL_CTX_set_tlsext_servername_arg` is a define to `SSL_CTX_ctrl`.
But don't actually implement the limit. Justification: verifiers limiting the chain length is best accomplished by _issuer_ certs instead specifying a path length constraint, and/or not issuing valid but long chains. Theoretically we could implement this inside `webpki`, or even do it ourselves by supplying a `verify_path` callback to it.
We can definitely do these, but not for an MVP.
rustls currently could support these with stateless resumption.
Definitely could implement this with some conversions: it is the same concept as `ClientCertVerifier::root_hint_subjects()`
This one seems unlikely.
We could implement this.
This just returns NULL forever, because rustls does not do compression. That's the same behaviour as OpenSSL built with OPENSSL_NO_COMP.
Will be needed for verify callback support.
This provides: - `SSL_CTX_set_min_proto_version` - `SSL_CTX_set_max_proto_version` - `SSL_CTX_get_min_proto_version` - `SSL_CTX_get_max_proto_version` - `SSL_set_min_proto_version` - `SSL_set_max_proto_version` - `SSL_get_min_proto_version` - `SSL_get_max_proto_version` (All of those functions are routed into `SSL_CTX_ctrl` & `SSL_ctrl`.)
Later commits will implement this, and session resumption more generally.
This uses the system nginx (assumed to be available) to start a server, then grabs a small html file and a larger 5MB download with the system curl (using system openssl).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR demonstrates nginx (1.18.0-6ubuntu14.4) support working.
The testing is very light; there is notably zero coverage of TLS downstreams (ie, nginx acting as a TLS client). The ideal for testing would be to get the nginx test suite (at least suites with names starting
ssl) to pass. However, that seems a few steps away from here.Things this PR leaves undone: