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

[API Proposal]: HttpClientHandler.Credentials should support Bearer auth type #91867

Open
zivkan opened this issue Sep 11, 2023 · 6 comments
Open
Labels
area-System.Net.Http enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@zivkan
Copy link
Member

zivkan commented Sep 11, 2023

Background and motivation

ASP.NET Core has middleware to support JWT authentication, however, it expects the token to be supplied in a Authorization: Bearer <token> header. By default it doesn't work with Authorization: Basic <encoded username:password>. Personally, I wouldn't be surprised if this is also true for other web development frameworks as well.

Meanwhile, NuGet provides a credential plugin system to allow NuGet to work with servers that expect HTTP basic, NTLM, Digest/Negotiate, and theoretically anything that HttpClient(Handler) supports. The way it does this is with a class that implements ICredentials (the BCL's CredentialCache class is kind of similar, but also different, but for an example's purpose, it's close enough), and the NuGet calls out to credential providers to fill in the details as needed.

This is because System.Net's AuthenticationHelper class hardcodes which auth schemes it will attempt to authenticate against:

return
TryGetValidAuthenticationChallengeForScheme(NegotiateScheme, AuthenticationType.Negotiate, authUri, credentials, authenticationHeaderValues, out challenge) ||
TryGetValidAuthenticationChallengeForScheme(NtlmScheme, AuthenticationType.Ntlm, authUri, credentials, authenticationHeaderValues, out challenge) ||
TryGetValidAuthenticationChallengeForScheme(DigestScheme, AuthenticationType.Digest, authUri, credentials, authenticationHeaderValues, out challenge) ||
TryGetValidAuthenticationChallengeForScheme(BasicScheme, AuthenticationType.Basic, authUri, credentials, authenticationHeaderValues, out challenge);

The requested change in HttpClient(Handler)'s behaviour will reduce the workload for server implementors, since clients would work "out of the box" and they won't need to handle JWT via Basic auth. And it will reduce workload on clients who need to authenticate to many different servers, by avoiding needing to special-case Bearer authentication when talking to servers who only allow JWT via Bearer auth.

As a real use case, here's a request the NuGet client team received asking to handle Bearer, since we don't currently special-case Bearer, and therefore currently NuGet doesn't support bearer, since HttpCLient doesn't:

API Proposal

No changes to public API needed, but a new feature requests didn't seem appropriate for the bug report template, so I chose this template instead 🤷

API Usage

string jwt = GetJwtFromSomewhere();
var credentialCache = new CredentialCache
{
    { new Uri(prefix), "Bearer", new NetworkCredential(userName: null, password: jwt) }
};
using HttpClientHandler httpClientHandler = new()
{
    PreAuthenticate = true,
    Credentials = credentialCache
};
using HttpClient httpClient = new(httpClientHandler);

Here's a full test program. Note that dotnet run will show that only a single http request is attempted, which does not authenticate, where as dotnet run -- basic will use HTTP basic authentication, and will authenticate on the second request:

using System.Net;

HttpListener listener = new HttpListener();
string prefix = "http://localhost:1234/";
listener.Prefixes.Add(prefix);
listener.Start();

var authScheme = args.Length > 0 ? args[0] : "Bearer";

Task httpHandlerTask = HandleHttpRequests(listener);

var credentialCache = new CredentialCache
{
    { new Uri(prefix), authScheme, new NetworkCredential("username", "password") }
};
using HttpClientHandler httpClientHandler = new()
{
    PreAuthenticate = true,
    Credentials = credentialCache
};
using HttpClient httpClient = new(httpClientHandler);

using var response = await httpClient.GetAsync(prefix);
Console.WriteLine($"Client: response code {response.StatusCode}");

listener.Stop();
await httpHandlerTask;

async Task HandleHttpRequests(HttpListener listener)
{
    while (true)
    {
        try
        {
            var request = await listener.GetContextAsync();
            var authenticationHeader = request.Request.Headers.Get("Authorization");
            if (string.IsNullOrEmpty(authenticationHeader))
            {
                Console.WriteLine("Server: unauthenticated request");
                request.Response.AddHeader("WWW-Authenticate", authScheme);
                request.Response.StatusCode = (int)HttpStatusCode.Unauthorized;
            }
            else
            {
                Console.WriteLine("Server: authenticated request");
                request.Response.StatusCode = (int)HttpStatusCode.OK;
            }
            request.Response.Close();
        }
        catch
        {
            // this is how http listener "gracefully" stops?
            return;
        }
    }
}

Alternative Designs

One problem with using CredentialCache to try to pass a bearer token (JWT, or otherwise), is that CredentialCache.Add takes in a NetworkCredential, which has UserName and Password properties. But a bearer token is a single, opaque, string, which doesn't make sense to split up into separate UserName and Password components. If it's acceptable to add a CredentialCache.Add(Uri prefix, string authType, string value), where AuthenticationHelper will then use by creating the header Authorization: {authType} {value}, that would make a lot more sense when trying to use bearer tokens.

Otherwise, documentation explaining how to use bearer with NetworkCredential would be needed. I assume that username will be ignored, and only password would be copied into the header.

Risks

No response

@zivkan zivkan added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Sep 11, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 11, 2023
@ghost
Copy link

ghost commented Sep 11, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

ASP.NET Core has middleware to support JWT authentication, however, it expects the token to be supplied in a Authorization: Bearer <token> header. By default it doesn't work with Authorization: Basic <encoded username:password>. Personally, I wouldn't be surprised if this is also true for other web development frameworks as well.

Meanwhile, NuGet provides a credential plugin system to allow NuGet to work with servers that expect HTTP basic, NTLM, Digest/Negotiate, and theoretically anything that HttpClient(Handler) supports. The way it does this is with a class that implements ICredentials (the BCL's CredentialCache class is kind of similar, but also different, but for an example's purpose, it's close enough), and the NuGet calls out to credential providers to fill in the details as needed.

This is because System.Net's AuthenticationHelper class hardcodes which auth schemes it will attempt to authenticate against:

return
TryGetValidAuthenticationChallengeForScheme(NegotiateScheme, AuthenticationType.Negotiate, authUri, credentials, authenticationHeaderValues, out challenge) ||
TryGetValidAuthenticationChallengeForScheme(NtlmScheme, AuthenticationType.Ntlm, authUri, credentials, authenticationHeaderValues, out challenge) ||
TryGetValidAuthenticationChallengeForScheme(DigestScheme, AuthenticationType.Digest, authUri, credentials, authenticationHeaderValues, out challenge) ||
TryGetValidAuthenticationChallengeForScheme(BasicScheme, AuthenticationType.Basic, authUri, credentials, authenticationHeaderValues, out challenge);

The requested change in HttpClient(Handler)'s behaviour will reduce the workload for server implementors, since clients would work "out of the box" and they won't need to handle JWT via Basic auth. And it will reduce workload on clients who need to authenticate to many different servers, by avoiding needing to special-case Bearer authentication when talking to servers who only allow JWT via Bearer auth.

API Proposal

No changes to public API needed, but a new feature requests didn't seem appropriate for the bug report template, so I chose this template instead 🤷

API Usage

string jwt = GetJwtFromSomewhere();
var credentialCache = new CredentialCache
{
    { new Uri(prefix), "Bearer", new NetworkCredential(userName: null, password: jwt) }
};
using HttpClientHandler httpClientHandler = new()
{
    PreAuthenticate = true,
    Credentials = credentialCache
};
using HttpClient httpClient = new(httpClientHandler);

Here's a full test program. Note that dotnet run will show that only a single http request is attempted, which does not authenticate, where as dotnet run -- basic will use HTTP basic authentication, and will authenticate on the second request:

using System.Net;

HttpListener listener = new HttpListener();
string prefix = "http://localhost:1234/";
listener.Prefixes.Add(prefix);
listener.Start();

var authScheme = args.Length > 0 ? args[0] : "Bearer";

Task httpHandlerTask = HandleHttpRequests(listener);

var credentialCache = new CredentialCache
{
    { new Uri(prefix), authScheme, new NetworkCredential("username", "password") }
};
using HttpClientHandler httpClientHandler = new()
{
    PreAuthenticate = true,
    Credentials = credentialCache
};
using HttpClient httpClient = new(httpClientHandler);

using var response = await httpClient.GetAsync(prefix);
Console.WriteLine($"Client: response code {response.StatusCode}");

listener.Stop();
await httpHandlerTask;

async Task HandleHttpRequests(HttpListener listener)
{
    while (true)
    {
        try
        {
            var request = await listener.GetContextAsync();
            var authenticationHeader = request.Request.Headers.Get("Authorization");
            if (string.IsNullOrEmpty(authenticationHeader))
            {
                Console.WriteLine("Server: unauthenticated request");
                request.Response.AddHeader("WWW-Authenticate", authScheme);
                request.Response.StatusCode = (int)HttpStatusCode.Unauthorized;
            }
            else
            {
                Console.WriteLine("Server: authenticated request");
                request.Response.StatusCode = (int)HttpStatusCode.OK;
            }
            request.Response.Close();
        }
        catch
        {
            // this is how http listener "gracefully" stops?
            return;
        }
    }
}

Alternative Designs

One problem with using CredentialCache to try to pass a bearer token (JWT, or otherwise), is that CredentialCache.Add takes in a NetworkCredential, which has UserName and Password properties. But a bearer token is a single, opaque, string, which doesn't make sense to split up into separate UserName and Password components. If it's acceptable to add a CredentialCache.Add(Uri prefix, string authType, string value), where AuthenticationHelper will then use by creating the header Authorization: {authType} {value}, that would make a lot more sense when trying to use bearer tokens.

Otherwise, documentation explaining how to use bearer with NetworkCredential would be needed. I assume that username will be ignored, and only password would be copied into the header.

Risks

No response

Author: zivkan
Assignees: -
Labels:

api-suggestion, area-System.Net.Http, untriaged

Milestone: -

@teo-tsirpanis teo-tsirpanis added feature-request and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Sep 12, 2023
@karelz karelz added enhancement Product code improvement that does NOT require public API changes/additions and removed feature-request labels Sep 13, 2023
@karelz karelz added this to the 9.0.0 milestone Sep 14, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Sep 14, 2023
@karelz karelz removed the untriaged New issue has not been triaged by the area owner label Sep 14, 2023
@karelz
Copy link
Member

karelz commented Sep 14, 2023

Triage: Looks like a useful thing we should consider. Adding to 9.0.

@filipnavara
Copy link
Member

filipnavara commented Sep 20, 2023

Couple of random notes:

  • In the context of HTTP some servers send WWW-Authenticate: Bearer to signal that OAuth authentication is possible, while others don't and/or do it conditionally. For example, the Exchange Online Autodiscover service only sends the WWW-Authenticate: Bearer header if the original request had Authorization: Bearer header (with no token). This behavior is unlike any other supported authentication scheme and we need to check specifications and decide how to handle this correctly.
    • We should have a way to send the Bearer token even if the server doesn't send WWW-Authenticate: Bearer. The developer may be aware that he's talking to OAuth-enabled server and supplies the token unconditionally. Perhaps this can be linked to the existing PreAuthenticate property?
    • Should we send the Authorization: Bearer header when Bearer credential is available?
    • Can we support sending the Authorization: Bearer header even if the Bearer credential is not available yet? How? This is particularly useful for the Autodiscover endpoints where the server can be Exchange Online with OAuth endpoint, or on-premise Exchange with NTLM/Negotiate authentication only.
  • In contexts other than HTTP the user name can be non-empty. Particularly, in IMAP it specifies which mailbox to access. That can be a delegated account different from the primary email address.
  • NetworkCredential can be directly assigned to the Credentials property and it claims to handle any authentication scheme. This makes it somewhat problematic to use it as a carrier of Bearer tokens without accidentally sending it in a different scheme as a password. In fact, you can have custom or derived implementations of ICredentials that make this even trickier. We could limit the Bearer support to CredentialCache and/or a new class, but this needs careful consideration. For example, we use wrappers around CredentialCache that conditionally disable some schemes, which would be broken with a strict type check.

@zivkan
Copy link
Member Author

zivkan commented Sep 20, 2023

Regarding the first notes/questions, I'm not sure if you'd like me, as the request author, to respond. But generally, I'd have guessed that bearer would work more or less the same way that basic does. If I have creds in the CrenentialCache where I've listed the auth type as "BASIC", then I wouldn't expect that credential to be used unless the server responded with a WWW-Authenticate: Basic header. Therefore, I wouldn't expect the bearer token to be sent until the server explicitly asked for it.

I acknowledge that there are examples of web servers not sending the WWW-Authentication: Bearer header in unauthorized requests, but this is the standard/convention, so I don't expect generic clients to work in non-standard ways. In fact, I'd argue that it's best to strictly implement the standards/RFCs, and if some customer needs to work with a non-compliant web server, then they'll need to explicitly set the header, rather than using this .Credentials & ICredential infrastrcture. I think there's lower risk of unexpected behaviour.

Anyway, I'm by no means an HTTP expert, definitely not a .NET HttpClient expert, I just happen to work on an app that uses it.

We could limit the Bearer support to CredentialCache and/or a new class, but this needs careful consideration.

This would be problematic for NuGet, as we have our own class that implements ICredentials: https://github.com/NuGet/NuGet.Client/blob/08e07ea13985fb259b35b9ce90fd99339f0fdef2/src/NuGet.Core/NuGet.Common/AuthTypeFilteredCredentials.cs#L14

The problem NuGet has with CredentialsCache is that CredentialsCache requires you to say what URL the credential is for, but NuGet creates a new HttpClient & HttpClientHandler for each package source, and then we want to use the same credential for all URLs used with that client.

NuGet's "V3 protocol" uses JSON-LD, so the first json document contains URLs where we get other data from. Therefore, it's theoretically possible that the second URL we hit does not start with the prefix where we got the first. Needing to populate the CredentialCache with every URL prefix we discover would be feasible, but feels "annoying". This simple class that implements ICredential is a lot off effort. Especially for OAuth2 scenarios where the token is short lived and then we need to refresh all the URLs with a new credential.

So, it would be the least effort for NuGet to adopt if getting bearer tokens from HttpClientHandler.Credential works though any ICredential implementation.

@filipnavara
Copy link
Member

Regarding the first notes/questions, I'm not sure if you'd like me, as the request author, to respond.

Any response is welcome. The request for this API came up in different contexts multiple times over the years. Some of the contexts are HTTP specific, some are not. Any API change should preferably address this as widely as possible.

Therefore, I wouldn't expect the bearer token to be sent until the server explicitly asked for it.

That would greatly reduce the utility of this added API. I guess it may be reasonable to send it only when WWW-Authenticate: Bearer is present, or when PreAuthenticate = true and the Bearer credential is present (unconditionally in the first request).

That doesn't necessarily rule out adding an escape hatch to send Authenticate: Bearer for the first request (either as a custom header, or empty Bearer credential?).

We have the need for this since we communicate with wide variety of 3rd-party servers (including Microsoft ones). Not addressing this in some way would make this API significantly less usable for our purposes, unfortunately.

I cannot comment on how the standards specifically define this behavior.

This would be problematic for NuGet, as we have our own class that implements ICredentials:

Glad to hear that, we are in the same boat for similar reasons :-)

So, it would be the least effort for NuGet to adopt if getting bearer tokens from HttpClientHandler.Credential works though any ICredentials implementation.

That's where lies the problem. NetworkCredential will happily answer to ICredentials.GetCredential(..., "Bearer") with an instance of itself. You can special case it in the NetworkCredential implementation or its consumer, but that would not work for any existing custom ICredentials implementation which implements ICredentials.GetCredential without checking the authenticationType parameter.

One way to solve this is to introduce BearerNetworkCredential : NetworkCredential since NetworkCredential is not sealed. That way you can limit the potential breakage quite easily. You can still return it from any ICredentials implementation, use it in CredentialCache, or even directly assign it to HttpClientHandler.Credential. Only BearerNetworkCredential would be usable along with the Bearer scheme, while regular NetworkCredential would be restricted to password-based authentication schemes. You can even add the extra constructor which omits the user name.

@zivkan
Copy link
Member Author

zivkan commented Oct 11, 2023

If this feature is going to support PreAuthenticate, then considering Bearer is often used with short lived OAuth2 tokens, then this other issue is probably relevant:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Net.Http enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

5 participants