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

[Tech Spec] Supporting Bearer Auth #14174

Open
wants to merge 24 commits into
base: dev
Choose a base branch
from

Conversation

jgonz120
Copy link
Contributor

Tech spec for #12877

@jgonz120 jgonz120 requested a review from a team as a code owner March 10, 2025 21:21
@jgonz120 jgonz120 requested review from Copilot and removed request for a team March 10, 2025 21:21
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR introduces a tech specification for supporting bearer token authentication for NuGet credential providers. The document outlines the functional and technical changes needed to update the NuGet CLI to support bearer auth, discusses drawbacks, and presents alternative schemes.

Reviewed Changes

File Description
accepted/2025/supporting-bearer-auth.md New tech spec document detailing changes to enable bearer token auth

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

@jgonz120 jgonz120 requested review from joelverhagen, kartheekp-ms and a team March 10, 2025 21:22

When deciding which protocol to use for authentication we will use a combination of the headers provided and the results from ICredentials.GetCredential.
If the header indicates the server accepts basic, and GetCredential returns a result for basic, we will use that for the request.
Similarly, if the header accepts bearer, and GetCredentials returns a result for bearer, we will use that.
Copy link
Member

Choose a reason for hiding this comment

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

Where will the token be stuffed? In the password field? Seems like an alternative is to have a simple new AuthorizationHeader property. This essentially gives the cred provider full control on setting the Authorization header on the next request side stepping the GetCredential and ICredential interface entirely.

I am reading between the lines here, but it seems that ICredential flow will NOT be used in conjunction with HttpClient when bearer is returned. Instead https://learn.microsoft.com/en-us/dotnet/api/system.net.http.headers.httprequestheaders.authorization?view=net-9.0 will be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, HttpClient doesn't support bearer tokens with ICredential. I love the idea of offloading all the responsibility of setting the headers to the cred provider! I'll check with @embetten to see what they think.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this more. Should there be symmetry between WwwAuthenticateHeader in the Request model and a AuthorizationHeader in the Response model?

So WwwAuthenticateHeader would provide most context the cred provider would need and AuthorizeHeader would explicitly side step the username+password+ICredential+NetworkCredential path.

But there would be two ways to do that same thing for, say, basic auth: ICredential+NetworkCredential+HttpClientHandler or an AuthorizeHeader = basic base64(username : password)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, if we went with this route I would say if we received an AuthorizeHeader from the cred provider we use that, if it's null, we check for the ICredential.

Choose a reason for hiding this comment

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

The artifacts-credprovider needs to call the ADO server to retrieve custom headers, including the WwwAuthenticateHeader. Even if the WwwAuthenticateHeader was provided, we would need to call our server, and we typically know what type of auth our feeds support. But I'm not sure how unique our situation is here.

Returning a token and auth type would be preferred to a new AuthorizationHeader property. In our experience, its advantageous to have the credprovider layer be as simple as possible. Considering that clients may change and place tokens in different locations over time (such as HTTP headers, API keys, directly in NuGet configs, or in URLs like pip), I think keeping the format of the returned token consistent enables more flexibility across versions. For all the other packaging clients we integrate with we return just the token, and this would increase the NuGet specific code we would need to maintain.

Copy link
Member

Choose a reason for hiding this comment

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

Even if the WwwAuthenticateHeader was provided, we would need to call our server, and we typically know what type of auth our feeds support

I don't know if you can get into it publicly, but the point of the WWW-Authenticate header is for servers to tell clients what auth schemes the server supports. I'm guessing by this comment that ADO is therefore using some non-standard way of sharing the same information.

Also, similar lines, sometimes I think about writing a cred provider for github, but it's impossible to know, solely from the URL, whether the server is a self-hosted GitHub Enterprise server. So, if NuGet can pass all HTTP headers to the cred provider, then it will help all cred providers for any server that can be hosted on-prem (or on a cloud hosted, but on a private domain).

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I just tried a curl -v to an Azure Artifacts private feed, and I see 3 www-authenticate headers:

www-authenticate: Bearer authorization_uri=https://login.windows.net/*****
www-authenticate: Basic realm="https://*******/"
www-authenticate: TFS-Federated

But, I also see a bunch of x-tfs-* and x-vss-* headers about auth realms, and redirect URLs.

So, I think this reinforces the point that for some cred providers to know how to auth correctly, they need the full list of headers, because the server might be using custom headers that the cred provider needs to make decisions.

Copy link
Member

Choose a reason for hiding this comment

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

