As a client, we must tolerate this to avoid interoperability failures
with allowed server behaviors.
BUG=117
Change-Id: I9c40a2a048282e2e63ab5ee1d40773fc2eda110a
Reviewed-on: https://boringssl-review.googlesource.com/12311
Reviewed-by: David Benjamin <davidben@google.com>
Draft 18 sadly loosens the requirements to only requiring the PRF hash
stay fixed.
BUG=117
Change-Id: Ic94d53fd9cabaee611fcf36b0071558075e10728
Reviewed-on: https://boringssl-review.googlesource.com/12310
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is generally much cleaner and makes it possible to implement the
more lax cipher matching in draft 18.
BUG=117
Change-Id: I595d7619d60bc92e598d75b43945286323c0b72b
Reviewed-on: https://boringssl-review.googlesource.com/12309
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
It doesn't particular matter, but AcceptAnySession should only skip the
things that would cause us to note accept a ticket. ExpectTicketAge is
an assertion, not part of protocol logic. Accordingly, fix the text.
Change-Id: I3bea9c58f4d5f912308252ec8834f183287d632f
Reviewed-on: https://boringssl-review.googlesource.com/12308
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The version check should run if AcceptAnyVersion is *not* set.
Change-Id: I4c137564f91a86cb5e6a26e09fd4670cce8f1dcb
Reviewed-on: https://boringssl-review.googlesource.com/12307
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
When debugging a flaky test, it's useful to be able to run a given test
over and over.
Change-Id: I1a7b38792215550b242eb8238214d873d41becb6
Reviewed-on: https://boringssl-review.googlesource.com/12301
Reviewed-by: David Benjamin <davidben@google.com>
The draft 18 implementation did not compute scts_requested correctly. As
a result, it always believed SCTs were requested. Fix this and add tests
for unsolicited OCSP responses and SCTs at all versions.
Thanks to Daniel Hirche for the report.
Change-Id: Ifc59c5c4d7edba5703fa485c6c7a4055b15954b4
Reviewed-on: https://boringssl-review.googlesource.com/12305
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Thanks to Eric Rescorla for catching this.
Change-Id: Id0a024d7f705519cfe76d350e0ef2688dbd11a22
Reviewed-on: https://boringssl-review.googlesource.com/12303
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Having that logic in two different places is a nuisance when we go to
add new checks like resumption stuff. Along the way, this adds missing
tests for the ClientHello cipher/session consistency check. (We'll
eventually get it for free once the cipher/resumption change is
unblocked, but get this working in the meantime.)
This also fixes a bug where the session validity checks happened in the
wrong order relative to whether tickets_supported or renew_ticket was
looked at. Fix that by lifting that logic closer to the handshake.
Change-Id: I3f4b59cfe01064f9125277dc5834e62a36e64aae
Reviewed-on: https://boringssl-review.googlesource.com/12230
Reviewed-by: Adam Langley <agl@google.com>
This was removed a while ago. As of -18, the early data indication
extension is just a boolean.
Change-Id: I328b9abfafad326d4c2a3b5fe981af111f8401ad
Reviewed-on: https://boringssl-review.googlesource.com/12302
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We missed that the TLS 1.3 code was inconsistent with the TLS 1.2 code.
Only on the server did we push an error code. But consistency between
client and server is probably worthwhile so, fix the 1.2 code to match
for now.
Change-Id: I17952c72048697dc66eacf0f144a66ced9cb3be8
Reviewed-on: https://boringssl-review.googlesource.com/12260
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Change-Id: I07c4b67206440d169b314f24e1b3c1c697dda24f
Reviewed-on: https://boringssl-review.googlesource.com/12204
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Certificate chain with intermediate taken from Chromium's tests. Though
it doesn't really matter because the runner tests don't verify
certificates.
BUG=70
Change-Id: I46fd1d4be0f371b5bfd43370b97d2c8053cfad60
Reviewed-on: https://boringssl-review.googlesource.com/12261
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We used to enforce after the version was set, but stopped enforcing with
TLS 1.3. NSS enforces the value for encrypted records, which makes sense
and avoids the problems gating it on have_version. Add tests for this.
Change-Id: I7fb5f94ab4a22e8e3b1c14205aa934952d671727
Reviewed-on: https://boringssl-review.googlesource.com/12143
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We have AEAD-level coverage for these, but we should also test this in
the TLS stack, and at maximum size per upstream's CVE-2016-7054.
Change-Id: I1f4ad0356e793d6a3eefdc2d55a9c7e05ea08261
Reviewed-on: https://boringssl-review.googlesource.com/12187
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
3c6a1ea674 switched what layer handled
the DTLS version mapping but forgot to correct the HelloVerifyRequest
logic to account for this.
Thanks to Jed Davis for noticing this.
Change-Id: I94ea18fc43a7ba15dd7250bfbcf44dbb3361b3ce
Reviewed-on: https://boringssl-review.googlesource.com/11984
Reviewed-by: David Benjamin <davidben@google.com>
TLS 1.3 ciphers are now always enabled and come with a hard-coded
preference order.
BUG=110
Change-Id: Idd9cb0d75fb6bf2676ecdee27d88893ff974c4a3
Reviewed-on: https://boringssl-review.googlesource.com/12025
Reviewed-by: Adam Langley <agl@google.com>
They will get very confused about which key they're using. Any caller
using exporters must either (a) leave renegotiation off or (b) be very
aware of when renegotiations happen anyway. (You need to somehow
coordinate with the peer about which epoch's exporter to use.)
Change-Id: I921ad01ac9bdc88f3fd0f8283757ce673a47ec75
Reviewed-on: https://boringssl-review.googlesource.com/12003
Reviewed-by: Adam Langley <agl@google.com>
The existing tests for this codepath require us to reconfigure the shim.
This will not work when TLS 1.3 cipher configuration is detached from
the old cipher language. It also doesn't hit codepaths like sessions
containing a TLS 1.3 version but TLS 1.2 cipher.
Instead, add some logic to the runner to rewrite tickets and build tests
out of that.
Change-Id: I57ac5d49c3069497ed9aaf430afc65c631014bf6
Reviewed-on: https://boringssl-review.googlesource.com/12024
Reviewed-by: Adam Langley <agl@google.com>
This change is based on interpreting TLS 1.3 draft 18.
Change-Id: I727961aff2f7318bcbbc8bf6d62b7d6ad3e62da9
Reviewed-on: https://boringssl-review.googlesource.com/11921
Reviewed-by: David Benjamin <davidben@google.com>
BUG=chromium:659593
Change-Id: I73a4751609b85df7cd40f0f60dc3f3046a490940
Reviewed-on: https://boringssl-review.googlesource.com/11861
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
On the client we'll leave it off by default until the change has made it
through Chrome's release process. For TLS 1.3, there is no existing
breakage risk, so always do it. This saves us the trouble of having to
manually turn it on in servers.
See [0] for a data point of someone getting it wrong.
[0] https://hg.mozilla.org/projects/nss/rev/9dbc21b1c3cc
Change-Id: I74daad9e7efd2040e9d66d72d558b31f145e6c4c
Reviewed-on: https://boringssl-review.googlesource.com/11680
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I18cee423675d6a686f83b4ef4b38696cb618392c
Reviewed-on: https://boringssl-review.googlesource.com/11683
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
BUG=103
Change-Id: I9a49fbaf66af73978ce264d27926f483e1e44766
Reviewed-on: https://boringssl-review.googlesource.com/11620
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Channel ID for TLS 1.3 uses the same digest construction as
CertificateVerify. This message is signed with the Channel ID key and
put in the same handshake message (with the same format) as in TLS 1.2.
BUG=103
Change-Id: Ia5b2dffe5a39c39db0cecb0aa6bdc328e53accc2
Reviewed-on: https://boringssl-review.googlesource.com/11420
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
{sha1, ecdsa} is virtually nonexistent. {sha512, ecdsa} is pointless
when we only accept P-256 and P-384. See Chromium Intent thread here:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/kWwLfeIQIBM/9chGZ40TCQAJ
This tweaks the signature algorithm logic slightly so that sign and
verify preferences are separate.
BUG=chromium:655318
Change-Id: I1097332600dcaa38e62e4dffa0194fb734c6df3f
Reviewed-on: https://boringssl-review.googlesource.com/11621
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Some new tests needed to be suppressed.
Change-Id: I4474d752c338a18440efb213e0795ae81ad754fb
Reviewed-on: https://boringssl-review.googlesource.com/11583
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This should land in the same group of revisions as the two parent
commits.
Change-Id: Id9d769b890b3308ea70b705e7241c73cb1930ede
Reviewed-on: https://boringssl-review.googlesource.com/11581
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
We'll never send cookies, but we'll echo them on request. Implement it
in runner as well and test.
BUG=98
Change-Id: Idd3799f1eaccd52ac42f5e2e5ae07c209318c270
Reviewed-on: https://boringssl-review.googlesource.com/11565
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This doesn't currently honor the required KeyUpdate response. That will
be done in a follow-up.
BUG=74
Change-Id: I750fc41278736cb24230303815e839c6f6967b6a
Reviewed-on: https://boringssl-review.googlesource.com/11412
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
These too must be rejected. Test both unknown extensions and extensions
in the wrong context.
Change-Id: I54d5a5060f9efc26e5e4d23a0bde3c0d4d302d09
Reviewed-on: https://boringssl-review.googlesource.com/11501
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is part of TLS 1.3 draft 16 but isn't much of a wire format change,
so go ahead and add it now. When rolling into Chromium, we'll want to
add an entry to the error mapping.
Change-Id: I8fd7f461dca83b725a31ae19ef96c890d603ce53
Reviewed-on: https://boringssl-review.googlesource.com/11563
Reviewed-by: David Benjamin <davidben@google.com>
We need to retain a pair of Finished messages for renegotiation_info.
SSL 3.0's is actually larger than TLS 1.2's (always 12 bytes). Take
renegotiation out in preparation for trimming them to size.
Change-Id: I2e238c48aaf9be07dd696bc2a6af75e9b0ead299
Reviewed-on: https://boringssl-review.googlesource.com/11570
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: Ifcdbeab9291d1141605a09a1960702c792cffa86
Reviewed-on: https://boringssl-review.googlesource.com/11561
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Change-Id: I5d4fc0d3204744e93d71a36923469035c19a5b10
Reviewed-on: https://boringssl-review.googlesource.com/11560
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The server acknowledging a non-existent session is a particularly
interesting case since getting it wrong means a NULL crash.
Change-Id: Iabde4955de883595239cfd8e9d84a7711e60a886
Reviewed-on: https://boringssl-review.googlesource.com/11500
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
BUG=77
Change-Id: If568412655aae240b072c29d763a5b17bb5ca3f7
Reviewed-on: https://boringssl-review.googlesource.com/10840
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
BUG=77
Change-Id: Id8c45e98c4c22cdd437cbba1e9375239e123b261
Reviewed-on: https://boringssl-review.googlesource.com/10763
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
EnableAllCiphers is problematic since some (version, cipher)
combinations aren't even defined and crash. Instead, use the
SendCipherSuite bug to mask the true cipher (which is becomes arbitrary)
for failure tests. The shim should fail long before we get further.
This lets us remove a number of weird checks in the TLS 1.3 code.
This also fixes the UnknownCipher tests which weren't actually testing
anything. EnableAllCiphers is now AdvertiseAllConfiguredCiphers and
does not filter out garbage values.
Change-Id: I7102fa893146bb0d096739e768c5a7aa339e51a8
Reviewed-on: https://boringssl-review.googlesource.com/11481
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is another case where the specification failed to hammer things
down and OpenSSL messed it up as a result. Also fix the SCT test in TLS
1.3.
Change-Id: I47541670447d1929869e1a39b2d9671a127bfba0
Reviewed-on: https://boringssl-review.googlesource.com/11480
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The client/server split didn't actually make sense. We're interested in
whether the client will notice the bad version before anything else, so
ignore peer cipher preferences so all combinations work.
Change-Id: I52f84b932509136a9b39d93e46c46729c3864bfd
Reviewed-on: https://boringssl-review.googlesource.com/11413
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
ConflictingVersionNegotiation really should be about, say 1.1 and 1.2
since those may be negotiated via either mechanism. (Those two cases are
actually kinda weird and we may wish to change the spec. But, in the
meantime, test that we have the expected semantics.)
Also test that we ignore true TLS 1.3's number for now, until we use it,
and that TLS 1.3 suitably ignores ClientHello.version.
Change-Id: I76c660ddd179313fa68b15a6fda7a698bef4d9c9
Reviewed-on: https://boringssl-review.googlesource.com/11407
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
They weren't updated for the new version negotiation. (Though right now
they're just testing that we *don't* implement the downgrade detection
because it's a draft version.)
Change-Id: I4c983ebcdf3180d682833caf1e0063467ea41544
Reviewed-on: https://boringssl-review.googlesource.com/11406
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Otherwise we panic. Thanks to EKR for reporting.
Change-Id: Ie4b6c2e18e1c77c7b660ca5d4c3bafb38a82cb6a
Reviewed-on: https://boringssl-review.googlesource.com/11405
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL recently had a regression here (CVE-2016-6309). We're fine,
but so that we stay that way, add some tests.
Change-Id: I244d7ff327b7aad550f86408c5e5e65e6d1babe5
Reviewed-on: https://boringssl-review.googlesource.com/11321
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
BUG=106
Change-Id: Iaa12aeb67627f3c22fe4a917c89c646cb3dc1843
Reviewed-on: https://boringssl-review.googlesource.com/11325
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Change-Id: I73f9fd64b46f26978b897409d817b34ec9d93afd
Reviewed-on: https://boringssl-review.googlesource.com/11080
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This mirror's 2dc0204603 on the C side.
BUG=90
Change-Id: Iebb72df5a5ae98cb2fd8db519d973cd734ff05ea
Reviewed-on: https://boringssl-review.googlesource.com/11320
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is in preparation for implementing the version extension and is
probably what we should have done from the beginning as it makes
intolerance bugs simpler.
This means knobs like SendClientVersion and SendServerVersion deal with
the wire values while knobs like NegotiateVersion and MaxVersion deal
with logical versions. (This matches how the bugs have always worked.
SendFoo is just a weird post-processing bit on the handshake messages
while NegotiateVersion actually changes how BoGo behaves.)
BUG=90
Change-Id: I7f359d798d0899fa2742107fb3d854be19e731a4
Reviewed-on: https://boringssl-review.googlesource.com/11300
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This GREASEs cipher suites, groups, and extensions. For now, we'll
always place them in a hard-coded position. We can experiment with more
interesting strategies later.
If we add new ciphers and curves, presumably we prefer them over current
ones, so place GREASE values at the front. This prevents implementations
from parsing only the first value and ignoring the rest.
Add two new extensions, one empty and one non-empty. Place the empty one
in front (IBM WebSphere can't handle trailing empty extensions) and the
non-empty one at the end.
Change-Id: If2e009936bc298cedf2a7a593ce7d5d5ddbb841a
Reviewed-on: https://boringssl-review.googlesource.com/11241
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Get us a little bit more room here.
BUG=79
Change-Id: Ifadad94ead7794755a33f02d340111694b3572af
Reviewed-on: https://boringssl-review.googlesource.com/11228
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
That is an extremely confusing name. It should be NPN-Declined-TLS13.
Change-Id: I0e5fa50a3ddb0b80e88a8bc10d0ef87d0fff0a54
Reviewed-on: https://boringssl-review.googlesource.com/11227
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We recently added a three-connection option, but the transcripts were
still assuming just -Normal and -Resume.
Change-Id: I8816bce95dd7fac779af658e3eb86bc78bb95c91
Reviewed-on: https://boringssl-review.googlesource.com/11226
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Both the C and Go code were sampling the real clock. With this, two
successive iterations of runner transcripts give the same output.
Change-Id: I4d9e219e863881bf518c5ac199dce938a49cdfaa
Reviewed-on: https://boringssl-review.googlesource.com/11222
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We want to ensure -fuzzer passes tests, except for the tests it
intentionally fails on. This ensures that we don't lose our ability to
refresh the fuzzer transcripts.
Change-Id: I761856c30379a3934fd46a24627ef8415b136f93
Reviewed-on: https://boringssl-review.googlesource.com/11221
Reviewed-by: Adam Langley <agl@google.com>
Apparently we never wrote one of those. Also send a decrypt_error alert
to be consistent with all the other signature checks.
Change-Id: Ib5624d098d1e3086245192cdce92f5df26005064
Reviewed-on: https://boringssl-review.googlesource.com/11180
Reviewed-by: David Benjamin <davidben@google.com>
SSL_peek works fine for us, but OpenSSL 1.1.0 regressed this
(https://github.com/openssl/openssl/issues/1563), and we don't have
tests either. Fix this.
SSL_peek can handle all weird events that SSL_read can, so use runner
and tell bssl_shim to do a SSL_peek + SSL_peek + SSL_read instead of
SSL_read. Then add tests for all the events we may discover.
Change-Id: I9e8635e3ca19653a02a883f220ab1332d4412f98
Reviewed-on: https://boringssl-review.googlesource.com/11090
Reviewed-by: Adam Langley <agl@google.com>
The old numbers violate a MUST-level requirement in TLS 1.2 to not
advertise anonymous (0x0700 ends in 0x00). The spec has been updated
with new allocations which avoid these.
BUG=webrtc:6342
Change-Id: Ia5663ada98fa1ebf0f8a7f50fe74a0e9206c4194
Reviewed-on: https://boringssl-review.googlesource.com/11131
Reviewed-by: Adam Langley <agl@google.com>
Found by libFuzzer and then one more mistake caught by valgrind. Add a
test for this case.
Change-Id: I92773bc1231bafe5fc069e8568d93ac0df4c8acb
Reviewed-on: https://boringssl-review.googlesource.com/11129
Reviewed-by: David Benjamin <davidben@google.com>
This is in preparation for using the supported_versions extension to
experiment with draft TLS 1.3 versions, since we don't wish to restore
the fallback. With versions begin opaque values, we will want
version_from_wire to reject unknown values, not attempt to preserve
order in some way.
This means ClientHello.version processing needs to be separate code.
That's just written out fully in negotiate_version now. It also means
SSL_set_{min,max}_version will notice invalid inputs which aligns us
better with upstream's versions of those APIs.
This CL doesn't replace ssl->version with an internal-representation
version, though follow work should do it once a couple of changes land
in consumers.
BUG=90
Change-Id: Id2f5e1fa72847c823ee7f082e9e69f55e51ce9da
Reviewed-on: https://boringssl-review.googlesource.com/11122
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Passing --quiet makes valgrind only print out errors, so we don't need
to suppress things. Combine that with checking valgrind's dedicated exit
code so we notice errors that happen before the "---DONE---" marker.
This makes that marker unnecessary for valgrind. all_tests.go was not
sensitive to this, but still would do well to have valgrind be silent.
Change-Id: I841edf7de87081137e38990e647e989fd7567295
Reviewed-on: https://boringssl-review.googlesource.com/11128
Reviewed-by: Adam Langley <agl@google.com>
If the test failed due to non-ASan reasons but ASan also had errors,
output those too.
Change-Id: Id908fe2a823c59255c6a9585dfaa894a4fcd9f59
Reviewed-on: https://boringssl-review.googlesource.com/11127
Reviewed-by: Adam Langley <agl@google.com>
Runner needs to implement fuzzer mode as well so we can record
transcripts from it. A bunch of tests were failing:
- C and Go disagreed on what fuzzer mode did to TLS 1.3 padding. So we
fuzz more code, align Go with C. Fuzzer mode TLS 1.3 still pads but
just skips the final AEAD.
- The deterministic RNG should be applied per test, not per exchange. It
turns out, if your RNG is deterministic, one tends to pick the same
session ID over and over which confuses clients. (Resumption is
signaled by echoing the session ID.)
Now the only failing tests are the ones one would expect to fail.
BUG=79
Change-Id: Ica23881a6e726adae71e6767730519214ebcd62a
Reviewed-on: https://boringssl-review.googlesource.com/11126
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
If we see garbage in ClientHello.version and then select static RSA,
that garbage is what goes in the premaster.
Change-Id: I65190a44439745e6b5ffaf7669f063da725c8097
Reviewed-on: https://boringssl-review.googlesource.com/11092
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Plain PSK omits the ServerKeyExchange when there is no hint and includes
it otherwise (it should have always sent it), while other PSK ciphers
like ECDHE_PSK cannot omit the hint. Having different capabilities here
is odd and RFC 4279 5.2 suggests that all PSK ciphers are capable of
"[not] provid[ing] an identity hint".
Interpret this to mean no identity hint and empty identity hint are the
same state. Annoyingly, this gives a plain PSK implementation two
options for spelling an empty hint. The spec isn't clear and this is not
really a battle worth fighting, so I've left both acceptable and added a
test for this case.
See also https://android-review.googlesource.com/c/275217/. This is also
consistent with Android's PskKeyManager API, our only consumer anyway.
https://developer.android.com/reference/android/net/PskKeyManager.html
Change-Id: I8a8e6cc1f7dd1b8b202cdaf3d4f151bebfb4a25b
Reviewed-on: https://boringssl-review.googlesource.com/11087
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
access_denied is only used to indicate client cert errors and Chrome
maps it to ERR_SSL_BAD_CLIENT_AUTH_CERT accordingly:
access_denied
A valid certificate was received, but when access control was
applied, the sender decided not to proceed with negotiation. This
message is always fatal.
We don't appear to be the cause of Chrome's recent
ERR_SSL_BAD_CLIENT_AUTH_CERT spike, but we should send these correctly
nonetheless.
If the early callback fails, handshake_failure seems the most
appropriate ("I was unable to find suitable parameters"). There isn't
really an alert that matches DoS, but internal_error seems okay?
internal_error
An internal error unrelated to the peer or the correctness of the
protocol (such as a memory allocation failure) makes it impossible
to continue. This message is always fatal.
There's nothing wrong, per se, with your ClientHello, but I just can't
deal with it right now. Please go away.
Change-Id: Icd1c998c09dc42daa4b309c1a4a0f136b85eb69d
Reviewed-on: https://boringssl-review.googlesource.com/11084
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
I'm not sure what happened here. These are both the same as
MissingKeyShare-Client.
Change-Id: I6601ed378d8639c1b59034f1e96c09a683bb62ca
Reviewed-on: https://boringssl-review.googlesource.com/11007
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
It's easy to forget to check those. Unfortunately, it's also easy to
forget to check inner structures, which is going to be harder to stress,
but do these to start with. In doing, so fix up and unify some
error-handling, and add a missing check when parsing TLS 1.2
CertificateRequest.
This was also inspired by the recent IETF posting.
Change-Id: I27fe3cd3506258389a75d486036388400f0a33ba
Reviewed-on: https://boringssl-review.googlesource.com/10963
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This will let us use the same test scenarios for testing messages with
trailing garbage or skipped messages.
Change-Id: I9f177983e8dabb6c94d3d8443d224b79a58f40b1
Reviewed-on: https://boringssl-review.googlesource.com/10962
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This mechanism is incompatible with deploying draft versions of TLS 1.3.
Suppose a draft M client talks to a draft N server, M != N. (Either M or
N could also be the final standard revision should there be lingering
draft clients or servers.) The server will notice the mismatch and
pretend ClientHello.version is TLS 1.2, not TLS 1.3. But this will
trigger anti-downgrade signal and cause an interop failure! And if it
doesn't trigger, all the clever tricks around ServerHello.random being
signed in TLS 1.2 are moot.
We'll put this back when the dust has settled.
Change-Id: Ic3cf72b7c31ba91e5cca0cfd7a3fca830c493a43
Reviewed-on: https://boringssl-review.googlesource.com/11005
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Not that this matters in the slightest, but the recent IETF mailing
reminded me we don't test this.
Change-Id: I300c96d6a63733d538a7019a7cb74d4e65d0498f
Reviewed-on: https://boringssl-review.googlesource.com/10961
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Although RFC 6066 recommends against it, some servers send a warning
alert prior to ServerHello on SNI mismatch, and, per spec, TLS 1.2
allows it.
We're fine here, but add a test for it. It interacts interestingly with
TLS 1.3 forbidding warning alerts because it happens before version
negotiation.
Change-Id: I0032313c986c835b6ae1aa43da6ee0dad17a97c2
Reviewed-on: https://boringssl-review.googlesource.com/10800
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Add a test that RSA-PSS is available in TLS 1.2 by default, both for
signing and verifying. Note that if a custom SSL_PRIVATE_KEY_METHOD is
used and it sets signing preferences, it won't use RSA-PSS if it doesn't
know about it. (See *-Sign-Negotiate-* tests.)
Change-Id: I3776a0c95480188a135795f7ebf31f2b0e0626cc
Reviewed-on: https://boringssl-review.googlesource.com/10723
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Changing parameters on renegotiation makes all our APIs confusing. This
one has no reason to change, so lock it down. In particular, our
preference to forbid Token Binding + renego may be overridden at the
IETF, even though it's insane. Loosening it will be a bit less of a
headache if EMS can't change.
https://www.ietf.org/mail-archive/web/unbearable/current/msg00690.html
claims that this is already in the specification and enforced by NSS. I
can't find anything to this effect in the specification. It just says
the client MUST disable renegotiation when EMS is missing, which is
wishful thinking. At a glance, NSS doesn't seem to check, though I could
be misunderstanding the code.
Nonetheless, locking this down is a good idea anyway. Accurate or not,
take the email as an implicit endorsement of this from Mozilla.
Change-Id: I236b05991d28bed199763dcf2f47bbfb9d0322d7
Reviewed-on: https://boringssl-review.googlesource.com/10721
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
For now, they can be restored by compiling with -DBORINGSSL_RC4_TLS.
Of note, this means that `MEDIUM' is now empty.
Change-Id: Ic77308e7bd4849bdb2b4882c6b34af85089fe3cc
Reviewed-on: https://boringssl-review.googlesource.com/10580
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Matt Braithwaite <mab@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Matt Braithwaite <mab@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
To ease the removal of RC4, use 3DES in cases where RC4 is not required,
but is just a placeholder for "ciphersuite that works in SSLv3."
Change-Id: Ib459173e68a662986235b556f330a7e0e02759d7
Reviewed-on: https://boringssl-review.googlesource.com/10523
Commit-Queue: Matt Braithwaite <mab@google.com>
Reviewed-by: Matt Braithwaite <mab@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Apparently we forgot to do this.
Change-Id: I348cf6d716ae888fddce69ba4801bf09446f5a72
Reviewed-on: https://boringssl-review.googlesource.com/10503
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
However, for now, we will only enable it if TLS 1.3 is offered.
BUG=85
Change-Id: I958ae0adeafee553dbffb966a6fa41f8a81cef96
Reviewed-on: https://boringssl-review.googlesource.com/10342
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
In TLS 1.3 draft 14, due to resumption using a different cipher, this
is actually not too hard to mess up. (In fact BoGo didn't quite get it
right.)
Fortunately, the new cipher suite negotiation in draft 15 should make
this reasonable again once we implement it. In the meantime, test it.
Change-Id: I2eb948eeaaa051ecacaa9095b66ff149582ea11d
Reviewed-on: https://boringssl-review.googlesource.com/10442
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Change-Id: I2e1ee319bb9852b9c686f2f297c470db54f72279
Reviewed-on: https://boringssl-review.googlesource.com/10370
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
BUG=84
Change-Id: Ie5eaefddd161488996033de28c0ebd1064bb793d
Reviewed-on: https://boringssl-review.googlesource.com/10484
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
9498e74 changed the default value of verify_result to an error. This
tripped up NGINX, which depends on a bug[1] in OpenSSL. netty-tcnative
also uses this behavior, though it currently isn't tripped up by 9498e74
because it calls |SSL_set_verify_result|. However, we would like to
remove |SSL_set_verify_result| and with two data points, it seems this
is behavior we must preserve.
This change sets |verify_result| to |X509_V_OK| when a) no client
certificate is requested or b) none is given and it's optional.
[1] See BUGS in https://www.openssl.org/docs/manmaster/ssl/SSL_get_verify_result.html
Change-Id: Ibd33660ae409bfe272963a8c39b7e9aa83c3d635
Reviewed-on: https://boringssl-review.googlesource.com/9067
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Also fix up those tests as they were a little confused. It is always the
shim that signs and has a configured certificate in these tests.
BUG=95
Change-Id: I57a6b1bad19986c79cd30aaa6cf3b8ca307ef8b2
Reviewed-on: https://boringssl-review.googlesource.com/10444
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
One less thing to keep track of.
https://github.com/tlswg/tls13-spec/pull/549 got merged.
Change-Id: Ide66e547140f8122a3b8013281be5215c11b6de0
Reviewed-on: https://boringssl-review.googlesource.com/10482
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The TLS 1.3 state machine is actually less in need of the aggressive
state machine coverage tests, but nonetheless, we should cover all
handshake shapes. PSK resumption and HelloRetryRequest were missing.
We were also accidentally running "DTLS" versions of the TLS 1.3 tests
but silently running TLS 1.2.
Change-Id: I65db4052b89d770db7e47738e73aaadde9634236
Reviewed-on: https://boringssl-review.googlesource.com/10441
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Right now the logic happens twice which is a nuisance.
Change-Id: Ia8155ada0b4479b2ca4be06152b8cd99816e14e8
Reviewed-on: https://boringssl-review.googlesource.com/10440
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Some version mismatch cases were not being covered due to TLS 1.2 and
TLS 1.3 having very different spellings for tickets resumption. Also
explicitly test that TLS 1.2 tickets aren't offered in the TLS 1.3 slot
and vice versa.
Change-Id: Ibe58386ea2004fb3c1af19342b8d808f13f737a9
Reviewed-on: https://boringssl-review.googlesource.com/10183
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
BUG=75
Change-Id: Ied864cfccbc0e68d71c55c5ab563da27b7253463
Reviewed-on: https://boringssl-review.googlesource.com/9043
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Much of the ClientHello logic queries hello.vers. To avoid it getting
confused, do all modifications right at the end, otherwise
SendClientVersion also affects whether the key share is sent.
Change-Id: I8be2a4a9807ef9ad88af03971ea1c37e4ba36b9c
Reviewed-on: https://boringssl-review.googlesource.com/10341
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The server should not be allowed select a protocol that wasn't
advertised. Callers tend to not really notice and act as if some default
were chosen which is unlikely to work very well.
Change-Id: Ib6388db72f05386f854d275bab762ca79e8174e6
Reviewed-on: https://boringssl-review.googlesource.com/10284
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
These are probably a good idea to ship so long as we have the PSK
callbacks at all, but they're not *completely* standard yet and Android
tests otherwise need updating to know about them. We don't care enough
about PSK to be in a rush to ship them, and taking them out is an easier
default action until then.
Change-Id: Ic646053d29b69a114e2efea61d593d5e912bdcd0
Reviewed-on: https://boringssl-review.googlesource.com/10225
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
If cert_cb runs asynchronously, we end up repeating a large part of very
stateful ClientHello processing. This seems to be mostly fine and there
are few users of server-side cert_cb (it's a new API in 1.0.2), but it's
a little scary.
This is also visible to external consumers because some callbacks get
called multiple times. We especially should try to avoid that as there
is no guarantee that these callbacks are idempotent and give the same
answer each time.
Change-Id: I212b2325eae2cfca0fb423dace101e466c5e5d4e
Reviewed-on: https://boringssl-review.googlesource.com/10224
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is more progress in letting other stacks use the test runner.
You can provide a per-shim configuration file that includes:
- A list of test patterns to be suppressed (presumably because
they don't work). This setting is ignored if -test is used.
- A translation table of expected errors to shim-specific errors.
BUG=92
Change-Id: I3c31d136e35c282e05d4919e18ba41d44ea9cf2a
Reviewed-on: https://boringssl-review.googlesource.com/9161
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We handle this correctly but never wrote a test for it. Noticed this in
chatting about the second ClientHello.version bug workaround with Eric
Rescorla.
Change-Id: I09bc6c995d07c0f2c9936031b52c3c639ed3695e
Reviewed-on: https://boringssl-review.googlesource.com/9154
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
tls13_process_certificate can take a boolean for whether anonymous is
allowed. This does change the error on the client slightly, but I think
this is correct anyway. It is not a syntax error for the server to send
no certificates in so far as the Certificate message allows it. It's
just illegal.
Change-Id: I1af80dacf23f50aad0b1fbd884bc068a40714399
Reviewed-on: https://boringssl-review.googlesource.com/9072
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
