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

Pluggable authentication #9857

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

Conversation

edolstra
Copy link
Member

@edolstra edolstra commented Jan 26, 2024

Motivation

This PR makes several improvements to HTTP authentication in Nix:

  • It allows the use of Git credential helpers to obtain authentication data (issue Credentials provider support for builtins.fetch* #8635). This is configured by the new auth-sources setting. For instance, adding extra-auth-sources = git-credential-libsecret allows Nix to obtain usernames/passwords from the KDE/Gnome keyrings.
  • It adds another builtin authentication source (enabled by adding builtin:nix to auth-sources), which reads usernames/passwords from files in ~/.local/share/nix/auth. The advantage over the netrc authentication source (now known as builtin:netrc) is that it has a file per secret, which makes it easier for scripts/installers to add authentication data.
  • The client will now interactively ask the user for a username/password (via $SSH_ASKPASS) and store this in the configured auth-sources for future use.
  • The Nix daemon can now request authentication data from the client (for trusted users only). E.g. if the user passes --substituters http://my-cache, the daemon will trigger the client to ask the user for the username/password for that cache.
  • Similarly, builtin:fetchurl requests authentication from the parent (outside of the sandbox), so e.g.
    nix build --expr 'import <nix/fetchurl.nix> { url = https://httpbin.org/basic-auth/foo/bar; hash = "sha256-zRy0rqgpwiXQg6mBNsKUqDHHNkW/Dk+3/QHdUlrBURg="; }'
    
    now works.

Context

Priorities and Process

Add 👍 to pull requests you find important.

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

@github-actions github-actions bot added the store Issues and pull requests concerning the Nix store label Jan 26, 2024
Comment on lines +16 to +43

template<typename T>
std::optional<T> readOptional(const StoreDirConfig & store, WorkerProto::ReadConn conn)
{
auto tag = readNum<uint8_t>(conn.from);
switch (tag) {
case 0:
return std::nullopt;
case 1:
return WorkerProto::Serialise<T>::read(store, conn);
default:
throw Error("Invalid optional tag from remote");
}
}


template<typename T>
void writeOptional(const StoreDirConfig & store, WorkerProto::WriteConn conn, const std::optional<T> & x)
{
if (!x.has_value()) {
conn.to << uint8_t{0};
} else {
conn.to << uint8_t{1};
WorkerProto::Serialise<T>::write(store, conn, *x);
}
}


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you have the same code in serialize.hh?

Comment on lines 238 to 240
template<typename T>
DECLARE_WORKER_SERIALISER(std::vector<T>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can make a partial template specialization like this for std::optional, and then you won't need the std::optional<auth::AuthData> instance. (Can also turn std::optional<TrustedFlag> into one for just TrustedFlag too.)

I used to have that but I removed it because I only had the special-cases instances at the time, but now we do have a use case for it.

@Ericson2314
Copy link
Member

Separate from the low level protocol minutiae, I think I've considered solving the same problem in a different way, which is having a user-specific remote builder for fixed output derivations. See #9344

@edolstra
Copy link
Member Author

Client-side building would be great but is a much bigger project (and probably wouldn't work on platforms like macOS).

@Ericson2314
Copy link
Member

Ericson2314 commented Jan 26, 2024

I don't think it's that big of a project. It's just a client side build + use daemon to add to store. (And the current registerValidPaths is awfully close to that.)

I think for fixed-output derivations without references it should work on macOS just fine. Since there are no references, there is no need to do fake roots / make $out appear in the store dir.

@github-actions github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels Jan 27, 2024
@edolstra edolstra force-pushed the pluggable-auth branch 2 times, most recently from 3487e19 to 703871a Compare February 3, 2024 14:19
@edolstra edolstra changed the title WIP: Pluggable authentication Pluggable authentication Feb 7, 2024
@edolstra edolstra marked this pull request as ready for review February 7, 2024 15:20
mk/templates.mk Outdated Show resolved Hide resolved
mk/templates.mk Outdated Show resolved Hide resolved
Nix can now read authentication data from
~/.local/share/nix/auth/*. The files in that directory have the
format:

  protocol=https
  host=my-cache.example.org
  username=alice
  password=blabla

Also, the Nix daemon can now call back to the client to get
authentication data from the client (if it's trusted). This makes it
possible to have the daemon substitute from an authenticated binary
cache, with the authentication coming from the client.

Issue NixOS#8635.
This is similar to 'git credential fill', i.e. it tries to fill an
authentication request using the configured authentication
sources. Mostly useful for testing.
@thufschmitt
Copy link
Member

Triaged during the Nix maintainers meeting on 2024-02-16.
To discuss further

  • Needs more docs, probably also more tests
  • Requires careful review

To discuss

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-02-16-nix-team-meeting-minutes-124/39870/1

@arianvp
Copy link
Member

arianvp commented Feb 16, 2024

This looks great! I have one question:

The Nix daemon can now request authentication data from the client (for trusted users only). E.g. if the user passes --substituters http://my-cache, the daemon will trigger the client to ask the user for the username/password for that cache.

Could we also do this for the s3:// substituter? It is super unergonomic that when e.g. you want to push to s3 bucket in CI that the AWS credentials need to be available to the nix daemon instead of the caller. e.g. this doesn't work if you have a nix-daemon in CI:

steps:
- uses: DeterminateSystems/nix-installer-action@main
# this step sets some AWS_ env vars
- uses: aws-actions/configure-aws-credentials@main
  with:
    role-arn: my-role
# This doesn't work as nix-daemon cant access the env vars
- run: nix copy --to s3://my-bucket

Instead you need to massage the credentials into a /root/.aws/credentials file which is awkard

@edolstra
Copy link
Member Author

Team meeting notes:

  • Needs more documentation.
  • Maybe the extra Unix domain socket can be replaced with a new STDERR_* message to have the daemon request auth from the client. Then it would work over ssh-ng as well.
  • Should be an experimental feature.
  • Currently auth tunneling is only supported for trusted users, but it might be okay to support it for all users. (Though we have to be careful with e.g. Attic since we don't want the client to override the user name used to connect to Attic, since then the server will return a different (possibly untrused) slice of the binary cache.)
  • Maybe the client should ask the user to approve sending credentials to the daemon, so that e.g. a remote SSH daemon cannot exfiltrate arbitrary secrets from the client.

@arianvp
Copy link
Member

arianvp commented Feb 23, 2024

Sorry I need to clarify on my previous comment:

nix copy --to s3:// works fine as it doesn't involve the nix daemon

but setting an s3 bucket as a substituter e.g.:


nix build --extra-substituters s3://my-bucket

doesn't work as the nix daemon doesn't have access to my local environment variables or my $HOME/.aws folder

So it would be cool if we can use the solution for the https substituter for the s3 substituter as well?

Edit:

I think it's not that easy actually. As all the credential logic is in the AWS SDK itself... So maybe this is not feasible

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-02-19-nix-team-meeting-minutes-126/40308/1

@edolstra
Copy link
Member Author

edolstra commented Mar 8, 2024

Some status updates:

  • There are now two experimental features: auth-forwarding (whether to support forwarding auth from the client to the daemon) and pluggable-auth (whether to support credential helpers).
  • There is a setting store-auth that controls whether to attempt to store interactively prompted auth data into one of the auth sources.
  • The ~/.local/share/nix/auth auth source now supports storing auth data.
  • There is a setting auth-forwarding that determines what users are allowed to forward auth to the daemon: false (nobody), trusted-users or all-users.
  • More tests.

@szlend
Copy link
Member

szlend commented Mar 9, 2024

Some thoughts:

  • From what I can tell, this only does plain http auth, but most services these days authenticate through headers. GitHub (Authorization), GitLab (Private-Token/Job-Token), etc. So ideally there was a way to configure the authentication method as well. But this can be added later on, just worth keeping this in mind.
  • I still think client-side fetching is the preferred long-term solution as it solves a lot of security concerns of passing around credentials. Especially when considering remote builds. If passing credentials to the daemon gets stabilized, it might end up having to support both methods. So worth being careful with how this is exposed. I know this is behind an experimental flag, but given how important this feature is, I expect this will get adopted by a lot of people very soon. Edit: I guessauth-forwarding and pluggable-auth are separate features so they can be stabilized separately.

@lorenzleutgeb
Copy link
Member

lorenzleutgeb commented May 21, 2024

IMO this goes in the wrong direction. Introduces lots of complexity to the Nix codebase. Instead, I suggest working on making fetchers as a whole pluggable. The complexity around builtins.fetchGit should be reduced, and it be used as a "bootstrap fetcher" to fetch fetchers. The fetched fetchers can then do more fancy things such as asking for credentials.

I comment motivated by the exchange in #10567.

cc @roberth

@roberth
Copy link
Member

roberth commented May 21, 2024

Introduces lots of complexity to the Nix codebase

I'm sympathetic to that.

"bootstrap fetcher" to fetch fetchers

This has security implications; any Nixpkgs package (or otherwise) could then steal credentials, by doing a builtin fetch with a malicious fetching plugin. The beauty of fixed-output derivations is that they sandbox the fetching logic, preventing this.
We should take great care to only un-sandbox or pass credentials to fetchers that are trustworthy.
That doesn't mean that opening a devshell on an unknown repo is safe (it's not); it's a defense in depth measure.

Comment on lines +1177 to +1179
/* Set up authentication tunneling for builtin:fetchurl. FIXME:
maybe we want to support this for arbitrary fixed-output
derivations. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* Set up authentication tunneling for builtin:fetchurl. FIXME:
maybe we want to support this for arbitrary fixed-output
derivations. */
/* Set up authentication tunneling for builtin:fetchurl.
We generally do not want to pass credentials to arbitrary
fixed output derivations, because that makes it possible
for any dependency (however deep down) to steal them. */

We'd need to come up with a scheme to safely pass authorization (with letter z) to specific derivations in such a way that the user controls where their credentials go; perhaps through some sort of unforgeable token that's passed around explicitly in expressions? Something for another day.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OT: notably we can't store the unforgeable token in the drv itself, because that would expose it to be read with readFile foo.drvPath.

This seems to be another instance where we should be tracking transient derivation metadata, just like we should have been doing with meta.timeout or perhaps the licenses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-cli Relating to the "nix" command security Security-related issues store Issues and pull requests concerning the Nix store with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: ⚖ To discuss
Development

Successfully merging this pull request may close these issues.

None yet

9 participants