nix icon indicating copy to clipboard operation
nix copied to clipboard

libstore/filetransfer: add support for MTLS authentication

Open vlaci opened this issue 7 months ago • 3 comments

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

  • [x] ~Is it okay to support only one certificate globally?
    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~
    It can be now configured per substituter
  • [x] ~I've added the settings to globals, as netrc and CA verification settings are already there. IDK if that is the right place.~
    Settings are now substituter specific

Add :+1: to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

vlaci avatar Apr 15 '25 14:04 vlaci

@vlaci added a regression test. Please check.

Mic92 avatar May 26 '25 10:05 Mic92

Have a look later at the test... Only tested without nix-build.

Mic92 avatar May 26 '25 12:05 Mic92

Have a look later at the test... Only tested without nix-build.

There is no openssl and python in the test derivation. I am not familiar with Nix's test suite. Should there be an additional layer of indirection using i.e nix-shell shebang or nix-run inside the test script?

vlaci avatar May 29 '25 08:05 vlaci

For the sake of moving this forward, I've just added to required curl, python3 and openssl dependencies to functional tests.

vlaci avatar Jul 18 '25 11:07 vlaci

I'm not clear about the implications of:

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."

Otherwise, this looks good.

tomberek avatar Jul 21 '25 20:07 tomberek

I'm not clear about the implications of:

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."

Otherwise, this looks good.

My understanding that it is done that way to ensure that the cert is used only if the request will go to cacheUri. Maybe the variable could have a more expressive name, but I don't really understand when the path should be requested from the cache and when not.

Also, the CI test timing out is not a fluke: the same full test run gets stuck locally as well. I am yet to figure out why and where, as the newly added test seemingly runs to completion.

vlaci avatar Jul 22 '25 10:07 vlaci

Hi @tomberek, @vlaci,

I'm not clear about the implications of:

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."

Otherwise, this looks good.

My understanding that it is done that way to ensure that the cert is used only if the request will go to cacheUri.

Right. Note that the conditions under which cacheUri is used have not changed; that check was previously "inlined" in the FileTransferRequest constructor.

It seems prudent not to indiscriminately apply the configured certificates to other hosts.

Maybe the variable could have a more expressive name, but I don't really understand when the path should be requested from the cache and when not.

I don't know under which circumstances HttpBinaryCacheStore::makeRequest is invoked with absolute HTTP(S) or file: URLs, but it doesn't seem to be under the normal "substituter" use-case. Any clues would be welcome!

ztzg avatar Jul 28 '25 12:07 ztzg