This was added in draft-11, which I'll update to more broadly in a
follow-up CL. This is an easily separable component: we don't want to
allow the DNS to arbitrarily insert strings in the ClientHello, so
invalid public names are rejected.
Unfortunately, we have a bit of a mess because DNS syntax does not
exclude IPv4 literals, yet everyone sticks DNS and IP literals in the
same string. The RFC3986 rules are alright, but don't match reality.
Reality is (probably?) the WHATWG rules, which are a mess.
The load-bearing bit of the spec is that, at certificate verification,
you should reject whatever strings your application refuses to represent
as a DNS name. I'll have Chromium call into its URL parser.
https://www.ietf.org/archive/id/draft-ietf-tls-esni-11.html#section-6.1.4.3-3
But there's still a bit at the validation step where clients "SHOULD"
run the IPv4 parser. In case downstream logic forgets, I've gone ahead
and implemented the WHATWG IPv4 parser.
https://www.ietf.org/archive/id/draft-ietf-tls-esni-11.html#section-4-6.6.1
Bug: 275
Change-Id: I15aa1ac0391df9c3859c44b8a259296e1907b7d4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48085
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
See also upstream's 9689a6aeed4ef7a2357cb95191b4313175440e4c.
X509_VERIFY_PARAM_ID made sense as a separate structure when
X509_VERIFY_PARAM was public, but now the struct is unexported.
Change-Id: I93bac64d33b76aa020fae07bba71b04f1505fdc4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48128
Reviewed-by: Adam Langley <agl@google.com>
This is a reland of 160a8891ae with go.mod
and go.sum fixed. This updates golang.org/x/crypto, adds the latest
golang.org/x/net as a direct dependency (it was previously an indirect
dependency via x/crypto), and cleans up stale entries from go.sum with
go mod tidy.
Original change's description:
> Add util/fetch_ech_config_list.go
>
> I wrote this tool to make it easier to test the ECH client against
> real-world servers with the bssl client tool. I found that manually
> extracting an ECHConfigList from a raw HTTPS record is unnecessarily
> painful.
>
> The tool queries DNS over UDP for HTTPS records. If it finds any HTTPS
> records in the response, it attempts to extract an ECHConfigList from
> the "ech" SvcParam. It can write each extracted ECHConfigList to a file
> in a given directory. Once the ECH client implementation lands, the bssl
> client tool should have a new flag that that takes the path to an
> ECHConfigList file.
>
> I am using golang.org/x/net/dns/dnsmessage to parse the DNS response. I
> recently added the |UnknownResource| type to this library to enable
> callers (like us) to extract the bytes of otherwise-unsupported records
> (like HTTPS). I updated the dependency with `go get -u golang.org/x/net`.
>
> Although the bssl client tool knows how to resolve the address of its
> "-connect" parameter, it is difficult to query HTTPS records in a
> platform-agnostic way. If we decide the bssl client should directly
> query HTTPS rather than leaning on fetch_ech_config_list.go, we should
> look into libresolv. Specifically, the |res_query| function enables the
> caller to query arbitrary record types. This may open its own can of
> cross-platform worms; macOS and Linux typically ship with different
> implementations and it is not available on Windows. For more info, see
> `man 3 resolver`.
>
> Bug: 275
> Change-Id: I705591658921f60a958164a18b68ffb697c2ea4b
> Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44104
> Reviewed-by: David Benjamin <davidben@google.com>
Bug: 275
Change-Id: I9571e96c7a2ad7e239d86a353929a4e556d71287
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48106
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This reverts commit 160a8891ae.
Reason for revert: This broke go.sum on CI for some reason. Will fix
and reland.
Original change's description:
> Add util/fetch_ech_config_list.go
>
> I wrote this tool to make it easier to test the ECH client against
> real-world servers with the bssl client tool. I found that manually
> extracting an ECHConfigList from a raw HTTPS record is unnecessarily
> painful.
>
> The tool queries DNS over UDP for HTTPS records. If it finds any HTTPS
> records in the response, it attempts to extract an ECHConfigList from
> the "ech" SvcParam. It can write each extracted ECHConfigList to a file
> in a given directory. Once the ECH client implementation lands, the bssl
> client tool should have a new flag that that takes the path to an
> ECHConfigList file.
>
> I am using golang.org/x/net/dns/dnsmessage to parse the DNS response. I
> recently added the |UnknownResource| type to this library to enable
> callers (like us) to extract the bytes of otherwise-unsupported records
> (like HTTPS). I updated the dependency with `go get -u golang.org/x/net`.
>
> Although the bssl client tool knows how to resolve the address of its
> "-connect" parameter, it is difficult to query HTTPS records in a
> platform-agnostic way. If we decide the bssl client should directly
> query HTTPS rather than leaning on fetch_ech_config_list.go, we should
> look into libresolv. Specifically, the |res_query| function enables the
> caller to query arbitrary record types. This may open its own can of
> cross-platform worms; macOS and Linux typically ship with different
> implementations and it is not available on Windows. For more info, see
> `man 3 resolver`.
>
> Bug: 275
> Change-Id: I705591658921f60a958164a18b68ffb697c2ea4b
> Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44104
> Reviewed-by: David Benjamin <davidben@google.com>
TBR=davidben@google.com,dmcardle@google.com
Change-Id: Iec36265dfa3b7c59eb811ed708219bfebb07a710
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 275
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48105
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
I wrote this tool to make it easier to test the ECH client against
real-world servers with the bssl client tool. I found that manually
extracting an ECHConfigList from a raw HTTPS record is unnecessarily
painful.
The tool queries DNS over UDP for HTTPS records. If it finds any HTTPS
records in the response, it attempts to extract an ECHConfigList from
the "ech" SvcParam. It can write each extracted ECHConfigList to a file
in a given directory. Once the ECH client implementation lands, the bssl
client tool should have a new flag that that takes the path to an
ECHConfigList file.
I am using golang.org/x/net/dns/dnsmessage to parse the DNS response. I
recently added the |UnknownResource| type to this library to enable
callers (like us) to extract the bytes of otherwise-unsupported records
(like HTTPS). I updated the dependency with `go get -u golang.org/x/net`.
Although the bssl client tool knows how to resolve the address of its
"-connect" parameter, it is difficult to query HTTPS records in a
platform-agnostic way. If we decide the bssl client should directly
query HTTPS rather than leaning on fetch_ech_config_list.go, we should
look into libresolv. Specifically, the |res_query| function enables the
caller to query arbitrary record types. This may open its own can of
cross-platform worms; macOS and Linux typically ship with different
implementations and it is not available on Windows. For more info, see
`man 3 resolver`.
Bug: 275
Change-Id: I705591658921f60a958164a18b68ffb697c2ea4b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44104
Reviewed-by: David Benjamin <davidben@google.com>
This CL fixes a couple of things. First, we never tested that SSL_write
refuses to write application data after a fatal alert, so add some tests
here. With those tests, we can revise some of this logic:
Next, this removes the write_shutdown check in SSL_write and instead
relies on the lower-level versions of the check in the write_app_data,
etc., hooks. This improves error-reporting on handshake errors:
We generally try to make SSL_do_handshake errors sticky, analogous to
handshakeErr in the Go implementation. SSL_write and SSL_read both
implicitly call SSL_do_handshake. Callers driving the two in parallel
will naturally call SSL_do_handshake twice. Since the error effectively
applies to both operations, we save and replay handshake errors
(hs->error).
Handshake errors typically come with sending alerts, which also sets
write_shutdown so we don't try to send more data over the channel.
Checking this early in SSL_write means we don't get a chance to replay
the handshake error. So this CL defers it, and the test ensures we still
ultimately get it right.
Finally, https://crbug.com/1078515 is a particular incarnation of this.
If the server enables 0-RTT and then reverts to TLS 1.2, clients need
to catch the error and retry. There, deferring the SSL_write check
isn't sufficient, because the can_early_write bit removes the write
path's dependency on the handshake, so we don't call into
SSL_do_handshake at all.
For now, I've made this error path clear can_early_write. I suspect
we want it to apply to all handshake errors, though it's weird because
the handshake error is effectively a read error in 0-RTT. We don't
currently replay record decryption failures at SSL_write, even though
those also send a fatal alert and thus break all subsequent writes.
Bug: chromium:1078515
Change-Id: Icdfae6a8f2e7c1b1c921068dca244795a670807f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48065
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Although not permitted by the TLS specification, systems sometimes
ossify TLS extension order, or byte offsets of various fields. To
keep the ecosystem healthy, add an API to reorder ClientHello
extensions.
Since ECH, HelloRetryRequest, and HelloVerifyRequest are sensitive to
extension order, I've implemented this by per-connection permutation of
the indices in the kExtensions structure. This ensures that all
ClientHellos within a connection are consistently ordered. As follow-up
work, permuting the other messages would also be nice, though any server
messages would need to be incorporated in handshake hints.
Change-Id: I18ce39b4df5ee376c654943f07ec26a50e0923a9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48045
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This would have caught an issue with some tests I was working on. It
also catches an issue with some per-message tests, so fix those.
Change-Id: I6b3ad8e0db0b1a6ccac4b346dcc652b16b73e006
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48046
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This comment dates to SSLeay. It appears to be describing the
incremental trial division strategy where they would pick a starting
candidate, compute moduli by small primes, and then update by
incrementing the candidate and saved moduli instead of dividing from
scratch. We use a simpler rejection sampling strategy.
Change-Id: If2203d616f2b1f632bcd7033ceb60a83d1b75674
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48047
Reviewed-by: Adam Langley <agl@google.com>
Based on an initial implementation by Dan McArdle at
https://boringssl-review.googlesource.com/c/boringssl/+/46784
This CL contains most of a client implementation for
draft-ietf-tls-esni-10. The pieces missing so far, which will be done in
follow-up CLs are:
1. While the ClientHelloInner is padded, the server Certificate message
is not. I'll add that once we resolve the spec discussions on how to
do that. (We were originally going to use TLS record-level padding,
but that doesn't work well with QUIC.)
2. The client should check the public name is a valid DNS name before
copying it into ClientHelloOuter.server_name.
3. The ClientHelloOuter handshake flow is not yet implemented. This CL
can detect when the server selects ClientHelloOuter, but for now the
handshake immediately fails. A follow-up CL will remove that logic
and instead add the APIs and extra checks needed.
Otherwise, this should be complete, including padding and compression.
The main interesting point design-wise is that we run through
ClientHello construction multiple times. We need to construct
ClientHelloInner and ClientHelloOuter. Then each of those has slight
variants: EncodedClientHelloInner is the compressed form, and
ClientHelloOuterAAD just has the ECH extension erased to avoid a
circular dependency.
I've computed ClientHelloInner and EncodedClientHelloInner concurrently
because the compression scheme requires shifting the extensions around
to be contiguous. However, I've computed ClientHelloOuterAAD and
ClientHelloOuter by running through the logic twice. This probably can
be done better, but the next draft revises the construction anyway, so
I'm thinking I'll rework it then. (In the next draft, we use a
placeholder payload of the same length, so we can construct the
ClientHello once and fill in the payload.)
Additionally, now that we have a client available in ssl_test, this adds
a threading test to confirm that SSL_CTX_set1_ech_keys is properly
synchronized. (Confirmed that, if I drop the lock in
SSL_CTX_set1_ech_keys, TSan notices.)
Change-Id: Icaff68b595035bdcc73c468ff638e67c84239ef4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48004
Reviewed-by: Adam Langley <agl@google.com>
We'll probably need to make this more complex later, but this should be
a start. I had hoped this would also simplify tests, MakeECHConfig() was
still needed to generate weird inputs for tests. I've instead tidied
that up a bit with a params structure. Now the only hard-coded ECHConfig
in tests is to check the output of the new API.
Bug: 275
Change-Id: I640a224fb4b7a7d20e8a2cd7a1e75d1e3fe69936
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48003
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Previously we would extract the KEM ID from the ECHConfig and then parse
the private key using the corresponding KEM type. This CL makes it take
a pre-pared EVP_HPKE_KEY and checks it matches. This does require the
caller pass the key type through externally, which is probably prudent?
(On the other hand we are still inferring config from the rest of the
ECHConfig... maybe we can add an API to extract the EVP_HPKE_KEM from a
serialized ECHConfig if it becomes a problem. I could see runner or tool
wanting that out of convenience.)
The immediate motivation is to add APIs to programmatically construct
ECHConfigs. I'm thinking we can pass a const EVP_HPKE_KEY * to specify
the key, at which point it's weird for SSL_ECH_KEYS_add to look
different.
Bug: 275
Change-Id: I2d424323885103d3fe0a99a9012c160baa8653bd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48002
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We didn't correctly handle tests where the versions figure into
resumeConfig and got by because the test didn't actually check the
version. Run it more accurately, and check more fields. Also add a
skipVersionNameCheck option for when the heuristic doesn't work.
(Some of the tests specify a TLS maximum version by passing in all the
-no-tls1, etc., flags for the other versions. Moreover, some of them
will set no flags for a maximum of TLS 1.3. Suppress the check on those
tests.)
Change-Id: I72c069b1baed09e29bf502036957fe0e90edbe60
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48000
Reviewed-by: Adam Langley <agl@google.com>
While the previous CL fixed a bug in computing this padding, we don't
actually need to pad the second (cleartext) ClientHello anyway. This
padding is to work around bugs in old F5 and WebSphere servers, which do
not speak TLS 1.3. Save a few bytes.
Change-Id: I9b5d9bb1c0d880f1b1c9182667a9d3d82588c04c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47999
Reviewed-by: Adam Langley <agl@google.com>
If we're dropping the PSK extension due to an HRR with mismatched hash
(looking back at that, we could easily have avoided that nuisance...
I've filed [0] on rfc8446bis), we don't predict the length correctly.
The consequence is we don't pad the second ClientHello to the desired
range. Fix this and add an assert.
[0] https://github.com/tlswg/tls13-spec/issues/1227
Change-Id: I47d880b6bdafa95840f7513b6b7718851f22554d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47998
Reviewed-by: Adam Langley <agl@google.com>
Computing the binders on ClientHelloInner is a little interesting. While
I'm in the area, tidy this up a bit. The exploded parameters may as well
be an SSL_SESSION, and hash_transcript_and_truncated_client_hello can
just get folded in.
Change-Id: I9d3a7e0ae9f391d6b9a23b51b5d7198e15569b11
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47997
Reviewed-by: Adam Langley <agl@google.com>
This makes calls to ssl_add_clienthello_tlsext a hair easier. Also we
only apply the [256, 511) compatibility hack to TLS, so we can just use
a constant.
Bug: 275
Change-Id: Ia2b5192aeef0cd8848ecfa1ea3b89a0a7382ff1a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47996
Reviewed-by: Adam Langley <agl@google.com>
ssl_add_clienthello_tlsext is about to get kinda messy with ECH. Move
the padding and GREASE extensions into a few helpers.
Bug: 275
Change-Id: I3bb702fb79dce4be68490c4a8fd889121ecdae58
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47995
Reviewed-by: Adam Langley <agl@google.com>
For TLS 1.3, since the bulk of extensions move to EncryptedExtensions,
we made the extension callbacks apply to EncryptedExtensions and pulled
the few ServerHello extensions out of the callback system. This means
the ServerHello naming is a little confusing.
We probably should rename these callbacks, though parse_server is a bit
ambiguous as to whether this is "parse the extension from the server" or
"parse as a server". For now, add a comment.
Change-Id: If1aa0846426de2cc8dcb2253695a8dd3285f7b76
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47994
Reviewed-by: Adam Langley <agl@google.com>
We'll need to maintain two transcripts on the ECH client and then, once
we know which of ClientHelloOuter or ClientHelloInner is used, overwrite
the default transcript with the alternate one.
Rather than indirect through a pointer, move support is easy enough.
Then this can just be hs->transcript = std::move(hs->inner_transcript).
Bug: 275
Change-Id: Id4b0a0a48b956cd65ce8fc3dacfd16eebe2eb778
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47993
Reviewed-by: Adam Langley <agl@google.com>
May not be strictly necessary, but similarly easier to reason about when
we need to interweave multiple ClientHellos.
Bug: 275
Change-Id: I9f85787860f3e8ce1653331ce52343d5bf5def23
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47992
Reviewed-by: Adam Langley <agl@google.com>
This is less effective than it seems because both
((const SSL_HANDSHAKE*)hs)->ssl and ((const SSL*)ssl)->s3 are both
non-const pointers. I considered moving hs->ssl to hs->ssl_ and adding
const-overloaded accessors, but I'd need to repeat the same with
ssl->s3, at which point this seemed not worth it yet. Maybe later if we
rewrite everything to more idiomatic C++.
Bug: 275
Change-Id: I9912a3df205916fdf2191a3687013d717528038d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47991
Reviewed-by: Adam Langley <agl@google.com>
This is kinda annoying and, like the grease_seed, demonstrates a
shortcoming with the idea of making add_clienthello completely const.
Strictly speaking, we only need idempotence. But I think this is okay:
const is much easier to enforce than idempotence, and we'll likely need
to do this anyway:
- While not in the current draft, I expect the draft's eventual HRR
resolution to retain the ECH extension, GREASE or not, on ECH reject.
Things are somewhat violating RFC8446 HRR rules right now. That means
we'll need to stash the ECH payload regardless.
- ECH binds all the other extensions in the outer ClientHello, so
computing the payload will need to move outside the callback system
anyway.
In some sense, all this is shifting our ClientHello output from the
"immediate mode" end of the spectrum (as we usually use) to the
"retained mode" end, which I suppose makes sense as this message becomes
more intricate.
Bug: 275
Change-Id: I9eb8cd1cde2ce264345b6ed3ee526d4eab81e911
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47990
Reviewed-by: Adam Langley <agl@google.com>
This lets ssl_get_grease_value be const. The lazy thing isn't a
deal-breaker (we only need idempotence, and a non-thread-safe const also
works fine), but just initializing it earlier seems simpler.
Bug: 275
Change-Id: Iad228ea4a9146ede9a3849f3339f7ec9e698e6eb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47988
Reviewed-by: Adam Langley <agl@google.com>
This is now never used. Instead, we rely on each renegotiation creating
a new handshake structure with fresh state. This simplifies things for
ECH.
(We probably could make an init hook work with ECH's two-ClientHello
scheme by either maintaining separate state per ClientHello or calling
init once for both ClientHellos. But the few uses of init were
removable, so this is easier.)
Bug: 275
Change-Id: Ie5e132fe072e5ea8db21ca16aa53fcd0895d8e48
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47987
Reviewed-by: Adam Langley <agl@google.com>
Like the early_data CL, this does shift a bit of logic that was
previously hidden away in the callbacks. For key_share, this is probably
a good move independent of ECH. The logic around HRR, etc., was a little
messy.
Bug: 275
Change-Id: Iafbcebdf66ce1f7957d798a98ee6b996fff24639
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47986
Reviewed-by: Adam Langley <agl@google.com>
Also add ECH GREASE state into the mix. Clearing this isn't critical,
especially now that we have an SSL_HANDSHAKE structure, but it's easy
enough.
Bug: 275
Change-Id: If1aa8d5c0c8fdb5af710852778ce452c507a2524
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47985
Reviewed-by: Adam Langley <agl@google.com>
ECH requires that we construct two ClientHellos. That means our
add_clienthello callbacks will need to be called multiple times and
should be const. (They already are called multiple times for
HelloRetryRequest, but we currently thread that through the callbacks a
bit. With ECH, I think we need to make them pure serialization.)
Bug: 275
Change-Id: I11f8195fd2ec4b8639f0a2af01a24d4974445580
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47984
Reviewed-by: Adam Langley <agl@google.com>
While decompression is deterministic, compression is not. New revisions
of the compression algorithm may start using different (hopefully
smaller!) compressions. So this doesn't cause hint mismatches, add a
certificate compression hint. If the shim's Certificate message matches
the handshaker, we'll reuse the already compressed message.
This also adds what appears to be a missing test for when the server
cannot find compression algorithms in common.
Change-Id: Idbedaceb20208463d8f61581ee27971c17fcd126
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48005
Reviewed-by: Adam Langley <agl@google.com>
This implements draft-ietf-tls-esni-10.
This will be used to test the client implementation. While I'm here,
I've switched the setup logic in the server tests to use the new
ServerECHConfig type. I'll probably need to patch in various features
later for testing, but this should be a usable starting point.
Based on an initial implementation by Dan McArdle in
https://boringssl-review.googlesource.com/c/boringssl/+/46786
Bug: 275
Change-Id: I69523cda70c3da2ae505bcab837fd358195fb9e9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47967
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Noticed this while I was in the area. We currently use an extremely lax
parse that even tolerates syntax errors. Instead use a strict parse that
ensures our client only sends what we expect.
Change-Id: Ifb0e1e1698489ff217db0c7a0317caa885e20759
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47966
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Having the nil vs. non-nil []byte for the sake of a couple tests with
invalid payloads is tedious. Use separate fields instead.
Bug: 275
Change-Id: I557d914d60ce94d68796c05162ff3dd2ab7684db
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47965
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
An ECHConfig is like a certificate in that knowing the fields isn't
sufficient. The exact byte representation is significant. (The ECHConfig
is bound into the encryption.) But the ECHConfig type only has fields,
so runner can only represent ECHConfigs that are the output of our
serialization function.
This matters less as a client testing a server because the server can
only parse ECHConfigs with fields we support. But as a server testing a
client, we need to see how the client reacts to extra extensions, etc.
Just using []byte to represent ECHConfigs is inconvenient, so instead
pattern this after x509.Certificate: you can parse one from a byte
string (not currently included since we don't need it yet), or you can
construct a new one from a template with the fields you want.
Bug: 275
Change-Id: I6602d0780b1cef12b6c4b442999bdff7b3d7dd70
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47964
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The test was not actually using a repeated config ID.
Bug: 275
Change-Id: I69519fde196247abb07dceba1da1bac22188f13f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47912
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
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 ECH server extension is defined for TLS 1.3 EncryptedExtensions, not
TLS 1.2 ServerHello.
Bug: 275
Change-Id: Ie6e76c238075d70e6a0694ec0192df07da3457d1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47910
Reviewed-by: Adam Langley <agl@google.com>
Make it a little clearer they shouldn't be updating sequence numbers,
enqueuing the message, etc. That's left to add_message. (ECH clients
need to construct a pair of parallel ClientHellos.)
Change-Id: I554a8f200d464727bc219b66931b3d0bae266f76
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47908
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>
This avoids needing to worry about the interaction with renegotiation
which, in turn, means we can drop the init callback. (If we did support
DTLS renegotiation, we'd probably want to forbid the parameter from
changing anyway. Changing your SRTP parameters partway through will
likely confuse the RTP half of the application anyway.)
Change-Id: Ifef1e9479d9df296b69b0d296f6bef57b13da68e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47905
Reviewed-by: Adam Langley <agl@google.com>
Because file names are not enclosed in quotation marks in the open call.
https://bugs.chromium.org/p/boringssl/issues/detail?id=415
```
cmake --build "C:\Projects\ Extern\Visual C++ 2015\x64 Debug\Build\BoringSSL\."
[9/439] Generating rdrand-x86_64.asm
FAILED: crypto/fipsmodule/rdrand-x86_64.asm
cmd.exe /C "cd /D "C:\Projects\ Extern\Visual C++ 2015\x64 Debug\Build\BoringSSL\crypto\fipsmodule" && "C:\Program Files\CMake\bin\cmake.exe" -E make_directory . && C:\Perl64\bin\perl.exe "C:/Projects/ Extern/Source/BoringSSL/crypto/fipsmodule/rand/asm/rdrand-x86_64.pl" nasm rdrand-x86_64.asm"
Can't open perl script "C:/Projects/": No such file or directory
error closing STDOUT at C:/Projects/ Extern/Source/BoringSSL/crypto/fipsmodule/rand/asm/rdrand-x86_64.pl line 87.
ninja: build stopped: subcommand failed.
```
Bug: 415
Change-Id: I83c4a460689b9adeb439425ad390322ae8b2002a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47884
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@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>
David Benjamin
Dan McArdle
Florin Crișan