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

libstore: Add the ability to use different auth methods with Http Binary Caches #10584

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

georgyo
Copy link
Member

@georgyo georgyo commented Apr 21, 2024

Motivation

Right now the only auth methods allowed with http caches was the default basic auth method. This meant if you wanted to have any other mechanism for authentication you requiring patching the code.

My personal motivation is enabling ?authmethod=negotiate.

Context

This PR is very similar to #10568 but made to be generic and support as many methods from libcurl as we can reasonably support.

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 Apr 21, 2024
@georgyo
Copy link
Member Author

georgyo commented Apr 25, 2024

@roberth Do you have any thoughts on this version of the PR?

@fricklerhandwerk fricklerhandwerk added the feature Feature request or proposal label May 15, 2024
@@ -56,6 +56,8 @@ struct FileTransferRequest
std::string expectedETag;
bool verifyTLS = true;
bool head = false;
std::string authmethod;
Copy link
Member

Choose a reason for hiding this comment

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

This should be an enum struct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Uploaded my attempt at this, however I don't write much C/C++ code.

I tried several different ways to make this a Setting<HttpAuthMethod> using the templates for BaseSetting, but I could not get this to work for a URL type parameters, and indeed none of the other url parameters are using enums.

@georgyo georgyo force-pushed the authmethod branch 4 times, most recently from 9943f90 to 8c2b15c Compare June 9, 2024 10:20
@georgyo georgyo requested a review from Ericson2314 June 9, 2024 17:19
@edolstra edolstra self-assigned this Jun 24, 2024
@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Jun 24, 2024

Discussed in Nix maintainer meeting:

  • @edolstra: this could done be in a more general way building on top of Pluggable authentication #9857
    • this change only enables authentication methods for the HTTP binary cache and nothing else
    • pluggable auth does not deliver the feature right now though
      • that other PR does two things actually, splitting it may make it easier to merge
  • we'd merge this PR if it didn't introduce a setting that we'd have to support forever
    • this is risky given this topic is currently under development
  • we recommend trying to implement the feature using pluggable auth, trying out the branch
    • a valuable contribution would also be to rebase the pluggable auth PR
    • another thing would be doing some research whether Git credential helpers can be used to get the authentication method

@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-06-24-nix-team-meeting-minutes-155/47739/1

@georgyo
Copy link
Member Author

georgyo commented Jun 28, 2024

An alternative approach to choosing the exact curl auth method would be to set to CURLAUTH_ANY, which is the git default. The pros here is that this will enable curl to upgrade the auth method to the most secure.

This is how Git behaves when talking to an HTTP repo and allows it to handle both Basic Auth and things like Negotiate. The downside to that approach is if the server advertise negotiation there is no way to force git to use basic.

Setting CURLAUTH_ANY would enable kerberos, (and digest/ntlm) support in nix without also introducing a flag that would have to be supported forever.

@georgyo georgyo force-pushed the authmethod branch 2 times, most recently from 1e5d8f0 to 41fd519 Compare August 12, 2024 01:03
@georgyo georgyo requested a review from edolstra as a code owner August 12, 2024 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request or proposal store Issues and pull requests concerning the Nix store
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants