Rebase of #9857 "Pluggable authentication"#16087
Open
Mic92 wants to merge 7 commits into
Open
Conversation
github-actions
Bot
added
new-cli
Mic92
force-pushed
the
pluggable-auth-reimpl
branch
from
f907a51 to
9a24130
Compare
xokdvium
reviewed
Jun 30, 2026
Mic92
force-pushed
the
pluggable-auth-reimpl
branch
6 times, most recently
from
e0fbcc8 to
a342e7a
Compare
Mic92
force-pushed
the
pluggable-auth-reimpl
branch
2 times, most recently
from
0b0fbdc to
074eb33
Compare
Mic92
commented
Jun 30, 2026
| `auth-sources`: | ||
|
|
||
| ``` | ||
| auth-sources = builtin:nix git-credential-gh |
Member
Author
There was a problem hiding this comment.
Tested this now end-to-end.
Member
Author
|
Found a race condition breaking one of the tests while doing this change: #16088 |
Contributor
|
Slightly related #15609. |
Credential helpers (git-credential protocol) receive their request on stdin. Add an optional input buffer to RunOptions and wire it through the Unix runProgram2 implementation so callers can pass small payloads without spawning a helper process themselves. Assisted-by: Claude:unspecified Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Mic92
force-pushed
the
pluggable-auth-reimpl
branch
from
074eb33 to
bb83f25
Compare
Member
Author
|
@edolstra any comments? |
xokdvium
reviewed
Jul 7, 2026
Comment on lines
+322
to
+324
| /// Credentials resolved by `setupAuth()`, kept so they can be | ||
| /// rejected if the server returns 401. | ||
| std::optional<auth::AuthData> authData; |
Contributor
There was a problem hiding this comment.
Why is this stored in the request? Ideally the request is immutable...
Member
Author
There was a problem hiding this comment.
I didn't change this from the original PR.
xokdvium
reviewed
Jul 7, 2026
Comment on lines
+373
to
+379
| if (options.input) { | ||
| in.readSide.close(); | ||
| /* Input is written in full before draining stdout, so this only | ||
| works for payloads small enough not to fill the stdout pipe. */ | ||
| writeFull(in.writeSide.get(), *options.input); | ||
| in.writeSide.close(); | ||
| } |
Contributor
There was a problem hiding this comment.
Do we need a pipe at all? Can we stuff the input into something like a memfd / something akin to that?
Member
Author
There was a problem hiding this comment.
I think a pipe is the write tool for this job. I don't see the advantage of using non-portable code. Our input will be smaller than pipe buf size.
xokdvium
reviewed
Jul 7, 2026
Comment on lines
+297
to
+312
| static auto authenticator = ({ | ||
| std::vector<ref<AuthSource>> authSources; | ||
|
|
||
| for (auto & s : authSettings.authSources.get()) { | ||
| if (hasPrefix(s, "builtin:")) { | ||
| if (s == "builtin:nix") | ||
| authSources.push_back(make_ref<NixAuthSource>()); | ||
| else | ||
| warn("unknown authentication source '%s'", s); | ||
| } else | ||
| authSources.push_back(make_ref<ExternalAuthSource>(s)); | ||
| } | ||
|
|
||
| make_ref<Authenticator>(std::move(authSources)); | ||
| }); | ||
| return authenticator; |
Contributor
There was a problem hiding this comment.
Having even more globals doesn't make me feel great... Proliferating this bad design even further isn't great - filetransfer filetime must be closely bound to the filetransfer instance. It's an existing problem of course, but it would be nice if we could pay off the tech debt at some point without piling more of it on top... rant over
Introduce an authentication framework that obtains user names and
passwords for HTTP requests from a configurable list of sources
('auth-sources'). The builtin 'builtin:nix' source reads credentials
from ~/.local/share/nix/auth; any other entry is an external credential
helper speaking Git's git-credential protocol.
The Authenticator caches results, can prompt interactively via
$SSH_ASKPASS when a credential is required, optionally stores prompted
credentials back to a source ('store-auth'), and can reject credentials
that the server refused so they are not reused.
This replaces the previous netrc-only model with an extensible mechanism;
netrc handling in the file transfer layer is left untouched.
Assisted-by: Claude:unspecified
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
File transfers now consult the authentication framework for a user name and password unless credentials were set explicitly (e.g. S3 access keys). Credentials are matched against the request's protocol, host and path. On a 401 response the credentials are rejected so they are not reused on subsequent requests. This gives eval-time fetchers (builtins.fetchTree and flake inputs over HTTP) access to the configured authentication sources, since they go through the same file-transfer layer. Assisted-by: Claude:unspecified Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Expose the authentication framework via a git-credential 'fill' compatible command, so credentials can be inspected and credential helpers tested. With --require it prompts interactively when no source provides the credential. Assisted-by: Claude:unspecified Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Exercise credential lookup, interactive prompting via $SSH_ASKPASS, store-auth persistence and external credential helpers. Assisted-by: Claude:unspecified Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
fill() held a mutex while forking external helpers, serialising concurrent transfers behind one slow helper. Guard only the cache with Sync<> and drop the unused add/setAuthSource mutators. reject() forked helper `erase` from the curl worker thread on 401, stalling all transfers; make it a cache-only invalidation. Assisted-by: Claude:unspecified
Move the authenticator ref and resolved AuthData off FileTransferRequest onto curlFileTransfer / TransferItem, so the request stays a plain input and no longer reaches for a global in its constructor. auth.hh drops out of the public header. Assisted-by: Claude:unspecified
Mic92
force-pushed
the
pluggable-auth-reimpl
branch
from
b327d7e to
996614b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a rebase of #9857 "Pluggable authentication".
I dropped the nix-daemon part since the pluggable authentication is still quiet useful for fetchtree use.