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

Open
wants to merge 22 commits into
base: master
from
Open

Conversation

@tmshort
Copy link
Contributor

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 Show resolved Hide resolved
@tmshort

This comment has been minimized.

Copy link
Contributor Author

tmshort commented Apr 19, 2019

@mattcaswell

This comment has been minimized.

Copy link
Member

mattcaswell commented Apr 19, 2019

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

@tmshort

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

mattcaswell commented Apr 19, 2019

When do we expect QUIC to go to RFC?

@mattcaswell

This comment has been minimized.

Copy link
Member

mattcaswell commented Apr 19, 2019

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;

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Apr 19, 2019

Member

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

This comment has been minimized.

Copy link
@tmshort

tmshort Apr 19, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@tmshort

tmshort Apr 19, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@tmshort

tmshort Apr 19, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@tmshort

tmshort Aug 29, 2019

Author Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

richsalz commented Apr 19, 2019

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

This comment has been minimized.

Copy link
Member

mattcaswell commented Apr 19, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

richsalz commented Apr 19, 2019

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

This comment has been minimized.

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.)

@tmshort tmshort force-pushed the akamai:master-quic-support branch from 7d0a27c to d07f338 Apr 26, 2019
@tmshort tmshort force-pushed the akamai:master-quic-support branch from c79aa70 to aa7a22a Jun 11, 2019
@t-j-h

This comment has been minimized.

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.

@tmshort

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

tmshort commented Jun 14, 2019

We could also get an opinion from @davidben

@t-j-h

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link

DaanDeMeyer commented Aug 2, 2019

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

t-j-h commented Aug 2, 2019

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

@richsalz

This comment has been minimized.

Copy link
Contributor

richsalz commented Nov 11, 2019

On August 2:

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

Has the discussion happened?

tmshort added 20 commits Apr 12, 2019
This adds a compatible API for BoringSSL's QUIC support, based
on the current |draft-ietf-quic-tls|.

Based on BoringSSL commit 3c034b2cf386b3131f75520705491871a2e0cafe
Based on BoringSSL commit c8e0f90f83b9ec38ea833deb86b5a41360b62b6a
Based on BoringSSL commit 3cbb0299a28a8bd0136257251a78b91a96c5eec8
Based on BoringSSL commit cc9d935256539af2d3b7f831abf57c0d685ffd81
Based on BoringSSL commit e6eef1ca16a022e476bbaedffef044597cfc8f4b
Based on BoringSSL commit 6f733791148cf8a076bf0e95498235aadbe5926d
Based on BoringSSL commit 384d0eaf1930af1ebc47eda751f0c78dfcba1c03
Based on BoringSSL commit a0373182eb5cc7b81d49f434596b473c7801c942
Based on BoringSSL commit b1b76aee3cb43ce11889403c5334283d951ebd37
Create quic_change_cipher_state() that does the minimal required
to generate the QUIC secrets. (e.g. encryption contexts are not
initialized).
@tmshort tmshort force-pushed the akamai:master-quic-support branch from 5aa62ce to 32edebd Nov 12, 2019
@tmshort

This comment has been minimized.

Copy link
Contributor Author

tmshort commented Nov 12, 2019

I rebased, and I'm trying to get the resumption secret calculation tested here.

@tmshort

This comment has been minimized.

Copy link
Contributor Author

tmshort commented Nov 13, 2019

The 1.1.1 QUIC branch (based on these changes) is:
https://github.com/akamai/openssl/tree/OpenSSL_1_1_1d-quic

@tmshort

This comment has been minimized.

Copy link
Contributor Author

tmshort commented Nov 15, 2019

On August 2:

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

Has the discussion happened?

Wondering the same. There was an OMC F2F recently. As an aside, the SCOTUS has taken up the Google v Oracle case.

@bagder

This comment has been minimized.

Copy link

bagder commented Nov 26, 2019

The TLS document in the QUIC wg is in the late stage process now, which should reduce the risk of it changing drastically going forward and thus making this PR even more valuable to get landed in a first take asap so that we can start hammering on it to smooth out all the quirks and have the functionality all shiny and neat by the time QUIC ships for real...

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Nov 26, 2019

The following slide from @bagder's talk http3-over-quic-all-is-new-but-still-the-same might serve as an additional stimulus for accelerating this pr ;-)

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.