Allow passing extra extensions to the Client, to be added to the handshake #168
Conversation
|
(Oh, and I should probably add at least one test. But would like your feedback first.) |
|
I have a couple of problems with this, I'm afraid:
I still think I prefer a QUIC-specific trait which encompasses the whole interface between rustls and a QUIC transport :) |
|
Fair enough! I'll rework my patches in this direction and see where we end up. I don't quite see why the QUIC-specific thing would have to be trait rather than just a struct, but I'll iterate and we can see where we end up. Given that you want to encapsulate QUIC-specific TLS needs in rustls, I'm a little confused by your decision that you didn't want the Perhaps this also has to do with my confusion about the trait, though. |
djc
force-pushed
the
quic
branch
3 times, most recently
from
589d653 to
c0ab642
Compare
I don't think this is a good way to go. This exposes rustls to ongoing breaking changes in the QUIC spec, and likely represents a larger and less reusable body of code than merely exposing extension points as other TLS implementations do. The design intention of QUIC is that TLS be embedded within QUIC, not vis versa. |
|
Fair enough. I haven't been paying much attention to QUIC, and I'm happy to learn from the knowledge of someone who has :) I had originally thought that QUIC was already "done" and had been deployed for several years, but now I see that it's only just being standardised. So I'm reasonably happy with these changes, on the basis that:
|
|
I'm not quite sure how to check what needs to be triggered in bogo, and how it can be. From quickly searching the bogo code on GitHub, it seems that it's mostly limited to checking for the transport parameters extension in the form of the code (26) and that it's followed by a byte array. In my current thinking, I think it'd make more sense to remove the transport parameters-related code from rustls again, and instead use the "extra exts" mechanism with an |
|
It should also be easy (and otherwise necessary) for us as QUIC implementers to do the |
|
I disagree with that assessment. As @ctz has already stated, passing in random extensions is not really an activity that any API user should just do. It makes sense to have the usage of these tightly-coupled general mechanisms (passing in an extension during the handshake, extracting secrets) shielded by a specific API. Plus, I think it makes for these things to be reviewed as part of the TLS crate. In that sense, I think in terms of coupling it's sensible to have the QUIC TLS draft live in rustls (preffed off by default, if @ctz can live with that), exposing only something more like the high-level interface which is discussed in the QUIC TLS draft as API. |
|
I'm not opposed to limited-use APIs being masked off by default, but I still don't think it makes sense to stuff a bunch of QUIC details into an otherwise protocol-agnostic TLS library. In addition to the other issues I've highlighted, this could easily place us in a situation where a QUIC implementation can't be updated on crates.io for draft changes or future new major versions until a breaking(!) rustls change is published, which will presumably be rare in the future. This is particularly true when the QUIC-specific code seems to (almost?) entirely be built on top of operations already exposed by the public API, meaning it would be entirely possible for it to live in a different crate with its own release schedule. That crate could be Regarding the details of the QUIC-specific APIs, would it make sense to, instead of supplying raw keying material, provide e.g. |
|
I think this could now benefit from another review round. I've substantially reduced the amount of QUIC-specific code, leaving just the basic mechanisms to pass in transport parameters (as pre-encoded bytes) and extract them again. This is done by some thin wrappers around |
| for ext in handshake.extra_exts.iter() { | ||
| exts.push((*ext).clone()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Could this be exts.extend(handshake.extra_exts.iter().cloned()), or perhaps even exts.extend(extra_exts.drain(..))?
There was a problem hiding this comment.
This is a fine idea; another alternative is:
- for ext in handshake.extra_exts.iter() {
- exts.push((*ext).clone());
- }
+ let extra_exts = mem::replace(&mut handshake.extra_exts, vec![]);
+ exts.extend(extra_exts);This means we can drop the commit adding Clone too.
There was a problem hiding this comment.
I was under the impression the client might need to resend the transport parameters if it has to send another ClientHello in response to a HelloRetryRequest. If that's not the case, then it indeed makes sense to drain extra_exts and get rid of the added Clone implementations.
There was a problem hiding this comment.
By the way, I have adopted the exts.extend(handshake.extra_exts.iter().cloned() idiom.
ctz
left a comment
ctz
left a comment
There was a problem hiding this comment.
Looks good to me. Is this enough to get QUIC basically working?
src/msgs/handshake.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn get_params_extension(&self) -> Option<Vec<u8>> { |
There was a problem hiding this comment.
Let's call this get_quic_params_extension?
src/msgs/handshake.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn get_params_extension(&self) -> Option<Vec<u8>> { |
|
I detailed a list of requirements for baseline usability in an earlier comment. Of those, the main thing I don't see right now is a convenient way to extract the negotiated handshake digest and symmetric cipher of the session. |
|
As for "extract the negotiated handshake digest and symmetric ciper", I was under the impression the |
That API does, strictly, expose the needed information, but as previously discussed, requiring QUIC implementations to painstakingly match on the Another issue with wrapping the session objects in QUIC-specific types is that they cannot be converted into a We're also still missing #151, aren't we? |
The
With the #151 indeed isn't there yet, but in my opinion it shouldn't necessarily block this PR from moving forward, as it is not strictly necessary to get QUIC sessions off the ground. |
|
I've just solved the interop problem on my implementation's side, so I believe the PR that's there is enough to establish at least some kind of flowing data ("basically working"). |
Oh, sweet!
I'm not sure how that addresses the problem of not being able to create a boxed |
|
I'm not sure what exactly you need |
|
That's not a trait object. Trait objects allow you to use values of different types in monomorphic code by dynamic dispatch, and are useful when you specifically need type erasure. For example, my QUIC impl does not distinguish between client and server at the type level. This works great with OpenSSL and with rustls's current API because they expose agnostic |
|
@ctz what's your take? I can do more work on this, but I'd like your feedback before doing so. |
|
I'd really like to see the bogo tests On the subject of Perhaps, all under
|
|
I was actually thinking along similar lines, although organization-wise I thought we might do it as an extension trait that still lives in the |
| use webpki; | ||
|
|
||
| /// Generic methods for QUIC sessions | ||
| pub trait QuicExt { |
There was a problem hiding this comment.
If this was pub trait QuicExt: Session { ... }, then Box<QuicExt> would be useful. I don't think I need this, and it's not a paradigm that composes well if we end up with various other extension traits, but it's easy.
There was a problem hiding this comment.
Actually, if we instead had pub trait Session: QuicExt, the composability problem is solved and I think the visibility story remains the same! Might need some gymnastics to #[cfg] out gracefully (say, an indirection through another trait with a blanket impl, or make the extension trait always-present but cfg its members), but should be doable.
|
That looks like a good improvement to me! |
|
Okay, so like this? I find the inverted trait bounds a bit counter-intuitive, but it does seem to work. |
Ralith
left a comment
Ralith
left a comment
There was a problem hiding this comment.
That's about what I had in mind, yeah. I agree it's a little weird but it solves the general problem of composable session extension traits tidily.
src/lib.rs
Outdated
| } | ||
|
|
||
| #[cfg(not(feature = "quic"))] | ||
| trait QuicExt {} |
|
@ctz what do you think? Would appreciate it if you can take a look at the bogo stuff. |
|
I'll look at that now. |
- bogo_shim needs quic feature - provide/check quic transport params in bogo_shim - reject servers that handshake at TLS1.2, but include a quic transport params extension. - don't expose quic transport params extension for TLS1.2 clients. These last two match BoringSSL.
|
Thanks! |
|
@ctz would you mind rebasing your draft-28 branch on top of current master? That makes testing a bit more convenient for me. |
|
Thanks to you too. I've rebased that branch. |
4 participants
This is what I've been using with my QUIC implementation; seems to work.
As a next step, we could pull out the
TransportParameterscode again, and I could just pass in anUnknownExtension, but I wanted to run this by you first.The clone is a bit ugly, but I'm not sure there is a better way right now. I suppose there could be a second
ext_refsthat copies a ref from the locally-created extensions, which we could then add the externally-added extensions to. There is also the risk of code size bulking from the extra impls, not sure how concerning that is to you.