[Feature Request] Allow relaxed parsing of URIs in SAML2

[glatzert]

I'm an AAI Admin at a german university and I'm currently in the process of bringing "BundID" into our AAI.
BundID is a federated login system to allow login via European Issued Identity Cards, is mostly a SAML2, but has some unique quirks. One of them being "AuthnContextClassRef" not being an absolute URI.

While this works well with Keycloak and (most likely) Shibboleth IdP, we're at a loss with Microsoft.IdentityModel, since there's an active check for IsAbsolute in https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blame/7699e533557a32fb3b74dc440c10a61c9b90df1b/src/Microsoft.IdentityModel.Tokens.Saml/Saml2/Saml2AuthenticationContext.cs#L81-L82.
From a standards view-point this might be the correct thing to do, but from a library user standpoint, this might be a little bit too strict.

So I'd like to propose a relaxation of the check by either using an configurable switch OR by removing the check alltogether - I'm aware this isn't 100% up to SAML2-Core, but it's at least interoperable with other SAML2 products.

Unfortunately my only alternatives are either: take the SAML Response, Decrypt it, validate signature by hand (that won't happen) or ditch the usage of Microsoft.IdentityModel and go down another route with e.g. Keycloak.

[brentschmaltz]

@glatzert this isn't the first time this has happened where strict adherence to the spec has caused interoperability issues. Most likely the authors were thinking about the results of composing xml where a relative uri could change the context.
This happens when a SAML token would be embedded into a WS* message.

I think we would need a switch with the default to be an absolute uri.

Would you like to propose a PR?

[glatzert]

I'm starting to work on it immediately 👍

[glatzert]

This was more involved than I expected, since moving a validation from the reader to the token handler, might have unforseen consequences, nevertheless I made a draft here:

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/compare/dev...jguzdv:azure-activedirectory-identitymodel-extensions-for-dotnet:ottenhus/relaxed-authncontextclassref-validation?expand=1

If that's principally an route to go, I'd add test cases whatever else is neccessary to make it a proper pull request

[brentschmaltz]

@glatzert i see a dilemma.
When the Saml2 token is read, this gets called:

azure-activedirectory-identitymodel-extensions-for-dotnet/src/Microsoft.IdentityModel.Tokens.Saml/Saml2/Saml2Serializer.cs

Line 638 in 7699e53

var authnContext = new Saml2AuthenticationContext(classRef);

Which will throw if the uri is not absolute.
However, when we read the uri a couple of lines up, we allow for a relative uri.

If we introduce an AppContext switch, AllowRelativeUrisInSaml2AuthnContext (false by default), then place that above the check here:

azure-activedirectory-identitymodel-extensions-for-dotnet/src/Microsoft.IdentityModel.Tokens.Saml/Saml2/Saml2AuthenticationContext.cs

Line 81 in 7699e53

if (!value.IsAbsoluteUri)

We would have a solution, but it would be for all tokens.

[glatzert]

I learned something today: AppContext is a thing.

So I created an AppContextSwitch and evaluated it in the code.
https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/compare/dev...jguzdv:azure-activedirectory-identitymodel-extensions-for-dotnet:ottenhus/allow-relative-authnContextUri?expand=1

I tried providing a test, but the test currently flip-flops, which seems to be due to AppContext being shared across Tests and Test classes - do you have advice how to handle that?

[brentschmaltz]

@glatzert the issue with AppContext is that tests that use a singleton such as an AppContextSwitch need to run serial.
Xunit is parallel by default.

You might want to look at: https://xunit.net/docs/shared-context#collection-fixture