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 several AuthenticationProviderKey values in the Route #1580

Closed
MayorSheFF opened this issue Jun 2, 2022 · 25 comments · Fixed by #1870
Closed

Support several AuthenticationProviderKey values in the Route #1580

MayorSheFF opened this issue Jun 2, 2022 · 25 comments · Fixed by #1870
Assignees
Labels
feature A new feature merged Issue has been merged to dev and is waiting for the next release Nov'23 November 2023 release

Comments

@MayorSheFF
Copy link
Contributor

MayorSheFF commented Jun 2, 2022

New Feature

Opportunity to support several AuthenticationProviderKey values in the Route.

Motivation for New Feature

At the moment, Route can contain one AuthenticationProviderKey value only. So, if we want to handle Route for different authentication providers, we have to specify two Routes and make Route path unique. Sometime it is not so easy to specify unique paths for the same endpoint.

Update by Maintainer on Jan 8, 2024 👇

Useful Links

Related

@MayorSheFF
Copy link
Contributor Author

MayorSheFF commented Jun 2, 2022

I made some changes locally to solve it, but I just want to discuss if it is necessary for everyone and maybe someone is currently trying to solve too. If it is necessary and noone is doing it, I can create a pull request.

@clintonbale
Copy link

I've gotten around this by setting the provider key to a policy scheme instead of directly to the authentication scheme, and doing custom logic on the selector to determine which method to use.

config:

"AuthenticationProviderKey": "AnyAuthenticated"

policy scheme:

builder.Services
    .AddAuthentication(JwtBearerDefaults.AuthenticationScheme)
    .AddPolicyScheme("AnyAuthenticated", null, p =>
    {
        p.ForwardDefaultSelector = context =>
        {
            if (context.Request.Headers.ContainsKey("X-Use-Example-Auth"))
            {
                return "ExampleAuthScheme";
            }
            return JwtBearerDefaults.AuthenticationScheme;
        };
    })
    .AddJwtBearer()
    .AddExampleScheme()

But I'm not the happiest with this approach and would prefer having support for an array in the config, something like:

"AuthenticationProviderKeys": [ "Bearer", "ExampleAuthScheme" ]

@MayorSheFF
Copy link
Contributor Author

I am still having these changes. Can someone from owners look at it and accept if it is OK?

@sivapriyanc
Copy link

Hi @MayorSheFF could you please share the code snippet

@MayorSheFF
Copy link
Contributor Author

MayorSheFF commented May 30, 2023

Hi, @sivapriyanc.

This function was added into the AuthenticationMiddleware class.

private async Task<AuthenticateResult> AuthenticateAsync(HttpContext httpContext, DownstreamRoute route)
{
    AuthenticateResult result = null;

    if (!string.IsNullOrWhiteSpace(route.AuthenticationOptions.AuthenticationProviderKey))
    {
        result = await httpContext.AuthenticateAsync(route.AuthenticationOptions.AuthenticationProviderKey);
        if (result.Succeeded)
        {
            return result;
        }
    }

    IEnumerable<string> authenticationProviderKeys = route
        .AuthenticationOptions
        .AuthenticationProviderKeys
        ?.Where(apk => !string.IsNullOrWhiteSpace(apk))
        ?? Array.Empty<string>();

    foreach (var authenticationProviderKey in authenticationProviderKeys)
    {
        result = await httpContext.AuthenticateAsync(authenticationProviderKey);
        if (result.Succeeded)
        {
            break;
        }
    }

    return result;
}

Then, it is using in Invoke method, namely var result = await AuthenticateAsync(httpContext, downstreamRoute); row.

public async Task Invoke(HttpContext httpContext)
{
    var downstreamRoute = httpContext.Items.DownstreamRoute();

    if (httpContext.Request.Method.ToUpper() != "OPTIONS" && IsAuthenticatedRoute(downstreamRoute))
    {
        Logger.LogInformation($"{httpContext.Request.Path} is an authenticated route. {MiddlewareName} checking if client is authenticated");

        var result = await AuthenticateAsync(httpContext, downstreamRoute);

        httpContext.User = result.Principal;

        if (httpContext.User.Identity.IsAuthenticated)
        {
            Logger.LogInformation($"Client has been authenticated for {httpContext.Request.Path}");
            await _next.Invoke(httpContext);
        }
        else
        {
            var error = new UnauthenticatedError(
            $"Request for authenticated route {httpContext.Request.Path} by {httpContext.User.Identity.Name} was unauthenticated");

            Logger.LogWarning($"Client has NOT been authenticated for {httpContext.Request.Path} and pipeline error set. {error}");

            httpContext.Items.SetError(error);
        }
    }
    else
    {
        Logger.LogInformation($"No authentication needed for {httpContext.Request.Path}");

        await _next.Invoke(httpContext);
    }
}

The following classes were changed too. It allows to get values from the configuration.

  • AuthenticationOptions class;
  • AuthenticationOptionsBuilder class;
  • AuthenticationOptionsCreator class;
  • FileAuthenticationOptions class;
  • RouteOptionsCreator class.

@raman-m
Copy link
Member

raman-m commented May 30, 2023

Hi Igor!
Welcome to Ocelot world! 😄

@MayorSheFF commented on May 30, 2023

Why not to create a PR?


@MayorSheFF commented on Mar 13, 2023:

I am still having these changes. Can someone from owners look at it and accept if it is OK?

Tom, as the owner, still manages this repo, but he is super busy.
I'm not owner sure, but I will accept the issue after a discussion, and provide code review for your PR(s).
Let me to fix your code blocks wrapping.
And let's start a discussion...

@raman-m
Copy link
Member

raman-m commented May 30, 2023

@MayorSheFF commented on Jun 2, 2022:

At the moment, Route can contain one AuthenticationProviderKey value only.

Correct!


So, if we want to handle Route for different authentication providers, we have to specify two Routes and make Route path unique.

So, it looks like a hack! 😺
I cannot get your user scenario about the downservice you're routing the traffic to!
Why do you need multiple auth-providers for one endpoint/route? Are you going to hack the system? 🤣
Just give me the reason please!

In theory your current JWT-token can be expired and for a some reason you cannot generate new token. In this case you are not the owner of endpoint (not able to support the downservice), or you cannot generate new tokens because your access to downservice is limited or forbidden, and/or your credentials (user/pwd) were changed/deleted, and/or the service has changed access policy by closing authentication endpoint (aka auth-method).

Practically, you are requesting absolutely new feature because current bearer-tokens auth approach is not aligned to your needs. Browse to the Authentication docs please, and you will see the following supported auth approaches:

I see you need more control on authentication stage, and you don't like current default ASP.NET bearer-tokens approach.
In this case you could try Identity Server approach where you have 100% of control on authentication.

Finally, I would ask you to share your user scenario having multiple auth-approaches for one route.
Please note, that your splitting route into multiple routes with different auth-approaches is absolutely correct.
Otherwise supporting multiple providers by one downservice app will be strange and risky. But in theory it is possible for sure.


Sometime it is not so ease to specify unique paths for the same endpoint.

Sometimes means your user's case is rare! You cannot define multiple routes for the same endpoint (downservice path). In this case Ocelot starting will fail with an exception of bad config in ocelot.json file.
Once again,

  • Could you share your user scenario please?
  • Provide details on configs, downservices, and paths of a downservice you're defining routes for, plz?

Waiting an interesting feedback from you...

@raman-m raman-m added feature A new feature question Initially seen a question could become a new feature or bug or closed ;) large effort Likely over a week of development effort medium effort Likely a few days of development effort proposal Proposal for a new functionality in Ocelot needs feedback Issue is waiting on feedback before acceptance waiting Waiting for answer to question or feedback from issue raiser labels May 30, 2023
@clintonbale
Copy link

  • Could you share your user scenario please?

Our scenario was that we needed to support both JWT Token authentication as well as API Key authentication for connected services or applications for all endpoints in the system.

My example code above works to change schemes based on the incoming header value, but it is not ideal.

@MayorSheFF
Copy link
Contributor Author

MayorSheFF commented May 31, 2023

Hi @raman-m,

Why not to create a PR?

I would be very glad to push my local branch to GitHub, but I have the following issue.

Remote: Permission to ThreeMammals/Ocelot.git denied to MayorSheFF.
Error encountered while pushing to the remote repository: Git failed with a fatal error.
Git failed with a fatal error.
unable to access 'https://github.com/ThreeMammals/Ocelot.git/': The requested URL returned error: 403

Maybe I make something wrong.

Provide details on configs, downservices, and paths of a downservice you're defining routes for, plz?

{
    "Routes": [
        {
            "AuthenticationOptions": {
                "AuthenticationProviderKeys": [
                    "Bearer",
                    "BearerB2B"
                ]
            },
            "DownstreamHostAndPorts": [
                {
                    "Host": "servicehost",
                    "Port": 8000
                }
            ],
            "DownstreamPathTemplate": "/{anyurl}",
            "DownstreamScheme": "http",
            "UpstreamPathTemplate": "/service/{anyurl}"
        }
    ]
}

Could you share your user scenario please?

We have microservices which have to be reachable for two kind of our users: our customers and our internal users. These two groups are in different Azure Active Directories. So, you can imagine these two Azure Active Directories as two different Identity Servers. By means of several AuthenticationProviderKeys you will be able to realize authentication with these Azure Active Directories without a lot of complementary implementation. I believe it is enough explanation. Should I add something?

@ThreeMammals ThreeMammals deleted a comment from sivapriyanc May 31, 2023
@raman-m
Copy link
Member

raman-m commented May 31, 2023

@MayorSheFF commented on May 31, 2023:

I would be very glad to push my local branch to GitHub, but I have the following issue.

Remote: Permission to ThreeMammals/Ocelot.git denied to MayorSheFF.
Error encountered while pushing to the remote repository: Git failed with a fatal error.
Git failed with a fatal error.
unable to access 'https://github.com/ThreeMammals/Ocelot.git/': The requested URL returned error: 403

Maybe I make something wrong.

Yes, you may!
You need to create a fork of Ocelot in your account!
Read these articles which will help you to make new forked repository:

As soon as you've created a fork you will be able to create feature branches, and create PR(s) for feature delivery to upstream (parent, source, base) repository, which is ThreeMammals/Ocelot repo.

@raman-m
Copy link
Member

raman-m commented May 31, 2023

@MayorSheFF

We have microservices which have to be reachable for two kind of our users: our customers and our internal users. These two groups are in different Azure Active Directories. So, you can imagine these two Azure Active Directories as two different Identity Servers. By means of several AuthenticationProviderKeys you will be able to realize authentication with these Azure Active Directories without a lot of complementary implementation. I believe it is enough explanation. Should I add something?

Well... It seems I understood your goal for such feature. But...
I see 3 ways of evolution of your project:

  • Develop, build custom Identity Server which incorporates both sources of "users".
  • Merge two security models of two Identity Servers inside of one ASP.NET app, based on different controllers. That is exactly what you're going to do. But you would like to have one CatchAll Route in Ocelot gateway, and that means possibly you have to implement custom Authorization filter attribute for all controllers. Or, you have to implement 2 authorization filters to support 2 Identity Servers.
  • Separate two security models into 2 microservices when each of them supports one security model. The most safest and valid approach on my opinion. For this case you have to prepare some shared packages, and/or implement integrations to the same business layer microservice.

I would recommend to discuss that with Software Architect of your project. I don't know either Azure is able to "merge" 2+ Identity Servers. or not. I believe Azure can merge, but I think a 3rd Active Directory service should be deployed. So, I cannot imagine the best possible solution here. Once again, I believe it should be possible to merge 2 Identity Servers and/or 2 Azure Active Directories.

@raman-m
Copy link
Member

raman-m commented May 31, 2023

Igor,

In my opinion, the easiest solution for your user's scenario will be the following:

  • Create two controllers in one ASP.NET Web API app to separate security models
  • Implement 2 authorization filters for each security model (for each Azure Active Directory)
  • Embed, add 2 authorization filters into ASP.NET MVC pipeline to authorization middleware
  • Create 2 Routes in Ocelot config to separate traffic for these 2 security models having one AuthenticationProviderKey scheme only for Ocelot Authentication middleware

What about this solution?

@MayorSheFF
Copy link
Contributor Author

Hi, @raman-m.

You need to create a fork of Ocelot in your account!

I will try to look at it today. Thank you for your advice.

@MayorSheFF
Copy link
Contributor Author

MayorSheFF commented Jun 1, 2023

@raman-m,

I will answer on all your suggestions what your described in two last messages.

On the figure below you can see very rough architecture how it was before these changes and how it is after these changes (new implementation).

image

Just imagine. Before our application had the internal users only. So, these internal users send requests to our microservices by means of different clients (web, mobile, desktop etc.). Then, business came to developers and infrastructures and ask for giving an ability other users to use the microservices. At that moment, external users appeared in our system. But the main question for business is how to do it with less resources: money, time, development, delivery etc. and leave our architecture transparent. Starting from this point, I will ask you the following question. Will you implement all your suggestions and in the same time adhere business requirements? Our answer was: "We can extend our API Gateway / Ocelot to handle two identity providers." So, we extended Ocelot to authenticate requests via several identity providers. It took about 3 days. OK. Let's say 5 days (1 working week). Maybe 4 days if you are in the country where 1 working week = 4 days. So, these 5 days are development, test and delivery. Also, it is a good solution from architecture perspective.

Maybe it is a very rough description, but the general idea is like that.

In my standpoint, this implementation can be useful for other people too who will run into the same questions. Due to it, I suggested Ocelot team to look at it.

@raman-m
Copy link
Member

raman-m commented Sep 8, 2023

@MayorSheFF commented on Jun 1, 2023

🆗 I see... Having multiple identity services in the project is the real challenge! 🤣
How did you solve the problem? Is your solution based on Ocelot AuthenticationMiddleware overriding to eliminate the problem of single provider key?!

You could also split Ocelot gateway into 2 instances which of them should support one provider key. But this approach requires more hardware resources and DevOps.

Also, as @clintonbale suggested on Jun 27, 2022, you can setup Active Directory services to generate special custom header, forward that HTTP header to Ocelot gateway, and reuse Clinton's solution services setup to recognize AD user and auth provider key.

So, what solution is in production now?

@raman-m raman-m removed question Initially seen a question could become a new feature or bug or closed ;) large effort Likely over a week of development effort proposal Proposal for a new functionality in Ocelot needs feedback Issue is waiting on feedback before acceptance waiting Waiting for answer to question or feedback from issue raiser labels Sep 8, 2023
@raman-m raman-m added the accepted Bug or feature would be accepted as a PR or is being worked on label Sep 8, 2023
@raman-m
Copy link
Member

raman-m commented Sep 8, 2023

The issue has been accepted due to already opened PR #1666

@raman-m
Copy link
Member

raman-m commented Sep 27, 2023

@MayorSheFF Igor,
Can I ask your help to search for the similar issues in backlog please?
Use keywords: AuthenticationProviderKey, Authentication, etc.
We could attach them to your current open PR if those issues will be relevant to provider keys...

@dkornel
Copy link

dkornel commented Jan 26, 2024

Hi all,

What is the status of this PR #1870 ?

Do you need help with testing ?

We are waiting for this feature and we are wondering if we can wait for you to release that or we need to implement the way around it.

Is it the matter of week/month ?

@raman-m
Copy link
Member

raman-m commented Jan 26, 2024

@dkornel Hi Kornel!

What is the status of this PR #1870 ?

In Progress. The author doesn't develop his PR anymore. As a Pair Programming participant I am finishing the PR to get "Code Complete" status.


Do you need help with testing ?

Sorry? How could you help us? Unit tests are ready. I'm finishing acceptance testing... They are most complicated part of the PR. Will you write some interesting user scenarios?
Pay attention I work in the author's feature branch. I have access as repo Maintainer. But you cannot push to forked repo branch. Asking the author to add you as collaborator probably will take to much time, finally with no reply from the author.
Also, I will not move the code to head repo branch because it is too late.


We are waiting for this feature and we are wondering if we can wait for you to release that or we need to implement the way around it.
Is it the matter of week/month ?

Week. No more!

@raman-m
Copy link
Member

raman-m commented Jan 26, 2024

@dkornel If you need this feature ASAP then there are 2 possible ways

@raman-m
Copy link
Member

raman-m commented Jan 26, 2024

@clintonbale commented on Jun 27, 2022:

config:

"AuthenticationProviderKey": "AnyAuthenticated"

...

But I'm not the happiest with this approach and would prefer having support for an array in the config, something like:

"AuthenticationProviderKeys": [ "Bearer", "ExampleAuthScheme" ]

Clinton,
Why did you propose the new property as extra one for AuthenticationProviderKey?
Why array-like approach?
Maybe 👇

"AuthenticationProviderKey": "Bearer ExampleAuthScheme",

Will not this simple convention work when you write all schemes in old property, separating by whitespace?

If extend the idea of using old property, we can propose any delimiter approach 👉

"AuthenticationProviderKey": "Bearer ExampleAuthScheme",
"AuthenticationProviderKey": "Bearer, ExampleAuthScheme",
"AuthenticationProviderKey": "Bearer | ExampleAuthScheme",
"AuthenticationProviderKey": "Bearer+Bearer2+ExampleAuthScheme",

So, having non-alpha character in the value will instruct the validator and logic that we are running into multiple auth schemes workflow.
I am not a fan of the new AuthenticationProviderKeys property in plural form of old one.

@dkornel
Copy link

dkornel commented Jan 26, 2024

@raman-m

We had a team discussion, that we will need this feature, and we need to assess if we have time to wait for this PR to land in the ocelot release or we should rather start developing our solution. We don't need this feature now but in a month.

Sorry? How could you help us? Unit tests are ready. I'm finishing acceptance testing... They are most complicated part of the PR. Will you write some interesting user scenarios?

I kindly ask if you need help because if the PR stucked, I could talk with manager and dedicate c# developer for help (explaining that time of building our solution > time of supporting this PR).

No offence.

@raman-m
Copy link
Member

raman-m commented Jan 26, 2024

@dkornel 🆗 Wait for our official release please next week! This feature will be merged next week before the release for sure.
We're finishing of testing in Prod env of our team member now...

Any contributions and interesting PRs are welcome! But seems we needn't your urgent help here.
The #1870 PR was stuck because I was on sick leave for 2+ weeks. 😉

@clintonbale
Copy link

clintonbale commented Jan 27, 2024

@raman-m commented on Jan 26

Plural approach makes sense because it is listing multiple keys. But for backwards compatibility the singular property should not be removed of course.

You could have both properties maybe? AuthenticationProviderKey could get/set AuthenticationProviderKeys[0]. Only one property allowed to be set at a time at the config level.

@raman-m
Copy link
Member

raman-m commented Jan 29, 2024

@clintonbale Good proposal...

But for backwards compatibility the singular property should not be removed of course.

Agree.

As a team, we've decided to go with the current design. So, this is the same you've recommended.
And we will mark old AuthenticationProviderKey property with [Obsolete] attribute.

@raman-m raman-m assigned raman-m and unassigned MayorSheFF Feb 4, 2024
raman-m added a commit that referenced this issue Feb 5, 2024
* #1580. Added an opportunity to use several authentication provider keys.

* Update src/Ocelot/Configuration/Builder/AuthenticationOptionsBuilder.cs

Co-authored-by: Raynald Messié <[email protected]>

* #1580. Replaced AuthenticationProviderKeys type from the list to the array.

* #1580. Added a doc how to use AuthenticationProviderKeys in a Route.

* #1580. Amended the description how to use AuthenticationProviderKeys in a Route.

* #1580. Added an opportunity to use several authentication provider keys.

* Update src/Ocelot/Configuration/Builder/AuthenticationOptionsBuilder.cs

Co-authored-by: Raynald Messié <[email protected]>

* #1580. Replaced AuthenticationProviderKeys type from the list to the array.

* #1580. Added a doc how to use AuthenticationProviderKeys in a Route.

* #1580. Amended the description how to use AuthenticationProviderKeys in a Route.

* Quick review

* #1580. Implemented review points.

* #1580. Initialized result with AuthenticateResult.NoResult().

* #1580. Added @ggnaegi suggestions.

* #1580. Brought back the idea not to allocate AuthenticateResult instance.

* quick review

* Return Auth result of the last key in the collection

* review unit tests

* Enable parallelization of unit tests

* Fix messages

* Disable parallelization for PollyQoSProviderTests

* Switch off unstable test

* Re-enable parallelization & Isolate unstable test

* Reflection issue in middleware base: remove getting Type object

* Switch off unstable test

* Clean code

* Make MiddlewareName as public property

* Code review by @RaynaldM

* AuthenticationMiddleware: Line & Branch coverage -> 100%

* AuthenticationOptionsCreator: coverage -> 100%

* Remove private helpers with one reference

* RouteOptionsCreator: coverage -> 100%

* FileAuthenticationOptions: Refactor ToString method

* FileConfigurationFluentValidator: coverage -> 100%

* RouteFluentValidator: Branch coverage -> 100%

* TODO and Skip unstable test

* Move acceptance tests to the separate folder

* Review and refactor acceptance tests

* Add AuthenticationSteps class.
Choose inheritance over composition: less code

* Add 'GivenIHaveATokenWithScope' to 'AuthenticationSteps'

* Temporarily disable 'Should_timeout_per_default_after_90_seconds' test

* Add CreateIdentityServer method

* Add draft test

* Update route validator to support multiple auth schemes

* Acceptance tests

* Revert "TODO and Skip unstable test"

This reverts commit 1ec8564.

* Revert "Make MiddlewareName as public property"

This reverts commit 6f50c76.

* Revert "Reflection issue in middleware base: remove getting Type object"

* Clean up

* Isolate unstable test

* Mark old property as `Obsolete`

* a tiny little bit of cleanup

* Handling cases when principal or identity are null

* Update Authentication feature docs

* Convert back to block scoped namespace

---------

Co-authored-by: Igor Polishchuk <[email protected]>
Co-authored-by: Raman Maksimchuk <[email protected]>
Co-authored-by: Raynald Messié <[email protected]>
Co-authored-by: Igor Polishchuk <[email protected]>
Co-authored-by: Guillaume Gnaegi <[email protected]>
@raman-m raman-m added merged Issue has been merged to dev and is waiting for the next release and removed medium effort Likely a few days of development effort accepted Bug or feature would be accepted as a PR or is being worked on labels Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature merged Issue has been merged to dev and is waiting for the next release Nov'23 November 2023 release
Projects
None yet
5 participants