Some of our calls handled it and others didn't.
Change-Id: I09f15d3db679954599bcf987d86357b6e12e9b9b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46532
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The ssl_buffer.cc code handles this, but since outgoing handshake I/O
goes through a different path, it was missing these checks.
Change-Id: I4fed62b435b577645c405d0d995511a58d47a702
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46531
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The check for ssl_hs_read_change_cipher_spec didn't do anything. Replace
it with an assert and add some comments since the hs->wait handling is a
little tricky.
Change-Id: I8e62ce3cceca9bed4611cb9d3faf0bfec3d3bdd4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46530
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
It's strange to have Serialize/Deserialize methods not inverses of each
other. Split the operation up and move the common parts out of the
subclass.
Change-Id: Iadfa57de19faca411c64b64d2568a78d2eb982e8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46529
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The delegated credentials bits got stuck in the middle of the handshake
bits.
Change-Id: I522d8a5a5f000de3e329934851ee74fc4ec613a7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46528
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
TLS 1.3 works, so no need to exclude version negotiation. We also now
only test QUICTransportParams with QUIC, so there is no need to exclude
it manually. Checking the protocol works as well.
Change-Id: Ie9d33095231a1f9eb74145db5147a287e4fdc930
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46527
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This is no longer needed.
Change-Id: Ie6dba524ecccd265f7f80a910b40c0fe1800356b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46526
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Do a better job with scopers for fds and posix_spawn_file_actions_t.
There's also no need to make a copy of handshaker_path with strdup.
The non-const parameter are because posix_spawn inherits execve's
C problem: unlike C++, C cannot cast from char *const * to
const char *const *, so POSIX APIs are not const-correct.
Finally, we freely use std::vector and friends in tests, so we don't
actually need to depend on bssl::Array.
Change-Id: I739dcb6b1a2d415d47ff9b2399eebec987aab0bc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46524
Reviewed-by: Adam Langley <agl@google.com>
Omitting the extension means we'll never issue tickets, but if the
client were to offer a ticket anyway, RFC8446 4.2.9 says we MUST reject
the ClientHello. It's not clear on what alert to use, but
missing_extension is probably appropriate.
Thanks to Ben Kaduk for pointing this out.
Change-Id: Ie5c720eac9dd2e1a27ba8a13c59b707c109eaa4e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46464
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This wasn't being used and wasn't even set correctly in split handshake
tests.
Change-Id: I03000db8dd3c227ea44e7bacaf3d1341259fae44
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46384
Reviewed-by: Adam Langley <agl@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>
This is a little inconvenient for external users of the test suite. It's
also not very helpful to pass -handshaker-path in build configurations
without a handshaker because there won't be a file there anyway.
Change-Id: I6a8fdcfbbf86288876c4c6fda2a46d32663efb69
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46244
Reviewed-by: Adam Langley <agl@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>
We already know all the supported curves in runner.go. No sense in
repeating this list in more places than needed. (I'm about to need a
similar construct for -signing-prefs, so I figure it's worth being
consistent.)
This CL also adds a ShimConfig option because others don't support the
same curves we do and will likely run into this quickly.
Change-Id: Id79cea16891802af021b53a33ffd811a5d51c4ae
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46186
Reviewed-by: Adam Langley <agl@google.com>
This flag causes the runner to execute the shim with the RR debugger.
See https://rr-project.org/.
Unlike typical debuggers, the RR workflow is to first record a session
and then replay it. The user cannot interact with the debugger while
recording and they replay the session multiple times. For these reasons,
I've opted not to launch xterm like -gdb and -lldb do.
The other difference is that -rr-record restricts the runner to exactly
one test. Otherwise, it's too easy to accumulate a bunch of unwanted
recordings. Also, `rr replay` uses the most recent recording by default,
so it's not very useful for runner to record multiple tests.
Change-Id: I2d29d64df5c4c832e50833325db3500ec2698e76
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46144
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This mirrors a change on the C side. Sessions may store the master
secret (main secret as of draft-ietf-tls-rfc8446bis-01) in TLS 1.2 or
the resumption PSK in TLS 1.3, so giving it any description other than
plain 'secret' isn't even accurate.
(Doing this separately from the rfc8446bis names since it's a bit less
mechanical.)
Change-Id: Iaf2b72fe298f17eeb4f4957cfd78b0015c3a9d89
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/45824
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>
The check was happening in code that only ran at TLS 1.2, so we weren't
testing anything. Additionally check the resumption case. While we do
handle it correctly, we only manage it due to the weird OpenSSL quirk
we've carried over from TLS 1.2 tickets where we synthesize a session ID
for TLS 1.3 tickets. (Is that still needed?)
That's subtle enough to warrant a test, and some other implementations
reuse our test suite, so it's worth the coverage there.
Change-Id: I83cc355bd853097ec6edcd2cc40b06b19e3b00e5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/45324
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
A linter was complaining about some instance, so just fix the lot of it.
Change-Id: I7e23cbada6e42da887d740b84a05de9f104a86ab
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/45284
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
QUICHE currently does not know to call
SSL_set_quic_use_legacy_codepoint, picking up the current default of the
legacy code point. It then assumes that the
TLSEXT_TYPE_quic_transport_parameters constant may be used to extract
transport parameters, so after
https://boringssl-review.googlesource.com/c/boringssl/+/44704, it
breaks.
To smooth over the transition, we now define three constants:
TLSEXT_TYPE_quic_transport_parameters_legacy,
TLSEXT_TYPE_quic_transport_parameters_standard, and the old constant.
The old constant will match whatever the default is (for now, legacy) so
the default is self-consistent. Then plan is then:
1. BoringSSL switches to the state in this CL: the default code point
and constant are the legacy one, but there are APIs to specify the
code point. This will not affect QUICHE, which only uses the
defaults.
2. QUICHE calls SSL_set_quic_use_legacy_codepoint and uses the
corresponding _legacy or _standard constant. It should *not* use the
unsuffixed constant at this point.
3. BoringSSL switches the default setting and the constant to the
standard code point. This will not affect QUICHE, which explicitly
configures the code point it wants.
4. Optional: BoringSSL won't switch the default back to legacy, so
QUICHE can switch _standard to unsuffixed and BoringSSL
can remove the _standard alias (but not the function) early.
5. When QUICHE no longer needs both code points, it unwinds the
SSL_set_quic_use_legacy_codepoint code and switches back to the
unsuffixed constant.
6. BoringSSL removes all this scaffolding now that it's no longer
needed.
Update-Note: This this fixes a compatibility issue with
https://boringssl-review.googlesource.com/c/boringssl/+/44704.
Change-Id: I9f75845aba58ba93e9665cd6f05bcd080eb5f139
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/45124
Reviewed-by: David Schinazi <dschinazi@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
When offering 0-RTT, the client should check that all carried-over
values are consistent with its preferences. This ensures that parameter
negotiation happens independently of 0-RTT. The ALPS version of this
check was a tad too aggressive: a session without ALPS should be treated
as always compatible.
I'll follow this with a fix to the draft spec to clarify this.
Change-Id: Ia3c2a60449c555d1d91c4e528215f8e551a90a9f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/45104
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@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>
They align exactly with TLS record types, so just use the existing
constants.
Change-Id: I693e7c740458cf73061e6b573eeb466d0fce93cc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44990
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This changes the format of the mock QUIC transport to include an
explicit encryption level, matching real QUIC a bit better. In
particular, we need that extra data to properly skip rejected early data
on the shim side. (On the runner, we manage it by synchronizing with the
TLS stack. Still, the levels make it a bit more accurate.)
Testing sending and receiving of actual early data is not very relevant
in QUIC since application I/O is external, but this allows us to more
easily run the same tests in TLS and QUIC.
Along the way, improve error-reporting in mock_quick_transport.cc so
it's easier to diagnose record-level mismatches.
Change-Id: I96175a4023134b03d61dac089f8e7ff4eb627933
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44988
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This originally didn't work because we installed an async BIO, while
QUIC uses the BIO to mock out a QUIC transport. Our QUIC IO callbacks
don't have a meaningful notion of sync vs async, so no-op this portion
of the -async flag.
The immediate motivation is I'd like to make addExtensionTests run over
all protocols, and having the async tests fail is inconvenient. However,
async tests in QUIC is still meaningful anyway to support various
callbacks, so I've removed the workaround in the state machine coverage
tests. (Though most of those async tests are redundant as they're
concerned with IO, not callbacks.) Along the way, the various handshake
record controls are irrelevant to QUIC, so this actually results in a
net decrease in redundant tests.
Change-Id: I67c1ee48cb2d85b47ae3328fecfac86a24aa2ed1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44987
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The state machine around EndOfEarlyData is a bit messy, which caused a
problem introducing the new message in QUIC. We keep waffling on whether
that state junction should no-op the EndOfEarlyData state or skip it.
Since skipping it caused us to miss this spot, let's try no-op-ing it.
As a test, so this CL is easier to cherry-pick, I've just duplicated the
basic server test. Better, however, would be to run all the extensions
tests under QUIC. (In particular, this is missing 0-RTT coverage.) But
this is a large diff and requires improving the mock QUIC transport,
etc., in runner. That work is done in follow-up CLs, which replace this
duplicated test.
Change-Id: I25b6feabdc6e5393ba7f486651986a90e3721667
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44985
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
There was a QUIC-specific ALPS bug, so I'm thinking we should loop
addExtensionTests at all protocols. To do so, we need to fix this bug in
the test expectation.
Change-Id: Ic05a4cb2ea32e7145441a0273cd65966c41534ec
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44986
Reviewed-by: Adam Langley <agl@google.com>
This wasn't the cause of the bug, but I noticed we never tested it, so
fill that in.
Change-Id: Ib38bc08309e69f43c1995ba2a387643c0a7bae99
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44984
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This CL implements the backend server behavior described in Section 7.2
of draft-ietf-tls-esni-09.
Bug: 275
Change-Id: I2e162673ce564db0cb75fc9b71ef11ed15037f4b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/43924
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Bug: 275
Change-Id: Ifef2b94f701ab75755893c2806335b626b655446
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44904
Commit-Queue: Dan McArdle <dmcardle@google.com>
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>
When the handshaker fails to parse a config it currently exits. This
causes the two pipes to signal EOF to the shim, but the control channel
is a datagram socket in order to be atomic, thus doesn't signal an
error.
In the shim, EOF on the wfd pipe causes a short loop and thus a hang
forever. Catching the EOF and returning an error doesn't work because
some tests will close the pipe but still return information over the
control channel. We can start a timeout once wfd is closed, but that
seems like it might be flakey.
Thus this change makes the handshaker send an explicit error over the
control channel. It doesn't catch crashes, but it will catch config
errors, which are much more common in cross-version tests.
Change-Id: I4b1afed17694c57e4713d1b0fa4e9ecb12f09ec5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/43865
Reviewed-by: David Benjamin <davidben@google.com>
In order to skip groups of tests in the cross-version testing (like
ALPS-*), it's useful to be able to match them by pattern.
Change-Id: Ic7e40c04a33b4bcbb08494fa04deb5e862f09d8f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/43864
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@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>
Add an earlyData and earlyDataRejected flag to configure the standard
0-RTT test options. It's too tedious otherwise. Along the way, I added
an -expect-cipher flag to a few of the tests which could do with them.
This does cause most 0-RTT tests to exchange a quick burst of data, so a
few more fuzzer mode suppressions are needed. I think that's probably
fine. Maybe we should mess with fuzzer mode so it's able to trial
decrypt as this is getting a little tedious.
Change-Id: Ib6490fe006d91294aab1a06d88f7793c6ae840c8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/43086
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
OpenSSL synchronizes bio->next_bio and ssl->rbio with a variety of
callbacks, so BIO_copy_next_retry worked. We do not, so attempting to
flush the BIO crashed.
The SSL BIO is a compatibility hack and intentionally much more limited,
so start by just copying things from the right BIO directly. Add a basic
unit test for SSL BIOs. If we need to, we can implement a more complex
synchronization later.
Additionally reject reconfiguring an SSL BIO because that will leak the
object right now.
Change-Id: I724c95ab6f1a3a1aa1889b0483c81ce3bdc534ae
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/43424
Reviewed-by: Adam Langley <agl@google.com>
The Go TLS implementation, at the time runner forked, had custom
testing-only equal methods on all the handshake messages. We've since
removed all of them except for ClientHello, where we repurposed the
function to check ClientHello consistency on HelloVerifyRequest and
HelloRetryRequest.
These are tedious to update. Upstream has since replaced them with
reflect.DeepEqual, but the comparison we want is even tighter. Even
unknown extensions aren't allowed to change. Replace the check with a
custom one that works on the byte serialization and remove
clientHelloMsg.equal.
Along the way, I've fixed the HRR PSK identity logic to match the spec a
bit more and check binders more consistently.
Change-Id: Ib39e8791201c42d37e304ae5110c7aeed62c8b3f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/43364
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This CL replaces clientHelloMsg's npnAfterAlpn and pskBinderFirst fields
with a new field: prefixExtensions. The extensions in prefixExtensions
are tried first when marshalling clientHelloMsg.
The ability to control extensions' marshalling order will make it
simpler to implement the "outer_extensions" behavior defined in
draft-ietf-tls-esni-07.
Bug: 275
Change-Id: Ib6dcc1e6fa0281f312cb65a9e204415c3f3ef2c6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/43064
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
David Benjamin
Dan McArdle
David Schinazi
Adam Langley
Steven Valdez