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

Support not only requesting client certificates but inspecting them once a connection established #482

Closed
Dridus opened this issue Dec 8, 2015 · 18 comments · Fixed by #783
Assignees
Labels

Comments

@Dridus
Copy link

Dridus commented Dec 8, 2015

There is a flag in TLSSettings, tlsWantClientCert, which pokes tls to ask for client certs after receiving the ClientHello however there are two and a half problems:

  1. There's no way to get the CertificateChain returned by the client after the demand.
  2. The TLS library by default (due to the default onClientCertificate hook) rejects the client certificates.
  3. There's no way to hook into onClientCertificate if you want to be selective about which client certs you accept.

I've crafted a patch which addresses 1 and 2, though because if how it exposes the certificate chain via Transport it entails a new dependency on the x509 package by the warp package for that type. I'm not sure if that's acceptable or not, so take the patch as an example if it's not acceptable for whatever reason.

My patch also exposes the runServeTLS and runServeTLSSocket functions which I'm guessing should have been exposed in the first place. This is in order to receive the Transport.

Branch: https://github.com/Dridus/wai/tree/client-certificates
Commit fixing 1 and 2: https://github.com/Dridus/wai/commit/c28f490eb3e2658a292077c28e67421a2671fad7
Commit exposing runServeTLS and runServeTLSSocket: https://github.com/Dridus/wai/commit/2dd2ffbbec593bb7f31662545f05da527d674038

Commit (on separate branch) with test app which exercises the new behavior: https://github.com/Dridus/wai/commit/36ef66f28a8072ed815ff1ff8fe8c0311cdcd788

@kazu-yamamoto kazu-yamamoto self-assigned this Dec 9, 2015
@kazu-yamamoto
Copy link
Contributor

Relating to #470.

@kazu-yamamoto
Copy link
Contributor

@Dridus Is this related to SNI? (#463)

@Dridus
Copy link
Author

Dridus commented Dec 9, 2015

@kazu-yamamoto

I don't think this is related to SNI, as far as I know that has to do with being able to present different certificates in response to the client hello, e.g.

Client: Client hello with server name indication
Server: Server hello
Server: Certificate, possibly chosen with indicated name

Whereas this has to do with additional handshake steps after that:

Client: Client hello
Server: Server hello
Server: Certificate
Server: Maybe key exchange
Server: Request certificate which is only sent if TLS.serverWantClientCert is set
Server: Server hello done
Client: Certificate

on the last step, receipt of the Certificate message from the client, TLS calls the hookReceiveCertificates (which my patch stashes in an IORef, problem 1 in my original description) and then checks the onClientCertificate hook (which by default rejects the client cert, problem 2 in my original description).

I'm not a TLS expert by any means though, so perhaps there's some interaction with SNI that I don't know about.

@dpwiz
Copy link
Contributor

dpwiz commented Mar 7, 2016

You don't have to attach x509 dependency to wai if you extract fields in tls wrapper and put them in a well-known location. That can be a:

  1. Maybe container with (Extracted, Certificate, Fields, In, Common, Types) or WAIExtractedClientCertWhatever of a same general shape.
  2. Injected X-Something-Something request headers. Stringly typed, yes, but easily accessible to every WAI app without modifications. Also, compatible with tls-terminating reverse proxy.
  3. Vault with an exported key. This one is... cryptic.

@jkarni
Copy link
Contributor

jkarni commented Mar 12, 2016

The onClientCertificate hook doesn't have access to the Network.WAI.Request though, no? Two use cases that would seem important to consider are (a) looking at a request header and at the certificate to check that the certificate corresponds to the client claimed by the request header; and (b) looking at the certificate and passing off client information to the Application (via Vault, headers, or a Request field, as suggested by @wiz).

For what it's worth, I'm very interested in seeing an easier way to do these things with warp-tls.

@eyeinsky
Copy link

eyeinsky commented Aug 4, 2019

Hi -- I'd like to resurrect this ticket. It looks like it's currently not possible to have access to client certificates in the request handler. The only field in the request that has to do with https is isSecure which is a boolean for if the connection to warp is over TLS.

There is a onClientCertificate hook in the tls settings which receives the certificate chain, but as @jkarni pointed out, no Request is available at that time, and neither is there any info on TLS passed in into the request handler in warp. Thus -- even having access to client certificate in the hook there is no way to tie that to a request handled later.

As I understand it, how warp-tls/warp accepts connections is that it sets up a socket with a "connection maker" and then, after creation, runs regular http over it. For warp-tls the session-making is done by the tls library, so to get info on the TLS connection (like client certificates, but perhaps other parameters as well) is that this info needs to be passed back in from the connection maker in the tls into warp-tls's code somehow.

A thing to consider is that multiple http requests happen over a single TLS session, and, as far as I can understand, established TLS sessions can be resumed over a new TCP connection. In both cases there wont be any new handshakes happening and thus no possibility to get access to client certificates again. So to have them for every http request they need to be kept around somehow.

To do this either the tls library needs to save them (as proposed in [1], but this has performance implications? @vdukhovni) and then pass them constantly in to warp-tls request handler, or the end-user of warp-tls gets some API to access the TLS parameters returned from the connection maker and use some custom way to persist them, and then also tie them to the requests. (I think on this latter case serving https in warp-tls will become two-tier: instead of just providing a request handler to runTLS like now, one would have "TLS session handler" which after running receives TLS parameters, and then could have a closure around the regular request handler to make those parameters available.

Found these related github issues:
[1] haskell-tls/hs-tls#275
[2] haskell-tls/hs-tls#133

It seems that whatever the implementation might be, both tls and warp-tls need to change.

Is there anyone else thinking about this? Are there other ways to get access to client certificates when handling a request?

@vdukhovni
Copy link

I'll try to take a look at this in the next few weeks, but can't make any promises... :-(

@eyeinsky
Copy link

eyeinsky commented Aug 5, 2019

Thanks! I might as well try "getting the thing I want" and then we could work out on what the API should be. I'm basically just looking for tips from someone more knowledgeable on if there is an obvious way this should be.

I was also thinking that maybe in the warp-tls there should be a form of runTLS that would take a new TLSApplication type (since it is warp-tls) which in addition to the Request and Respond -> IO ResponseReceived would also take info on the TLS session. But that would mean the tls library would need to keep those around. I guess it comes down to who will do the persisting of info of the TLS session-- the user of the library or tls itself --and that defines how the API will be.

@vdukhovni
Copy link

In terms of "persisting" across resumptions, I would ensure that sufficient client certificate information is encoded in session tickets, so that the state is kept client-side, and returned to the server on each resumption. Ticketless resumption (TLS 1.2 and earlier support both tickets and session-ids) would then not be supported, maintaining server-side state for disconnected clients is too expensive IMHO, though a priority search list can help keep the total size bounded.

OpenSSL session objects store only the leaf certificate and its validation status, and the application is expected to configure a session context string that disambiguates sessions that perform validations differently (say different set of trust anchors, ...).

Haskell TLS could use the server's SNI string as a proxy for the validation policy, clients should not resume sessions across distinct SNI values. If one wanted to save some space, the saved client certificate could be just the "TBS" portion, without the signature, but that creates some API friction, since most APIs for extracting client cert data expect full certificates, not just the TBS portion. One could of course encapsulate the TBS inside a structure with a fake signature that is compact/constant to save some memory pressure.

Once the client certificate is available both on full handshake and each resumption, there needs to be some IORef in each HTTP server connection handle that is used in the TLS onClientCertificate callback or in a suitable (new if required) callback on resumption.

@eyeinsky
Copy link

eyeinsky commented Aug 6, 2019

In terms of "persisting" across resumptions, I would ensure that sufficient client certificate information is encoded in session tickets, so that the state is kept client-side, and returned to the server on each resumption. Ticketless resumption (TLS 1.2 and earlier support both tickets and session-ids) would then not be supported, maintaining server-side state for disconnected clients is too expensive IMHO, though a priority search list can help keep the total size bounded.

I don't know too much of TLS internals -- do you mean that it is possible to store custom data inside the session ticket and that the client certificate could be fitted in there so that it would be available at resumption?

OpenSSL session objects store only the leaf certificate and its validation status, and the application is expected to configure a session context string that disambiguates sessions that perform validations differently (say different set of trust anchors, ...).

Haskell TLS could use the server's SNI string as a proxy for the validation policy, clients should not resume sessions across distinct SNI values. If one wanted to save some space, the saved client certificate could be just the "TBS" portion, without the signature, but that creates some API friction, since most APIs for extracting client cert data expect full certificates, not just the TBS portion. One could of course encapsulate the TBS inside a structure with a fake signature that is compact/constant to save some memory pressure.

Do you mean that in most cases there should be a separate onClientCertificate hook for each domains? (Or at least have distinct code paths for them, perhaps behind an if or case, if the domain were available as an argument for the hook.) As I understand it, currently the same hook is run regardless of the domain, and the domain isn't even known inside it, but it could be if the connection had been made with SNI.

Once the client certificate is available both on full handshake and each resumption, there needs to be some IORef in each HTTP server connection handle that is used in the TLS onClientCertificate callback or in a suitable (new if required) callback on resumption.

Could it also make sense to have some pure TLSInfo type that could be collected throughout setting up the TLS connection and then passed in as a pure value to the warp request handler? Or maybe it's too much work to weave it throughout everything. As in: pass it in as an argument, return as part of the result, e.g even onClientCertificate would get a type something like CertificateChain -> TLSInfo -> IO (CertificateUsage, TLSInfo), and in the returned TLSInfo the client certificate field could have a Just certificate instead of a Nothing if the application writer added it in the hook. And the same TLSInfo would be passed in and out of other functions as well. Or maybe this is overthinking and there isn't much else happening, other than the client certificate, that people would want to have available in their request handlers?

@vdukhovni
Copy link

I don't know too much of TLS internals -- do you mean that it is possible to store custom data inside the session ticket and that the client certificate could be fitted in there so that it would be available at resumption?

The session ticket is an opaque encrypted blob, whose content and syntax is private to the issuing server, a server can put anything it wants to in a session ticket, all that's required is that it be able to use the session ticket and its STEK (session ticket encryption key) to recover the master secret for the session. Whatever else the server puts into a session ticket is up to the server.

Haskell TLS could use the server's SNI string as a proxy for the validation policy, clients should not resume sessions across distinct SNI values. If one wanted to save some space, the saved client certificate could be just the "TBS" portion, without the signature, but that creates some API friction, since most APIs for extracting client cert data expect full certificates, not just the TBS portion. One could of course encapsulate the TBS inside a structure with a fake signature that is compact/constant to save some memory pressure.

Do you mean that in most cases there should be a separate onClientCertificate hook for each domains?

No. Rather the session tickets issued for a particular SNI name should capture that SNI name, and the server should ignore tickets on resumption if the extracted SNI does not match the requested SNI. This would then prevent saved state about client certs from being used across different virtual servers, that might (at some future date if not yet) have different client cert validation policies (trust anchors, supported algorithms, ...).

Could it also make sense to have some pure TLSInfo type that could be collected throughout setting up the TLS connection and then passed in as a pure value to the warp request handler?

I don't think that works well, because the data returned is a waste for some servers and too little for others, and will likely change over time, making for a poor API. Instead each hook should be able to optionally collect whatever relevant state is made available to that hook, and squirrel it away in some suitable IORef (which it captures as a closure).

So my sense is that we want an enriched set of callbacks that have access to more of the TLS internals as the handshake is happening, and it is then up to the callbacks to capture whatever state they need. Of course if some state is retained for the lifetime of the connection, then there could be additional functions to look that up where not already provided.

@eyeinsky
Copy link

Alright, I've now dug deeper into tls.

This is perhaps a random note but re-reading this haskell-tls/hs-tls#275 issue I found that at least to me it looks like the Information data type is pretty much transient. It's created by contextGetInformation from a Context and has no life of its own. So the idea to store anything there makes no sense by itself -- it needs to be stored in the Context instead.

Continuing from the discussion before, if we were to store the certificate in an IORef then it looks like the best best place to put the IORef would be in the Context data type, no? This could be done by adding a field to the Context and passing either the specific IORef or the entire Context into the onClientCertificate hook so the library users could fill it if they wanted to.

But since this github issue is just one use case then it would be better to have some generic method to store TLS related info from the hooks. So I had the following ideas:

  1. Instead of adding a place for just the client certificate would it be possible to have an unique id for every ongoing TLS session? This id would be passed into the hooks and then the consumers of these APIs could create their own concurrent data structures where they could use this id as a key and store whatever they want about the TLS session as value. This key would also be passed into the http request handler and since this encloses over the concurrent variable they could query it using the id for the data they stored.

    Maybe the thing that could be used as the id already exists?

  2. Instead of Context we could have Context a, where the a is some user decided data type behind an IORef. Hooks would be passed in this IORef a and they could store stuff in it, and this same IORef a could be passed in to the http request handler. The reason for this being that this way there would be no id-based lookups within the request handlers since this shouldn't be needed anyway -- the fact of "which TLS session is going on" is known when setting up the TLS session.

Perhaps these latter ideas are more elaborate and I could just start by adding an IORef to the Context for the possible storage of the client certificate and then we could massage it forward to a more generic solution.

@eyeinsky
Copy link

eyeinsky commented Aug 20, 2019

I forked tls and warp-tls (wai) and did these changes:

  • in tls: add a new field to the context eyeinsky/hs-tls@e9e3fcb, then add a function to get this (it doesn't have the ctx prefix and maybe what it will return will not only be client certificates, thus the more general name) eyeinsky/hs-tls@64945ee;
  • in warp-tls: this commit eyeinsky@6906b8b is the actual change, i add the TLSApplication type which takes an extra argument, which is the TLS related info. From Network.Wai.Handler.Warp.Run (between the <copy> tags) I copied in and modified a bunch of functions to accommodate for the change (I think these should be refactored somehow, but not sure how yet).

The main idea is that

  • Context has a new field in a IORef (Maybe CertificateChain), this is passed in to the onClientCertificate hook, so it can be filled there when the user wants to;
  • this same field is passed into the http request handler (the one that warp's run* functions take) as the first argument. It has type type TLSAppInfo = Maybe (I.IORef (Maybe TLS.CertificateChain)) and the outer Maybe is there because warp-tls can also do non-tls http, so there is not always a Context available.

Hope this makes sense!

Edit: wai I changes are in the tls-application branch, tls changes are on top of master, sorry about the inconsistency.

@eyeinsky
Copy link

eyeinsky commented Nov 5, 2019

@vdukhovni @kazu-yamamoto Hi! Sorry, I have been busy with other life, but would like to continue with the topic of making tls information available at request time. The need we had was to authenticate requests based on client certificates (based on the email saved in a certificate) that are installed in the browser. We got this working in my forks of hs-tls and wai and are using it.

Just to make sure -- is there any interest in getting this into these libraries? And if so then what should I do? I guess rebase both on latest master for both, then perhaps create a small test app that shows what it does? There is probably some refactoring that needs to be done, but could use some help in deciding on how to do it, what is ok and what is not, etc.

Also, maybe there is someone else I should ping and work together with?

Thank you for your attention!

@kazu-yamamoto
Copy link
Contributor

@eyeinsky Since this is a real problem, I would like to fix it.
I'm one of maintainers of both tls and warp-tls, I could help you.

The first thing you should do is send a PR of ctxClientCerts to hs-tls.

@eyeinsky
Copy link

I created the pull request here: haskell-tls/hs-tls#405

@kazu-yamamoto
Copy link
Contributor

New versions of tls, warp and warp-tls have been released. So, this issue should be fixed.

@eyeinsky
Copy link

eyeinsky commented Jan 8, 2020

Depended on the new releases in our project with positive results :). AFAIK this issue is now resolved, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants