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

WIP: master QUIC support #8797

Closed
wants to merge 32 commits into from
Closed

Conversation

tmshort
Copy link
Contributor

@tmshort tmshort commented Apr 19, 2019

Implement the BoringSSL QUIC-related APIs in OpenSSL.

Checklist
  • documentation is added or updated
  • tests are added or updated

ssl/ssl_locl.h Outdated Show resolved Hide resolved
ssl/ssl_quic.c Outdated Show resolved Hide resolved
@tmshort
Copy link
Contributor Author

tmshort commented Apr 19, 2019

@davidben

@mattcaswell
Copy link
Member

My understanding is that this is a new implementation that conforms to the same API. Right?

@tmshort
Copy link
Contributor Author

tmshort commented Apr 19, 2019

Yes, an implementation of the BoringSSL API. Due to code divergence, it's not a direct port.

@mattcaswell
Copy link
Member

When do we expect QUIC to go to RFC?

@mattcaswell
Copy link
Member

And are different QUIC draft implementations generally interoperable with each (or not at all like TLSv1.3)?

ssl/statem/statem_quic.c Outdated Show resolved Hide resolved
ssl/tls13_enc.c Outdated Show resolved Hide resolved
#include <openssl/ssl.h>

typedef struct ssl_quic_method_st SSL_QUIC_METHOD;
typedef enum ssl_encryption_level_t OSSL_ENCRYPTION_LEVEL;
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm enum in a public API we tend not to like that.

Copy link
Contributor Author

@tmshort tmshort Apr 19, 2019

Choose a reason for hiding this comment

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

Unfortunately, it's how BoringSSL does it. For source compatibility, I could change this to be a typedef int OSSL_ENCRYPTION_LEVEL and #define the enumerated values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually... I can't swap it as the QUIC stack may use enum ssl_encryption_t directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OSSL_HANDSHAKE_STATE is an enum. along with SSL_CT_VALIDATION_..., but yeah, this is an input type...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From a later comment:

There is already enum use in method signatures in ct.h, bio.h, ec.h, and x509_vfy.h.

(Enums are also defined in ssl.h and ui.h, but not used in method signatures.)

For API compatibility with BoringSSL, an enum is required.

@tmshort
Copy link
Contributor Author

tmshort commented Apr 19, 2019

When do we expect QUIC to go to RFC?

@kaduk ??

And are different QUIC draft implementations generally interoperable with each (or not at all like TLSv1.3)?

The significant difference between 18 and 19 dealt with version negotiation which was removed in 19. What little is said about draft negotiation is that multiple versions can be supported. Regardless, this interface is intended to make the TLSv1.3 handshake available for QUIC, and not to implement the QUIC protocol.

@richsalz
Copy link
Contributor

QUIC implementations are handled similarly to TLS 1.3 draft versions. No surprise since it's much of the same crew. ;) I would not guess when QUIC will be in WGLC, but a few months is reasonable.

@mattcaswell
Copy link
Member

Regardless, this interface is intended to make the TLSv1.3 handshake available for QUIC, and not to implement the QUIC protocol.

So what is the objective here? Are you seeking to make the Boring QUIC stack runnable on top of OpenSSL. Or is the plan to additionally import the protocol code?

@tmshort
Copy link
Contributor Author

tmshort commented Apr 19, 2019

So what is the objective here? Are you seeking to make the Boring QUIC stack runnable on top of OpenSSL. Or is the plan to additionally import the protocol code?

The objective is to allow QUIC stacks to use either BoringSSL or OpenSSL for QUIC's TLSv1.3 negotiation, without code change.
There is no intent (by me) to port QUIC into OpenSSL.

@richsalz
Copy link
Contributor

As a point of clarification, it's the Chromium QUIC stack. That currently requires boringSSL; this PR makes it possible to use Chromium QUIC with OpenSSL. There was a previous PR that did similar things around the 1.1.0 timeframe, but QUIC changed a lot and that PR was closed.

Admittedly, this code enables one particular QUIC stack. How QUIC uses TLS is documented in https://tools.ietf.org/html/draft-ietf-quic-tls-13 as an IETF standards-track document, however, so it's not unreasonable to assume that other QUIC implementations that want to use OpenSSL will need the same kind of thing. There is no guarantee of this, of course.

Tempting as it might be, OpenSSL should not be trying to implement QUIC as part of this project. In addition to the expertise questions, there is the simple matter of scheduling and resources -- the project is committed to FIPS and the timelines for that are already aggressive. Enabling one reference implementation seems to be the best way to get QUIC into the hands of the OpenSSL community.

@kaduk
Copy link
Contributor

kaduk commented Apr 22, 2019

When do we expect QUIC to go to RFC?

Sorry, this got lost in the deluge of mail while I was on vacation.
It looks like the current claimed timeline would have it published September at the earliest, with October more likely given that the RFC Production Center is expected to be going through a lot of churn before then. (I don't have great insight into the intra-WG timeline itself, though, at the moment.)

@t-j-h
Copy link
Member

t-j-h commented Jun 14, 2019

@tmshort has google officially released these fixes under the Apache License 2.0?
Although I see @richsalz comment on previous statements, we have nothing on file that I know of that states that google has made such a statement.

Until that is resolved I'm placing a -1 block on this commit so we don't proceed until we have that clearly sorted out.

@t-j-h t-j-h added the hold: need omc decision The OMC needs to make a decision label Jun 14, 2019
@tmshort
Copy link
Contributor Author

tmshort commented Jun 14, 2019

@t-j-h, the code is not the same as BoringSSL's, only the API is. It is an OpenSSL-specific implementation of BoringSSL APIs.

Per their LICENSE document, it's released under the ISC license.

ISC license used for completely new code in BoringSSL:

/* Copyright (c) 2015, Google Inc.
 *
 * Permission to use, copy, modify, and/or distribute this software for any
 * purpose with or without fee is hereby granted, provided that the above
 * copyright notice and this permission notice appear in all copies.
 *
 * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
 * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
 * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
 * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
 * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
 * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
 * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */

@t-j-h
Copy link
Member

t-j-h commented Jun 14, 2019

You have directly referenced boringssl commits in what you have here and it includes google written code that is not under Apache License 2.0. That needs to be resolved.

@tmshort
Copy link
Contributor Author

tmshort commented Jun 14, 2019

We could also get an opinion from @davidben

@t-j-h
Copy link
Member

t-j-h commented Jun 14, 2019

I think we are clear on the intent here and @davidben should comment - however we have to have things covered by the various CLA documents - and this is not covered at the moment as far as I can tell - unless you are claiming that there is zero code from boringssl actually included (i.e. this is not derived code) and it certainly doesn't look that way to a quick look through the two code bases. Note: I have not done a detailed analysis.

@tmshort
Copy link
Contributor Author

tmshort commented Jun 14, 2019

The BoringSSL commits are references to the commits of the APIs. Due to the divergence of OpenSSL and BoringSSL, other than the APIs and some simple accessors, these are not direct ports.

  • BoringSSL 3c034b2c is the QUIC transport params API.
  • BoringSSL c8e0f90f is the QUIC method API (SSL_CTX)
  • BoringSSL 3cbb0299 is the QUIC method API (SSL).
  • BoringSSL cc9d9352 is a change to the QUIC method API (add_handshake_data)
  • BoringSSL e6eef1ca is a the SSL_process_quic_post_handshake() API
  • BoringSSL 6f733791 are definitions of X25519 lengths (public API)
  • BoringSSL 384d0eaf is a change to the behavior of SSL_get_current_cipher()**
  • BoringSSL a0373182 is a change to the draft extension number
  • BoringSSL b1b76aee is SSL_cipher_get_prf_nid()

** This one is a small behavioral change. The function itself is quite small, and there's not really another way to do it.

@agl
Copy link
Contributor

agl commented Jun 14, 2019

Please consider the code in BoringSSL to be available to OpenSSL under the terms of the CLA, except for code in our third_party directory.

@tmshort
Copy link
Contributor Author

tmshort commented Jul 9, 2019

Based on CLA message on the openssl-project from @t-j-h, it seems to me that this falls under category 3:

  1. the contribution is non-trivial and the copyright is owned by someone other than the submitter and the copyright owner acknowledges that the submission is on their behalf - ICLA (and CCLA) from the copyright owner required.

What still needs to be done to move this forward (or at least remove the hold)?

@DaanDeMeyer
Copy link

Does this pr support sending 0-RTT data with QUIC? As far as I know, that hadn't been added to the BoringSSL API last time I checked. Might be worth looking into to at least make sure the current proposed API's don't impede adding 0-RTT support in the future.

@davidben
Copy link
Contributor

davidben commented Aug 2, 2019

(We're also likely to tweak our APIs soon to better support ESNI's padding needs.)

@tmshort
Copy link
Contributor Author

tmshort commented Aug 2, 2019

OpenSSL really can't be changing APIs after releases... so knowing what those changes are will be useful; I'd put it into here. However, this PR is still on hold, I'm still not clear what needs to be done (or if there's even anything I specifically need to do). @t-j-h

@t-j-h
Copy link
Member

t-j-h commented Aug 2, 2019

It remains on hold. There is an OMC discussion topic related to this PR.

@magsys
Copy link

magsys commented Oct 13, 2021

Thanks for the clarification about 3.1 and QUIC, I was wondering whether to start work implementing this PR, but it would seem better to wait for the long term supported OpenSSL version.

Angus

@tmshort
Copy link
Contributor Author

tmshort commented Oct 13, 2021

see also https://www.mail-archive.com/openssl-project@openssl.org/msg02585.html

That's what I was referring to.

@paulocoghi
Copy link

see also https://www.mail-archive.com/openssl-project@openssl.org/msg02585.html

Specific statement:

PR#8797 will not be merged and compatibility with the APIs proposed in that PR is a non-goal.

@jasnell
Copy link

jasnell commented Oct 13, 2021

That is an exceedingly irritating and unfortunate outcome that will cause quite a good amount of pain for folks downstream but... Ok, their decision to make.

@larseggert
Copy link

Wait, OpenSSL is implementing its own QUIC stack? 🙄

@martinduke
Copy link

martinduke commented Oct 13, 2021

I have a lot of trouble understanding how an independent QUIC implementation could be the consensus path forward:

  • QUIC is complicated and it will take a long time to deliver a fully functional implementation: implementing any QUIC/TLS API is a tiny subset of this work
  • Transports are subtle and optimizing performance is non-trivial
  • QUIC is designed to be extensible and the OMC is committing to implementing an endless series of protocol extensions (e.g., DATAGRAM) to support use cases, optimize performance, and remain relevant
  • The QUIC-TLS API in RFC9001 is much more likely to be stable than the QUIC application API, for which there is no standards effort and, indeed, great divergence already
  • Important openssl users with their own QUIC implementations will have to consider long-term movement away from this community

By what metric is a full protocol implementation good for the project?

@LPardue
Copy link

LPardue commented Oct 13, 2021

@martinduke raises many good points. I would be more blunt and say working on a transport implementation is a waste of good engineering time. Building a multistream transport where the application only supports one stream is similarly a waste of time.

If there's a strong desire to have an s_client application, then build that on top of one of the libraries that's already had years worth of accrued engineering value.

@richsalz
Copy link
Contributor

FYI:

@bagder
Copy link

bagder commented Oct 13, 2021

As someone with a client (curl) doing h3 and quic (using no less than two already existing and capable QUIC/h3 libraries) who'd die to get QUIC API support added to OpenSSL yesterday: I'm disappointed.

@larseggert
Copy link

Unfortunately here comes the bad news: QUIC's port UDP 443 is currently blocked by a lot of firewalls in many educational institutions and orgnaisations worldwide.

Sure. But that is overall less than 5% of Internet paths IIRC, and that percentage will be shrinking with QUIC delivering QoE improvements over TCP/TLS.

In any event, whether ports are or are not blocked is completely orthogonal to whether openssl should implement an API suitable for QUIC (yes, ASAP) or whether they should implement the entire QUIC transport (no, please).

@tmshort
Copy link
Contributor Author

tmshort commented Oct 14, 2021

As author of this PR, I'm obviously disappointed that it won't be merged. However, my concern is that, with the focus on QUIC, other RFC PRs I have (RFC7250 #16620, RFC8879/RFC7924/RFC8478bis #9155) will be ignored and/or delayed. They are complete features, but also big PRs.

@t-j-h t-j-h added the resolved: wont fix The issue has been confirmed but won't be fixed label Oct 20, 2021
@t-j-h t-j-h closed this Oct 20, 2021
@Atulin
Copy link

Atulin commented Oct 26, 2021

I guess with this PR being closed as wontfix, it's time to start spreading the word of the quictls/openssl fork

@t-j-h
Copy link
Member

t-j-h commented Oct 26, 2021

Read https://www.mail-archive.com/openssl-project@openssl.org/msg02585.html which contains the OMC Release Requirements and covers QUIC.

@wtarreau
Copy link

It's a huge disappointment. If the OMC didn't agree with the direction taken by this PR 2.5 years ago, at the very least it ought to have been stated by then so that the work could go into the desired direction.

Everyone suffers from forks and this decision will leave no option but force yet another one for every project providing an HTTP implementation (client or server or both). This will also include high-level languages that provide user-friendly HTTP-native frameworks. Projects hadn't yet recovered from the porting to 3.0.0 and they'll already have to think about switching away. What a loss, going nowhere after so many years! It's really sad :-(

Now at this point I guess it means that there is nothing the OpenSSL project can do to keep its users anymore :-( RIP will all due respect for the work done till now.

@kaduk
Copy link
Contributor

kaduk commented Nov 2, 2021

Read https://www.mail-archive.com/openssl-project@openssl.org/msg02585.html which contains the OMC Release Requirements and covers QUIC.

Hi @t-j-h , with all due respect, it really doesn't cover QUIC at a useful level of clarity.

For example,

The minimum viable product (MVP) for the next release is a pluggable record layer interface and a single stream QUIC client in the form of s_client that does not require significant API changes. In the MVP, interoperability should be prioritized over strict standards compliance.

The MVP will not contain a library API for an HTTP/3 implementation (it is a non-goal of the initial release). Our expectation is that other libraries will be able to use OpenSSL to build an HTTP/3 client on top of OpenSSL for the initial release.

This invites a comparison between a "QUIC client" and a "library API for an HTTP/3 implementation" (unspecified, so presumably both client and server). But that is changing on three axes at once -- QUIC is not HTTP/3, client-only is not full-featured client and server, and s_client support does not intrinsically imply a usable API interface (though to be fair, it does strongly imply one). Will the MVP be constrained in all three of these axes, or only some of them? If I can expect to use OpenSSL to build an HTTP/3 client, which of HTTP layer, QUIC congestion control, QUIC stream and flow control, and QUIC record layer do I need to supply? Presumably I do not need to supply the TLS 1.3 layer or the QUIC/TLS shims from RFC 9001, since otherwise I wouldn't need OpenSSL at all. Each of those is a significant chunk of work in its own right.

Once we have a fully functional QUIC implementation (in a subsequent release), it should be possible for external libraries to be able to use the pluggable record layer interface and it should offer a stable ABI (via a provider).

And here it's back to just QUIC, with no mention of HTTP/3, and maybe implying that the record layer will only be available in a subsequent release, which maybe is a partial answer to my earlier question ... and maybe isn't. (Having recently read all the relevant pre-RFCs in detail, I am comfortable saying that implementing HTTP/3 will be a very substantial additional amount of work over just implementing QUIC.)

I think the OMC should provide more clear guidance on what is or isn't expected on these separate axes (actually, I think the OMC should just delegate a lot more to the OTC, since there are inherently technical aspects to what is reasonable to support in a new release in a reasonable timeframe, but if the OMC is going to take charge, the current guidance does not provide sufficient clarity to be able to make plans against). To just point to the existing OMC statement and imply that it answers the major questions in this space is doing the community a great disservice.

I also have several technical and architectural concerns with the proposed layout of work, but this is not the right forum to go into them in.

@ekr
Copy link

ekr commented Nov 2, 2021

I generally concur with @martinduke's comments.

I think the place I would start is to ask what the advantage of having a QUIC implementation in OpenSSL is? There are already quite a few open source QUIC stacks, so even stipulating that the OpenSSL one will be excellent, will it be enough better than the alternatives to justify the amount of work, especially when compared to the value of adding other functions to OpenSSL that only make sense in an SSL/TLS stack and then could benefit both TLS/TCP and QUIC (ECH, HPKE, etc.)

@nponeccop
Copy link

nponeccop commented Nov 10, 2021

what the advantage of having a QUIC implementation in OpenSSL is?

There are a few obvious advantages, stemming from the fact that OpenSSL is an established and trusted cryptographic library:

  • OpenSSL is here to stay
  • OpenSSL already has a TLS implementation, is QUIC any different? Technically maybe yes, but logically it isn't.
  • OpenSSL is heavily audited, so flaws in OpenSSL are more likely to get fixed in time than a random guy or a random company's lib.
  • OpenSSL is (relatively) FOSS-friendly. Some people may refuse to use an implementation from "evil" guys.

It doesn't need to be in the main OpenSSL library. It may be a separate component sharing crypto-primitives with OpenSSL. At least the relevant cryptography should be in the OpenSSL core but my understanding that it is there already.

@wtarreau
Copy link

what the advantage of having a QUIC implementation in OpenSSL is?

There are a few obvious advantages, stemming from the fact that OpenSSL is an established and trusted cryptographic library:
(...)

Anyone who had to implement QUIC in their product will have a hard time agreeing with this due to the difficult integration into existing products. You need to have a very fast path from the socket to the QUIC entry point, and once you're there, the way you'll deal with your connections and streams is quite specific to your product, and there is no one-size-fits-all. It's not by accident that there are already so many QUIC implementations, some do consider that it will be more flexible for the long term to implement their own than to have to adapt their architecture to an existing one, and are not doing this by fun but by necessity.

Actually I would value much more a QUIC library offering some SSL services as a byproduct than an SSL library offering QUIC as a killer feature!

@wtarreau
Copy link

wtarreau commented May 5, 2023

Apparently the project is interested in knowing how we value its relation with the community and the transparency of the decisions taken, the form is here, I'm sure many of us have educated opinions on these matters (I filled mine, it's really less than 5 minutes, just be honest when responding, it should speak for itself):

https://docs.google.com/forms/d/e/1FAIpQLSe2ubPRCDyDNLXMozq_509oM8qnBRCCWdwY2zI9j2RcUCbQQw/viewform

@richsalz
Copy link
Contributor

richsalz commented May 5, 2023

Note that the feedback form does not give the option of getting an email copy of your responses unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch hold: need omc decision The OMC needs to make a decision resolved: wont fix The issue has been confirmed but won't be fixed triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet