Implement TLSFlags extension #151

jhoyla · 2023-08-07T20:22:57Z

This PR implements the first half of the TLS Flags extension.

bwesterb · 2023-08-07T20:30:33Z

It needs some tests.

cjpatton

+1 to tests. We should at least test that, when the flags are enabled by the client in crypto/tls.Config, the server sees the value in the ClientHello.

Another high-level question: How does this interact with ECH?

Is the the extension present in both the inner and outer handshake?
If so, should it appear only in the inner handshake?

cjpatton · 2023-08-07T20:43:01Z

src/crypto/tls/common.go

@@ -851,6 +858,8 @@ type Config struct {
 	// See https://tools.ietf.org/html/draft-ietf-tls-subcerts.
 	SupportDelegatedCredential bool



Please document here what TLSFlagSet is.

TLSFlagsSupported still needs to be documented. It appears that it controls both the client and server-supported flags.

You should document that this is currently limited to controlling TLS Flags in the Server Hello. The spec also allows EncryptedExtensions, Certificate, Hello Retry Request.

cjpatton · 2023-08-07T20:44:30Z

src/crypto/tls/common.go

+type TLSFlag uint16
+
+const (
+	FlagSupportMTLS TLSFlag = 0x50


Please document what this is. IIRC this is not yet a codepoint that's documented? If so, we should mark it as experimental in the API.

Suggested change

FlagSupportMTLS TLSFlag = 0x50

/// ExperimentalFlagSuppportMTLS ...

ExperimentalFlagSupportMTLS TLSFlag = 0x50

Is it intentional that this flag occupies 11 bytes? 0x50 is 80 decimal, and since we start counting at 0, this occupies ceil(81 / 8) = 11 bytes.

bwesterb · 2023-08-07T21:15:34Z

Another high-level question: How does this interact with ECH?

Very good question.

jhoyla · 2023-08-07T23:27:06Z

Added code for server side.
Tests to come tomorrow.

cjpatton · 2023-08-08T16:12:32Z

Another high-level question: How does this interact with ECH?
1. Is the the extension present in both the inner and outer handshake?

2. If so, should it appear only in the inner handshake?

Currently the extension will be added to the inner handshake only: https://github.com/cloudflare/go/blob/cf/src/crypto/tls/ech.go#L81-L85

Only specific extensions are copied into the outer handshake: https://github.com/cloudflare/go/blob/cf/src/crypto/tls/ech.go#L92-L106

In my opinion, this is the correct behavior, assuming conservatively that the value of the TLS flags extension is privacy sensitive: https://www.ietf.org/archive/id/draft-ietf-tls-esni-16.html#name-outer-clienthello

Note that since it appears in the inner handshake, it will be used by the server to terminate the connection. OTOH, if ECH is rejected, then it won't be used by the server to terminate the connection.

Lekensteyn · 2023-09-19T16:24:31Z

(Rebased on the cf branch based on Go 1.21.1, no other changes.)

Lekensteyn · 2023-09-19T16:34:02Z

src/crypto/tls/handshake_messages.go

+					return false
+				}
+				m.tlsFlags = append(m.tlsFlags, flagByte)
+			}


I think that this whole block can be simplified to:

var flagsList cryptobyte.String if !extData.ReadUint8LengthPrefixed(&flagsList) || flagsList.Empty() { return false } m.tlsFlags = flagsList

That would save some unnecessary memory allocations as well by avoiding the append. A cryptobyte.String is an alias for []byte: https://pkg.go.dev/golang.org/x/crypto/cryptobyte#String

Lekensteyn · 2023-09-19T17:37:18Z

src/crypto/tls/handshake_server_tls13.go

@@ -318,6 +319,23 @@ GroupSelection:
 		c.sendAlert(alertIllegalParameter)
 		return errors.New("tls: invalid client key share")
 	}
+	if len(hs.clientHello.tlsFlags) != 0 {


Missing check: hs.clientHello.tlsFlags[0] != 0 or else you need to c.sendAlert(alertIllegalParameter).

To comply with: "An implementation that receives an all-zero value for this extension or a value that contains trailing zero bytes MUST generate a fatal illegal_parameter alert."

Lekensteyn · 2023-09-19T17:38:16Z

src/crypto/tls/handshake_server_tls13.go

+	if len(hs.clientHello.tlsFlags) != 0 {
+		supportedFlags, err := encodeFlags(hs.c.config.TLSFlagsSupported)
+		if err != nil {
+			return errors.New("tls: invalid server flags")


Perhaps you should also send an internal error alert here?

Lekensteyn · 2023-09-19T17:44:20Z

src/crypto/tls/common.go

@@ -465,6 +466,12 @@ const (
 	ECDSAWithSHA1 SignatureScheme = 0x0203
 )

+type TLSFlag uint16


Let's make clear that this is the flag number (index) rather than the raw representation:

Suggested change

type TLSFlag uint16

// TLSFlag represents the flag number.

type TLSFlag uint16

Lekensteyn · 2023-09-19T17:47:41Z

src/crypto/tls/common.go

@@ -125,6 +125,7 @@ const (
 	extensionRenegotiationInfo       uint16 = 0xff01
 	extensionECH                     uint16 = 0xfe0d // draft-ietf-tls-esni-13
 	extensionECHOuterExtensions      uint16 = 0xfd00 // draft-ietf-tls-esni-13
+	extensionTLSFlags                uint16 = 0xfe01 // draft-ietf-tls-tlsflags-12


0xfe01 (65025) belongs to the "Unassigned" namespace. Unless we have it assigned, we should probably stick to the "Reserved for Private Use" namespace which has a range starting at 0xff02 (65282).

https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#tls-extensiontype-values-1

Lekensteyn · 2023-09-19T17:48:19Z

src/crypto/tls/common.go

@@ -851,6 +858,8 @@ type Config struct {
 	// See https://tools.ietf.org/html/draft-ietf-tls-subcerts.
 	SupportDelegatedCredential bool



TLSFlagsSupported still needs to be documented. It appears that it controls both the client and server-supported flags.

You should document that this is currently limited to controlling TLS Flags in the Server Hello. The spec also allows EncryptedExtensions, Certificate, Hello Retry Request.

Lekensteyn · 2023-09-19T17:52:02Z

src/crypto/tls/handshake_server_tls13.go

+				flagNo := byteIndex*8 + i
+				if flagNo >= int(maxTLSFlag) {
+					return nil, fmt.Errorf("TLS flag is out of range: %d", flagNo)
+				}


Optimization: This check can be removed by checking outside the outer loop. Fail on len(flagBytes) > 255 or alternatively len(flagBytes) * 8 > maxTLSFlag.

Lekensteyn · 2023-09-19T17:55:32Z

src/crypto/tls/common.go

+type TLSFlag uint16
+
+const (
+	FlagSupportMTLS TLSFlag = 0x50


Is it intentional that this flag occupies 11 bytes? 0x50 is 80 decimal, and since we start counting at 0, this occupies ceil(81 / 8) = 11 bytes.

Add README Co-authored-by: Peter Wu <pwu@cloudflare.com>

Include the `-cf` tag so this fork can be identified. Include the `devel` tag such that we can potentially add new APIs to the api/next.txt file in order to please the TestDependencies test.

This allows applications to use build tags to maintain compatibility with both this fork as well as standard Go. [ bas 2023-10-6: Fix cfgo build tag and add a test (#156) ]

Tests can be run from the repo with: `docker-compose run test`

[pwu: Go 1.22.0: resolve conflicts: git rm -r .github/ISSUE_TEMPLATE]

The "API check" test requires new APIs to be tracked in api/go1.X.txt or api/next/X.txt. Since Go 1.19 (commit b7041c7), every line in these files also need a comment with the approval issue number. To reduce development friction, we disable the requirement of updating these files when the `-cf` tag is present in the VERSION file.

Add basic support for handshake metrics: * Adds the ability to set a callback via the CFEventHandlerContextKey context value on the handshake context. It will be called at various points during the handshake to respond to various events. See #146. * Use this callback to expose client and server intra-handshake state machine durations, respectively. Each event records elapsed timestamps (durations) for relevant events during the course of a connection, such as reading and writing handshake messages of interest. This will be useful for recording intra-stack costs of TLS extensions such as ECH and KEMTLS. [pwu: Go 1.20.4: moved Config.CFEventHandler to context value] [pwu: Go 1.20.4: moved CFEvent code from tls_cf.go to cfevent.go]

This patch adds: - X25519Kyber768Draft00, this is the de facto standard for early deployment, see https://mailarchive.ietf.org/arch/msg/tls/HAWpNpgptl--UZNSYuvsjB-Pc2k/ https://datatracker.ietf.org/doc/draft-tls-westerbaan-xyber768d00/02/ - X25519Kyber768Draft00Old, which is the same as the previous, but under an old identifiers. - X25519Kyber512Draft00. This should only be used for testing, whether the smaller shares are advantageous. - P256Kyber768Draft00. Uses a non-standard identifier. Should not be used. Adds CFEvents to detect `HelloRetryRequest`s and to signal which key agreement was used. Co-authored-by: Christopher Wood <caw@heapingbits.net> Co-authored-by: Peter Wu <pwu@cloudflare.com>

To avoid having to regenerate all testdata files, add an option to control whether PQ signature algorithms are advertised. Tests were added for the client side. Since Go 1.19, FIPS-only mode must remain disabled to enable PQ sigalgs. [pwu: Go 1.17: moved parsePublicKey changes from x509/x509.go to x509/parser.go] Co-authored-by: Christopher Patton <chrispatton@gmail.com> Co-authored-by: Peter Wu <pwu@cloudflare.com>

* Define API for delegated credentials so they are fetched using the same mechanisms used to fetch certificates * Allow the usage of other keyUsage when checking for the DC extension. * Add tool for generating delegated credentials. Co-authored-by: Jonathan Hoyland <jhoyland@cloudflare.com>

Adds support for draft 13 of the Encrypted ClientHello (ECH) extension for TLS. This requires CIRCL to implement draft 08 or later of the HPKE specification (draft-irtf-cfrg-hpke-08). Adds a CFEvent for reporting when ECH is offered or greased by the client, when ECH is accepted or rejected by the server, and when the outer SNI doesn't match the public name of the ECH config. Missing ECH features: * Record-level padding. * Proper validation of the public name by the client. * Retry after rejection. * PSKs are disabled when ECH is accepted.

…o send RTG-2919 [ Bas 1.21.3: Send empty keyshare extension instead of leaving it out ]

In contrast to upstream Go, we will send an HelloRetryRequest and accept an extra roundtrip if there is a more preferred group, than the one the client has provided a keyshare for in the initial ClientHello. Cf. https://datatracker.ietf.org/doc/draft-davidben-tls-key-share-prediction/

DummyKex is a key agreeement similar in size but purposefully incompatible with X25519. The goal is to have a key agreement that servers will not support, so we can test HelloRetryRquest.

bwesterb self-requested a review August 7, 2023 20:30

bwesterb approved these changes Aug 7, 2023

View reviewed changes

cjpatton requested changes Aug 7, 2023

View reviewed changes

jhoyla changed the title ~~Implement client side of TLSFlags extension~~ Implement TLSFlags extension Aug 7, 2023

bwesterb force-pushed the cf branch from a888ac3 to e229e8c Compare August 14, 2023 14:26

bwesterb force-pushed the cf branch from c6d8c70 to 6ba8a88 Compare September 6, 2023 22:18

Lekensteyn force-pushed the jhoyla/tls-flags branch from fd0be4b to 1b8ed4f Compare September 19, 2023 16:23

Lekensteyn reviewed Sep 19, 2023

View reviewed changes

bwesterb force-pushed the cf branch 2 times, most recently from 3086991 to 891ed94 Compare October 10, 2023 16:44

bwesterb force-pushed the cf branch from 891ed94 to 34129e4 Compare December 6, 2023 12:56

Lekensteyn force-pushed the cf branch from 34129e4 to cf490a0 Compare February 9, 2024 16:29

bwesterb and others added 13 commits March 5, 2024 23:14

Add README (#158)

1da61bf

Add README Co-authored-by: Peter Wu <pwu@cloudflare.com>

VERSION: add cf tag

412e679

Include the `-cf` tag so this fork can be identified. Include the `devel` tag such that we can potentially add new APIs to the api/next.txt file in order to please the TestDependencies test.

cmd/go/internal/imports, go/build: add cfgo build tag

afa7242

This allows applications to use build tags to maintain compatibility with both this fork as well as standard Go. [ bas 2023-10-6: Fix cfgo build tag and add a test (#156) ]

.github: add docker-compose.yaml and GitHub Action for CI

ea0a548

Tests can be run from the repo with: `docker-compose run test`

.github: replace upstream templates and CODE_OF_CONDUCT.md

e5515b0

[pwu: Go 1.22.0: resolve conflicts: git rm -r .github/ISSUE_TEMPLATE]

go/build: don't check dependencies of vendored circl library

85a53c6

Set GOTOOLCHAIN to local to prevent auto-download of new Go

956501e

bwesterb added 3 commits March 5, 2024 23:14

Add tls.Config.ClientCurveGuess to allow specifying which keyshares t…

84835c7

…o send RTG-2919 [ Bas 1.21.3: Send empty keyshare extension instead of leaving it out ]

Add dummy key agreement with codepoint 0xfe33 (#162)

087d810

DummyKex is a key agreeement similar in size but purposefully incompatible with X25519. The goal is to have a key agreement that servers will not support, so we can test HelloRetryRquest.

Lekensteyn force-pushed the cf branch from cf490a0 to 087d810 Compare March 14, 2024 16:56

jhoyla force-pushed the jhoyla/tls-flags branch from 1b8ed4f to b071690 Compare March 17, 2024 04:26

Implement TLS Flags and the request mTLS flag.

ead11a3

jhoyla force-pushed the jhoyla/tls-flags branch from b071690 to ead11a3 Compare March 17, 2024 04:37

Lekensteyn force-pushed the cf branch from 087d810 to ec0a014 Compare April 5, 2024 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement TLSFlags extension #151

Implement TLSFlags extension #151

jhoyla commented Aug 7, 2023

bwesterb commented Aug 7, 2023

cjpatton left a comment

cjpatton Aug 7, 2023

Lekensteyn Sep 19, 2023

cjpatton Aug 7, 2023

Lekensteyn Sep 19, 2023

bwesterb commented Aug 7, 2023

jhoyla commented Aug 7, 2023

cjpatton commented Aug 8, 2023

Lekensteyn commented Sep 19, 2023

Lekensteyn Sep 19, 2023

Lekensteyn Sep 19, 2023

Lekensteyn Sep 19, 2023

Lekensteyn Sep 19, 2023

Lekensteyn Sep 19, 2023

Lekensteyn Sep 19, 2023

Lekensteyn Sep 19, 2023

Lekensteyn Sep 19, 2023

		@@ -851,6 +858,8 @@ type Config struct {
		// See https://tools.ietf.org/html/draft-ietf-tls-subcerts.
		SupportDelegatedCredential bool

	FlagSupportMTLS TLSFlag = 0x50
	/// ExperimentalFlagSuppportMTLS ...
	ExperimentalFlagSupportMTLS TLSFlag = 0x50

	type TLSFlag uint16
	// TLSFlag represents the flag number.
	type TLSFlag uint16

Implement TLSFlags extension #151

Are you sure you want to change the base?

Implement TLSFlags extension #151

Conversation

jhoyla commented Aug 7, 2023

bwesterb commented Aug 7, 2023

cjpatton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwesterb commented Aug 7, 2023

jhoyla commented Aug 7, 2023

cjpatton commented Aug 8, 2023

Lekensteyn commented Sep 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment