Skip to content

Add an API for other QUIC stacks to use our TLS implementation #26683

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

Closed
wants to merge 8 commits into from

Conversation

mattcaswell
Copy link
Member

We provide some callbacks for third party QUIC stacks to use in order
to be able to reuse the OpenSSL TLS implementation in that stack. This is
essentially a thin wrapper around the same API that OpenSSL's own QUIC
stack uses in order to integrate TLS.

We provide some callbacks for third party QUIC stacks to use in order
to be able to reuse the OpenSSL TLS implementation in that stack. This is
essentially a thin wrapper around the same API that OpenSSL's own QUIC
stack uses in order to integrate TLS.
@mattcaswell mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer triaged: feature The issue/pr requests/adds a feature tests: present The PR has suitable tests present labels Feb 10, 2025
@github-actions github-actions bot added severity: fips change The pull request changes FIPS provider sources severity: ABI change This pull request contains ABI changes labels Feb 10, 2025
@mattcaswell mattcaswell added the style: waived exempted from style checks label Feb 10, 2025
@t-j-h t-j-h added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Feb 10, 2025
@richsalz
Copy link
Contributor

How close is this the API from #8797 ? How much work (effort, lines of code, any metric you choose) would it take? Do you think it is even possible?

@mattcaswell
Copy link
Member Author

I think some kind of compatibility shim should be possible - although its not a simple 1-2-1 mapping.

Looking at the callbacks in ssl_quic_method_st in #8797 they are not too far from similar callbacks in this PR (although there is no "flush" in this PR).

You would have to implement some kind of buffering to implement SSL_provide_quic_data because in this PR the TLS stack "pulls" data when it wants it, rather than having it "pushed" in.

You would have to track some state somewhere to implement the SSL_quic_read_level and SSL_quic_write_level functions - but that shouldn't be hard.

There seems to be a fairly clear mapping for the transport params stuff.

You would have a problem with the SSL_is_quic function because that already exists as a function in OpenSSL.

Not sure about the legacy codepoint/transport version stuff. That would need a closer look to figure out.

To track the state required you could do this by associating some ex_data to the SSL object.

@richsalz
Copy link
Contributor

Thanks for the detailed feedback. Just for the record, we have no plans to do this so others will have to pick it up.

@davidben
Copy link
Contributor

I haven't looked at this closely, but I can explain some background on how our API was meant to work.

although there is no "flush" in this PR

The reason for the "flush" step is that the TLS 1.3 ServerHello flight spans two different epochs. (Three if you jam half-RTT tickets in there.) A good QUIC implementation would want to pack those into one UDP packet, so we opted to output an explicit "flush" signal so that QUIC stacks don't have to guess about when to flush the packet out. (And if the QUIC stack didn't need the signal because they just implicitly flush after SSL_do_handshake returns, they can just ignore that signal.)

Depending on which direction you're doing the compatibility, you could certainly fake it by ignoring the flush call in one direction, or synthesizing extra flush calls in the other direction, though the latter might have performance impacts if the QUIC stack in question actually used the flush call.

Not sure about the legacy codepoint/transport version stuff. That would need a closer look to figure out.

Eh? I'm quite surprised to see that in #8797. That is an old artifact of IETF-QUIC's development. They used a different codepoint for transport parameters in some of the earlier drafts compared to the final version. BoringSSL had to have a toggle to help some early adopters that were following along the protocol's development. It doesn't make sense in a PR for OpenSSL.

OSSL_FUNC_SSL_QUIC_TLS_yield_secret_fn

There are two very subtle points that we learned in the process of working on all this.

First, you really should promise to callers what order the secrets will be released in. QUIC ACKs packets at the epoch they were received in (except for 0-RTT packets, which are ACKed at 1-RTT application data keys). We didn't ever want to be in a situation where we decrypted a packet but did not have the keys to generate an ACK. That seems a surefire way to enter some extremely untested codepath that might crash.

And so BoringSSL's API, if correctly implemented (I have not checked if quictls implements it correctly) guarantees that an encryption level's ACK-writing keys will always be released before the packet-decrypting keys. This means:

  • handshake write before handshake read
  • app data write before app data read
  • app data write before early data read

(Yes, that last one is surprising, but it actually matches the order in which servers process things.)

Second, there are some ordering requirements between handshake completion and when application data can be processed. This section of the RFC discusses this:
https://www.rfc-editor.org/rfc/rfc9001.html#name-receiving-out-of-order-prot

Because, again, we were trying to avoid exposing these subtle corner cases outside the library, BoringSSL's API, if correctly implemented (again, I don't know if quictls gets this right) makes sure not to release these keys until when the QUIC stack is allowed to use them.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@mattcaswell mattcaswell added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Feb 11, 2025
openssl-machine pushed a commit that referenced this pull request Feb 11, 2025
We provide some callbacks for third party QUIC stacks to use in order
to be able to reuse the OpenSSL TLS implementation in that stack. This is
essentially a thin wrapper around the same API that OpenSSL's own QUIC
stack uses in order to integrate TLS.

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #26683)
openssl-machine pushed a commit that referenced this pull request Feb 11, 2025
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #26683)
openssl-machine pushed a commit that referenced this pull request Feb 11, 2025
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #26683)
openssl-machine pushed a commit that referenced this pull request Feb 11, 2025
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #26683)
openssl-machine pushed a commit that referenced this pull request Feb 11, 2025
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #26683)
@mattcaswell
Copy link
Member Author

