IdentityServer / IdentityServer3.WsFederation

WS-Federation Plugin for IdentityServer v3
Apache License 2.0
25 stars 36 forks source link

Missing AuthenticationStatement with external provider #61

Closed sortling closed 8 years ago

sortling commented 8 years ago

Hello,

we are using IdentityServer3 with the WS-Federation Plugin as a Claims Provider Trust for AD FS, AD FS acts as the SAML2 IDP for a Shibboleth-SP. Local login with username and password works fine. We've been trying to add an external authentication via AusweisApp2/nPa, but the resulting SAML2 delivered to the Shibboleth-SP is missing an AuthnStatement element and creating a Shibboleth session fails.

After some research I think the reason is that the necessary claims are only added in the SignInResponseGenerator if the AuthenticationMethod is "password", see line 180 in WsFederationPlugin\ResponseHandling\SignInResponseGenerator.cs:

if (validationResult.Subject.GetAuthenticationMethod() == Constants.AuthenticationMethods.Password)
    {

        mappedClaims.Add(new Claim(ClaimTypes.AuthenticationMethod, AuthenticationMethods.Password));
        mappedClaims.Add(AuthenticationInstantClaim.Now);

    }

Arbitrary values for ClaimTypes.AuthenticationMethod seem to trouble either AD FS or shibboleth, I would suggest AuthenticationMethods.Unspecified for anything not password:

if (validationResult.Subject.GetAuthenticationMethod() == Constants.AuthenticationMethods.Password)
{
    mappedClaims.Add(new Claim(ClaimTypes.AuthenticationMethod, AuthenticationMethods.Password));
}
else
{
    mappedClaims.Add(new Claim(ClaimTypes.AuthenticationMethod, AuthenticationMethods.Unspecified));
}
mappedClaims.Add(AuthenticationInstantClaim.Now);
leastprivilege commented 8 years ago

yea that makes sense.

Another approach would be to implement what is proposed here

https://github.com/IdentityServer/IdentityServer3.WsFederation/pull/54

But as a quick fix we could add the "unspecified". Want to send a PR?

sortling commented 8 years ago

PR for a quick fix sent. Was there a specific reason to not create the claims for the AuthnStatement for non-password authentication methods?

leastprivilege commented 8 years ago

merged - will release later.

sortling commented 8 years ago

Thanks!