Dynamic RedirectUris validation

[gioccher]

I'd like to use IdentityServer as a single sign-on server for a big number of websites we develop and host.
Thinktecture.IdentityServer.Core.Models.Client exposes the List<Uri> RedirectUris property, which is very convenient to fill during the application startup if you have an immutable and small list of clients.
In our case this list would be fairly large (about 15000 hostnames, including developer/staging/preview versions of each website), and can change at any time.

A way to address this would be to allow users of the IdentityServer library to specify their own RedirectUri validation.
Maybe something as simple as public Predicate<Uri> IsRedirectUriAllowed { get; set; } instead of/in addition to public List<Uri> RedirectUris { get; set; } could do the trick.
In this way, we could provide a delegate that gets called for every authorization request and that looks up the supplied URI in our DB.

[leastprivilege]

RedirectUri validation is the most common reason for security vulenerabilities - so we are a bit reluctant to open that up - but we will discuss this.

[brockallen]

If the client (and thus its redirect URIs) is configured in the DB, then why can't you just add to this list in the DB?

[gioccher]

Because it isn't a fixed list: after the authentication client has been initialized, the list of websites allowed to use that client can change at any time (mostly new websites will be added, but it may also happen that some websites will be removed).

[johnkors]

Hi guys,

We don't have that a huge list of client apps, but we do deploy our client apps to a whole lot of different environments, even for a single application. So +1 for a bit more possiblities in Redirect URI validation if it's possible without much hassle /impact on security :)

Examples of Redirect URI validation patterns that would be nice:
Some devs like to use virtual paths locally, or on a shared test server. For instance each feature-branch may be deployed to individual virtual paths under the same Site in IIS.

RedirectUri = http://localhost/{_}/signincallback, where * = is any virtual path
RedirectUri = http://testserver/{_}/signincallback where *= for instance branchname/featurename

Or in a more common environment scenario with defined hostnames:
RedirectUri = http://myapp.{*}.mydomain.com where * = dev, test, stage ++

Just some thoughts.

[brockallen]

Right, just trying to gather your use cases and requirements. So it seems that the thing that changes is the host name, not the path. This makes me less nervous :)

[brockallen]

In any event, I think we'll try to add something to help you out.

[johnkors]

Cool, thanks!

[leastprivilege]

"+1 for a bit more possiblities in Redirect URI validation if it's possible without much hassle /impact on security :)"

It is not a lot of hassle - But since you will be in full control over the redirect validation process - you define the impact on security yourself then ;)

[johnkors]

That depends on what kind of API you like to expose - partial control or full control. Is it all or nothing in this case?

var pattern = new RedirectUriPattern("http://myapp.{placeholder}.domain.com/callback")
(optional:)
pattern.AddWhiteList("test", "stage", string.Empty);

RedirectUri = new List<RedirectUriPattern>{ pattern }

If no whitelist is added, all values for {placeholder} would be allowed.

Again, just some thoughts. I trust your judgement if you say it's a potential risk :)

[leastprivilege]

Do your own research - google for "covert redirect" - and how facebook, github et al got hacked..

The more target URLs you allow to send a token to - the higher is the chance that one of them has a vulnerability.

Anyways - I will add a hook for it - but it will be "use at your own risk".

[johnkors]

Right, I see. I think we'll manage without this feature then (cannot speak for the OP, though). Thanks for clarifying, guys.

[leastprivilege]

@gioccher Can you confirm that you still like to have that feature?

[gioccher]

Yes, sorry for not replying earlier to this conversation.
In the POC project I built I ended up modifying the server as follows.

diff --git a/source/Core/Models/Client.cs b/source/Core/Models/Client.cs
index 2f85cc9..dee8467 100644
--- a/source/Core/Models/Client.cs
+++ b/source/Core/Models/Client.cs
@@ -75,6 +75,11 @@ namespace Thinktecture.IdentityServer.Core.Models
         /// </summary>
         public List<Uri> RedirectUris { get; set; }

+       /// <summary>
+       /// Validates the redirect URI supplied by the client. Defaults to check if the supplied URI is contained in <see cref="RedirectUris"/>.
+       /// </summary>
+       public Predicate<Uri> IsRedirectUriAllowed { get; set; }
+
         /// <summary>
         /// Specifies allowed URIs to redirect to after logout
         /// </summary>
@@ -181,6 +186,8 @@ namespace Thinktecture.IdentityServer.Core.Models

             IdentityProviderRestrictions = Enumerable.Empty<string>();
             AllowLocalLogin = true;
+
+           IsRedirectUriAllowed = uri => RedirectUris.Contains(uri);
         }
     }
 }
\ No newline at end of file
diff --git a/source/Core/Validation/AuthorizeRequestValidator.cs b/source/Core/Validation/AuthorizeRequestValidator.cs
index 6720ef6..2b8c308 100644
--- a/source/Core/Validation/AuthorizeRequestValidator.cs
+++ b/source/Core/Validation/AuthorizeRequestValidator.cs
@@ -349,7 +349,7 @@ namespace Thinktecture.IdentityServer.Core.Validation
             //////////////////////////////////////////////////////////
             // check if redirect_uri is valid
             //////////////////////////////////////////////////////////
-            if (!_validatedRequest.Client.RedirectUris.Contains(_validatedRequest.RedirectUri))
+            if (!_validatedRequest.Client.IsRedirectUriAllowed(_validatedRequest.RedirectUri))
             {
                 Logger.ErrorFormat("Invalid redirect_uri: {0}", _validatedRequest.RedirectUri);
                 return Invalid(ErrorTypes.User, Constants.AuthorizeErrors.UnauthorizedClient);

It isn't a big change for me to apply to your new releases, but it would be nice if something like this could be supported in the original library so I could just grab the updated library from nuget without any modifications.

[leastprivilege]

I added an extensibility point - see here

068093d

You can implement that interface and register your implementation on the factory in startup.

[leastprivilege]

Does that work for you?

[gioccher]

yes, that will work. Thanks

[milomasters]

I am in a similar situation where I need this.

I see that you started implementing this 068093d, but I don't see it in use on the AuthorizeRequestValidator. Maybe this is a stupid question, but how do I register this so that it gets used in the AuthorizeRequestValidator?

[leastprivilege]

It is all on the dev branch for now. Check the DI docs on the wiki to learn how register your own services.