Pushed to master. This will be in the 3.5 release in April. There was a trivial conflict in CHANGES.md which I resolved during the merge.

@davidben
Copy link
Contributor

How does the OpenSSL API intend to handle the key release ordering subtleties I mentioned above? It seems undocumented in the merged PR. In my experience working with QUIC implementations here, these invariants are quite important to avoid getting them into bad states.

@mattcaswell
Copy link
Member Author

This means:

handshake write before handshake read
app data write before app data read
app data write before early data read

At the moment our QUIC stack does not support early data - so the final item does not (yet) come into play.

On the server side the keys are issued in this order:

Handshake write: Immediately after write of ServerHello
Handshake read: Immediately after above
Application write: Immediate after write of Server Finished
Application read: Immediately after read of Client Finished

On the client side the keys have this order:

Handshake read: Immediately after read of ServerHello
Handshake write: Immediately after above
Application read: Immediately after read of Server Finished
Application write: Immediately after write of Client Finished

So the server side already adheres to the order you suggest, but it is the other way around on the client side. There is probably not much opportunity for problems with the handshake read/write keys on the client side since they happen one after the other within the same SSL_do_handshake - although swapping the order here would be trivial.

We would have to defer the issue of the app read key until after the write of the client Finished to swap the order of the app data keys. I suppose the possible bug here is in the case of a "server writes first" scenario and we are able to decrypt the app data packets (and therefore want to ack them) before we have got as far as writing our Client Finished.

@mattcaswell
Copy link
Member Author

I created a task to investigate this further and look into whether we need to make a change.

@nibanks
Copy link

nibanks commented Feb 12, 2025

At the moment our QUIC stack does not support early data

Is there an ETA for this? This is a significant gap compared to quictls.

@bagder
Copy link

bagder commented Feb 12, 2025

I think some kind of compatibility shim should be possible - although its not a simple 1-2-1 mapping.

Out of curiosity: since you decided to provide a quite different API than what was once offered to you and what several QUIC stacks are already using. How did you come up with this API? Is this what QUIC stack implementers have asked for? Have you for example made ngtcp2 use this API to confirm that it is a convenient API to use when implementing QUIC stacks?

@t-j-h
Copy link
Member

t-j-h commented Feb 14, 2025

The API is layered on what we use internally to plug in our QUIC client and server implementations in a clean manner - as we originally indicated, we planned to get our implementations in place to ensure that the layers we had for interfacing into the TLS crypto handling were appropriate. You can see the overlap of the interface with other stacks - and you can also see a variety of interfaces across various implementations as well. We did get some feedback from other QUIC stacks - but the proof will be in actual usage which we expect will occur most likely after the 3.5 release is out.

@bagder
Copy link

bagder commented Feb 14, 2025

Thanks!

@t-j-h
Copy link
Member

t-j-h commented Feb 14, 2025

You're welcome - and feedback is very much welcome too on the interface and experiences in working with it.

If we encounter gaps, those gaps will be looked at - as our intent is for this interface to be usable and long term supportable.
We don't however dictate what the various other stacks do - they now simply have another option to consider - and one which is a stable API and will be long term supported.

OpenSSL-3.5 has a lot of stuff that is hitting the tree as we merge in the feature branches and there is a queue of features for subsequent releases. This interface was always planned - all we have done is bring it forward a release (it was planned for the release after the QUIC server code was merged).

@mattcaswell
Copy link
Member Author

At the moment our QUIC stack does not support early data

Is there an ETA for this? This is a significant gap compared to quictls.

It got added and will be in the 3.5 release.

definability pushed a commit to definability/openssl that referenced this pull request Feb 25, 2025
We provide some callbacks for third party QUIC stacks to use in order
to be able to reuse the OpenSSL TLS implementation in that stack. This is
essentially a thin wrapper around the same API that OpenSSL's own QUIC
stack uses in order to integrate TLS.

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#26683)
definability pushed a commit to definability/openssl that referenced this pull request Feb 25, 2025
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#26683)
definability pushed a commit to definability/openssl that referenced this pull request Feb 25, 2025
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#26683)
definability pushed a commit to definability/openssl that referenced this pull request Feb 25, 2025
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#26683)
definability pushed a commit to definability/openssl that referenced this pull request Feb 25, 2025
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#26683)
quarckster pushed a commit to quarckster/openssl-fork that referenced this pull request Feb 27, 2025
We provide some callbacks for third party QUIC stacks to use in order
to be able to reuse the OpenSSL TLS implementation in that stack. This is
essentially a thin wrapper around the same API that OpenSSL's own QUIC
stack uses in order to integrate TLS.

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#26683)
quarckster pushed a commit to quarckster/openssl-fork that referenced this pull request Feb 27, 2025
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#26683)
quarckster pushed a commit to quarckster/openssl-fork that referenced this pull request Feb 27, 2025
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#26683)
quarckster pushed a commit to quarckster/openssl-fork that referenced this pull request Feb 27, 2025
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#26683)
quarckster pushed a commit to quarckster/openssl-fork that referenced this pull request Feb 27, 2025
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#26683)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch severity: ABI change This pull request contains ABI changes severity: fips change The pull request changes FIPS provider sources style: waived exempted from style checks tests: present The PR has suitable tests present triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants