Usage of static ProtocolVersion in handshakes

[manuels]

I noted that the protocol version 1.2 is hard coded in a lot of code for the handshaking (e.g. https://github.com/ctz/rustls/blob/master/src/client_hs.rs#L931).
This looks a bit odd (I'd expect this version to be sess.common.negotiated_version as soon as SeverHello) is transmitted - so I just want to check with you guys if this is ok or it's a bug.

[manuels]

Also see RFC v1.2: https://tools.ietf.org/html/rfc5246#section-6.2.1 under version.
On the other hand RFC v1.3 says it should be ProtocolVersion::TLS_v10 and should be ignored: https://tools.ietf.org/html/draft-ietf-tls-tls13-18#section-5

Confused.

[ctz]

For TLS1.3 the key thing to note is that TLS13MessageEncrypter sets the right version here: https://github.com/ctz/rustls/blob/master/src/cipher.rs#L306 and ignores the version in the message.

TLS13MessageDecrypter likewise sets all versions to TLS1.3, which allows the messages to be decoded properly (because the encoding of some variant types changed between TLS1.2 and TLS1.3 and the version is the only way work out the difference -- ugly!)

This is going to be much more of an issue for TLS1.1 support -- so I'm leaving it open.

[manuels]

I see, thanks for clarification. But shouldn't this be v1.0 in case of TLS v1.3?

https://tools.ietf.org/html/draft-ietf-tls-tls13-18#section-5.2 says:

The legacy_record_version field is identical
to TLSPlaintext.legacy_record_version and is always 0x0301. Note
that the handshake protocol including the ClientHello and
ServerHello messages authenticates the protocol version, so this
value is redundant.

[ctz]

shouldn't this be v1.0 in case of TLS v1.3

On the wire its 1.0, but internally its 1.3. The translation happens in the message encryption/decryption functions.