Skip to content

libstore/filetransfer: add support for MTLS authentication #13030

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vlaci
Copy link

@vlaci vlaci commented Apr 15, 2025

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

  • 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
  • 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 👍 to pull requests you find important.

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

@vlaci vlaci requested a review from Ericson2314 as a code owner April 15, 2025 14:18
@Mic92 Mic92 added this to Nix team Apr 15, 2025
@github-project-automation github-project-automation bot moved this to To triage in Nix team Apr 15, 2025
@github-actions github-actions bot added the store Issues and pull requests concerning the Nix store label May 16, 2025
@vlaci vlaci requested a review from edolstra as a code owner May 16, 2025 12:33
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label May 26, 2025
@Mic92
Copy link
Member

Mic92 commented May 26, 2025

@vlaci added a regression test. Please check.

@Mic92
Copy link
Member

Mic92 commented May 26, 2025

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

@vlaci
Copy link
Author

vlaci commented May 29, 2025

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?

@tomberek tomberek added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Jun 4, 2025
@tomberek tomberek self-requested a review June 4, 2025 20:18
@tomberek tomberek moved this from To triage to 🏁 Review in Nix team Jun 4, 2025
@vlaci
Copy link
Author

vlaci commented Jul 18, 2025

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

@tomberek
Copy link
Contributor

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.

@vlaci
Copy link
Author

vlaci commented Jul 22, 2025

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.

@ztzg
Copy link

ztzg commented Jul 28, 2025

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!

lf- pushed a commit to lix-project/lix that referenced this pull request Jul 29, 2025
This is a collection of Lix plugins that showcase how to write one for
various usecases.

The first is a mTLS store plugin that enable mTLS cache URIs
(`https+mtls://`).

We enable meson build system support for this plugin but we are not
going to distribute it in the official packaging of Lix, we will
repackage each relevant plugin downstream in Nixpkgs.

These plugins have *NO* guarantee support, they are provided as useful
references and are possibly production-ready if your usecase is simple
enough.

Reference: NixOS/nix#13030 (this change has
resemblances but our APIs are different, the tests harness is mostly
from CppNix).

Change-Id: Ib354271981b35dff6c134b12c4748c3eaf743fcb
Co-authored-by: Jörg Thalheim <[email protected]>
Co-authored-by: László Vaskó <[email protected]>
Signed-off-by: Raito Bezarius <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. store Issues and pull requests concerning the Nix store with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: 🏁 Review
Development

Successfully merging this pull request may close these issues.

Client certificate authentication support for binary caches
4 participants