Skip to content
This repository has been archived by the owner on Jul 21, 2020. It is now read-only.

[WIP] Port to netstandard2.0 #12

Closed
wants to merge 29 commits into from
Closed

Conversation

616b2f
Copy link

@616b2f 616b2f commented Jan 10, 2019

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

@dnfclas
Copy link

dnfclas commented Jan 10, 2019

CLA assistant check
All CLA requirements met.

@leastprivilege
Copy link
Member

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.

  • can ADFS import the metadata?
  • can ADFS use this as a claims provider
  • can SharePoint use this as an IdP

ADFS and SharePoint are the two main reasons this sample was created in the first place...

@Mich-b
Copy link

Mich-b commented Feb 27, 2019

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?

@616b2f
Copy link
Author

616b2f commented Feb 27, 2019

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

@Mich-b
Copy link

Mich-b commented Feb 27, 2019

Seem to get a nullreferenceexception when accessing the metadata endpoint (/wsfederation):

2019-02-28 00:22:56.083 +01:00 [DBG] Start WS-Federation metadata request
2019-02-28 00:22:56.098 +01:00 [ERR] An unhandled exception has occurred while executing the request.
System.NullReferenceException: Object reference not set to an instance of an object.
at IdentityServer4.WsFederation.MetadataResponseGenerator.GenerateAsync(String wsfedEndpoint)
at IdentityServer4.WsFederation.WsFederationController.Index()
at Microsoft.AspNetCore.Mvc.Internal.ActionMethodExecutor.TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
at System.Threading.Tasks.ValueTask`1.get_Result()
at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.InvokeActionMethodAsync()
at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.InvokeNextActionFilterAsync()
at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.Rethrow(ActionExecutedContext context)
at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.InvokeInnerFilterAsync()
at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeNextResourceFilter()
at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Rethrow(ResourceExecutedContext context)
at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeFilterPipelineAsync()
at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeAsync()
at Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke(HttpContext httpContext)
at IdentityServer4.Hosting.IdentityServerMiddleware.Invoke(HttpContext context, IEndpointRouter router, IUserSession session, IEventService events) in C:\ballen\github\identity\IdSvr4\IdentityServer4\src\IdentityServer4\Hosting\IdentityServerMiddleware.cs:line 72
at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
at Microsoft.AspNetCore.Cors.Infrastructure.CorsMiddleware.Invoke(HttpContext context)
at IdentityServer4.Hosting.BaseUrlMiddleware.Invoke(HttpContext context) in C:\ballen\github\identity\IdSvr4\IdentityServer4\src\IdentityServer4\Hosting\BaseUrlMiddleware.cs:line 43
at Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)
at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

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?

@616b2f
Copy link
Author

616b2f commented Feb 28, 2019

@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.

@Mich-b
Copy link

Mich-b commented Mar 1, 2019

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:

     public async Task<WsFederationConfiguration> GenerateAsync(string wsfedEndpoint)
     {
        //var signingKey = (await _keys.GetSigningCredentialsAsync()).Key as X509SecurityKey;
        var credential = await _keys.GetSigningCredentialsAsync();
        var signingKey = credential.Key;

        var issuer = _contextAccessor.HttpContext.GetIdentityServerIssuerUri();
        var signingCredentials = new SigningCredentials(signingKey, SecurityAlgorithms.RsaSha256Signature, SecurityAlgorithms.Sha256Digest);
        var config = new WsFederationConfiguration()
        {
            Issuer = issuer,
            TokenEndpoint = wsfedEndpoint,
            SigningCredentials = signingCredentials,
        };
        config.SigningKeys.Add(signingKey);
        config.KeyInfos.Add(new KeyInfo(signingKey));
        return config;
    }

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.

@Mich-b
Copy link

Mich-b commented Mar 1, 2019

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 user

line 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 ADFS

ADFS fails with the error:

End element 'Lifetime' from namespace 'http://docs.oasis-open.org/ws-sx/ws-trust/200512' expected. Found element 'wsu:Created' from namespace 'http://www.w3.org/2005/08/addressing'

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.

4) Too few attributes in the token issued from identityserver to ADFS

The only attribute is 'name'. Is there an option to include more attributes in the claim issued to the ws-fed relying party? ADFS expects at least an 'anchorclaimtype' claim.
Edit: I should have read the documentation, this also worked in the original library.

5) Support encryption in both signingresponse and metadataresponse

There seems to be no support to add an encryption certificate to the metadata, and there also seems to be an encryption 'todo' left in the signinresponsegenerator.

// TODO: check EncryptingCredentials()

My setup

I'm trying to use IdentityServer as an Identity Provider for ADFS 2019. This works very well, ONLY issue 4 is making life hard, but I'm hoping you can point me in the right direction. Issue 5 is also something that I'd love to have.

My clients config

identityresources in config.cs (identityserver)

    public static IEnumerable<IdentityResource> GetIdentityResources()
    {
        var customGroupClaim = new IdentityResource(
           name: "group",
           displayName: "Group",
           claimTypes: new[] { "http://schemas.xmlsoap.org/claims/Group" });

        var customAnchortypeClaim = new IdentityResource(
           name: "anchorclaimtype",
           displayName: "Anchorclaimtype",
           claimTypes: new[] { "http://schemas.microsoft.com/ws/2014/01/identity/claims/anchorclaimtype" });

        var customUpnClaim = new IdentityResource(
           name: "upn",
           displayName: "upn",
           claimTypes: new[] { ClaimTypes.Upn });

        return new IdentityResource[]
        {
           new IdentityResources.OpenId(),
           new IdentityResources.Profile(),
           customGroupClaim,
           customUpnClaim,
           customAnchortypeClaim,
        };
    }

Clients in config.cs (identityserver)

            new Client
            {

                ClientId = "https://adfs.xxx.tk/adfs",
                ProtocolType = ProtocolTypes.WsFederation,
                AllowedGrantTypes = GrantTypes.Code,
                RequireClientSecret = false,
                RedirectUris = { "https://adfs.xxx.tk/adfs/ls/" },
                IdentityTokenLifetime = 36000,

                AllowedScopes = { "openid", "profile", "group", "upn", "anchorclaimtype" }
            }

RelyingParty in config.cs (identityserver)

    public static List<RelyingParty> GetRelyingParties()
    {
        return new List<RelyingParty> {
            new RelyingParty {
                // Same as ClientId. Used to link config
                Realm = "https://adfs.xxx.tk/adfs",

                // SAML 1.1 token type required by ADFS
                TokenType = WsFederationConstants.TokenTypes.Saml11TokenProfile11,

                // Transform claim types
                ClaimMapping = new Dictionary<string, string>
                {
                    { JwtClaimTypes.Name, ClaimTypes.Name },
                    { JwtClaimTypes.Subject, ClaimTypes.NameIdentifier },
                    { JwtClaimTypes.Email, ClaimTypes.Email },
                    { JwtClaimTypes.GivenName, ClaimTypes.GivenName },
                    { JwtClaimTypes.FamilyName, ClaimTypes.Surname },
                    { JwtClaimTypes.BirthDate, ClaimTypes.DateOfBirth },
                    { JwtClaimTypes.WebSite, ClaimTypes.Webpage },
                    { JwtClaimTypes.Gender, ClaimTypes.Gender },
                    { JwtClaimTypes.Role, ClaimTypes.Role },
                    { "http://schemas.xmlsoap.org/claims/Group", "http://schemas.xmlsoap.org/claims/Group" },
                    { "http://schemas.microsoft.com/ws/2014/01/identity/claims/anchorclaimtype", "http://schemas.microsoft.com/ws/2014/01/identity/claims/anchorclaimtype" },
                    { ClaimTypes.Upn, ClaimTypes.Upn}
                },

            //Encryption
            //todo: How?

            // Defaults
            DigestAlgorithm = SecurityAlgorithms.Sha256Digest,
            SignatureAlgorithm = SecurityAlgorithms.RsaSha256Signature,
            SamlNameIdentifierFormat = WsFederationConstants.SamlNameIdentifierFormats.UnspecifiedString
            }
        };
    }

@616b2f
Copy link
Author

616b2f commented Mar 10, 2019

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?

@Mich-b
Copy link

Mich-b commented Mar 11, 2019 via email

@Mich-b
Copy link

Mich-b commented Mar 11, 2019

Verified with IdentityServer 2.4.0-preview1 and Kowalew.IdentityServer4.WsFederation 2.4.0-alpha1:

  • can ADFS 2019 import the metadata?
  • can ADFS 2019 use this as a claims provider

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.

@leastprivilege
Copy link
Member

This looks promising! Is there any way you can test with SharePoint?

@616b2f
Copy link
Author

616b2f commented Mar 12, 2019

@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.

@616b2f
Copy link
Author

616b2f commented Mar 25, 2019

@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
Can really recommend it, super easy so use once you understand how the tools fit together.

@leastprivilege
Copy link
Member

Cool! Need to schedule some time with @brockallen to review...thanks for doing this!

@616b2f
Copy link
Author

616b2f commented Apr 2, 2019

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.

@Mich-b
Copy link

Mich-b commented Jun 27, 2019

Hi, any timeline on when/if this will be merged?

@616b2f
Copy link
Author

616b2f commented Jul 5, 2019

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.

@Mich-b
Copy link

Mich-b commented Jul 5, 2019

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) .

@Mich-b
Copy link

Mich-b commented Jul 6, 2019

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:

outboundClaim.Properties[Microsoft.IdentityModel.Tokens.Saml.ClaimProperties.SamlNameIdentifierFormat] = result.RelyingParty.SamlNameIdentifierFormat; outboundClaims.RemoveAll(c => c.Type == ClaimTypes.NameIdentifier); //remove previesly added nameid claim

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:
image

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.

@RemcoBlok
Copy link

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.
One thing that was lacking in IdentityServer3.WsFederation and also seems to be lacking in IdentityServer4.WsFederation is support for WFresh="0". When I use OIDC to ADFS 2016 with prompt="login" ADFS 2016 passes WFresh="0" to the external IdP, but unfortunately it is not supported by IdentityServer3/4.WsFederation. Is that something that could be added?

thanks

Remco

@616b2f
Copy link
Author

616b2f commented Aug 12, 2019

@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.

@brockallen
Copy link
Member

brockallen commented Aug 12, 2019

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.

@Mich-b
Copy link

Mich-b commented Aug 20, 2019

What's the best approach to keep this package updated? It still references the 2.4.0 preview version.

@616b2f
Copy link
Author

616b2f commented Aug 22, 2019

@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 IdentityServer4 (>= 2.4.0-preview1) which means you can reference 2.4.0 which is per SemVer definitions higher then 2.4.0-preview1

@leastprivilege
Copy link
Member

Maybe we should put it on a branch so we can keep both the new and old implementation as both provide value?!

@616b2f
Copy link
Author

616b2f commented Aug 27, 2019

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?

@leastprivilege
Copy link
Member

Yea - I like the idea. I will rename dev to Net461 and create a NetStandard2.0 branch.

@leastprivilege
Copy link
Member

Done - created a new branch

https://github.com/IdentityServer/IdentityServer4.WsFederation/tree/netstandard2.0

please re-base. thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants