-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement TLSFlags extension #151
base: cf
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -126,6 +126,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 | ||||||||
) | ||||||||
|
||||||||
// TLS signaling cipher suite values | ||||||||
|
@@ -364,6 +365,15 @@ type ConnectionState struct { | |||||||
// This means the client has offered ECH or sent GREASE ECH. | ||||||||
ECHOffered bool | ||||||||
|
||||||||
// PeerTLSFlags is the set of TLS Flags sent by the Peer. | ||||||||
PeerTLSFlags []TLSFlag | ||||||||
|
||||||||
// AgreedTLSFlags is the set of TLS Flags mutually supported by the Client and Server | ||||||||
AgreedTLSFlags []TLSFlag | ||||||||
|
||||||||
// RequestClientCert is true if the server decided to request a client certificate | ||||||||
RequestClientCert bool | ||||||||
|
||||||||
// ekm is a closure exposed via ExportKeyingMaterial. | ||||||||
ekm func(label string, context []byte, length int) ([]byte, error) | ||||||||
} | ||||||||
|
@@ -469,6 +479,12 @@ const ( | |||||||
ECDSAWithSHA1 SignatureScheme = 0x0203 | ||||||||
) | ||||||||
|
||||||||
type TLSFlag uint16 | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's make clear that this is the flag number (index) rather than the raw representation:
Suggested change
|
||||||||
|
||||||||
const ( | ||||||||
FlagSupportMTLS TLSFlag = 0x50 | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it intentional that this flag occupies 11 bytes? 0x50 is 80 decimal, and since we start counting at 0, this occupies |
||||||||
) | ||||||||
|
||||||||
// ClientHelloInfo contains information from a ClientHello message in order to | ||||||||
// guide application logic in the GetCertificate and GetConfigForClient callbacks. | ||||||||
type ClientHelloInfo struct { | ||||||||
|
@@ -905,6 +921,8 @@ type Config struct { | |||||||
// See https://tools.ietf.org/html/draft-ietf-tls-subcerts. | ||||||||
SupportDelegatedCredential bool | ||||||||
|
||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please document here what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||||||||
TLSFlagsSupported []TLSFlag | ||||||||
|
||||||||
// mutex protects sessionTicketKeys and autoSessionTicketKeys. | ||||||||
mutex sync.RWMutex | ||||||||
// sessionTicketKeys contains zero or more ticket keys. If set, it means | ||||||||
|
@@ -995,6 +1013,7 @@ func (c *Config) Clone() *Config { | |||||||
Renegotiation: c.Renegotiation, | ||||||||
KeyLogWriter: c.KeyLogWriter, | ||||||||
SupportDelegatedCredential: c.SupportDelegatedCredential, | ||||||||
TLSFlagsSupported: c.TLSFlagsSupported, | ||||||||
ECHEnabled: c.ECHEnabled, | ||||||||
ClientECHConfigs: c.ClientECHConfigs, | ||||||||
ServerECHProvider: c.ServerECHProvider, | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,6 +90,7 @@ type clientHelloMsg struct { | |
alpnProtocols []string | ||
scts bool | ||
supportedVersions []uint16 | ||
tlsFlags []byte | ||
cookie []byte | ||
keyShares []keyShare | ||
earlyData bool | ||
|
@@ -239,6 +240,14 @@ func (m *clientHelloMsg) marshal() ([]byte, error) { | |
}) | ||
}) | ||
} | ||
if len(m.tlsFlags) > 0 { | ||
exts.AddUint16(extensionTLSFlags) | ||
exts.AddUint16LengthPrefixed(func(exts *cryptobyte.Builder) { | ||
exts.AddUint8LengthPrefixed(func(exts *cryptobyte.Builder) { | ||
exts.AddBytes(m.tlsFlags) | ||
}) | ||
}) | ||
} | ||
if len(m.cookie) > 0 { | ||
// RFC 8446, Section 4.2.2 | ||
exts.AddUint16(extensionCookie) | ||
|
@@ -577,6 +586,18 @@ func (m *clientHelloMsg) unmarshal(data []byte) bool { | |
} | ||
m.supportedVersions = append(m.supportedVersions, vers) | ||
} | ||
case extensionTLSFlags: | ||
var flagsList cryptobyte.String | ||
if !extData.ReadUint8LengthPrefixed(&flagsList) || flagsList.Empty() { | ||
return false | ||
} | ||
for !flagsList.Empty() { | ||
var flagByte uint8 | ||
if !flagsList.ReadUint8(&flagByte) { | ||
return false | ||
} | ||
m.tlsFlags = append(m.tlsFlags, flagByte) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
case extensionCookie: | ||
// RFC 8446, Section 4.2.2 | ||
if !readUint16LengthPrefixed(&extData, &m.cookie) || | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,8 @@ type serverHandshakeStateTLS13 struct { | |
transcript hash.Hash | ||
clientFinished []byte | ||
certReq *certificateRequestMsgTLS13 | ||
peerTLSFlags []TLSFlag | ||
tlsFlags []TLSFlag | ||
|
||
hsTimings CFEventTLS13ServerHandshakeTimingInfo | ||
} | ||
|
@@ -132,7 +134,9 @@ func (hs *serverHandshakeStateTLS13) handshake() error { | |
|
||
c.handleCFEvent(hs.hsTimings) | ||
c.isHandshakeComplete.Store(true) | ||
|
||
c.agreedTLSFlags = hs.tlsFlags | ||
c.peerTLSFlags = hs.peerTLSFlags | ||
c.requestClientCert = hs.requestClientCert() | ||
return nil | ||
} | ||
|
||
|
@@ -317,6 +321,29 @@ GroupSelection: | |
c.sendAlert(alertIllegalParameter) | ||
return errors.New("tls: invalid client key share") | ||
} | ||
if len(hs.clientHello.tlsFlags) != 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing check: 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." |
||
supportedFlags, err := encodeFlags(hs.c.config.TLSFlagsSupported) | ||
if err != nil { | ||
return errors.New("tls: invalid server flags") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps you should also send an internal error alert here? |
||
} | ||
var mutuallySupportedFlags []byte | ||
for i, sFB := range supportedFlags { | ||
if i >= len(hs.clientHello.tlsFlags) { | ||
break | ||
} | ||
mutuallySupportedFlags = append(mutuallySupportedFlags, hs.clientHello.tlsFlags[i]&sFB) | ||
} | ||
|
||
peerTLSFlags, err := decodeFlags(hs.clientHello.tlsFlags) | ||
if err == nil { | ||
hs.peerTLSFlags = peerTLSFlags | ||
} | ||
|
||
tlsFlags, err := decodeFlags(mutuallySupportedFlags) | ||
if err == nil { | ||
hs.tlsFlags = tlsFlags | ||
} | ||
} | ||
|
||
selectedProto, err := negotiateALPN(c.config.NextProtos, hs.clientHello.alpnProtocols, c.quic != nil) | ||
if err != nil { | ||
|
@@ -356,6 +383,23 @@ GroupSelection: | |
return nil | ||
} | ||
|
||
func decodeFlags(flagBytes []byte) ([]TLSFlag, error) { | ||
var flags []TLSFlag | ||
for byteIndex, b := range flagBytes { | ||
for i := 0; !(b == 0); i++ { | ||
if (b & 1) == 1 { | ||
flagNo := byteIndex*8 + i | ||
if flagNo >= int(maxTLSFlag) { | ||
return nil, fmt.Errorf("TLS flag is out of range: %d", flagNo) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optimization: This check can be removed by checking outside the outer loop. Fail on |
||
flags = append(flags, TLSFlag(flagNo)) | ||
} | ||
b >>= 1 | ||
} | ||
} | ||
return flags, nil | ||
} | ||
|
||
func (hs *serverHandshakeStateTLS13) checkForResumption() error { | ||
c := hs.c | ||
|
||
|
@@ -892,6 +936,11 @@ func (hs *serverHandshakeStateTLS13) sendServerParameters() error { | |
} | ||
|
||
func (hs *serverHandshakeStateTLS13) requestClientCert() bool { | ||
for _, flag := range hs.tlsFlags { | ||
if flag == FlagSupportMTLS { | ||
return true | ||
} | ||
} | ||
return hs.c.config.ClientAuth >= RequestClientCert && !hs.usingPSK | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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