Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Client auth server side #331

Merged
merged 19 commits into from
Jan 13, 2019
Merged

Client auth server side #331

merged 19 commits into from
Jan 13, 2019

Conversation

kazu-yamamoto
Copy link
Collaborator

Passing test cases of client auth and usage.

Interoperability with OpenSSL is confirmed:

% stack exec tls-simpleserver -- -v --key=$SOMEWHERE/serverkey.pem --certificate=$SOMEWHERE/servercert.pem 13443 --tls13 --trust-anchor=$SOMEWHERE/cacert.pem

NG:

% ./opensslwrap.sh s_client 127.0.0.1:13443

OK:

% ./opensslwrap.sh s_client -cert=$SOMEWHERE/clientcert.pem -key=$SOMEWHERE/clientkey.pem 127.0.0.1:13443

ocheron
ocheron reviewed Dec 27, 2018
View reviewed changes
Copy link
Contributor

@ocheron ocheron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main scenario is here but not all interactions are covered, see my comments.

core/Tests/Tests.hs Show resolved Hide resolved
core/Network/TLS/Handshake/Common13.hs Outdated Show resolved Hide resolved
core/Network/TLS/Handshake/Server.hs
-- Remember cert chain for later use.
--
usingHState ctx $ setClientCertChain certs
clientCertificate sparams ctx certs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FIXME comment just below should probably be part of the function too.

core/Network/TLS/Handshake/Server.hs Outdated Show resolved Hide resolved
core/Network/TLS/Handshake/Server.hs Outdated Show resolved Hide resolved
core/Network/TLS/Handshake/Server.hs Outdated Show resolved Hide resolved
@kazu-yamamoto
Copy link
Collaborator Author

I'm now inactive since I'm on winter vacation. I will take care of this on 7th Jan.

@kazu-yamamoto
fixing test of client authentication for TLS 1.3.
f3de848
@kazu-yamamoto
sending TLS 1.3 CertReq from server.
866064c
@kazu-yamamoto
handling cert and cert verify.
9ce0da9
@kazu-yamamoto
making functions for common jobs.
1d0863d
@kazu-yamamoto
removing the unnecessary parameter.
1c10df2
@kazu-yamamoto
using the same function name convention as Client.
6f8ad68
@kazu-yamamoto
style change.
ecf2bf3
@kazu-yamamoto
checking Client CertVerify in server.
9623f8c
@kazu-yamamoto
enabling prop_handshake_clt_key_usage for TLS 1.3.
7f7f47f
@kazu-yamamoto
accepting hlint suggestion.
10a7c34
@kazu-yamamoto
using isClientContext to reduce the number of functions.
ef38cc4
@kazu-yamamoto
correcting an error message.
0a0f12a
@kazu-yamamoto
checking certificate request context.
7601d4c
@kazu-yamamoto
setting proper condition.
c5da186
@kazu-yamamoto
avoiding name conflict.
e7912cf
@kazu-yamamoto
extracting processHandshake13.
74f8b05
@kazu-yamamoto
using runRecvHandshake13 in server.
2e311f0
@kazu-yamamoto
better namings.
8e20a09
@kazu-yamamoto
skipping expectCertVerify if certs is null.
d690792
@kazu-yamamoto
Copy link
Collaborator Author

Popping the action will work but it's not a very clean solution. Can't we just not use pending actions instead? From a user perspective it may be confusing to have invocation of callbacks and possible authentication failures transferred from handshake to recvData based on the client version. Since it is not post-handshake authentication.

Right. I have implemented this using recvHandshake13postUpdate.

Note that I dropped one commit and rebased this branch onto master and pushed -f.

ocheron
ocheron approved these changes Jan 13, 2019
View reviewed changes
Copy link
Contributor

@ocheron ocheron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit more complex that I anticipated regarding the transcript. I think it can be simplified further by just giving the previous transcript as argument to the actions.

But this is already quite clean and provides what we need, thanks!

@ocheron ocheron merged commit d690792 into haskell-tls:master Jan 13, 2019
ocheron added a commit that referenced this pull request Jan 13, 2019
@ocheron
Merge pull request #331 from kazu-yamamoto/client-auth-server-side
94975fb 
Client auth server side
@kazu-yamamoto kazu-yamamoto deleted the client-auth-server-side branch January 13, 2019 22:47
@kazu-yamamoto
Copy link
Collaborator Author

Thank you for reviewing and merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ocheron ocheron ocheron approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kazu-yamamoto @ocheron