-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Authentication.WsFederation breaks with on-premise AD FS use #52099
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Thanks for contacting us. |
Microsoft.AspNetCore.Authentication.AuthenticationFailureException: An error was encountered while handling the remote login. |
Microsoft.AspNetCore.Authentication.AuthenticationFailureException: An error was encountered while handling the remote login. ---> (Inner Exception #2) Microsoft.IdentityModel.Tokens.SecurityTokenMalformedException: IDX14100: JWT is not well formed, there are no dots (.). --- End of inner exception stack trace --- |
We have encountered this issue as well and implemented a workaround. This may be an issue with a difference in how JsonWebTokenHandler and SamlSecurityTokenHandler handles configuration fetching from ConfigurationManager. I have reported this issue to Microsoft.IdentityModel here: AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2406 The JsonWebTokenHandler fetches BaseConfiguration from the ConfigurationManager when validating a token, the Saml*SecurityTokenHandler does not. This change a56e968#diff-42233637069e62d711d83e163c06bbf11849c4acb594daaf5587697c3ef0789fL99 changed configuration fetching so that this now is delegated to the TokenHandler when using the default ConfigurationManager or StaticConfigurationManager (inherits from BaseConfigurationManager). While this works for JsonWebTokenHandler, the SAML handlers is not compatible. |
Received the following response via email, does not appear here. From: Stephen Halter It's undocumented breaking change. We recommend updating your code to utilize the newer, more-optimized types. But if that's not possible, you can set WsFederationOptions.UseSecurityTokenHandlers = true which is the second option listed in the "Recommended action" section of the announcement. |
Thank you for your response. I am aware of the breaking change as described. Unfortunately, that is not the issue here. I can confirm the issue noted above #52099 (comment), and I can work around the initialization problem related to that which was introduced in the associated ASP.NET Core change to the WsFederation component. While that initialization issue may have a root cause deeper in the architecture and in a separate project, the issue is exposed in a component from this project. There is a separate issue in the new token handler in that it does not appear to handle WS-Fed tokens properly. But again, that is internal to the ASP.NET Core component implementation in its choice of relying on other projects. I am not using the token handlers directly, there is nothing in the code or the sample app to be switched. It appears that ASP.NET Core's Authentication.WsFederation component has relied internally on one or more broken components from other project(s). I would expect that such internal issues between projects would be addressed by the involved projects' members. To reiterate: this is not a question about the breaking change that was noted, it's an issue with the component not working as documented, requiring user space work arounds. |
Hi, I can confirm what @csowa assumes, that this is not a matter of the state of the That change relates to the use of JWT tokens (and handling them with one class or the other), but the error messages in the bug report clearly indicate that @csowa uses SAML tokens, which are handled by completely different classes. @Hakon has, as far as I can tell, correctly diagnosed the problem and has also published a great workaround in this issue: AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2406 The error message in this bug report indicates that the workaround should be applicable as-is, but users who get a SAML 2.0 version token may need to subclass As @csowa writes, this is fine for those of us who are in control of the code that configures WsFederation, but if that code resides in a separate component that cannot be easily modified, it of course becomes a direct obstacle to adoption .NET 8.0. |
This is correct. It was not intended as a general fix for everyone but rather a hint at how to work around the problem. I am working on a proper fix in IdentityModel. |
Thank you for the warning. I have submitted a fix to IdentityModel. |
Just hit this as well. This is a pretty big breaking change to be undocumented! And how come this has not been fixed? It completely breaks ADFS Integration. Thanks for the workaround, @Hakon! It works great. |
can you link to the fix. did it make it into net8? |
That is the bug covered by this issue. This appears to have been prematurely closed, pending resolution / confirmation of WsFederation's dependency. |
Microsoft.AspNetCore.Authentication.WsFederation has not been updated to reference 7.4.1, which is the one that contains the fix. The latest version 8.0.4 still references to 7.1.2. Do you know if there is any plan to get that updated ? Thanks |
@mkArtakMSFT This issue was specifically for Authentication.WsFederation breaking. Despite the fix to Microsoft.IdentityModel, Authentication.WsFederation still has not been updated to reference the fixed version, and thus the issue raised by this ticket still exists. This was closed prematurely. |
It would appear that the 4th preview version for the package with net 9 has finally increased the minimum version to 7.4.1 (and a couple later bumped it again). I'd love if they could release a version with 8, but if you ever upgrade it should eventually be resolved. |
please re-open. this is still broken |
Generally speaking, we do not force higher dependency versions for external dependencies in patch releases. Updating IdentityModel packages has not been without issues, and forcing an update to Microsoft.IdentityModel.Protocols.WsFederation to 7.4.1 would also force us to also update other packages which are much more widely used like Microsoft.IdentityModel.Tokens to 7.1.2 to 7.4.1. Unlike with new major versions, it's a lot harder for us to build confidence that taking a major update to external dependencies will not be too breaking since we do not have multiple preview releases to try to iron out any kinks. Instead, we have to rely entirely on internal automated and manual testing. I'm sympathetic to wanting AD FS to work out of the box without updating any NuGet packages with ASP.NET Core 8, but the workaround of updating outdated NuGet packages does not seem too onerous. You should generally be doing this to get security updates as fast as possible in any case. @mkArtakMSFT @wtgodbe @JeremyLikness @danroth27 @jennyf19 Given the severity of the issue, would we be willing to make an exception to our usual patching policy and take a dependency update to Microsoft.IdentityModel.Protocols.WsFederation, Microsoft.IdentityModel.Tokens, et al. in a .NET 8 patch? And would 7.4.1 be the version we pick? It does seem risky and has implications beyond just the Microsoft.AspNetCore.Authentication.WsFederation package, and the workaround is simply updating NuGet packages. |
@mkArtakMSFT @wtgodbe @JeremyLikness @danroth27 @jennyf19 any thoughts on the above? |
To be clear, the issue is not only affecting apps using adfs, but any app that uses an external idp saml metadata file. |
What's stopping users from updating the PackageReference to WSFederation in their own app to work around the issue? That would generally be our recommendation for situations like this. I generally agree w/ @halter73's interpretation here - we definitely wouldn't upgrade to a new Major version in a servicing release, and upgrading to a new Minor version would be highly unusual |
What do you mean with that ? If you reference the latest version for WSFederation, it includes the version 7.1.2 for Microsoft.IdentityModel that is broken. The only workaround is to go and manually reference the assembly with the latest version of the SamlHandler that fixes the issue. |
Sorry, looks like I got some of the details wrong.
This is what I was trying to suggest - Would adding a PackageReference to the 7.4.1 version of |
@wtgodbe Even if I don't find it a very clean solution, yes: manually forcing System.IdentityModel.Protocols.WsFederation to the version (at least) 7.4.1 fixes the issue: |
@FlavioMH thanks for confirming - we're happy to take these kind of dependency updates in our active development branches (e.g. we've already done this update in .NET 9 & 10), but our policy is not to take Minor or Major version updates in Servicing releases, as they can contain breaking changes. Given that the proposed workaround fixes the problem, I'm going to close this issue. Feel free to re-open if you feel there's an angle that we've missed here. |
@wtgodbe As I understand the problem, angle you missed in last message is that WsFederation package is almost completely broken without this update. |
@wtgodbe Not to mention that this is a regression compared to .NET 7. It should absolutely be fixed in a servicing update for .NET 8, especially since .NET 8 is an LTS release with a longer lifespan. |
Unfortunately we aren't able to take the dependency update in 8.0, as there is a known breaking change between 7.1.2 and 7.4.1 - AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2513. If we were to take that update, then later realize that we'd broken even more people, it would be impossible for us to roll it back. We realize that this isn't a great state to be in, but we're essentially stuck between a rock and a hard place on this one. I'm going to re-open this issue to track our ongoing work to improve the documentation for this one (to suggest adding a |
And just an FYI, I updated the repro project with a branch that verifies the proposed work-around, in this case taking an explicit dependency on Microsoft.IdentityModel.Protocols.WsFederation v8.0.0, which is version-number-consistent with the rest of the packages. |
@wtgodbe you probably want to mention this issue in WsFederation project readme and other high visibility places. I just wanted to make sure you understand that package is completely broken OOB without update. |
I do not have really deep insight into this, but maybe this could be helpful for someone:
|
Having last year used Hakon's workaround … I concur with vojta-vorel that after adding a reference to Microsoft.IdentityModel.Tokens.Saml Version="8.6.0", I no longer need the workaround, it all seems to works fine. |
Verified @vojta-vorel's Microsoft.IdentityModel.Tokens.Saml Version="8.6.0" proposal with the repro app: csowa/aspdotnetcore-auth-sample#2 Also note, that's the most recent version at this time. |
Be careful that including that dependency in net9.0 can trigger another bug in other auth scenarios: |
Is there an existing issue for this?
Describe the bug
Using Authentication.WsFederation for authentication results in error regardless of new UseSecurityTokenHandlers setting.
options.UseSecurityTokenHandlers = true;
SecurityTokenInvalidIssuerException: IDX10204: Unable to validate issuer. validationParameters.ValidIssuer is null or whitespace AND validationParameters.ValidIssuers is null or empty.
Expected: behavior prior to change introduced with issue 49469.
options.UseSecurityTokenHandlers = false;
XmlReadException: IDX30011: Unable to read XML. Expecting XmlReader to be at ns.element: 'urn:oasis:names:tc:SAML:2.0:assertion.Assertion', found: 'urn:oasis:names:tc:SAML:1.0:assertion.Assertion'.
Expected: to be able to handle SAML 1.0 assertion emitted by WsFed server.
Expected Behavior
options.UseSecurityTokenHandlers = true;
Expected: behavior prior to change introduced with issue 49469.
options.UseSecurityTokenHandlers = false;
Expected: to be able to handle SAML 1.0 assertion emitted by WsFed server.
Steps To Reproduce
Repro project: https://github.com/csowa/aspdotnetcore-auth-sample
Demonstrates WS-Federation issue with change introduced for #49469
Based on example at https://learn.microsoft.com/en-us/aspnet/core/security/authentication/ws-federation?view=aspnetcore-8.0#use-ws-federation-without-aspnet-core-identity
Build and run. Home page uses [Authorize] attribute, authentication begins when loading.
Dependency: ADFS server required. Server version tested: 10.0.17763.4644
Exceptions (if any)
options.UseSecurityTokenHandlers = true;
SecurityTokenInvalidIssuerException: IDX10204: Unable to validate issuer. validationParameters.ValidIssuer is null or whitespace AND validationParameters.ValidIssuers is null or empty.
options.UseSecurityTokenHandlers = false;
XmlReadException: IDX30011: Unable to read XML. Expecting XmlReader to be at ns.element: 'urn:oasis:names:tc:SAML:2.0:assertion.Assertion', found: 'urn:oasis:names:tc:SAML:1.0:assertion.Assertion'.
.NET Version
8.0.100
Anything else?
No response
The text was updated successfully, but these errors were encountered: