Initial support for RSA-PSS (preparation for client) #35

Lekensteyn · 2017-09-21T14:32:40Z

This is part of the changes that are required for a TLS 1.3 client.

It refactors the SAH type from TLS 1.2 to SignatureScheme in TLS 1.3, these changes have been proposed upstream:
~~https://go-review.googlesource.com/c/go/+/62210~~ (merged upstream)
~~https://go-review.googlesource.com/c/go/+/64610~~ abandoned
This PR might change based on upstream feedback, ~~but so far the only concern is about naming.~~
Based on feedback to the above abandoned patch, here is another shot at preparing for and actually implementing PSS:
https://go-review.googlesource.com/c/go/+/79735 (preparation patch)
https://go-review.googlesource.com/c/go/+/79736 (PSS support for handshake messages)
https://go-review.googlesource.com/c/go/+/79738 (RFC: enable PSS and add tests)

EDIT: this PR will likely be superseded by the contents of https://github.com/cloudflare/tls-tris/tree/pwu/sigalgs-pss but is pending (upstream) review!

After that, adding support for RSASSA-PSS signatures for handshake messages becomes easier.
NOTE: RSASSA-PSS is not advertised yet in the TLS 1.2 client handshake because it technically does not support RSASSA-PSS public keys or signatures in certificates. Furthermore, all testdata would have to be updated after such a change, so I was hesitant to do that now.

To implement full RSASSA-PSS support, upstream crypto/x509 would need to be changed and the TLS WG must settle on the required functionality (see this list post).

As a hack, a future TLS 1.3 client change will include PSS anyway in the supported_algorithms extension (but only for 1.3, not for 1.2) because the handshake will fail otherwise (PSS is currently mandatory up to at least draft -21).

lziest · 2017-09-27T22:51:42Z

handshake_client.go

 		switch key.Public().(type) {
 		case *ecdsa.PublicKey:
 			signatureType = signatureECDSA
 		case *rsa.PublicKey:
-			signatureType = signatureRSA
+			signatureType = signatureRSAPKCS1


so now this value will be 0? I have a feeling something is not quite right

It is not sent over the network, so the exact value does not matter. Maybe I should use iota + 1 though to allow for "uninitialized" values to be recognized.

That could work, or we just assign 0 to some None constant.

lziest · 2017-09-27T22:53:21Z

common.go

 const (
-	signatureRSA   uint8 = 1


I think I missed something. Now signatureRSAPKCS1=0, RSAPSS=1, and signatureECDSA=2?

Correct, the exact values do not exactly matter since the signatureType is not sent over the network, it is used only internally to track what signature verification method needs to be called.

In TLS 1.2 the SignatureAndHash (SAH) type consisted of two fields, these were merged in TLS 1.3 into a single 16-bit number. The hash type and the SAH types have been removed in the previous patch, this one removed the signature type from SAH as well.

Make sense. I only briefly touched those code before, I didn't know the change in progress.

lziest · 2017-09-27T22:56:12Z

can we push this change upstream to go master branch? I am not feeling super comfortable about remove some RFC-specific constants? I might be wrong. I need to learn.

Lekensteyn · 2017-09-28T11:34:40Z

Upstream had no objections for the patch, I'll poke again. Ideally the patches are reviewed+merged first in upstream to reduce the changes that we have to carry.

lziest · 2017-09-29T16:13:56Z

Thanks, I will look again.

lziest · 2017-09-29T16:29:47Z

We have signatureType, signatureAlgorithm, and signatureScheme, those names are a little bit too confusing. I actually like the name SigAndHash because then I know there would be a Hash component. Now Hash are being eliminated, and Hash info are embedded in signatureAlgorithm? (sorry I am a little dizzy right now :-))

Lekensteyn · 2017-09-29T16:40:42Z

I tried to follow the TLS 1.3 draft closely here, we have two types:

signatureType: e.g. signatureECDSA
SignatureScheme: e.g. ECDSAWithSHA256

The type is named "SignatureScheme" in TLS 1.3 because TLS 1.2 defined the "SignatureAlgorithm" type differently. But I actually use "signatureAlgorithm" as variable name since that's the draft says that throughout the document, it will refer to SignatureScheme as "SignatureAlgorithm".

It is called "SignatureScheme" because there are also signature algorithms without configurable hash, so SignatureAndHashAlgorithm would not be entirely accurate. Some combinations of hash functions and signature algorithms are also incompatible.

Lekensteyn · 2017-09-29T16:47:25Z

By the way, I have updated the patches upstream to use iota+1 for the two types (signatureType and pubkeyType) and renamed signatureRSAPKCS1 to signaturePKCS1v15, but the functionally the patches are still the same.

lziest · 2017-09-29T18:17:11Z

LGTM, I understand that now.

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

Lekensteyn · 2017-10-02T16:31:41Z

I've rebased the patch on the latest master branch and changed the following:

substitute the "replace signatureAndHash" and "clarify distinction" patches for three patches that do the same (including the signaturePKCS1v15 rename and iota changes mentioned before)
renamed to "signaturePKCS1v15" in the "initial support for RSASSA-PSS" patch.
added another fix for an issue that was discovered while running bogo client tests (namely, missing support for PSS in ServerKeyExchange)

The tree patches consist of two upstream patches (still in review) plus a change needed due to our TLS 1.3 changes (see the commit message).

Lekensteyn · 2017-11-27T19:34:17Z

Replaced by #54

This was referenced Sep 21, 2017

Support for RSASSA-PSS public keys in certificates #39

Open

Initial TLS 1.3 client support (v2) #43

Closed

lziest reviewed Sep 27, 2017

View reviewed changes

Lekensteyn added 5 commits October 2, 2017 17:12

crypto/tls: replace signatureAndHash by SignatureScheme (TLS 1.3 part).

e3546b7

TODO squash this into "crypto/tls: implement TLS 1.3 minimal server" when "crypto/tls: replace signatureAndHash by SignatureScheme." is merged upstream.

crypto/tls: accept RSASSA-PSS signatures in SKE

66fb268

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

Lekensteyn force-pushed the pwu/sigalgos branch from 81c69e0 to 66fb268 Compare October 2, 2017 16:27

Lekensteyn added the waiting-for-upstream-feedback label Oct 31, 2017

Lekensteyn mentioned this pull request Nov 14, 2017

Merge upstream go post-1.9 (master) crypto/tls changes #51

Merged

Lekensteyn added the needs-work label Nov 23, 2017

Lekensteyn mentioned this pull request Nov 27, 2017

Support for RSA-PSS (v2) #54

Merged

Lekensteyn closed this Nov 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support for RSA-PSS (preparation for client) #35

Initial support for RSA-PSS (preparation for client) #35

Lekensteyn commented Sep 21, 2017 •

edited

Loading

lziest Sep 27, 2017

Lekensteyn Sep 28, 2017

lziest Sep 29, 2017

lziest Sep 27, 2017

Lekensteyn Sep 28, 2017

lziest Sep 29, 2017

lziest commented Sep 27, 2017

Lekensteyn commented Sep 28, 2017

lziest commented Sep 29, 2017

lziest commented Sep 29, 2017

Lekensteyn commented Sep 29, 2017

Lekensteyn commented Sep 29, 2017

lziest commented Sep 29, 2017

Lekensteyn commented Oct 2, 2017

Lekensteyn commented Nov 27, 2017

Initial support for RSA-PSS (preparation for client) #35

Initial support for RSA-PSS (preparation for client) #35

Conversation

Lekensteyn commented Sep 21, 2017 • edited Loading

lziest Sep 27, 2017

Choose a reason for hiding this comment

Lekensteyn Sep 28, 2017

Choose a reason for hiding this comment

lziest Sep 29, 2017

Choose a reason for hiding this comment

lziest Sep 27, 2017

Choose a reason for hiding this comment

Lekensteyn Sep 28, 2017

Choose a reason for hiding this comment

lziest Sep 29, 2017

Choose a reason for hiding this comment

lziest commented Sep 27, 2017

Lekensteyn commented Sep 28, 2017

lziest commented Sep 29, 2017

lziest commented Sep 29, 2017

Lekensteyn commented Sep 29, 2017

Lekensteyn commented Sep 29, 2017

lziest commented Sep 29, 2017

Lekensteyn commented Oct 2, 2017

Lekensteyn commented Nov 27, 2017

Lekensteyn commented Sep 21, 2017 •

edited

Loading