Superseeded by #1101, Closing

Hello, this is a work in progress which aims at upgrading the TLS version to 1.3 in the scan functionality. This follows issue #1089.

I'll update this PR with the relevant info as I progress. Comments along the way are welcome !

Warning

My goal was to have a small and simple PR to show my workflow; the more I progress, the more I realize this PR cannot be small.

Roadmap

My goal: cfssl scan -family TLSSession google.com works and uses TLS1.3 instead of 1.2
Secondary goal: minimal changes (as I am unaware of many constraints on this codebase). Despite my attempts, I could not avoid changing the modules & vendor/ folder, sorry for the large diff.

TODOs:

• Upgrade wire format to be compatible with TLS1.3
• Identify & code needed extensions for cfssl scan
• Wire-in these required extensions in the client/server handshakes.
• Figure out why the tests in crypto/tls are ignored & fail

Minimum required for TLS1.3

Cipher Suites:

• TLS_AES_128_GCM_SHA256 (added)
• rsa_pkcs1_sha256 (already there)
• ecdsa_secp256r1_sha256 (already there)
• rsa_pss_rsae_sha256 (not needed for scan ?)

New extensions:

• supported_version
• key_share
• server_name
• signature_algorithms (already there)
• signature_algorithms_cert
• cookie
• supported_groups

(last 4 are coded for marshalling but not yet used in the client)

Questions / Decisions to be made

Where to split between TLS 1.2 / 1.3

The reference implementation has a whole different client/server files for TLS1.3; these files are cleaner than what is done in this PR, where TLS 1.2/1.3 are multiplexed into the same client code. Not sure what is the best approach here. Currently we have very ugly things like:

hs := &clientHandshakeState{
	...
	suite:           suite,
	suiteTLS13:      suiteTLS13,
	finishedHash:    finishedHashTLS12,
	transcriptTLS13: transcriptTLS13,
}

Where suite and finishedHash are not needed in TLS1.3. Using polymorphism here seems like a bad clutch. It would be cleaner to do like in the reference implementation, with handshake_client.go and handshake_client_tls13.go (I didn't start this way because I wanted to do the minimal changes to enable 1.3, now I'm a bit stuck in a bad place).

Relation with the reference implementation

In general, do we want to "copy-paste" the reference implementation ? Only the needed features ? "Carefully" copy-paste (additionally double-checking that it is conform with the RFC) ?

Suggestions / Future improvements of this PR

Use cryptobyte package

Go's reference implementation has a nice way of building the packets with the cryptobyte package. It seems less error-prone and avoid a duplication (1st computing the size, then actually embedding the info).

Example: in cfssl's scan/crypto/tls/handshake_messages.go, the code:

// compute size
if len(m.supportedCurves) > 0 {
    extensionsLength += 2 + 2 * len(m.supportedCurves)
    numExtensions++
}
[...]
// embed curves
if len(m.supportedCurves) > 0 {
    // http://tools.ietf.org/html/rfc4492#section-5.5.1
    z[0] = byte(extensionSupportedCurves >> 8)
    z[1] = byte(extensionSupportedCurves)
    l: = 2 + 2 * len(m.supportedCurves)
    z[2] = byte(l >> 8)
    z[3] = byte(l)
    l -= 2
    z[4] = byte(l >> 8)
    z[5] = byte(l)
    z = z[6: ]
    for _, curve: = range m.supportedCurves {
        z[0] = byte(curve >> 8)
        z[1] = byte(curve)
        z = z[2: ]
    }
}

is reduced to the following in Golang's crypto/tls/handshake_messages.go:

if len(m.supportedCurves) > 0 {
    // RFC 4492, sections 5.1.1 and RFC 8446, Section 4.2.7
    b.AddUint16(extensionSupportedCurves)
    b.AddUint16LengthPrefixed(func(b * cryptobyte.Builder) {
        b.AddUint16LengthPrefixed(func(b * cryptobyte.Builder) {
            for _, curve: = range m.supportedCurves {
                b.AddUint16(uint16(curve))
            }
        })
    })
}

Minor

Add a CLI option to explicitly scan in 1.2 or 1.3 ?