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.

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

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

jhoyla force-pushed the jhoyla/tls-flags branch 2 times, most recently 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

bwesterb force-pushed the cf branch from ec0a014 to 048a673 Compare May 8, 2024 10:10

bwesterb force-pushed the cf branch from 048a673 to 962efcd Compare June 24, 2024 09:37

Lekensteyn force-pushed the cf branch from a920985 to f4334cd Compare July 4, 2024 12:56

Implement TLS Flags and the request mTLS flag.

4092d3f

jhoyla force-pushed the jhoyla/tls-flags branch from ead11a3 to 4092d3f Compare September 9, 2024 16:36

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