It seems like there is a spectrum of options, ranging from:

  • Simple cred provider interface, where access to 401 response headers and subsequent request heads is minimal. Example: stuff bearer token in the current password, no new WwwAuthenticateHeader/AuthorizationHeader properties.

to

  • Powerful cred provider interface, where the cred provider can get and set a lot of headers. Example: put all 401 response headers in the GetAuthenticationCredentialsRequest and allow any request header to be set on the subsequent responses with a property bag on GetAuthenticationCredentialsResponse (this would allow custom auth models beyond WWW-Authenticate + Authorization).

Personally, I would like to move more to that 2nd extreme than stick to the 1st (which is where we are right now prior to this design). Putting all headers in the protocol is a more flexible protocol allowing custom auth schemes, but there are a lot of options in between that would meet the goals of OIDC.

What's not clear to me is what flavor of the protocol change aligns with NuGet client team's long-term goals and what would accommodate enhancements or "purifications" in the Artifacts cred provider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it could make sense to provide all the response headers we received to the cred provider, regardless of which option we pick.

We could optimize for flexibility and allow the credential providers to return a network credential or the full auth header.

Update GetAuthenticationCredentialsRequest to include HttpResponseHeaders ResponseHeaders

Update GetAuthenticationCredentialsResponse to include HttpRequestHeaders/NameValueCollection RequestHeaders

Update CredentialResponse to include RequestHeaders

This will allow the credential provider to specify any headers needed to auth, even custom ones.

NuGet.Client will still need to check for values in Username and Password fields since that will be used by legacy credential providers.

So if we see that RequestHeaders is empty from the credential provider we use Username/Password + AuthenticationTypes combination to decide what we do.

Something like this

if(CredResponse.RequestHeaders is not null && CredResponse.RequestHeaders.Count > 0) {
    foreach(var header in CredResponse.RequestHeaders) {
        request.AddHeader(header.key, header.value);
    }
}
else if(CredResponse.Credential.Get("basic") == null) { 
    var bearer = CredResponse.Credential.Get("bearer");
    if(bearer is not null)  {
        request.AddHeader("bearer", bearer");
    }
}
//do other stuff and make a request 🤷

We can handle both, and we can message clearly that we are adding support for bearer tokens but moving forward if credential providers want to handle another type of authentication, they need to be updated to request the request headers necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I like this solution a lot. It means a new provider could even add X-NuGet-ApiKey for an old service. I recommend picking a simpler model for RequestHeaders/ResponseHeaders the more clearly maps to the JSON stdout/stdin bidirectional pipe. Perhaps a List of { name: "", value: "" } POCOs or an Dictionary<string, string[]>. Technically header names are not unique in the list of request/response headers. The HttpHeaders type does validation on HTTP header which can cause pain. I think treating them as strings and doing TryAddWithoutValidation in client code on the returned RequestHeaders would be most flexible. But I'm touching on impl. details.

LGTM.

joelverhagen
joelverhagen previously approved these changes Mar 11, 2025
Copy link
Member

@joelverhagen joelverhagen left a comment

Choose a reason for hiding this comment

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

Looks good. It is a relatively high level doc with some details left to the implementation. I am fine with this if your team is. Seems like some things are best figured out once you see how the code is factored.

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.

just nitpick comments, but hardly surprising since I talked to you about this before you wrote the spec 😁

jgonz120 and others added 6 commits March 12, 2025 10:26

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Andy Zivkovic <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Andy Zivkovic <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Andy Zivkovic <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
jgonz120 and others added 6 commits March 12, 2025 10:56

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Andy Zivkovic <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Andy Zivkovic <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a technical specification detailing support for bearer token authentication in NuGet’s credential providers.

  • Introduces a new spec document outlining the requirements and technical rationale.
  • Describes the updated authentication behaviors and the limitations of the current .NET HttpClientHandler.
  • Lists alternatives and future possibilities regarding credential handling.
Comments suppressed due to low confidence (1)

accepted/2025/supporting-bearer-auth.md:50

  • The interface is referenced as 'ICredential', but the linked documentation refers to 'ICredentials'; please update for consistency.
Even though we are adding support for bearer tokens, the ICredential.GetCredential explicitly returns a NetworkCredential, which requires a username and password.

jgonz120 and others added 2 commits March 13, 2025 11:05

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Copilot <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Copilot <[email protected]>
jgonz120 and others added 2 commits March 13, 2025 13:50

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Andy Zivkovic <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Andy Zivkovic <[email protected]>
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.

None yet

6 participants