dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.47k stars 4.76k forks source link

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

Open zivkan opened 1 year ago

zivkan commented 1 year ago

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: https://github.com/dotnet/runtime/blob/de0ab156194eb64deae0e1018db9a58f7b02f4a3/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/AuthenticationHelper.cs#L124-L128

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

ghost commented 1 year ago

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 ` header. By default it doesn't work with `Authorization: Basic `. 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: https://github.com/dotnet/runtime/blob/de0ab156194eb64deae0e1018db9a58f7b02f4a3/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/AuthenticationHelper.cs#L124-L128 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 ```cs 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: ```cs 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: -
karelz commented 1 year ago

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

filipnavara commented 1 year ago

Couple of random notes:

zivkan commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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: