The first thing any deployment will want to monitor is whether ECH was
actually used. Also it's useful if the command-line tool can output
this. (The alert is how the client signals it discarded the connection
due to ECH reject.)
This also disables ECH with the handoff mechanism for now. (The
immediate cause being that ech_accept isn't serialized.) We'll probably
need to make some decisions around the ordering here, since ECH affects
where the true ClientHello is available.
Bug: 275
Change-Id: Ie4559733290e653a514fcd94431090bf86bc3172
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47911
Reviewed-by: Adam Langley <agl@google.com>
The remaining remnants of Channel ID all configure the private key ahead
of time. Unwind the callback machinery, which cuts down on async points
and the cases we need to test.
This also unwinds some odd interaction between the callback and
SSL_set_tls_channel_id_enabled: If a client uses
SSL_set_tls_channel_id_enabled but doesn't set a callback, the handshake
would still pause at SSL_ERROR_WANT_CHANNEL_ID_LOOKUP. This is now
removed, so SSL_set_tls_channel_id_enabled only affects the server and
SSL_CTX_set1_tls_channel_id only affects the client.
Update-Note: SSL_CTX_set_channel_id_cb is removed.
SSL_set_tls_channel_id_enabled no longer enables Channel ID as a client,
only as a server.
Change-Id: I89ded99ca65e1c61b1bc4e009ca0bdca0b807359
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47907
Reviewed-by: Adam Langley <agl@google.com>
The channel_id_valid bit is both used for whether channel_id is filled
in (SSL_get_tls_channel_id), and whether this particular handshake will
eventually negotiate Channel ID.
The former means that, if SSL_get_tls_channel_id is called on the
client, we'll return all zeros. Apparently we never fill in channel_id
on the client at all. The latter means the state needs to be reset on
renegotiation because we do not currently forbid renegotiation with
Channel ID (we probably should...), which is the last use of the init
callback for extensions.
Instead, split this into a bit for the handshake and a bit for the
connection. Note this means we actually do not expose or even retain
whether Channel ID was used on the client.
This requires a tweak to the handoff logic, but it should be compatible.
The serialized ssl->s3->channel_id was always a no-op: the handback
happens before the ChannelID message, except in RSA key exchange. But we
forbid Channel ID in RSA key exchange anyway.
Update-Note: SSL_get_tls_channel_id will no longer return all zeros
during the handshake or on the client. I did not find any callers
relying on this.
Change-Id: Icd4b78dd3f311d1c7dfc1cae7d2b86dc7e327a99
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47906
Reviewed-by: Adam Langley <agl@google.com>
Also now that it's finalized, flip the default for
SSL_set_quic_use_legacy_codepoint.
Update-Note: QUIC APIs now default to the standard code point rather
than the draft one. QUICHE has already been calling
SSL_set_quic_use_legacy_codepoint, so this should not affect them. Once
callers implementing the draft versions cycle out, we can then drop
SSL_set_quic_use_legacy_codepoint altogether. I've also bumped
BORINGSSL_API_VERSION in case we end up needing an ifdef.
Change-Id: Id2cab66215f4ad4c1e31503d329c0febfdb4603e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47864
Reviewed-by: David Schinazi <dschinazi@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This is part of a very deep dependency chain. I'm sniffing at making all
the add_clienthello callbacks const. Between HelloVerifyRequest,
HelloRetryRequest, and soon ECH, we're creating lots of ClientHellos per
connection. That's probably easiest to manage if constructing a
ClientHello had no side effects.
Update-Note: The change to the return type isn't quite compatible, but I
only found one caller of this function, which has since been fixed. (If
we need to return a non-const value for compatibility, we can do that
and document that the caller should not mutate the output.)
Change-Id: I21f18f7438920a5b03d874fa548f054af3a42c4a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47664
Reviewed-by: Adam Langley <agl@google.com>
We misread (or maybe it changed?) the draft padding scheme. The current
text does not round the whole payload to a multiple of 32, just the
server name as a fallback. Switch the GREASE size selection to match.
Although, we may want to change the draft here. See also
https://github.com/tlswg/draft-ietf-tls-esni/issues/433
While I'm here, update some references from draft-09 to draft-10. Also
make the comment less verbose.
Bug: 275
Change-Id: I3c9f34159890bc3b7d71f6877f34b895bc7f9b17
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47644
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We didn't end up deploying this. We also never implemented the final
RFC, so what we do have isn't useful for someone who wishes to deploy
it anyway.
Update-Note: Token binding APIs are removed.
Change-Id: Iecea7c3dcf9d3e2644a3b7afaf61511310b45d5f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47584
Reviewed-by: Adam Langley <agl@google.com>
This aligns with OpenSSL. In particular, we clear not_resumable as soon
as the SSL_SESSION is complete, but it may not have an ID or ticket.
(Due to APIs like SSL_get_session, SSL_SESSION needs to act both as a
resumption handle and a bundle of connection properties.)
Along the way, use the modified function in a few internal checks which,
with the ssl_update_cache change, removes the last dependency within the
library on the placeholder SHA256 IDs.
Change-Id: Ic225109ff31ec63ec08625e9f61a20cf0d9dd648
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47447
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Callers using private key callbacks may retain non-trivial state with a
private key. In many cases, the private key is no longer necessary
immediately after the first round-trip (e.g. non-HRR TLS 1.3
connections). Add a function that callers can query to drop the state a
hair earlier.
This is tested in two ways. First, the asserts in front of using the
key, combined with existing tests, ensure we don't start reporting it
too early. Second, I've added tests in ssl_test.cc to assert we report
it as early as we expect to.
In doing so, the number of parameters on ConnectClientAndServer()
started getting tedious, so I've split that into a
CreateClientAndServer() and CompleteHandshakes(). Callers that need to
configure weird things or drive the handshake manually can call
CreateClientAndServer() (which takes care of the BIO pair business) and
continue from there.
Bug: b/183734559
Change-Id: I05e1edb6d269c8468ba7cde7dc90e0856694a0ca
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47344
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
See go/handshake-hints (internal).
CL originally by Bin Wu <wub@google.com>. I just reworked the tests and
tidied it up a bit. This is the start of a replacement for the split
handshakes API. For now, only TLS 1.3 is supported. It starts with an
initial set of hints, but we can add more later. (In particular, we
should probably apply the remote handshaker's extension order to avoid
needing to capability protect such changes.)
Change-Id: I7b6a6dfaa84c6c6e3436d2a4026c3652b8a79f0f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46535
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
See also 86a90dc749af91f8a7b8da6628c9ffca2bae3009 from upstream. This
differs from upstream's which treats {NULL, 2} as a valid way to spell
the empty list. (I think this is a mistake and have asked them about
it.)
Upstream's CL also, for them, newly makes the empty list disable ALPN,
when previously they'd disable it but misread it as a malloc failure.
For us, we'd already fixed the misreading due to our switch to
bssl::Array and bssl::Span, but the documentation was odd. This CL
preserves that behavior, but updates the documentation and writes a
test.
Update-Note: SSL_CTX_set_alpn_protos and SSL_set_alpn_protos will now
reject invalud inputs. Previously, they would accept them, but silently
send an invalid ALPN extension which the server would almost certainly
error on.
Change-Id: Id5830b2d8c3a5cee4712878fe92ee350c4914367
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46804
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This CL adds an initial implementation of the ECH server, with pieces of
the client in BoGo as necessary for testing. In particular, the server
supports ClientHelloInner compression with ech_outer_extensions. When
ECH decryption fails, it can send retry_configs back to the client.
This server passes the "ech-accept" and "ech-reject" test cases in
tls-interop-runner[0] when tested against both the cloudflare-go and nss
clients. For reproducibility, I started with the main branch at commit
707604c262d8bcf3e944ed1d5a675077304732ce and updated the endpoint's
script to pass the server's ECHConfig and private key to the boringssl
tool.
Follow-up CLs will update HPKE to the latest draft and catch us up to
draft-10.
[0]: https://github.com/xvzcf/tls-interop-runner
Bug: 275
Change-Id: I49be35af46d1fd5dd9c62252f07d0bae179381ab
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/45285
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This reverts commit a3437c09c7. There was
a miscommunication and it does not seem like we currently need this. If
that changes later, it's in Git and we can bring it back easily.
Change-Id: Ibbce29df2258a2d893d725ab3ee6fd78c5b6cb00
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46286
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
See draft-davidben-tls13-pkcs1-00. The code point is disabled by default
and must be configured in SSL_set_verify_algorithm_prefs and
SSL_set_signing_algorithm_prefs. It is also only defined for TLS 1.3
client certificates and otherwise ignored.
This required reworking the tests a bit since this is the first
signature algorithm that's disabled by default, and the first algorithm
that behaves differently between client and server.
Change-Id: Iac4aa96a4963cbc33688c252e958a572c5c3b511
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46187
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This aligns with OpenSSL's behavior. RFC7301 says servers should return
no_application_protocol if the client supported ALPN but no common
protocol was found. We currently interpret all values as
SSL_TLSEXT_ERR_NOACK. Instead, implement both modes and give guidance on
whne to use each. (NOACK is still useful because the callback may be
shared across multiple configurations, some of which don't support ALPN
at all. Those would want to return NOACK to ignore the list.)
To match upstream, I've also switched SSL_R_MISSING_ALPN, added for
QUIC, to SSL_R_NO_APPLICATION_PROTOCOL.
Update-Note: Callers that return SSL_TLSEXT_ERR_ALERT_FATAL from the
ALPN callback will change behavior. The old behavior may be restored by
returning SSL_TLSEXT_ERR_NOACK, though see the documentation for new
recommendations on return values.
Change-Id: Ib7917b5f8a098571bed764c79aa7a4ce0f728297
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/45504
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
IETF QUIC draft 33 is replacing the TLS extension
codepoint for QUIC transport parameters from 0xffa5
to 57. To support multiple versions of Chrome, we
need to support both codepoints in BoringSSL. This
CL adds support for the new codepoint in a way that
can be enabled on individual connections.
Note that when BoringSSL is not in QUIC mode, it
will error if it sees the new codepoint as a server
but it will ignore the legacy codepoint as that could
be a different private usage of that codepoint.
Change-Id: I314f8f0b169cedd96eeccc42b44153e97044388c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44704
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
IETF QUIC draft 33 is replacing the TLS extension
codepoint for QUIC transport parameters from 0xffa5
to 57. To support multiple versions of Chrome, we
need to support both codepoints in BoringSSL. This
CL adds support for the new codepoint in a way that
can be enabled on individual connections.
Change-Id: I3bf06ea0710702c0dc45bb3ff2e3d772e9f87f9b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44585
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
It's not even accurate. The term "master key" dates to SSL 2, which we
do not implement. (Starting SSL 3, "key" was replaced with "secret".)
The field stores, at various points, the TLS 1.2 master secret, the TLS
1.3 resumption master secret, and the TLS 1.3 resumption PSK. Simply
rename the field to 'secret', which is as descriptive of a name as we
can get at this point.
I've left SSL_SESSION_get_master_key alone for now, as it's there for
OpenSSL compatibility, as well as references to the various TLS secrets
since those refer to concepts in the spec. (When the dust settles a bit
on rfc8446bis, we can fix those.)
Change-Id: I3c1007eb7982788789cc5db851de8724c7f35baf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44144
Reviewed-by: Adam Langley <agl@google.com>
These APIs were used by Chromium to control the carve-out for the TLS
1.3 downgrade signal. As of
https://chromium-review.googlesource.com/c/chromium/src/+/2324170,
Chromium no longer uses them.
Update-Note: SSL_CTX_set_ignore_tls13_downgrade,
SSL_set_ignore_tls13_downgrade, and SSL_is_tls13_downgrade now do
nothing. Calls sites should be removed. (There are some copies of older
Chromium lying around, so I haven't removed the functions yet.) The
enforcement was already on by default, so this CL does not affect
callers that don't use those functions.
Change-Id: I016af8291cd92051472d239c4650602fe2a68f5b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44124
Reviewed-by: Adam Langley <agl@google.com>
QUICHE has a switch-case converting ssl_early_data_reason_t to a string
for logging. This causes a lot of churn when we add a new value.
Instead, add a function for this. Bump BORINGSSL_API_VERSION so we can
easily land a CL in QUICHE to start using the function without
coordinating repositories.
Change-Id: I176ca07b4f75a3ea7153a387219459665062aad9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/43724
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
(Original CL by svaldez, reworked by davidben.)
Change-Id: I8570808fa5e96a1c9e6e03c4877039a22e73254f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/42404
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This was introduced in OpenSSL 1.1.1, and wpa_supplicant expects us to
have it. We had this same function as SSL_CIPHER_get_value (to match
SSL_get_cipher_by_value). Align with upstream's name.
It seems we also had a ssl_cipher_get_value lying around, so fold them
together. (I've retained the assert in ssl_cipher_get_value as it seems
reasonable enough; casting a hypothetical SSLv2 cipher ID to uint16_t
would not behave correctly.)
Change-Id: Ifbec460435bbc483f2c3de988522e321f2708172
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/42966
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This version adds signature algorithms to the extension
Change-Id: I91dc78d33ee81cb7a6221c7bdeefc8ea460a2d6c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/42424
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Update-Note: If an SSL_QUIC_METHOD is set, connections will now fail if
ALPN is not negotiated. This new behavior can be detected by checking
if the value of BORINGSSL_API_VERSION is greater than 10.
Bug: 294
Change-Id: I42fb80aa09268e77cec4a51e49cdad79bd72fa58
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/42304
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reason codes 1000+N correspond to receiving an alert N from the peer,
rather than observing the corresponding error condition locally. This
has generally been a source of confusion for folks.
They were originally named like SSL_R_TLSV1_ALERT_DECRYPTION_FAILED, but
OpenSSL introduced a few without the "ALERT" token in
739a543ea863682f157e9aa0ee382367eb3d187c.
We then inadvertently carried the mistake over in
SSL_R_TLSV1_UNKNOWN_PSK_IDENTITY and SSL_R_TLSV1_CERTIFICATE_REQUIRED.
Fix all these to include the "ALERT" for consistency and make it
slightly less confusing. (Although perhaps it should have been
RECEIVED_ALERT or so.) Add compatibility #defines for the original
OpenSSL ones and SSL_R_TLSV1_CERTIFICATE_REQUIRED. The latter can be
removed when downstream code is fixed. The OpenSSL ones we'll probably
just leave around.
Update-Note: The renamed alerts will log slightly different strings, but
the constants used by external code are still there.
Bug: 366
Change-Id: I30c299c4ad4b2bed695bd71d0831fbe6755975a7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/42384
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
David Benjamin
Daniel McArdle
Yuchen Dai
David Schinazi
Adam Langley
Steven Valdez
Watson Ladd
Nick Harper