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.

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

Save client certificate into Context #405

Closed
wants to merge 2 commits into from

Conversation

eyeinsky
Copy link

Add a new field ctxClientCerts to Context data type, so that we could pass it in to the onClientCertificate hook and fill it with the client certificate if we wanted to.

Things I'm wondering about:

  • couldn't we just always fill it so that the hook doesn't need to? Maybe even remove the hook then since the chain is available at request time and hook is no longer required?
  • if we always filled it then could we remove the IORef part because the client cert can't change during the lifetime of a session?

(Both of these are about making the "client cert save" pure, but perhaps this in not possible.)

--

I created a sample web server here that uses the modified warp-tls and prints the client certificate when visited (at https://localhost:3000):
https://github.com/eyeinsky/tls-info/tree/eee46cf761963bcc80410fd524c7bce0a880f6c4

All this is related to this issue on warp-tls and there will be another pull request in warp-tls that would pass the IORef into the request handler.

It's stores certificate chain in an IORef. This can be filled in
the onClientCertificate hook.
@kazu-yamamoto
Copy link
Collaborator

I don't understand why ctxClientCerts is set by onClientCertificate. Why don't you set it internally without changing the signaute of onClientCertificate?

@eyeinsky
Copy link
Author

I saw this comment which said it would be good to do it this way for performance reasons. As it is now in this PR, passing the IORef into the hook, the users of warp-tls can decide whether to save the client certificate or not. I myself lack any info on how much overhead it would create. Maybe @vdukhovni knows better, or perhaps I misunderstood the comment.

When it wouldn't be passed into the hook then could the client certificate be stored even without the IORef around it? Since (as far as I understand) the client certificate won't change over the lifetime of the tls connection.

@ocheron
Copy link
Contributor

ocheron commented Nov 21, 2019

The client certificate can change during the lifetime of a session:

  • Initially there is none
  • The first handshake may provide a client certificate
  • Renegotiation handshakes and post-handshake certificate requests may provide a different certificate each time

I completely agree we should improve the story for client certificates.

Function onClientCertificate can be used to capture the certificate but it's not its primary goal. The function is invoked at a point in the handshake where the peer certificate is received but not verified.

A full and flexible solution would be to add a new callback providing the peer identity after it has been verified. Application code would be allowed to return or modify a piece of session state to be stored in SessionData. And this information would be available back after resumption.

Simple and limited solution is just to expose getClientCertificateChain.

Why preferring a callback design instead of direct context-state update:

  • makes no assumption about what part of the certificate chain the server wants to retain
  • makes no assumption about how to update the server state when having multiple authentications (especially now with PHA which is very flexible)

@kazu-yamamoto
Copy link
Collaborator

When it wouldn't be passed into the hook then could the client certificate be stored even without the IORef around it? Since (as far as I understand) the client certificate won't change over the lifetime of the tls connection.

Suppose that we decide to store client's CertificateChain in Context, the following API is necessary to use it:

getClientCertificateChain :: Context -> IO CertificateChain

If we decide to store it through onClientCertificate :: CertificateChain -> IO CertificateUsage, the following API should be provided:

setClientCertificateChain :: Context -> CertificateChain -> IO ()

Make sense?

@kazu-yamamoto
Copy link
Collaborator

A full and flexible solution would be to add a new callback providing the peer identity after it has been verified. Application code would be allowed to return or modify a piece of session state to be stored in SessionData. And this information would be available back after resumption.

@ocheron Would you show the code which implements this idea?

@eyeinsky
Copy link
Author

A few more ideas based on the previous comments:

  • in the onClientCertificate the application author can either reject or accept the certificate, so does the "full solution" mean adding an automatic chain checking to that?
  • should the onClientCertificate actually be called onClientCertificateChain since it does receive a chain?
  • regarding the stages of the client certificate being available: couldn't the IORef be a Maybe instead? So on the client hello/server hello time it's Nothing, and then a Just CertificateChain (if it was asked for, otherwise continues to be Nothing). I do understand that a new certificate could be provided, but this seems like a very advanced feature, at least in a web server context (which is all I know).

@vdukhovni
Copy link
Collaborator

The reason to expose the client certificate only when the server registers a callback that squirrels it away somewhere else is that saving the certificate (chain) every time has a significant on the memory footprint of the server, because once the chain is saved it persists in memory for the lifetime of the connection (and possibly also server session cache entries). A busy server handling many thousands of clients may see a noticeable performance impact.

If the server validates the client certificate as it is processed, and discards the chain, perhaps keeping just an extracted authorization identifier, or just the fact that the client is authorized to use the service.

The idea is to let the application decide what should be stored, rather than store large objects that drive up the memory footprint and may often go unused.

To the extent that we move to session tickets only and no server-side resumable sessions (which we should), the verified chain can go into the session ticket, which the client will present on resumption, and so the server need never store locally, but gets to inspect it at each full and resumed handshake, with application callbacks available to save this data for the lifetime of the connection as-needed.

@kazu-yamamoto
Copy link
Collaborator

kazu-yamamoto commented Nov 27, 2019

@eyeinsky May I confirm your motivation why you would like to store CertificateChain to Context?

I modified your gist: https://gist.github.com/kazu-yamamoto/a7712642ad6c05cbab8017536375c71f

This code does not require any changes to TLS and WarpTLS libraries and shows IORef can be shared by multiple functions. Isn't it good enough?

@eyeinsky
Copy link
Author

@kazu-yamamoto Hi, yes: the reason is to have the client cert that was presented available in the request handler. Right now although I get access to the cert it in the server hook then I can't pass it to the request handler (or: I can, but it might be from another handshake, if using a single IORef. I also tried a hacky solution of using a hashmap from thread id to cert, but the thread id's for the hook and request handler were different.

The bigger reason is to do different things in the request handler based on the client certificate (much like a session cookie).

@kazu-yamamoto
Copy link
Collaborator

Got it. I made a POC:

  1. tls: https://github.com/kazu-yamamoto/hs-tls/tree/client-cert
  2. warp and warp-tls: https://github.com/kazu-yamamoto/wai/tree/client-cert

(1) exposes getClientCertificateChain :: Context -> IO (Maybe CertificateChain)
(2) exposes clientCertificate :: Request -> Maybe

So, your applications should be able to get CertificateChain from Request. Note that I don't test these code. But I'm sure that this approach is a right way to go.

@eyeinsky
Copy link
Author

Thank you @kazu-yamamoto, I will test it out!

@kazu-yamamoto
Copy link
Collaborator

@eyeinsky Any progress? Are western people enjoying vacations? :-)

@eyeinsky
Copy link
Author

@kazu-yamamoto No -- it's plain old being busy and then lazy :p. I will try it out this weekend, thank you for the ping!

@eyeinsky
Copy link
Author

Hi @kazu-yamamoto, I tried it and it works as expected, thank you!

I changed my example here using your forks eyeinsky/tls-info@05d4512 and the result was the same. (If anyone else wants to try this then apparently Firefox at least requires a proper server certificate with the root installed to the browser -- having a self signed one on macOS didn't work.)

@ocheron ocheron closed this in #407 Dec 22, 2019
@eyeinsky eyeinsky deleted the tls-application branch January 8, 2020 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants