DuendeSoftware / IdentityServer

The most flexible and standards-compliant OpenID Connect and OAuth 2.x framework for ASP.NET Core
https://duendesoftware.com/products/identityserver
Other
1.49k stars 344 forks source link

Added support for multiple valid token issuers #1590

Closed AliKameli closed 4 days ago

AliKameli commented 2 months ago

Adds support for this problem: https://github.com/DuendeSoftware/Support/issues/1062

narbit commented 2 months ago

This would be very useful to us too. Either allow configuring multiple issuers or make TokenValidator extensible. The former is obviously less invasive. The issue came up several times and similar PR has been previously proposed but hasn't been merged:

https://github.com/IdentityServer/IdentityServer4/pull/4967 https://github.com/DuendeSoftware/Support/issues/707 https://github.com/IdentityServer/IdentityServer4/issues/1847

josephdecock commented 1 month ago

Thanks for the PR! I do have some reservations about this though.

narbit commented 1 month ago

Our use case is very similar to https://github.com/DuendeSoftware/Support/issues/707 and https://github.com/DuendeSoftware/Support/issues/1062#issuecomment-1895826541.

Some of the reasons for using multiple domains for the same IDP are:

If allowing multiple issuers is not something you're willing to support out of the box, would you consider making token validator extensible as also requested here and here?

josephdecock commented 1 month ago

Multiple issuers is absolutely a valid use case. What makes less sense to me is why you have clients using the issuers inconsistently - e.g., if you're getting a token on the public dns name, why switch to the private dns name for the userinfo call?

narbit commented 1 month ago

Some services may be purely public, some may be purely private while some are a mixed-use. It's for these mixed use services where it becomes a problem. Also, it's not just userinfo that is affected, it's also an introspection.

josephdecock commented 1 month ago

Oh, ok. I'm with you now. Thanks for taking the time to get into the detail of your use cases. I'm leaning much more favorably towards this change now.

Maybe we can expand the xmldoc to make the intended use more obvious. I also still want to think about how this interacts with the static issuer config and the inferred issuers - maybe this should constrain which issuers can be inferred.

We'd also want to get some automated tests in place before merging this.

narbit commented 1 month ago

I also still want to think about how this interacts with the static issuer config and the inferred issuers - maybe this should constrain which issuers can be inferred.

That's an option, yeah.

We'd also want to get some automated tests in place before merging this.

@AliKameli , would you be able to provide these?

narbit commented 1 month ago

@josephdecock Let me know if the tests are the only missing part here, please. I could probably add these. Or if you need more time to consider different approaches. Thank you! :pray:

AliKameli commented 1 month ago

Sorry i hadn't checked GitHub for a while. I don't know how to write the tests that are needed. This problem affected us because we migrated our old identityServer from v4 to v7 and changed the address of the authority in the process, but some of our old legacy services had tokens saved that was from the older authority address and we couldn't change the code.

josephdecock commented 4 days ago

Sorry that this was left open for a long time! We had some more internal discussion about this and now I'm thinking that there area couple of better alternatives to this approach.

The downside to this approach is that normally an issuer is defining a trust domain. (E.g., in a multi-tenant system, you want to enforce that a token from one tenant is never used in another.) Multiple expected issuers breaks that boundary, so I'm very reluctant to add options that encourage users to do so.

Having a resource server straddle a trust boundary and expect tokens from multiple issuers isn't the most idiomatic design. If you have resources that are shared, a more typical approach would be to put the shared resources in their own trust domain, and perform token exchange to move over the trust boundary.

Another option would be to statically configure the issuer, so that every token would always use the same issuer. Then you could do your rate limiting based on the host without the issuer inference getting in the way. But do note that if you do that you are removing the trust boundary that different inferred issuers implies. But even so, you could still create boundaries using resource indicators or scopes within the token.