Initial TLS 1.3 client support (v2) #43
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lekensteyn
force-pushed
the
pwu/client
branch
2 times, most recently
from
89c9f6b
to
28e17cb
Compare
The BadCBCPadding255 test from bogo failed because at most 255 trailing bytes were checked, but for a padding of 255 there are 255 padding bytes plus 1 length byte with value 255. Change-Id: I7dd237c013d2c7c8599067246e31b7ba93106cf7
The record layer splits application data into chunks of at most 2^14 octets. When record protection is engaged in TLS 1.3, the application data is serialized into a TLSInnerPlaintext which has an additional byte for the content type, resulting in a maximum length of 2^14+1. Fixes LargeMessage, TLS13-AEAD-CHACHA20-POLY1305-LargeRecord, TLS13-AEAD-AES128-GCM-SHA256-LargeRecord and TLS13-AEAD-AES256-GCM-SHA384-LargeRecord bogo tests. Fixes: #46
Consolidate the signature and hash fields (SignatureAndHashAlgorithm in TLS 1.2) into a single uint16 (SignatureScheme in TLS 1.3 draft 21). This makes it easier to add RSASSA-PSS for TLS 1.2 in the future. Fields were named like "signatureAlgorithm" rather than "signatureScheme" since that name is also used throughout the 1.3 draft. The only new public symbol is ECDSAWithSHA1, other than that this is an internal change with no new functionality. Change-Id: Iba63d262ab1af895420583ac9e302d9705a7e0f0
A public key algorithm like RSA can be used in multiple different signature algorithms (e.g. RSASSA-PKCS1-v1_5 and RSASSA-PSS). Introduce two special types to prevent confusion. Change-Id: Ia4a59f63ab7f377d46afc792f53755296762f300
TODO squash this into "crypto/tls: implement TLS 1.3 minimal server" when "crypto/tls: replace signatureAndHash by SignatureScheme." is merged upstream.
Accept RSASSA-PSS for CertificateVerify and ServerKeyExchange messages. This signature algorithm was defined in the TLS 1.3 draft for TLS 1.2. Note that the algorithms are not advertised in ClientHello because PSS certificates are not supported yet, see also http://ietf.org/mail-archive/web/tls/current/msg24440.html Now it passes the RSA-PSS-Default-Sign bogo test. TODO advertise PSS in CH and refresh testdata TODO implement support for PSS public keys, needs changes in crypto/x509
Since we advertise RSASSA-PSS support when maxVersion is VersionTLS13, be sure to implement at accept such signatures in SKE. TODO refactor code to share signature verification with doFullHandshake
Currently the server Handshake method returns as soon as it has sent its
parameters, but it does not wait for the client to authenticate the
handshake via a Finished message. This broke a test that assumed that
the Handshake message performs a full handshake and also
(unintentionally?) enabled the server to send application data before
the handshake is complete ("0.5RTT data").
Fix this by moving the implicit Finished message check in the handshake
message reader to the server handshake itself (previously readRecord
would process the Finished message as a side-effect of requesting
recordTypeApplicationData). And in the special case where 0RTT data is
actually desired, process the Finished message in the ConfirmHandshake
and Read functions.
NOTE: 0.5RTT is not disabled yet when the server enables 0RTT. It is the
server responsibility to use ConfirmHandshake before writing anything.
Explicitly panic when trying ConfirmHandshake for client connections,
this is not the intended use of that API.
The version "c.vers" has not been negotiated yet, check the actual ServerHello message to learn about the version in use.
Skip reading the session cache if TLS 1.3 is in use (the cache has no use), skip storing a session if TLS 1.3 is in use (sessionCache can still be set when TLS 1.2 is allowed).
Accept TLS 1.3-style Server Hello, send supported versions and extract the version and cipher suite from the correct message.
No equivalent is added for processServerHello because session resumption is not supported yet.
Lekensteyn
commented
Oct 4, 2017
13.go
Outdated
|
|
||
| // initializeKeyShare ensures that the current keyshare matches the requested | ||
| // group. An existing keyshare with a matching group will not be replaced. | ||
| func (hs *clientHandshakeState) initializeKeyShare(group CurveID) error { |
There was a problem hiding this comment.
This was intended to be used for HRR, it should be removed here since it is unused.
Includes a Key Share for the first preferred curve (note that HRR is not implemented yet). Certificate validation, Encrypted Extensions processing is also missing (see TODO notes). For simplicity only a single key share is remembered. This key share should be updated with a HRR (when implemented).
Fixes PartialEncryptedExtensionsWithServerHello bogo test.
Fixes: ("crypto/tls: initial TLS 1.3 client support.")
Moved some code and added a comment in preparation for extending the TLS 1.3 client with certificate validation. No functional change.
Support validation of ECDSA and RSASSA-PSS signatures. Explicitly do not support PKCS1-v1_5 signatures since these are not allowed for handshake messages.
Until PSS certificates are properly implemented, let's not advertise support for that. Since TLS 1.3 however mandates PSS, we have no other option than advertising this even if we have not added complete support. Another reason why I apply it to just TLS 1.3 and not 1.2 is because the latter would require updating the testdata.
Test that validation of the server cert actually works in the client.
Enables client tests.
Prepare framework for testing tls-tris as client against other servers. Currently only boringssl is implemented, but the idea is to add support for others too (NSS, OpenSSL, picotls, tris, ...). ecdsa.pem, rsa.pem are copied from tris-localserver. The boringssl image is reused for the server since the binary was built anyway, maybe this will be changed if it turns out to be unmaintainable. Note: the boringssl version is also bumped to something closer to master to fix a build error and make the -loop and -www options work.
This was referenced Nov 24, 2017
Closed
|
Superseded by #55 which adapts the code to upstream changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Initial TLS 1.3 client support was started in #25, but there were some unsolved issues raised during review. This PR takes a different approach than #25, organizing the functionality in a different way.
Rather than completely forking the SH processing, it integrates with existing SH processing. Where possible, changes were prepared/proposed for upstream. Hopefully this makes rebasing easier.
(Unfortunately it will still conflict with golang/go@e085a891f0, I
can cherry-pick that change and rebase on it if desired.have merged all those patches and will have to update this PR accordingly)Other notes:
bssltoo used for testing has improved.Suggestion for review
Independent changes are organized in separate PRs:
Since this client work is based on the other changes, those commits also duplicated into this PR. Please start reviewing this PR from "crypto/tls: allow client code to handle TLS 1.3 ServerHello.".