Skip to content
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

Subject/NameId in AuthnRequest #430

Open
magohl opened this issue Mar 24, 2016 · 13 comments
Open

Subject/NameId in AuthnRequest #430

magohl opened this issue Mar 24, 2016 · 13 comments

Comments

@magohl
Copy link

magohl commented Mar 24, 2016

I have a scenario where i need support for specifying Subject/NameId in the AuthnRequest. The NameId must be "per login". I am thinking of using an URL query parameter.

I have tried this with success in a rough proof-of-concept but are now looking into adding this in a nice way following the projects structure, naming-convention etc. so that it can be followed by a PR.

Any tips, aside from the general guidelines for contributing as how to best implement this?

In my rough proof-of-concept i have the SignInCommand getting an instance of a (new) RequestNameId class from the HttpRequestData using a query string. The Saml2AuthenticationRequest then have an AddNameId method similar to the current AddNameIdPolicy.

I am also thinking of having the parsing of Subject/NameId enabled through configuration.

Any ideas are welcome. Thanks!

@AndersAbel
Copy link
Member

Thanks for reaching out - it's so much better to be able to discuss things before implementation.

SignInCommand is the right place to grab the parameter from the query string and following the example for NameIdPolicy seems right.

There's one thing I'd like to have included (or at least prepared for) though and it is a possibility for the Owin middleware to set a requested nameid through an AuthenticateResponseChallenge property. The owin handler should get the Subject/NameId from the properties in ApplyResponseChallengeAsync and then pass it as a parameter in the call at https://github.com/KentorIT/authservices/blob/master/Kentor.AuthServices.Owin/KentorAuthServicesAuthenticationHandler.cs#L60.

@AndersAbel
Copy link
Member

I'm doing some work right now on an extension method to ClaimsIdentity/Claim to create a Saml2NameIdentifier. That's something you might want to use. I'll push it in a few hours.

@magohl
Copy link
Author

magohl commented Mar 24, 2016

Great, thanks. I will take a look at that. Good idea to support Owin as well for the Subject/NameId.

@AndersAbel
Copy link
Member

Just pushed the fix that contains the Saml2NameIdentifier helpers. Check tests or LogoutRequest to see how it's used.

@magohl
Copy link
Author

magohl commented Mar 25, 2016

Great, got the code. Will use it.

While the NameId will be dynamically set each SignIn my intention was to have the format attribute set through configuration. As this might be different between idps i'm thinking on keeping the settings on identityprovider level of the configuration and not the sp. Opinions?

So something like these two attributes on the idp:

  • allowAuthnRequestNameId (optional, bool, default to false)
  • AuthnRequestNameIdFormat

@AndersAbel
Copy link
Member

Is the config options for allowAuthnRequestNameId really required? The default behaviour if no nameid is supplied in call to SignIn command (either through query param or OWIN challenge) would be to not send any. In what cases would the application send a NameId and it would be improper for the SP to use it in the AuthnRequest?

If the option is different on different Idps it should be on the Idp level. There should also be an option on the Federation configuration that is applied to all Idps generated from the federation metadata.

@magohl
Copy link
Author

magohl commented Mar 25, 2016

The scenario i wanted to avoid is the (malicious) end-user being able to get a NameId generated in the AuthnRequest simply by altering the URL, when the intention of the SP is that it should not. To far-fetched...?

I could combine the two options and make the assumption that if no AuthnRequestNameIdFormat is specified then it does not create the Subject/NameId. Better?

@AndersAbel
Copy link
Member

I've tried to figure out how setting a NameId could be used in any malicious way, but haven't found out any. First of all - the AuthnRequest is in most cases not signed, so adding or changing a NameID is already trivial. I think we can leave it without an option. It will make the library more simple to use, which is an important consideration to me. The AutnnRequestNameIdFormat should default to Unspecified, so that if no config for it is added, it will still work.

@magohl
Copy link
Author

magohl commented Mar 25, 2016

Cool, lets do it without an option then.

Another idea i had was to support setting the NameId not only using an url query paramter but optionally also using an http header. In some scenarios url query strings, even sent over https, are logged on servers and saved in browser history. NameIds are probably not sensitive i most scenarios but in others when dealing with civicids or similar they might be. Opinions?

I added owin support as suggested by you earlier and it seems to work fine.

@AndersAbel
Copy link
Member

Don't overwork it unless you need it. To avoid using a query string I would prefer using POST rather than URL headers.

If you have something basic working, I'd suggest you submit a pull request for review. If prefer several small, well defined PRs.

@magohl
Copy link
Author

magohl commented Mar 29, 2016

Nothing ready for PR yet but hopefully within a week or so.

@magohl
Copy link
Author

magohl commented Apr 13, 2016

Status update. This is still being worked on but due to project prioritization it has been delayed for a couple of weeks.

@AndersAbel
Copy link
Member

Please see #450 for some discussion and a foundation on how to modify the AuthnRequest.

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

No branches or pull requests

2 participants