Good job

@DorianMazur Thanks for the PR.

IMO this requires a bit more work to be secure by default and flexible at the same time. Few considerations regarding this PR:

• This PR allows wildcards by default. I think that it will be good to not go this way, but rather allow wildcards just if it is explicitly allowed by the corresponding executor from the client profile, which is allowed by some client policy
• It seems to me that this PR allows to use wildcards everywhere. In other words, you can use redirect URI like "https://*/foo" or "https://www.myhost.com/*/something" . In my opinion, it will be better to allow have support for wildcards just in the subdomains. Regarding this, it is important to consider the possible attack described in this JIRA comment https://issues.redhat.com/browse/KEYCLOAK-14071?focusedCommentId=15741460&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15741460 and not allow redirect URIs like https://evilhackingsite.com/evil.apps.legitsite.io/...

IMO, the possible solution for this might be to add the option on the SecureClientUrisExecutor. The option might be like "Wildcards in Redirect URI" with the values like:

• no wildcard supported (Default value)
• Wildcards at the end (Will allow stuff like "https//www.myhost.com/foo/*" similarly like now)
• Wildcards at the end and in subdomains (Will allow stuff like "https://.myhost.com/foo" or "https://.myhost.com/foo/", but not "https:///foo")

The alternative, which will be more flexible, is to use option like "Allowed Redirect URI regex pattern", which will allow more flexibility to specify both wildcards and protocol. This might be even better approach probably, however a bit harder for administrators to setup as not everyone is familiar with regexes. So we would need to document examples with the common use-cases (EG. Example for wildcards in the subdomain, wildcards at the end of the path etc).

One thing to note when comparing this to the current capability, especially seen from a security angle, is that you can actually set just a lone wildcard currently, eg: valid_request_uri: "*". Ofcourse from a security point of view this is quite abhorrent, but it's just to say that while this flexible wildcard capability does add more refined ways to shoot oneself in the foot, the current approach has exactly the same potential for the same. And there are actually usecases (like those mentioned in the issue) that invite it.

I believe the patch should considered as-is, perhaps with the addition of an interface warning when a wildcard is used anywhere in a redirect string.

How about using * as a strict wildcard that preserves the current behavior
and introducing ** as dynamic wildcard that behaves as proposed in this PR?

Is this something that will be merged? Really need this implementation

I am closing for now. Anyone is welcome to open the discussions with more details. Please add the discussion link as a comment here to this PR when you do it. Bottom line is that we won't allow more wildcards by default, but we may need to drive this with client policies if better flexibility is needed (for example with wildcard in subdomains).

I vote for reopening this issue.

Why cant we implement like others already proposed:
https://*.foo.com/*
which would allow https://preview-1.foo.com but not https://evilpage.com/evil.foo.com

Is it just a question of security for the Keycloak team? I dont understand how such a feature would introduce any problems. As long as the wildcard is limited to the root domain foo.com

For someone that uses deploy preview urls that are dynamically created for each pull request for the application, keycloak is rendered completely unuseable without this feature

Wildcards in subdomains may be perfectly fine in some environments, but not in others. As previously mentioned we are happy to have this included, but not as a default. We are moving towards not allowing wildcards at all by default, and using client policies to opt-in to allow wildcards for all or some clients.

For now you can use simple "*" for redirect uri instead of "https://*.review.example.com". It just works for me.

I'm wondering what exactly is this attack vector that allows someone to add dns entries to match *.example.com, but prevents them from hijacking the foo.example.com record that's specified in your redirect URI?

I think that for review deployments any kind of attack don't make sense, so i don't care about it.

See here for a follow-up discussion on this topic:
#9278

@jputney there are plenty of situations where a sub-domain is not fully owned by an individual application, and where subdomains are created dynamically on-behalf of other users (AWS for example).

@navrocky * is a terrible thing to use or advocate, and leads to a magnitude of vulnerabilities. I would suggest you refer to OAuth 2.0 best practices to read up on the various vulnerabilities that are associated with insufficient redirect-uri validation.

@navrocky * is a terrible thing to use or advocate, and leads to a magnitude of vulnerabilities. I would suggest you refer to OAuth 2.0 best practices to read up on the various vulnerabilities that are associated with insufficient redirect-uri validation.

At least "https://*.review.example.com" is something that is needed for every feature branch based test environment.

I also support this for the reason @to-bee cites.

Hi @stianst

Real question: if this is a "terrible thing to use or advocate, and leads to a magnitude of vulnerabilities", why the simple * is allowed in 'redirect_urls' ?