crypto/tls: extend coverage of BoGo test suite #72006
Comments
|
Change https://go.dev/cl/650715 mentions this issue: |
|
Change https://go.dev/cl/650735 mentions this issue: |
|
Change https://go.dev/cl/650735 mentions this issue: |
|
Change https://go.dev/cl/650736 mentions this issue: |
|
Change https://go.dev/cl/650716 mentions this issue: |
|
Related Issues Related Code Changes
(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.) |
gabyhelp
added
the
Implementation
|
Change https://go.dev/cl/650737 mentions this issue: |
|
Change https://go.dev/cl/650717 mentions this issue: |
|
Change https://go.dev/cl/652995 mentions this issue: |
|
Change https://go.dev/cl/650738 mentions this issue: |
|
Change https://go.dev/cl/652997 mentions this issue: |
seankhliao
changed the title
1. onResumeShimWritesFirst is unused, replace the binding with an underscore. 2. in the bogoShim() function when looping through resumeCount+1 the tlsConn read for loop only breaks for non-nil err, so there's no need to check that again after the loop body. Updates #72006 Change-Id: Ieff45d26df33d71003a2509ea5b2b06c5fa0e1d6 Reviewed-on: https://go-review.googlesource.com/c/go/+/650715 Reviewed-by: Roland Shoemaker <roland@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Michael Pratt <mpratt@google.com>
While not clearly motivated by normative language in RFC 8446 it seems clear that an empty opaque ticket value is non-operable, and so we should reject it with an appropriate alert/error. This allows removing the SendEmptySessionTicket-TLS13 BoGo test from the bogo excluded tests configuration. Fixes #70513 Updates #72006 Change-Id: I589b34e86fb1eb27a349a230e920c22284597cde Reviewed-on: https://go-review.googlesource.com/c/go/+/650735 Reviewed-by: Filippo Valsorda <filippo@golang.org> Reviewed-by: Roland Shoemaker <roland@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: David Chase <drchase@google.com> Auto-Submit: Daniel McCarney <daniel@binaryparadox.net>
This commit removes SkipNewSessionTicket from the bogo_config.json excluded tests list. Previously this test was being skipped with a TODO that there might be a bug here. In practice it seems like there's no bug and the test is handled correctly by crypto/tls. When activated, a TLS 1.2 client connecting to the bogo dispatcher goes through the normal handshake process with the exception that the server skips sending the NewSessionTicket msg expected by the client in response to the client's final flight of handshake msgs. The crypto/tls TLS 1.2 client_handshake.go logic correctly rejects the unexpected message that follows (ChangeCipherSpec) when trying to read the bytes necessary to unmarshal the expected NewSessionTicket message that was omitted. Updates #72006 Change-Id: I9faea4d18589d10b163211aa17b2d0da8af1187e Reviewed-on: https://go-review.googlesource.com/c/go/+/650736 Reviewed-by: Junyang Shao <shaojunyang@google.com> Auto-Submit: Daniel McCarney <daniel@binaryparadox.net> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Roland Shoemaker <roland@golang.org>
When encountering alertUserCanceled in a TLS 1.3 handshake, ignore the alert and retry reading a record. This matches existing logic for how TLS 1.2 alertLevelWarning alerts are handled. For broader context, TLS 1.3 removed warning-level alerts except for alertUserCanceled (RFC 8446, § 6.1). Since at least one major implementation (https://bugs.openjdk.org/browse/JDK-8323517) misuses this alert, many TLS stacks now ignore it outright when seen in a TLS 1.3 handshake (e.g. BoringSSL, NSS, Rustls). With the crypto/tls behaviour changed to match peer implementations we can now enable the "SendUserCanceledAlerts-TLS13" BoGo test. "SendUserCanceledAlerts-TooMany-TLS13" remains ignored, because like "SendWarningAlerts*" fixing the test requires some general spam protocol message enhancements be done first. Updates #72006 Change-Id: I570c1fa674b5a4760836c514d35ee17f746fe28d Reviewed-on: https://go-review.googlesource.com/c/go/+/650716 Reviewed-by: Michael Pratt <mpratt@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Roland Shoemaker <roland@golang.org>
When this command line flag is provided to the BoGo runner it will: * Disable some timeouts * Limit concurrency to 1 worker at a time * Pass the -wait-for-debugger flag to the shim process * Print the PID of the shim process to status output On the shim-side, we need to react to -wait-for-debugger by sending ourselves a SIGSTOP signal. When a debugger attaches to the shim the process will be resumed. This makes it possible to debug both the runner side and the shim side of a BoGo interaction without resorting to print style debugging. Since SIGSTOP is not a signal we can use on Windows this functionality is limited to unix builds. Updates #72006 Change-Id: Iafa08cf71830cdfde3e6ee4826914236e3cd7e57 Reviewed-on: https://go-review.googlesource.com/c/go/+/650737 Reviewed-by: Roland Shoemaker <roland@golang.org> Reviewed-by: Junyang Shao <shaojunyang@google.com> Reviewed-by: Filippo Valsorda <filippo@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Previously this test was skipped without a comment clarifying why. In practice it's because crypto/tls doesn't generate GREASE extensions at this time, and the test expects to find one in the NewSessionTicket message extensions produced by a server. We're already skipping some other GREASE related test as not-yet-implemented without explicit bogo_config.json exclusion by way of the -enable-grease flag not being implemented, however for TLS 1.3 servers the BoGo expectation is that they _always_ send GREASE, and so the -enable-grease flag isn't provided and an explicit skip must be used. We should revisit this alongside implementing GREASE ext production in general for both clients and servers. Updates #72006 Change-Id: I8af4b555ac8c32cad42215fbf26aa0feae90fa21 Reviewed-on: https://go-review.googlesource.com/c/go/+/650717 Reviewed-by: Roland Shoemaker <roland@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Junyang Shao <shaojunyang@google.com>
For malformed client/server certificates in a TLS handshake send a decode_error alert, matching BoringSSL behaviour. Previously crypto/tls used a bad_certificate alert for this purpose. The TLS specification is imprecise enough to allow this to be considered a spec. justified choice, but since all other places in the protocol encourage using decode_error for structurally malformed messages we may as well do the same here and get some extra cross-impl consistency for free. This also allows un-ignoring the BoGo GarbageCertificate-[Client|Server]-[TLS12|TLS13] tests. Updates #72006 Change-Id: Ide45ba1602816e71c3289a60e77587266c3b9036 Reviewed-on: https://go-review.googlesource.com/c/go/+/652995 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Junyang Shao <shaojunyang@google.com> Reviewed-by: Roland Shoemaker <roland@golang.org>
If we weren't resuming an existing session, and we constructed a TLS 1.3 compatible client hello, ensure the server doesn't echo back the made up compatibility session ID if we end up handshaking for TLS 1.2. As part of an effort to make the initial stages of a TLS 1.3 handshake compatible with TLS 1.2 middleboxes, TLS 1.3 requires that the client hello contain a non-empty legacy_session_id value. For anti-ossification purposes it's recommended this ID be randomly generated. This is the strategy the crypto/tls package takes. When we follow this approach, but then end up negotiating TLS 1.2, the server should not have echoed back that random ID to us. It's impossible for the server to have had a session with a matching ID and so it is misbehaving and it's prudent for our side to abort the handshake. See RFC 8446 Section 4.1.2 for more detail: https://www.rfc-editor.org/rfc/rfc8446#section-4.1.2 Adopting this behaviour allows un-ignoring the BoGo EchoTLS13CompatibilitySessionID testcase. Updates #72006 Change-Id: I1e52075177a13a7aa103b45498eae38d8a4c34b9 Reviewed-on: https://go-review.googlesource.com/c/go/+/652997 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Roland Shoemaker <roland@golang.org> Reviewed-by: Junyang Shao <shaojunyang@google.com> Reviewed-by: Filippo Valsorda <filippo@golang.org>
|
Before signing in to GitHub, read the linked issue leoDOTorg in German conventional letter With the site link https://go.dev/doc/install I need a ticket. |
|
Change https://go.dev/cl/669175 mentions this issue: |
|
Change https://go.dev/cl/669156 mentions this issue: |
|
Change https://go.dev/cl/669158 mentions this issue: |
|
Change https://go.dev/cl/669155 mentions this issue: |
|
Change https://go.dev/cl/669157 mentions this issue: |
|
Change https://go.dev/cl/671195 mentions this issue: |
|
Change https://go.dev/cl/671235 mentions this issue: |
|
"Filing a Ticket" button: comment: has the filing a ticket button ever been in use? |
|
The sentence includes... by filing a ticket on our issue tracker. For. . . This commit updates the client hello processing logic to allow this ... processing - the behaviour could be that the sentence is a readable text content. The word client is without ampersands for readability as a proofreader because it is not shortcutted to a Webassambly token. For a read and write commit change if TLS 1.3 is not in use. What is it at all? |
|
This commit the client hello logic |
|
I am a host, and the services on a server are set as text to the client without a web assembly token. |
|
Change https://go.dev/cl/671415 mentions this issue: |
cherrymui
added
the
NeedsFix
Previously if the clientHandshakeState for the TLS 1.2 client code encountered a server helo message that contained a compression method other than compressionNone, we would emit an unexpected message alert. Instead, it seems more appropriate to return an illegal parameter alert. The server hello message _was_ expected, it just contained a bad parameter option. Making this change also allows enabling the InvalidCompressionMethod bogo test. Updates #72006 Change-Id: I27a2cd231e4b8762b0d9e2dbd3d8ddd5b87fd5c3 Reviewed-on: https://go-review.googlesource.com/c/go/+/669155 Reviewed-by: Roland Shoemaker <roland@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Cherry Mui <cherryyz@google.com>
Unlike in earlier TLS versions, in TLS 1.3 when processing a server hello the legacy_compression_method MUST have the value 0. It is no longer a parameter that offers a choice of compression method. With this in mind, it seems more appropriate to return a decode error when we encounter a non-zero compression method in a server hello message. We haven't found a parameter value we reject, we've found a message that doesn't decode according to its specification. Making this change also aligns with BoringSSL and allows enabling the TLS13-HRR-InvalidCompressionMethod bogo test. Updates #72006 Change-Id: I27a2cd231e4b8762b0d9e2dbd3d8ddd5b87fd5c4 Reviewed-on: https://go-review.googlesource.com/c/go/+/669156 Reviewed-by: Roland Shoemaker <roland@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Cherry Mui <cherryyz@google.com>
If a client or server explicitly offers point formats, and the point formats don't include the uncompressed format, then error. This matches BoringSSL and Rustls behaviour and allows enabling the PointFormat-Client-MissingUncompressed bogo test. Updates #72006 Change-Id: I27a2cd231e4b8762b0d9e2dbd3d8ddd5b87fd5c5 Reviewed-on: https://go-review.googlesource.com/c/go/+/669157 Reviewed-by: Roland Shoemaker <roland@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
These two bogo tests mutate the version number used for the premaster secret calculation for a client RSA key exchange, with the expectation the server rejects the handshake. Per the comment in the end of rsaKeyAgreement.processClientKeyExchange we explicitly choose *not* to verify the version number. This commit adds the two version number tests to the ignore list. They coincidentally happen to produced the expected failure because they use a non-default ciphersuite. When we add this ciphersuite to the client config for the bogo test they will start to fail unless ignored. Updates #72006 Change-Id: I27a2cd231e4b8762b0d9e2dbd3d8ddd5b87fd5c6 Reviewed-on: https://go-review.googlesource.com/c/go/+/669175 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: Roland Shoemaker <roland@golang.org>
Previously a handful of large record tests were in the bogo config ignore list. The ignored tests were failing because they used insecure ciphersuites that aren't enabled by default. This commit adds the non-default insecure ciphersuites to the bogo TLS configuration and re-enables the tests. Doing this uncovered a handful of unrelated tests that needed to be fixed, each handled before this commit. Updates #72006 Change-Id: I27a2cd231e4b8762b0d9e2dbd3d8ddd5b87fd5c7 Reviewed-on: https://go-review.googlesource.com/c/go/+/669158 Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: Roland Shoemaker <roland@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Previously for protocol versions older than TLS 1.3 our server handshake implementation sent an alertBadCertificate alert in the case where the server TLS config indicates a client cert is required and none was received. This commit updates the relevant logic to instead send alertHandshakeFailure in these circumstances. For TLS 1.2, RFC 5246 §7.4.6 unambiguously describes this as the correct alert: If the client does not send any certificates, the server MAY at its discretion either continue the handshake without client authentication, or respond with a fatal handshake_failure alert. The TLS 1.1 and 1.0 specs also describe using this alert (RFC 4346 §7.4.6 and RFC 2246 §7.4.6) both say: If client authentication is required by the server for the handshake to continue, it may respond with a fatal handshake failure alert. Making this correction also allows enabling the RequireAnyClientCertificate-TLS1* bogo tests. Updates #72006 Change-Id: I27a2cd231e4b8762b0d9e2dbd3d8ddd5b87fd5c8 Reviewed-on: https://go-review.googlesource.com/c/go/+/671195 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: Roland Shoemaker <roland@golang.org>
If the client hello legacy version is >= TLS 1.3, and no supported_versions extension is sent, negotiate TLS 1.2 or lower when supported. On the topic of supported version negotiation RFC 8446 4.2.1 indicates TLS 1.3 implementations MUST send a supported_versions extension with a list of their supported protocol versions. The crypto/tls package enforces this when the client hello legacy version indicates TLS 1.3 (0x0304), aborting the handshake with an alertMissingExtension alert if no supported_versions were received. However, section 4.2.1 indicates different behaviour should be used when the extension is not present and TLS 1.2 or prior are supported: If this extension is not present, servers which are compliant with this specification and which also support TLS 1.2 MUST negotiate TLS 1.2 or prior as specified in [RFC5246], even if ClientHello.legacy_version is 0x0304 or later. This commit updates the client hello processing logic to allow this behaviour. If no supported_versions extension was received we ignore the legacy version being >= TLS 1.3 and instead negotiate a lower supported version if the server configuration allows. This fix in turn allows enabling the BoGo ClientHelloVersionTooHigh, MinorVersionTolerance, and MajorVersionTolerance tests. Updates #72006 Change-Id: I27a2cd231e4b8762b0d9e2dbd3d8ddd5b87fd5c9 Reviewed-on: https://go-review.googlesource.com/c/go/+/671235 Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: Roland Shoemaker <roland@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
Change https://go.dev/cl/673236 mentions this issue: |
The BoGo IgnoreClientVersionOrder test checks that a client that sends a supported_versions extension with the list [TLS 1.2, TLS 1.3] ends up negotiating TLS 1.3. However, the crypto/tls module treats this list as being in client preference order, and so negotiates TLS 1.2, failing the test. Our behaviour appears to be the correct handling based on RFC 8446 §4.2.1 where it says: The extension contains a list of supported versions in preference order, with the most preferred version first. This commit updates the reason we skip this test to cite the RFC instead of saying it's something to be fixed. Updates #72006 Change-Id: I27a2cd231e4b8762b0d9e2dbd3d8ddd5b87fd5ca Reviewed-on: https://go-review.googlesource.com/c/go/+/671415 Reviewed-by: Roland Shoemaker <roland@golang.org> Reviewed-by: Michael Knyszek <mknyszek@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Daniel McCarney <daniel@binaryparadox.net>
|
Change https://go.dev/cl/673736 mentions this issue: |
|
Change https://go.dev/cl/673735 mentions this issue: |
|
Change https://go.dev/cl/673756 mentions this issue: |
|
Change https://go.dev/cl/673737 mentions this issue: |
|
Change https://go.dev/cl/673757 mentions this issue: |
|
Change https://go.dev/cl/673738 mentions this issue: |
|
Hello. My investigations on the crime scene on the Internet are concluded with a so-called mass. The mentioned interpolation key is not to introduce variable spaces for personal word creations. The tutorial for GO!lang does not mention that with a Windows installation, the interpolation symbol turns the cmd not into a bash. |
In #51434 a shim for the BoringSSL BoGo test suite, and a unit test to drive it, were added to
crypto/tls. Seebogo_shim_test.go. To limit the scope of that initial work many tests were disabled in the associated JSON config.To maximize the value of this test shim we should work through this list and either provide a clear reason for why skipping the test makes sense based on some additional context (missing features, protocol ambiguity, etc) or implement the required fixes to enable the test.
The text was updated successfully, but these errors were encountered: