libstore/filetransfer: add support for MTLS authentication #13030
Conversation
Certificate/private-key pair can be configured globally and it will be handled by libcurl.
| @@ -371,6 +371,11 @@ struct curlFileTransfer : public FileTransfer | |||
| curl_easy_setopt(req, CURLOPT_SSL_VERIFYHOST, 0); | |||
| } | |||
|
|
|||
| if (settings.clientCertFile != "" && settings.clientKeyFile != "") { | |||
| curl_easy_setopt(req, CURLOPT_SSLCERT, settings.clientCertFile.get().c_str()); | |||
There was a problem hiding this comment.
Is there a privacy implication if we start sending this to all binary caches unconditionally?
There was a problem hiding this comment.
My understanding is that the server asks the client to present its certificate by sending the list of accepted issuers in a CertificateRequest message. It won't be presented, unless the configured certificate matches the requested issuer.
There was a problem hiding this comment.
Would this allow to have multiple ssl certificates in a single file and curl than uses whatever is requested? Than there would be a reasonable way to support multiple caches with different certs each.
There was a problem hiding this comment.
There was a problem hiding this comment.
- We could add a config mapping similar to
trusted-public-keysmapping domains to certificate paths. A cert and private key can be put in the same file. - or we could also support a directory structure similar to what docker uses for mtls. This clashes with the existing server CA configuration option a bit, though.
There was a problem hiding this comment.
However I think if this just becomes a Store setting and part of the substitutes url, this should make a new settings system harder than what we currently have.
I am not sure, I understand this sentence. Should I try to reimplement this in a similar way as #12976?
There was a problem hiding this comment.
Yes. Updated my response. If we can specify this per URL than it's more flexible because it doesn't make sense to have this as a global configuration.
There was a problem hiding this comment.
Sorry for not jumping on this earlier, but you may want to steal bits from these two commits, which implement per-substituter mTLS via ...?ssl-cert=...&ssl-key=... (the semantics are cURL's):
https://github.com/ztzg/nix/commits/xt-16104-iwei-mtls-store/
There was a problem hiding this comment.
@ztzg looks good. I only don't understand why this adds ssl certificates for file://
There was a problem hiding this comment.
Hi @Mic92,
I only don't understand why this adds ssl certificates for file://
I suppose you are referring to this fragment:
bool absolute = hasPrefix(path, "https://") || hasPrefix(path, "http://") || hasPrefix(path, "file://");
The expression was just lifted from the existing code to store its value; its semantics haven't changed.
Note the subsequent if (!absolute) [...]: the certificate and key are not configured for such "absolute" requests, as their URL components would not be applied to the substituter's "base URL."
(There are probably better ways of filtering out absolute URLs, but I haven't tried improving that.)
Certificate/private-key pair can be configured globally and it will be handled by libcurl.
Motivation
In our setup, we use client certificate authentication extensively to access company resources. It would be very easy for us to deploy a binary cache and setup authentication the same way.
Context
Fixes #13002
Questions
I've gone with that as I don't think, that many people use different certificates for different HTTPS services, and implementing per-domain certificate handling would greatly complicate the implementation
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.