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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

libstore: Enable kerberos negotiation for http binary caches #10568

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

Conversation

georgyo
Copy link

@georgyo georgyo commented Apr 19, 2024

Motivation

It would be nice to be able to do kerberos negotiation when fetching or pushing stuff to an HTTP binary store. This PR makes it possible to add ?negotiate=true to have libcurl do krb negotiation.

I debated on just adding CURLAUTH_ANY unconditionally here, which is what git does and would automatically do krb negotiation if the server supports it, but figured that might be a too dramatic change as that could change existing behavior.

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 19, 2024
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Could you add a test for this?
I think this needs to be a NixOS test, because that makes it easy to set up the necessary services, and doesn't add dependencies to the nix package.
We don't have a NixOS test for binary caches, so you could add a new one under tests/nixos.

Comment on lines +17 to +18
const Setting<bool> negotiate{this, false, "negotiate",
"Whether to do kerberos negotiate when talking to the http binary cache."};
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be a configuration item, or could it be autodetected?

I think negotiate is a bit vague. Would kerberos be a better name?

Copy link
Author

Choose a reason for hiding this comment

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

We could add CURLAUTH_ANY instead of CURLAUTH_NEGOITATE, which would do the detection for you. We could add that flag at all times, and libcurl will use the best available authentication it finds. This is what git does, and that works pretty well. The downside is that if the HTTP server will accept negotiation OR another protocol, libcurl will always choose negotiation, which has a very slim chance of breaking use cases. This may be a very acceptable risk as git has been doing this for a decade already.

negotiate is the best name here, it matches the curl command line argument --negotiate, and is widely known as that.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not too familiar with that, but it's generally good to mirror existing interfaces.
What about CURLAUTH_ANYSAFE ?
Maybe we should similarly expose a single auth setting, instead of specific ones for each possible solution.

Copy link
Author

Choose a reason for hiding this comment

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

A decent idea, I've created #10584 as an alternate proposal to this one.

@roberth roberth added the feature Feature request or proposal label Apr 21, 2024
@georgyo
Copy link
Author

georgyo commented Apr 21, 2024

Could you add a test for this? I think this needs to be a NixOS test, because that makes it easy to set up the necessary services, and doesn't add dependencies to the nix package. We don't have a NixOS test for binary caches, so you could add a new one under tests/nixos.

Testing kerberos/negotiation auth is notoriously hard. It requires setting up a KDC, generating service keytabs, users keytabs and configuring servers to do it. In the end we are testing a single option to libcurl, of which libcurl is very well tested.

Creating a nixos test for this would take more time than I can devote to this feature.

@roberth
Copy link
Member

roberth commented Apr 21, 2024

this would take more time than I can devote to this feature.

NixOS does come with kerberos.nix test for NFS, which presumably has most of the setup required, so maybe you want to reconsider, but I can respect your argument.

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.

None yet

2 participants