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

OIDC redirect-path property #17563

Closed
fouad-j opened this issue May 31, 2021 · 16 comments · Fixed by #17921
Closed

OIDC redirect-path property #17563

fouad-j opened this issue May 31, 2021 · 16 comments · Fixed by #17921
Labels
area/oidc kind/enhancement New feature or request
Milestone

Comments

@fouad-j
Copy link

fouad-j commented May 31, 2021

Description

We faced an issue with quarkus.oidc.tenantName.authentication.redirect-path=http://mydomain.com/tenantName

Current behavior of redirect-path is to append value set in application properties to request URL from context

For example for quarkus.oidc.tenantName.authentication.redirect-path=http://mydomain.com/tenantName

The redirect url would be http://requestContextUrl.com/http://mydomain.com/tenantName

Implementation ideas

I have a proposal to fix this issue #17562

If the redirect-path start with http or https we take it as it is

@fouad-j fouad-j added the kind/enhancement New feature or request label May 31, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented May 31, 2021

/cc @pedroigor, @sberyozkin

@sberyozkin
Copy link
Member

sberyozkin commented May 31, 2021

Hi @fouad-j Can you explain please why do you need to use an absolute redirect path ? If a user needs to access a given Quarkus endpoint then why would the user be redirected, after the authentication, to the endpoint in some other domain ?
I appreciate it may be necessary but I'd like to understand why.
Do you also set a cookie-domain property for the redirect to the other domain endpoint to complete successfully (for the state cookie be available, etc) ?

@AJ1062910
Copy link

AJ1062910 commented May 31, 2021

Hi @sberyozkin , thank you for your answer, I work with @fouad-j,

Our case is that after login to the provider (let's say : tenant-a) quarkus redirect (the app to the redirect-path) as we configured in application.properties with : quarkus.oidc.tenantName.authentication.redirect-path=/token. (let's say /token endpoint as redirect path)
And the /token endpoint is a quarkus backend endpoint (here we can inject Userinfo, JsonWebToken ...)

