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

Support for NuGet authentication plugins deployed via .NET tools #13242

Merged
merged 70 commits into from
Apr 26, 2024

Conversation

kartheekp-ms
Copy link
Contributor

Related - #12567

Rendered Spec

@kartheekp-ms kartheekp-ms requested a review from a team as a code owner February 14, 2024 01:03
@kartheekp-ms
Copy link
Contributor Author

Tagging @JohnSchmeichel, @embetten and @phil-hodgson for feedback on this spec.

@kartheekp-ms kartheekp-ms requested a review from a team February 14, 2024 01:09
Copy link
Member

@zivkan zivkan left a comment

Choose a reason for hiding this comment

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

Great spec, despite all my comments! 👍

I found the spec easy to read until the end of the functional spec section, but then I found the technical explanation and alternatives sections wordy and hard to follow. I know I have the exact same problem when I write specs (or any kind of documentation), but I think shorter descriptions would help, as more information can be counter productive.

@baronfel
Copy link

I have some longer-form thoughts on this that I want to write up to do it justice, but I'm super glad to see the NuGet team thinking about distribution of credential providers!

One key scenario that I want to highlight that a tools-based install mechanism doesn't handle well is elevated nuget operations (aka sudo commands on Linux/macOS). We currently have recurring problems here in the SDK especially around workloads whose packages are in authenticated NuGet feeds, because the entire user command (sudo workload install aspire, for example) happens as an entirely different user, and so by its very nature any user-directory-centric credential lookup scheme will not work. This is a key scenario to address in my mind because workloads are such a critical part of the .NET SDK experience these days.

Some ideas I've been bouncing around here:

  • have some secondary multi-user location where credential providers should be installed to and looked up from by default?
  • change our recommendations for .NET tools entirely to install to a non-user-local location?

@JonDouglas
Copy link
Contributor

My 2c:

I think we need something baked into the .NET SDK where a user can easily acquire it through a known dotnet NuGet command w/ respective parameters. Tools I feel can fit this job, but they would need to be surfaced through a SDK command. For example, imagine a user hits a private feed after cloning a repo, they should be empowered with some type of .NET SDK command that can in one single invocation get them on the happy path or be invoked implicitly to start that flow. The underlying implementation I think needs to be flexible enough to address various concerns in this proposal.

I think the general direction that sounds ideal is a concept of credential helpers and the configuration is built into the .NET SDK to make the out of the box experience great.

I think dotnet nuget credential-provider install Microsoft.Azure.Artifacts.CredentialProvider is the closest to that personally to acquire the various credential provider needed per the "registry" required. But I think it should all be flowed through a dotnet nuget login or similar type of entry point. Similar to Azure CLI, gcloud CLI, and even npm. Because another synergy here would be utilizing a direct nuget.org flow by default as well. See #10658 for more thoughts on that.

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

Thanks for the write-up @kartheekp-ms

I see lots of comments that can be paraphrased to "Should the plugins be written in .NET".

Is that something that needs addressed or is it just a consequence of thinking about the design leads to this possibilities.

@dsplaisted
Copy link

Looks like there's been a lot of good discussion already.

To me, having to specify the tool path when installing the tool is a bit ugly. Would it be possible for NuGet to look for authentication plugis on the PATH? That way you could install the tool as a normal global tool. Or you could acquire it any way you want and just add it to your PATH.

Since I don't think there's a fixed list of authentication plugins, I think you'd have to scan for executables matching a pattern such as NuGet.CredentialProvider.* on the PATH. We'd have to go through a security threat model on that I think (but probably we need to do so for this feature anyway).

@kartheekp-ms
Copy link
Contributor Author

To me, having to specify the tool path when installing the tool is a bit ugly.

Thanks, Daniel, for the feedback. I proposed an alternative design in #13242 (comment).

lint issues

update spec

update spec

Add support for other extensions on Windows as future possibility
@kartheekp-ms kartheekp-ms force-pushed the dev-kpms-credentialproviders-.nettools branch from b55514d to fb3b5b1 Compare March 30, 2024 04:06
@kartheekp-ms kartheekp-ms requested review from zivkan and nkolev92 April 3, 2024 23:43
@kartheekp-ms
Copy link
Contributor Author

Given that this spec has evolved since the first version, I have resolved a bunch of comments related to the previous revision of the spec to make navigating through the unresolved conversations in this pull request easier. If you have any questions about the resolved comments, please feel free to unresolve them and post a comment. I will respond as soon as possible.

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

I think this looks pretty good.

I may have some small feedback on the doc, but so consider this approval of the general concepts :)

Great job @kartheekp-ms, thanks for driving consensus.

Kartheek Penagamuri added 2 commits April 4, 2024 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants