DuendeSoftware / Support

Support for Duende Software products
21 stars 0 forks source link

Is this a bug? Overriding Redirect Uri value #436

Closed dcrinion closed 1 year ago

dcrinion commented 1 year ago

We have a multi-tenant website where we allow customers to set up their own custom domain to point to it via CNAME. the main site is exampleA.com

A custom domain can be pointed to that same site - e.g. exampleB.com

We set up Identity Server at sts.example.com

So, the flow is exampleA.com -> sts.example.com -> exampleA.com

but we also need to allow exampleB.com -> sts.example.com -> exampleB.com

exampleA.com and exampleB.com are the same client application.

In identity server we configure these both as allowed RedirectUris

In the www project, the startup redirectUri can only take one value, but Microsoft documentation says that you can override this at the request level by using the RedirectToIdentityProvider notification and set the redirectUri there to match the domain the user is on - The OWIN OpenID Connect Middleware | Microsoft Press Store

When we did this and stepped through the code, it was correctly overriding the redirectUri. However, IdentityServer seemed to ignore it and use the redirectUri that is inside the ProtocolMessage.State object instead.

The workaround we used was to decrypt the state, modify the redirectUri in there and then encrypt it again and this worked

// The following code should update the redirectUri but identity server doesnt seem to honor it
// https://www.microsoftpressstore.com/articles/article.aspx?p=2473126&seqNum=2
n.ProtocolMessage.RedirectUri = $"{dynamicBaseUri}/signin-oidc";
n.ProtocolMessage.PostLogoutRedirectUri = dynamicBaseUri;

// instead we update the state object 
var p = n.Options.StateDataFormat.Unprotect(n.ProtocolMessage.State.Replace("OpenIdConnect.AuthenticationProperties=", ""));
p.Dictionary["OpenIdConnect.Code.RedirectUri"] = n.ProtocolMessage.RedirectUri;                        
n.ProtocolMessage.State = "OpenIdConnect.AuthenticationProperties=" + n.Options.StateDataFormat.Protect(p);

It seems like from the other examples including the Microsoft one in the link above, that we shouldn't have to update the state.

The workaround seems to work fine but I wanted to check if this is a bug or if not, is there a better way to accomplish what we're doing.

Thanks!

josephdecock commented 1 year ago

The Owin plumbing definitely gets less attention than the .NET Core stuff these days, so it's possible that you've run into a bug. But, IdentityServer doesn't and can't use the state to determine where you get redirected. The state is data protected by the client application, and IdentityServer doesn't normally share data protection keys with the client. Thus, there's no way that IdentityServer could have decrypted that value.

Are you able to tempoarily disable the update to the state object, reproduce the issue, and capture network traffic during the flow? I'd be really interested to see that.

Umar-Mukhtar commented 1 year ago

@dcrinion seems like we are in the same boat and building the same type of application and stuck at the same point 🙂 let's connect to overcome the duende challenges together and discuss. Connect with me malikumarmukhtar@gmail.com

@josephdecock appreciate your feedback on other questions i posted on the forum.

josephdecock commented 1 year ago

Looking at this code again, I see the problem. The issue is that the authorize and token requests are required to send the same redirect_uri parameters, and the mechanism that the OIDC Auth Handler uses to accomplish that isn't aware of your customization updating the ProtocolMessage.RedirectUri. IdentityServer is correctly validating the incoming requests to token and authorize, it is your customization of the OIDC handler that is causing the two requests to have mismatched redirectUris.

This code updates the protocol message on the authorize request:

n.ProtocolMessage.RedirectUri = $"{dynamicBaseUri}/signin-oidc";

But the subsequent token request uses the value saved in the authentication properties under the key "OpenIdConnect.Code.RedirectUri".

The handler sets that auth property based on Options.RedirectUri, so that value won't ever match your dynamic base uri.

Thus, if you need to set the redirect uri in this way, you have to also set it in the authentication properties.

Long story short, this is fine. I would maybe look a little bit into if it is possible to set the authentication property you need earlier in the process - before you are at the point of needing to unprotect this data - but I'm not sure if that is possible.

Umar-Mukhtar commented 1 year ago

i just used IRedirectUriValidator to add ridrect uri at runtime and now i am able to map any wildcard against the domain for duende. Make sure the calling app and duende redirecturi match 100%. I am injecting custom from the db at runtime. I am posting the sample

 public class WildcardRedirectUriValidator  : IRedirectUriValidator
    {
        IInjectService service; 

        public WildcardRedirectUriValidator(IInjectService _service)
        {
            service = _service;

        }
        public   Task<bool> IsPostLogoutRedirectUriValidAsync(string requestedUri, Client client)
        {
            List<string> tenantUris = client.RedirectUris.ToList();
            var tenants = service.GetAllTenantUrls().Result;
            if (tenants.Count() > 0)
                tenantUris.AddRange(tenants);
            return MatchUriAsync(requestedUri, tenantUris);
        }

        public   Task<bool> IsRedirectUriValidAsync(string requestedUri, Client client)
        {
            List<string> tenantUris = client.RedirectUris.ToList();
            var tenants =   service.GetAllTenantUrls().Result;
            if(tenants.Count() > 0)
            tenantUris.AddRange(tenants);
            return MatchUriAsync(requestedUri, tenantUris);
        }

        private async Task<bool> MatchUriAsync(string requestedUri, List<string> allowedUris)
        {
            var rules = allowedUris.Select(ConvertToRegex).ToList();
            var res = rules.Any(r => Regex.IsMatch(requestedUri, r, RegexOptions.IgnoreCase));
            return  (res);
        }

        private const string WildcardCharacter = @"[a-zA-Z0-9\-]";

        private static string ConvertToRegex(string rule)
        {
            if (rule == null)
            {
                throw new ArgumentNullException(nameof(rule));
            }

            return Regex.Escape(rule)
                        .Replace(@"\*", WildcardCharacter + "*")
                        .Replace(@"\?", WildcardCharacter);
        }
    }