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

Oauth2 Not Adding Token to Request Header #2615

Closed
drewby opened this issue Mar 2, 2023 · 5 comments · Fixed by #2616
Closed

Oauth2 Not Adding Token to Request Header #2615

drewby opened this issue Mar 2, 2023 · 5 comments · Fixed by #2616
Assignees
Labels
kind/bug Something isn't working
Milestone

Comments

@drewby
Copy link
Contributor

drewby commented Mar 2, 2023

Expected Behavior

Oauth2 Middleware component for Authorization Code Grant flow is supposed to add the Bearer token to a request header after authentication. This header is then sent along with the request to the app for validation and claims lookup. This works correctly in v1.9.

Actual Behavior

In v1.10, the Oauth2 Middleware component is adding the Bearer token to the Response headers instead of the Request headers. The token is going back to the client application which is not expected.

Steps to Reproduce the Problem

  1. Simple controller to return headers (ASP.NET core / C#):
var app = WebApplication.Create(args);

app.MapGet("/Headers", (HttpRequest request) =>
{
    var headers = request.Headers;
    return new Microsoft.AspNetCore.Mvc.ObjectResult(headers);    
});

app.Run();
  1. Create component config
apiVersion: dapr.io/v1alpha1
kind: Component
metadata:
  name: oauth2
spec:
  type: middleware.http.oauth2
  version: v1
  metadata:
  - name: clientId
    value: <clientId>
  - name: clientSecret
    value: <clientSecret>
  - name: scopes
    value: "api://appId/TodoList.Read, api://appId/TodoList.ReadWrite"
  - name: authURL
    value: "https://login.microsoftonline.com/tenantId/oauth2/v2.0/authorize"
  - name: tokenURL
    value: "https://login.microsoftonline.com/tentId/oauth2/v2.0/token"
  - name: redirectURL
    value: "http://localhost:8080/v1.0/invoke/test/method/Headers"
  - name: authHeaderName
    value: "Authorization" 
  1. Configure middleware
apiVersion: dapr.io/v1alpha1
kind: Configuration
metadata:
  namespace: nginx
  name: dapr-config
spec: 
  httpPipeline:
    handlers:
      - name: oauth2 
        type: middleware.http.oauth2
  1. Run dapr
dapr run --app-id test --config config.yaml --components-path ./components/ --app-port 5000 -H 8080 -- dotnet run --project ./src --urls http://localhost:5000

With daprd 1.9, we see the Bearer token header. In daprd 1.10, there is no Bearer token header.

Release Note

RELEASE NOTE: FIX Oauth2 middleware adds Bearer token to request headers.

@drewby drewby added the kind/bug Something isn't working label Mar 2, 2023
@ItalyPaleAle
Copy link
Contributor

Thanks for creating this issue. I think this happened while we migrated from fasthttp to net/http for the middlewares

What I don't understand however is why you'd need the bearer token added to the request? How I thought this middleware was being used was by adding this to the app pipeline so for example calls between Dapr sidecars have the bearer token.

@drewby
Copy link
Contributor Author

drewby commented Mar 2, 2023

@ItalyPaleAle We use the bearer token at the API endpoint. We validate the token and then enforce claim requirements (for example, Read, Write, ReadWrite, etc).

I'm not aware of how Dapr sidecars use the token. Is their a Dapr feature somewhere that looks for the token and uses it for authorization?

@ItalyPaleAle
Copy link
Contributor

We use the bearer token at the API endpoint. We validate the token and then enforce claim requirements (for example, Read, Write, ReadWrite, etc).

Would you mind getting a bit more into the details? What API are you referring to? Where are the calls coming from?

@drewby
Copy link
Contributor Author

drewby commented Mar 2, 2023

Its an HTML client calling to an ASP.NET Web API. Right now its just a POC sample for a customer. They are also looking at APIM to do the same, but I'd like to see the Dapr component continue to improve.

Program.cs:

// Add JWT authentication
builder.Services.AddAuthentication()
    .AddJwtBearer("Bearer", options =>
    {
        // Get configuration settings
        var tenantId = builder.Configuration["TenantId"];
        var audienceId = builder.Configuration["AudienceId"];

        // Get the OpenID configuration for the tenant
        var stsDiscoveryEndpoint = $"https://login.microsoftonline.com/{tenantId}/v2.0/.well-known/openid-configuration";
        var configurationManager = new ConfigurationManager<OpenIdConnectConfiguration>(stsDiscoveryEndpoint, new OpenIdConnectConfigurationRetriever());
        var openIdConfig = configurationManager.GetConfigurationAsync(CancellationToken.None).GetAwaiter().GetResult();

        // Configure the JWT validation using OpenID configuration
        options.TokenValidationParameters = new TokenValidationParameters
        {
            ValidIssuer = openIdConfig.Issuer,
            ValidAudiences = new[] { audienceId },
            IssuerSigningKeys = openIdConfig.SigningKeys,
        };
    });

// Add authorization using scopes (enables the [RequiredScope] attribute)
builder.Services.AddRequiredScopeAuthorization();

Controller example:

    [ProducesResponseType(typeof(IEnumerable<Todo>), StatusCodes.Status200OK)]
    [RequiredScope("TodoList.Read")]
    [HttpGet]
    public async Task<IActionResult> Get()
    {
        _logger.LogInformation($"TodoController.Get() called");
        return new ObjectResult(await _todos.GetList());
    }

I'll send you the repo which is private.

@ItalyPaleAle
Copy link
Contributor

Following up on our conversation on Teams. I understand the issue now, and you're right. I'll get the PR reviewed and merged. Thanks!

@DeepanshuA DeepanshuA added this to the v1.11 milestone May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants