-
-
Notifications
You must be signed in to change notification settings - Fork 54
Conversation
thank you @Compufreak345
Hey, thanks! We are too busy right now to do a proper review. From what I can see it looks good. But it needs some real world testing, e.g.
ADFS and SharePoint are the two main reasons this sample was created in the first place... |
Hi, how does this component relate to the one offered as a commercial plugin (https://www.identityserver.com/products/)? Looking for ways to add Identityserver as a claims provider trust in ADFS, and this seems to provide that, am I right? |
Hi @Mich-b I don't know the commercial plugin, but this one is really basic. What I mean by basic is, it provides not all features WS-Federation provides, but should be enough for most simple use cases. If you could try it and maybe report here if it worked for you with ADFS, this would be awesome as I don't have an ADFS server to test it against. You can use this nuget package for that: https://www.nuget.org/packages/Kowalew.IdentityServer4.WsFederation/2.1.0-alpha2 |
Seem to get a nullreferenceexception when accessing the metadata endpoint (/wsfederation):
This is likely due to me starting from a later version of Identityserver (2.3.1) that I then downgraded to 2.1.3 (had to remove some other files as well). Also, some other changes have been made since 2.1.3 (including the key material / pfx file). Any idea how to get this to work with the latest version? |
@Mich-b you are right, it is written for an older version of identityserver (nuget package version matches the version of identityserver it was written for). I will update it in the next couple of days and upload it to nuget.org. |
It seems the issue is at line 31 (and therefore also at 32 and 42) of MetadataResponseGenerator.cs : you expect the signingcredentials to be a X509SecurityKey, but it doesn't have to be. In the newer IdentityServer versions it is a RsaSecurityKey. Something like this solves it:
But I'm not sure if it is a) secure and b) portable enough to be able to deal with several key types. Also see https://github.com/IdentityServer/IdentityServer4/blob/63a50d7838af25896fbf836ea4e4f37b5e179cd8/src/ResponseHandling/Default/DiscoveryResponseGenerator.cs , in the CreateJwkDocumentAsync the type of key is explicitly checked. |
Ok, tested this in more detail with ADFS 2019 and with X509Certificates (which means my previous reply does not really apply anymore, when using X509certs everything worked smoothly). A couple of bugs/issues I ran into:1) nullreference due to null userline 77 to 84 in 'WSFederationController.cs: what are they doing? There is no user, so it fails with a nullreference. It seems the only purpose of those lines is logging, but still they break it all. Fix : uncommented these 2) specification of a granttype is required, otherwise it fails with 'invalid client configuration'This seems weird, since these granttypes are about oauth2/oidc, not about ws-fed A couple of bugs/issues I ran into with ADFS 2019:3) wrong use of a namespace in the token issued from identityserver to ADFSADFS fails with the error:
Fix: replace "WsUtility.Namespace" by "http://docs.oasis-open.org/wss/2004/01/oasis-200401-wss-wssecurity-utility-1.0.xsd" in the file 'RequestSecurityTokenResponse'. . Possibly we should not make this hardcoded.
|
I updated the dependencies to v2.4.0-preview1. It works fine with sample clients included here. @Mich-b can you test it with the newer version of IdentityServer and the new package? |
Great! I will test it in the coming days.
|
Verified with IdentityServer 2.4.0-preview1 and Kowalew.IdentityServer4.WsFederation 2.4.0-alpha1:
This is great, thanks for the effort! Specification of a granttype was also not required anymore. Note that after importing the metadata in ADFS, the 'encryption' tab and 'offered claim types' tab are empty in ADFS. It is however, not required and can be configured manually. Probably this can also be added to the metadata. |
This looks promising! Is there any way you can test with SharePoint? |
@Mich-b Thank you for testing. I have tasks for encryption and offered claim types added to my todo list. I will look into it whats possible with the Microsoft.IdentityModel.* libraries, at the moment they are really limited (Serializing Encrypted SAML Assertions will be added probably in next major release v6.x which can take a while). Maybe I can do serialization on my own as a workaround until its supported by official libraries. @leastprivilege I will look into it in the next couple of days if I can use vagrant to set up sharepoint for my local machine from a template. |
@leastprivilege So it took a little bit longer than I thought but got it tested with Sharepoint 2016. It works fine as IdP. Used uplift tools to setup SP 2016 instance using this samples: https://github.com/SubPointSolutions/uplift-contrib |
Cool! Need to schedule some time with @brockallen to review...thanks for doing this! |
Look forward to your both feedback :). I need to thank for that awesome open source work you and the community do. Just trying to give something back. |
Hi, any timeline on when/if this will be merged? |
Hi @Mich-b I think we would merge this when the functionality is at least is the same as provided by this repo (for this we need to implement the encryption for assertions, I wait till microsoft makes this feature available in the package I use). But in the meantime you can still use the NuGet package I linked above, it has basically the same code as when we would merge this PR. |
Ok, I'm using this one anyway so that's fine indeed. However, one thing I noticed again is that when running the solution with an RSA temporary key (as is default when running identityserver in development due to AddDeveloperSigningCredential), it fails as explained in #12 (comment) . |
UPDATE: Ok, I can control it by adding a nameidentifier claim (http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier) and then making sure I map it in the defaultmapping (I just map it to itself). Then the following code kicks in:
All ok, thanks! original question: Hi @616b2f , I have a requirement where the subjectID may not be passed due to privacy restrictions. It seems though that in the WSFed code this is automatically added as a nameidentifier: Removing the nameidentifier mapping does not help, since this claim is always added. Is there any way I can filter out this value, or replace it with another identifier? Changing the model for the GetSubjectId is not possible due to the impact that would have on the rest of the app. |
Hi I am looking forward to this being merged. I currently use OIDC to ADFS 2016 with a third-party external IdP via WsFederation. On our dev, test and load test environments I use IdentityServer3 with WsFederation as the external IdP in ADFS 2016. In order to upgrade to IdentityServer4 I would need WsFederation support. thanks Remco |
@Mich-b @RemcoBlok at the moment I don't have time to do the changes, but i will take time in September to look in to it. @RemcoBlok WFresh support is definitely something that could (and should) be added. Thanks to bringing this up. I will add it to my TODO list. |
On a side note, I think I will have some time to look into this in the coming weeks -- I have a project that might require some of the effort gone into this. Thanks. |
What's the best approach to keep this package updated? It still references the 2.4.0 preview version. |
@Mich-b I updated the reference to IdentityServer4 NuGet package, however I think it was not really necessary because the nuget.org shows the dependencies as |
Maybe we should put it on a branch so we can keep both the new and old implementation as both provide value?! |
Sure would you prefer to keep the branch with the old version as the main branch until the new is more feature complete, at least till it has the same features implemented as the old? |
Yea - I like the idea. I will rename dev to |
7f17ade
to
acfefa8
Compare
Done - created a new branch https://github.com/IdentityServer/IdentityServer4.WsFederation/tree/netstandard2.0 please re-base. thanks! |
Hi,
i tried to port the library to netstandard 2.0 using the Microsoft.IdentityModel library
things TODO (to provide same functionality as before):
TODO (additional features):
Please provide feedback :)
Edit:
My intension was creating a NuGet package that other users can use to get basic WS-Fed implementation. This repo is like a samples repo @brockallen @leastprivilege do you think it's appropriate place to add it here or should I use different repo? And are you even interested to have it in IdentityServer/* space?
Edit: Added more TODOs from comment below
resolve: #13