Supporting RSAPSS defined in TLS 1.3 #207

kazu-yamamoto · 2017-04-07T01:57:08Z

This implements #206.

Since TLS 1.3 flattens the structure of (Hash, Signature) and Hash 0x08 is a magic word for TLS 1.3, implementation is not straightforward and a little bit tricky.

ocheron · 2017-04-12T05:31:16Z

Yes this is tricky. I see we don't have much choice regarding HashAlgorithm and SignatureAlgorithm considering the assignments and backward compatibility.

However internally I think we should stop using the tuple components individually as much as we can.
Especially move away from using type SignatureAlgorithm as an indication of key-exchange signature algorithm. sigAlgExpected only need the values RSA / DSS / ECDSA. Introducing a new data type would help readability in the handshake code.

kazu-yamamoto · 2017-04-12T06:07:15Z

Rebased and pushed -f to make discussion easier.

kazu-yamamoto · 2017-04-12T06:11:12Z

I will rebase my TLS 1.3 branch onto this first and will try to understand whether or not the new data type help readability.

ocheron · 2017-04-12T17:43:16Z

I experimented ideas related to typing in branch ocheron:kxsigalg (without RSA PSS, but that should be easily extensible).

The benefits I see are:

does not need (Hash, Bool) parameters in crypto code when not necessary. EdDSA will not need Hash. This API is approximately what we have in x509-validation.
swapping signatureEqual arguments is not possible anymore (renamed signatureCompatible)
type HashAndSignatureAlgorithm is now opaque in "Client" and "Server" code. Only two functions in module "Signature" need to look inside.

My understanding is that such a change would not interfere with TLS 1.3. Could you look into this?
Constructors for Ed25519 and Ed448 can be added if client/server have certificates and private keys.

ocheron · 2017-04-12T17:43:37Z

Also a remark about your PSS code: are you sure RSA blinding is not desirable?
(ie. function PSS.signSafer vs PSS.sign Nothing)

kazu-yamamoto · 2017-04-13T01:52:37Z

Since I need to finish another job in this week, I will get back to this issue in the next week.
Sorry for you inconvenience.

ocheron · 2017-04-13T17:27:05Z

OK, take the time you need.

DSS is defined only for SHA1 and does not need a hash. The data type will be extensible to RSA-PSS variant and EdDSA.

Code needing to decompose the tuple is now centralized, to better cope with an API change or TLS13 assignments.

We have only 3 possibilities for sigAlgExpected, in direct correspondance with cipher key exchange and private/public keys.

kazu-yamamoto · 2017-04-19T07:45:53Z

I have ported my code onto your kxsigalg branch. Yes, your idea is really great. My code got much simpler.

I'm now trying to check if modifications are necessary forCredentials.hs.

kazu-yamamoto · 2017-04-19T13:51:44Z

I think I have done.
@ocheron May I ask you to review this PR again?

ocheron · 2017-04-19T21:20:04Z

I'm glad you like it, and it's good we can use it for credentials too.

ocheron

Mostly minor items, except default supportedHashSignatures where there is a real choice to be made.

ocheron · 2017-04-19T21:07:53Z

core/Network/TLS/Handshake/Server.hs


                        -- Check that a candidate cipher with a signature requiring
                        -- a hash will have at least one hash available.  This avoids
                        -- a failure later in 'decideHash'.
                        hasSigningRequirements =
                            case cipherKeyExchange cipher of
-                                CipherKeyExchange_DHE_RSA      -> SignatureRSA   `elem` possibleSigAlgs
+                                CipherKeyExchange_DHE_RSA      -> isJust $ find (RSA `signatureCompatible`) possibleHashSigAlgs


signatureCompatible can be used for DSS and ECDSA too.

elemBy is Data.List.any

ocheron · 2017-04-19T21:10:35Z

core/Network/TLS/Struct.hs

+-- and cipher key exchange.
+data DigitalSignatureAlg = RSA | DSS | ECDSA | Ed25519 | Ed448
+                           deriving (Show, Eq)
+


I don't like DigitalSignatureAlg being defined in module "Struct" as it's internal and not protocol-related. Module "Types" would be better perhaps.

And if possible signatureCompatible should remain in "Handshake.Signature" where signatureParams is also defined (because those two must stay in sync).

Does Crypto.Types make sense?

Ah. In my old implementation, signatureCompatible is necessary for the Credential module which causes cyclic import. But this is not the case anymore. I will try.

ocheron · 2017-04-19T21:19:15Z

core/Network/TLS/Parameters.hs

@@ -197,7 +197,8 @@ defaultSupported = Supported
    { supportedVersions       = [TLS12,TLS11,TLS10]
    , supportedCiphers        = []
    , supportedCompressions   = [nullCompression]
-    , supportedHashSignatures = [ (Struct.HashSHA512, SignatureRSA)
+    , supportedHashSignatures = [ (Struct.HashTLS13,  SignatureRSApssSHA256)


Ordering is questionable, I think SHA-512 and SHA-384 should still have priority over SignatureRSApssSHA256.

Oh. This is a mistake. I don't want to modify this module.
I modified this file for local testing. I will revert this.

kazu-yamamoto · 2017-04-20T01:50:06Z

I did push -f. If you like Crypto.Types, this PR is ready to be merged.

ocheron · 2017-04-20T05:17:00Z

Yes Crypto.Types will be fine, so good to merge.

kazu-yamamoto · 2017-04-20T07:18:21Z

Thanks. Merged.

ocheron · 2017-05-30T19:15:34Z

Reading draft-ietf-tls-rfc4492bis, I realize we could rename HashTLS13 to HashIntrinsic (cf. § 5.1.3. The signature_algorithms Extension and EdDSA). Otherwise it will be confusing to apply that to TLS 1.0, 1.1 and 1.2 too.

Also I wonder if we chose the best definition to DigitalSignatureAlg if EdDSA applies to ECDHE_ECDSA ciphers.

kazu-yamamoto · 2017-06-01T01:49:39Z

Reading draft-ietf-tls-rfc4492bis, I realize we could rename HashTLS13 to HashIntrinsic (cf. § 5.1.3. The signature_algorithms Extension and EdDSA). Otherwise it will be confusing to apply that to TLS 1.0, 1.1 and 1.2 too.

Oh, yes! Please rename it.

Also I wonder if we chose the best definition to DigitalSignatureAlg if EdDSA applies to ECDHE_ECDSA ciphers.

I have no idea on this.

ocheron · 2017-06-01T19:23:39Z

OK thanks. I can explore this as I have some code for EdDSA certificates.

kazu-yamamoto requested a review from ocheron April 7, 2017 01:57

kazu-yamamoto force-pushed the rsapss branch from c3b7570 to e4c9395 Compare April 12, 2017 06:05

ocheron and others added 13 commits April 19, 2017 13:04

Introduce ADT holding signature parameters

ca57b68

DSS is defined only for SHA1 and does not need a hash. The data type will be extensible to RSA-PSS variant and EdDSA.

Use HashAndSignatureAlgorithm exclusively

9fcb187

Code needing to decompose the tuple is now centralized, to better cope with an API change or TLS13 assignments.

Do not use type SignatureAlgorithm as KX signature

b2a4abe

We have only 3 possibilities for sigAlgExpected, in direct correspondance with cipher key exchange and private/public keys.

adding HashTLS13

d1d3ac9

adding TLS 1.3 signature schemes.

344dceb

s/hashDescr/sigParam/

1121012

s/certificateVerifyCreate/createCertificateVerify/

e02ce9e

s/certificateVerifyCheck/checkCertificateVerify/

161b483

s/signatureVerifyWithHashDescr/signatureVerifyWithCertVerifyData/

6ad9d2f

adding RSAPSS to tests.

997d7ff

supporting RSA PSS.

54444cb

using signatureCompatible in the server side.

cb602ac

adding Ed25519 and Ed448.

cfbae3e

kazu-yamamoto force-pushed the rsapss branch from e4c9395 to cfbae3e Compare April 19, 2017 07:38

using 'find' instead of elemBy.

ab634a5

kazu-yamamoto force-pushed the rsapss branch from e583052 to 86e1a04 Compare April 19, 2017 13:50

ocheron reviewed Apr 19, 2017

View reviewed changes

using DigitalSignatureAlg for Credentials.

eba8ac3

kazu-yamamoto added 2 commits April 20, 2017 09:33

using any instead of find.

df26507

using the same logic to check signature type.

8aa9a89

kazu-yamamoto force-pushed the rsapss branch from 86e1a04 to 8aa9a89 Compare April 20, 2017 00:45

kazu-yamamoto added a commit to kazu-yamamoto/hs-tls that referenced this pull request Apr 20, 2017

Merge PR haskell-tls#207.

336efef

kazu-yamamoto merged commit 8aa9a89 into haskell-tls:master Apr 20, 2017

kazu-yamamoto deleted the rsapss branch April 20, 2017 07:18

kazu-yamamoto mentioned this pull request May 26, 2017

TLS 1.3 #167

Closed

15 tasks

kazu-yamamoto mentioned this pull request Sep 13, 2018

Check list for TLS 1.3 #282

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supporting RSAPSS defined in TLS 1.3 #207

Supporting RSAPSS defined in TLS 1.3 #207

kazu-yamamoto commented Apr 7, 2017

ocheron commented Apr 12, 2017

kazu-yamamoto commented Apr 12, 2017

kazu-yamamoto commented Apr 12, 2017

ocheron commented Apr 12, 2017

ocheron commented Apr 12, 2017

kazu-yamamoto commented Apr 13, 2017

ocheron commented Apr 13, 2017

kazu-yamamoto commented Apr 19, 2017

kazu-yamamoto commented Apr 19, 2017

ocheron commented Apr 19, 2017

ocheron left a comment

ocheron Apr 19, 2017

ocheron Apr 19, 2017

kazu-yamamoto Apr 19, 2017

kazu-yamamoto Apr 19, 2017

ocheron Apr 19, 2017

kazu-yamamoto Apr 19, 2017

kazu-yamamoto commented Apr 20, 2017

ocheron commented Apr 20, 2017

kazu-yamamoto commented Apr 20, 2017

ocheron commented May 30, 2017

kazu-yamamoto commented Jun 1, 2017

ocheron commented Jun 1, 2017

Supporting RSAPSS defined in TLS 1.3 #207

Supporting RSAPSS defined in TLS 1.3 #207

Conversation

kazu-yamamoto commented Apr 7, 2017

ocheron commented Apr 12, 2017

kazu-yamamoto commented Apr 12, 2017

kazu-yamamoto commented Apr 12, 2017

ocheron commented Apr 12, 2017

ocheron commented Apr 12, 2017

kazu-yamamoto commented Apr 13, 2017

ocheron commented Apr 13, 2017

kazu-yamamoto commented Apr 19, 2017

kazu-yamamoto commented Apr 19, 2017

ocheron commented Apr 19, 2017

ocheron left a comment

Choose a reason for hiding this comment

ocheron Apr 19, 2017

Choose a reason for hiding this comment

ocheron Apr 19, 2017

Choose a reason for hiding this comment

kazu-yamamoto Apr 19, 2017

Choose a reason for hiding this comment

kazu-yamamoto Apr 19, 2017

Choose a reason for hiding this comment

ocheron Apr 19, 2017

Choose a reason for hiding this comment

kazu-yamamoto Apr 19, 2017

Choose a reason for hiding this comment

kazu-yamamoto commented Apr 20, 2017

ocheron commented Apr 20, 2017

kazu-yamamoto commented Apr 20, 2017

ocheron commented May 30, 2017

kazu-yamamoto commented Jun 1, 2017

ocheron commented Jun 1, 2017