Add the ability to load TLS credentials from IORefs #806
Conversation
There was a problem hiding this comment.
@kazu-yamamoto do you have any issues with the changes here?
|
I can bump the version to |
|
LGTM. @snoyberg Should we bump the version to 3.3.0? |
|
This should close #463 |
|
Yes, let's go for the 3.3.0 version bump. |
|
Done. |
|
I would like to test this code for a while. After that I will take care of merging and releasing. |
|
v3.3.0 has been released. Thank you for your contribution! |
| data CertSettings | ||
| = CertFromFile !FilePath ![FilePath] !FilePath | ||
| | CertFromMemory !S.ByteString ![S.ByteString] !S.ByteString | ||
| | CertFromRef !(I.IORef S.ByteString) ![I.IORef S.ByteString] !(I.IORef S.ByteString) |
There was a problem hiding this comment.
I'm rather late to party, sorry about that, just noticed this PR today, and sadly it has been merged for some time now. And yet perhaps I should make my comments anyway. My first issue is with the choice of separate IORefs for each of the cert, chain and keys. I think that's likely a mistake. Rather I think that the IORef representation should have been:
CertFromRef !(I.IORef (S.ByteString, [S.ByteString], S.ByteString))That is, a single IORef holding a triple with the certificate, issuer chain, and key. This would allow atomic retrieval of the triple, in which one can be confident that all three objects correspond to each other. The current design is subject race conditions between reader and writer, because the writer updates multiple IORefs separately, and the reader may see an inconsistent view of the data. :-(
| tlsSettingsRef | ||
| :: I.IORef S.ByteString -- ^ Reference to certificate bytes | ||
| -> I.IORef (S.ByteString) -- ^ Reference to key bytes | ||
| -> TLSSettings |
There was a problem hiding this comment.
I would have argued strongly against providing this function. The most widely used CA on the planet is now "Let's Encrypt" and it along with almost all the other CAs does not sign the leaf certificate directly with a trusted root CA. Instead pretty much all certificates that chain up to one of the usual WebPKI CAs are signed by an intermediate CA.
When you have a certificate signed by an intermediate CA, while various browsers are willing to pay the cost of dynamically locating intermediate CA certs from various certificate extensions, and may have an intermediate cert cache, other software tools (e.g. the ubiquitous curl, etc.) are not, and the TLS specifications DO NOT make intermediate certificates optional. They are a required part of the server's certificate chain.
Here the caller can always provide an empty chain if that's what they really want to do, but there's no need to encourage such negligence by providing an API entry point that can only do the wrong thing. The more general entry point below is the only one that should have been offered.
| tlsSettingsChainRef | ||
| :: I.IORef S.ByteString -- ^ Reference to certificate bytes | ||
| -> [I.IORef S.ByteString] -- ^ Reference to chain certificate bytes | ||
| -> I.IORef (S.ByteString) -- ^ Reference to key bytes | ||
| -> TLSSettings |
There was a problem hiding this comment.
And of course in this function, there should have been just a single IORef holding a triple of (Cert, [Chain], Key) bytestrings/lists of bytestrings.
| CertFromRef certRef chainCertsRef keyRef -> do | ||
| cert <- I.readIORef certRef | ||
| chainCerts <- mapM I.readIORef chainCertsRef | ||
| key <- I.readIORef keyRef | ||
| cred <- either error return $ TLS.credentialLoadX509ChainFromMemory cert chainCerts key | ||
| return $ TLS.Credentials [cred] |
There was a problem hiding this comment.
Which brings us to perhaps the most important issue. What is the purpose of the IORef alternative, if ultimately loadCredentials converts them (if I'm not mistaken just once) immediately to a set of pure objects used in:
runTLSSocket :: TLSSettings -> Settings -> Socket -> Application -> IO ()
runTLSSocket tlsset set sock app = do
credentials <- loadCredentials tlsset
mgr <- getSessionManager tlsset
runTLSSocket' tlsset set credentials mgr sock appThis still means that the application loop needs to be restarted when new certificates are available, and so I fail to see the difference between this CertFromMemory. What does the indirection via IORefs achieve?
It seems like there's much missing code here to make a practically useful deployment model. Perhaps the idea is to also provide a restart loop (I may start work on that shortly) and have the restart loop be configured once with these (or ideally just one) IORefs, allowing another thread to just worry about updating the data as needed?
If so I think that code should probably have appeared concurrently with or even before this PR, which would have made it easier to see whether this API is the right one for the job.
[ My tentative loop restart implementation idea will only work on POSIX systems that support dup(2) on socket file descriptors. Which precludes Windows. I hope that having reasonably clean certificate reload without dropping any connections even briefly, but just on Unix-like systems is an acceptable design choice. More ambitious designs would likely require invasive changes in the TLS library, which are perhaps a good idea, but I'm not planning to go there at this time. ]
This PR adds three new configuration fields to
TLSSettingsfor retrieving the certificate bytes, chain certificate bytes, and key bytes fromIORefs. This is potentially useful for scenarios where the certificate may need to change at runtime, e.g. if ACME is used. In theory, this would prevent an ACME implementation from having to terminate the server thread in order to callrunTLSwith new TLS credentials. However, there is currently no ACME implementation in Haskell (to my knowledge), so this is a somewhat speculative PR.