Problem is, in our case, quarkus redirect to our: KUBERNETES_INGRESS_HOST/token, which is not our "backend" Url domain, but the DNS of our front end part. So we wanted to just change the "KUBERNETES_INGRESS_HOST" part to our Backend URL (let's say : https://project.com) and the redirect path registered in the tenant-a provider is : https://project.com/token.

So we wanted just to change the redirect-path property :
quarkus.oidc.tenantName.authentication.redirect-path=https://project.com/token, but it doesn't work like that because quarkus take the Context requested URL : it will give : http://requestedURLhttps://project.com/token, as redirect path.

That is why we want to have the choice of an absolute path for the redirect-path , we would like that the configuration give :
https://project.com/token and not KUBERNETES_INGRESS_HOST/token

So the problem is not that we want to redirect in some other domain but more because quarkus redirect to the bad redirect path, (in our case at least: redirect context is KUBERNETES_INGRESS_HOST instead of https://project.com which is our url for our backend services)
I do not know if I was understandable.
thanks

@sberyozkin
Copy link
Member

@AJ1032480 Hi, thanks, I believe we've had similar problems reported before.
It is a technical problem which for example in case of Keycloak should be resolvable as documented here:
https://github.com/quarkusio/quarkus/blob/main/docs/src/main/asciidoc/security-openid-connect-web-authentication.adoc#external-and-internal-access-to-openid-connect-provider

Can that help ?

@AJ1062910
Copy link

AJ1062910 commented Jun 1, 2021

It is interresting to see that keykloak have a KEYCLOAK_FRONTEND_URL mode,
in our case, we do not use keycloak, I do not think the providers we used have a similar property to set,
Without that,
how can we set the full url of the redirect path ? (on quarkus)

@fouad-j
Copy link
Author

fouad-j commented Jun 1, 2021

Hello @sberyozkin,

Thanks for your reactivity.

I drew a diagram to explain the flow

image

  1. User call https://company.com/authentication?tenantId=tenant1 to get authenticated

  2. Kubernetes Ingress forwards the request to the right service in our cluster

  3. When the user is authenticated, the service redirect to http://identityprovider.com?state=111&scopes=ss&**redirect_uri=http://auth-svc.cluster.local/callback**

http://auth-svc.cluster.local is accessible only in the cluster.

@sberyozkin
Copy link
Member

@fouad-j thanks, have you tried your patch in your deployment ? What happens when, after the redirect, Quarkus attempts to exchange the authorization code for the tokens ?

@sberyozkin
Copy link
Member

@AJ1032480 @fouad-j Here is another link for you:

https://quarkus.io/guides/vertx#reverse-proxy

Can you try:

quarkus.http.proxy.enable-forwarded-prefix=true
quarkus.http.proxy.allow-forwarded=true

Can you please check what X-Forwarded heades the Kubernetes Ingress proxy sets before forwarding to the internal endpoint if the above does not work ?

I'm quite sure we've had such an issue before, can't find the Zulip thread.

We'll make sure it works for you - but if possible I'd like to avoid tweaking the code to workaround various networking setup issues

@AJ1039593
Copy link

Hello @sberyozkin,

Thanks a lot for last message, it was very helpful.

Based on X-Forwarded and X-Original headers, we did a bad workaround

Workaround

application.properties

quarkus.http.proxy.proxy-address-forwarding=true
quarkus.http.proxy.allow-forwarded=false
quarkus.http.proxy.enable-forwarded-host=true

quarkus.http.proxy.allow-forwarded=false should be false otherwise X-Forwaded are not taken ForwardedParser.java#L135

CustomResolver

@ApplicationScoped
public class CustomTenantResolver implements TenantResolver {

    @Context
    HttpServerRequest request;

    @Override
    public String resolve(RoutingContext context) {
        String originalHost = context.request().headers().get("X-ORIGINAL-HOST");
        context.request().headers().set("X-Forwarded-Host", url);
        context.request().headers().set("X-Forwarded-For", url);

        ....
    }
}

We override X-Forwarded headers by X-Original values because ForwardedParser.java doesn't manage X-Original headers.

Here are headers we receive from ingress

X-Forwarded-Host: kubedns.cluster.local:80
X-Forwarded-Port: 443
X-Forwarded-Proto: https
X-Scheme: https
X-Original-Forwarded-For: 99.999.999.99:47111
X-Original-URL: /authentication?tenantId=tenant1
X-ORIGINAL-HOST: company.com

It is interresting to see that keykloak have a KEYCLOAK_FRONTEND_URL mode,
in our case, we do not use keycloak, I do not think the providers we used have a similar property to set,
Without that,
how can we set the full url of the redirect path ? (on quarkus)

It works because the URL we defined refers to the public DNS of our authentication service

Questions

For workaround:

  • Should we create a ticket to ask if it's possible to manage X-Original in ForwardedParser.java ?

For Oidc extension

  • Is our PR good for you ? or should we do it differently maybe we have to create a new properties instead of quarkus.oidc.tenantName.authentication.redirect-path

@sberyozkin
Copy link
Member

@AJ1039593 Thanks for making it work, it is a good enough workaround IMHO :-)

Should we create a ticket to ask if it's possible to manage X-Original in ForwardedParser.java ?

Please do - I think we need to make it very easy for cases where the server is running behind Kubernetes Ingress and i can imagine it would not only be needed for quarkus-oidc.

Is our PR good for you ? or should we do it differently maybe we have to create a new properties instead of quarkus.oidc.tenantName.authentication.redirect-path

The redirect-path alone is enough - but can you confirm your PR is enough and all works when this PR is applied ?

@sberyozkin
Copy link
Member

sberyozkin commented Jun 2, 2021

@AJ1039593 can you try adding:

quarkus.http.proxy.forwarded-host-header=X-ORIGINAL-HOST

See https://github.com/quarkusio/quarkus/blob/main/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/ForwardedParser.java#L146

That should really do it without any workaround

@sberyozkin
Copy link
Member

sberyozkin commented Jun 2, 2021

I'll update the OIDC docs once it works - it should really be supported well at the Quarkus Vert.x level - for the users not having to be concerned about such details as the external URLs a given Quarkus endpoint can be accessed at (OIDC logout would be another redirect which would be affected, etc)

@fouad-j
Copy link
Author

fouad-j commented Jun 8, 2021

Hello @sberyozkin,

Sorry for delay, we managed to install OIDC MULTI-TENANCY in our microservices architecture and behind Kubenetes.

We used two identity providers

Here is the configuration of Gigya (it may help someone)

quarkus.oidc.gigya.auth-server-url=https://url.gigya.sap.com
quarkus.oidc.gigya.client-id=${GIGYA_CLIENTID:test}
quarkus.oidc.gigya.credentials.client-secret.value=${GIGYA_CLIENTSECRET:test}
quarkus.oidc.gigya.authentication.redirect-path=/authentication/gigya
quarkus.oidc.gigya.authentication.scopes=profile,email
quarkus.oidc.gigya.application-type=web-app
quarkus.oidc.gigya.authentication.user-info-required=true

The configuration to make OIDC works behind proxy/reverse proxy like ingress

Solution 1 (easy)

Merge my PR or extends OidcAuthenticationMechanism and CodeAuthenticationMechanismto apply the fix if PR is reject.

then set you redirect_uri in application.properties

quarkus.oidc.tenantName.authentication.redirect-path=http://mydomain.com/tenantName

http://mydomain.com/tenantName refer to the public DNS of our authentication service in the cluster

Solution 2 (hard)

This solution is a little difficult because you need to know how (kubernetes, ingress, proxy/reverse proxy, headers...) work

Here is the configuration that you may adjust to your case (more details)

#mandatory config when quarkus is running behind a reverse proxy
quarkus.http.proxy.proxy-address-forwarding=true
quarkus.http.proxy.allow-forwarded=false
quarkus.http.proxy.enable-forwarded-host=true
quarkus.http.proxy.forwarded-host-header=X-ORIGINAL-HOST

@sberyozkin thanks a lot for your help, please let me know if it's not clear or if you need more details

@sberyozkin
Copy link
Member

Hi @fouad-j thanks for the update, and glad to hear it works with configuring the forwarded header.

The hardest part here is finding out the name of X-ORIGINAL-HOST - I will document it and it will become easier - and it is likely documented somewhere in Kunerners networking guides.

Setting http://mydomain.com/tenantName does look easier - but it basically requires the Quarkus endpoint to be configured to point to its own absolute address - which is a workaround with its own side-effects - the configuration is tied to the external environment properties while we have users depending on supporting the forwarded headers - which appears to be a common approach to adapt between the various external and internal HTTP properties

@sberyozkin
Copy link
Member

@pedroigor What is your opinion ? The problem is well descried above: when the OIDC provider (it is not Keycloak, so no KEYCLOAK_FRONTEND_URL) is redirecting back to the endpoint running behing Kubernetes Ingress the endpoint is not found - because when quarkus-oidc calculates the redirect_uri it uses a relative redirect-path which is added to the current request URI - which is an internal URI.

So one solution which already works is that the Quarkus forwarding filter is configured to recognize the external address (similar approach is already used for SSL terminating proxies, forwarded prefixes, etc)
@fouad-j has also proposed to support absolute redirect-path values - but this effectively requires the Quarkus endpoint configuration to include the absolute address of this endpoint - which may appear easier but does not feel right at the same time - especially if we add a requirement to support for ex the forwarding prefixes - where the forwarding filter would have to be used anyway.

Hence I'm thinking of resolving this issue with a PR updating the docs with the info how to configure the forwarding filter in cases like this one.

@sberyozkin
Copy link
Member

@AJ1039593 @AJ1032480 I've updated the docs in #17921 and it will close this issue, and I will close your PR.
The time you've spent looking into this issue is appreciated and it helped to improve the docs which will help other users.
We can revisit your PR if checking the X-Forwarded-* proves problematic in some cases
thanks

@quarkus-bot quarkus-bot bot added this to the 2.1 - main milestone Jun 16, 2021
@gsmet gsmet modified the milestones: 2.1 - main, 2.0.0.Final Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/oidc kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants