TLS 1.3 client authentication in the client side

[vdukhovni]

Tested with and without client certificates against an OpenSSL (Postfix) server that requested client certificates. Both cases work.

This is a large patch, please study it carefully. It would also be great if someone implemented the server side support for requesting TLS13 client auth. Then this could be covered by the tests.

The code could perhaps use more polish. Perhaps some helper functions should be moved to other modules? And more work remains...

All the various ways of referring to public key algorithms could really use a more uniform treatment. As it stands there's

DigitalSignatureAlg         (e.g. RSA)
PubkeyAlg                   (e.g. PubKeyRSA)
HashAndSignatureAlgorithm   (e.g. (Intrinsic, SignatureRSApssRSAeSHA256))
CertificateType             (e.g. CertificateType_RSA_Sign)
...

and conversions between these are a hit or miss afair. One area of cleanup might to systematically map out clean interfaces for moving between all these related things.

Another thing that seems to be rather missing is an API for loading a certificate chain from a single file, rather than an array of files with a single certificate in each... Is something like that provided by X.509? TLS seems to have credentialLoadX509Chain, which is far from ideal.

[kazu-yamamoto]

Relating to #274.

[kazu-yamamoto]

I compiled OpenSSL 1.1.1 and run s_server:

% ./opensslwrap.sh s_server -accept 13443 -key $SOMEWHERE/serverkey.pem -cert $SOMEWHERE/servercert.pem -CAfile $SOMEWHERE/cacert.pem -verify 5

And I ran tls-simpleclient with out this PR:

% stack exec tls-simpleclient -- 127.0.0.1 13443 --tls13 --no-valid --client-cert=$SOMEWHERE/clientcert.pem:$SOMEWHERE/clientkey.pem

The server said:

ERROR
4435281344:error:14094438:SSL routines:ssl3_read_bytes:tlsv1 alert internal error:ssl/record/rec_layer_s3.c:1528:SSL alert number 80
shutting down SSL
CONNECTION CLOSED

Then, I ran tls-simpleclient with this PR. The server said:

depth=1 C = JP, ST = Tokyo, L = Chiyoda-ku, O = IIJ-II, CN = ca.mew.org, emailAddress = kazu@ca.mew.org
verify return:1
depth=0 C = JP, ST = Tokyo, O = IIJ-II, CN = kazu.mew.org, emailAddress = kazu@mew.org
verify return:1
GET / HTTP/1.0

DONE
shutting down SSL
CONNECTION CLOSED

OK. Good.

[vdukhovni]

The other test case is when a server asks for a client certificate, but the client has none, and sends an empty certificate list. That also worked for me, but should be part of any tests that are added once hs-tls supports sending certificate requests from the server, and verifying client chains in return.

[kazu-yamamoto]

The other test case is when a server asks for a client certificate, but the client has none, and sends an empty certificate list. That also worked for me, but should be part of any tests that are added once hs-tls supports sending certificate requests from the server, and verifying client chains in return.

What about this?

• Merge this PR
• Try to implement the server side
• Enable the disabled test case of client authentication
• Enrich test cases

[vdukhovni]

Yes, I think that's the basic idea, though of course I would still encourage you to review the code and comments, and perhaps consider moving some functions to other modules if some of the new "top-level" functions are not in the right place. Perhaps some move to "Common.hs"?

I think got the functionality roughly right, but my familiarity with this code base is not yet very good, so someone more familiar with how it is organized might find opportunities for improvement...

[kazu-yamamoto]

I added some commits: https://github.com/kazu-yamamoto/hs-tls/commits/vdukhovni-tls13-client-auth

Continuing to review.

[ocheron]

Promising but for now I'll prefer to focus on other topics first (continuing to read v1.3).
You can merge if you think ready, but please analyze non-TLS13 impacts before merging.

If you have bandwidth you can also look if this conflicts badly with branch ocheron:newcurves or not.

[kazu-yamamoto]

I need to check this with #302, too.

[kazu-yamamoto]

@ocheron ocheron:newcurves gets rid of two warnings. Very nice!

[kazu-yamamoto]

LGTM

[kazu-yamamoto]

I have reviewed the most complicated part (Client.hs) and think that the quality is good enough.
I would like to merge this as https://github.com/kazu-yamamoto/hs-tls/tree/proj
See also #309.

[vdukhovni]

Please let me know if you need anything further from me on this PR. My impression is that nobody is presently waiting for me, and this PR is queued up pending some related work...

[kazu-yamamoto]

As I said in #309, I'm waiting for newcurves. But if it takes time, I will merge this PR first.

[kazu-yamamoto]

Merged.