search

handnot2 / samly

Elixir Plug library to enable SAML 2.0 SP SSO in Phoenix/Plug applications.
MIT License
126 stars 93 forks source link

Added config setting for nameid_format #41

Closed calvinb closed 5 years ago

calvinb commented 5 years ago

Added config setting for nameid_format and updated README with usage. When omitted, preserves current behavior of using 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient'.

handnot2 commented 5 years ago

Can you check issue: #23 ?

There is a recommendation to pickup the nameid format from the IdP metadata config and not introduce a separate config at samly level. Can you please check if that works? That would avoid adding a new config setting.

you can pull it out from this branch: nameid_format_from_metadata

calvinb commented 5 years ago

That makes sense. Are you concerned that that would be a breaking change for users who are counting on the default of transient?

calvinb commented 5 years ago

Also, I believe if the request does not specify a name format at all, the response will use the format specified in the metadata. If this is correct, then there is no need to pass a format unless it is something different than what's in the metadata, which the config setting would be useful for. If I'm not wrong here, I think maybe the way to go would be to use the config setting if it's there, and otherwise don't bother to send a format. What do you think?

handnot2 commented 5 years ago

You are right. We can go with the new config setting. Default to transient if not specified keeping it compatible with the current implementation. I think that is what you have in the PR. Do you expect to make any further changes/corrections?

calvinb commented 5 years ago

No, I'm good with how it is if you are. Thanks.

On Fri, Jan 11, 2019, 12:47 PM handnot2 <notifications@github.com wrote:

You are right. We can go with the new config setting. Default to transient if not specified keeping it compatible with the current implementation. I think that is what you have in the PR. Do you expect to make any further changes/corrections?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/handnot2/samly/pull/41#issuecomment-453617728, or mute the thread https://github.com/notifications/unsubscribe-auth/AAB7lFkPgEX1srXOmTkMs5Jg6eoKK3H5ks5vCNw2gaJpZM4Z6Pm6 .

handnot2 commented 5 years ago

How does this behave for you with PingFederate? Assuming:

Do you get nameid in the format specified in the config?

Just checked SimpleSAMLphp. It seems to ignore what is set in the config (sent as part of AuthnRequest). It returns the nameid in AuthnResponse in the format the IdP is configured in.

calvinb commented 5 years ago

Yes, PingFederate does pay attention to the name id format we specify in the request. It doesn't accept all the valid values, but the two it does accept result in different name id values. If we specify the format it has in its metadata, it makes no difference, but if we specify transient from the config, we get a hash of the name id instead of the name id itself.

handnot2 commented 5 years ago

Need to check the Shibboleth IdP behavior.

calvinb commented 5 years ago

I haven't forgotten about this; just been busy. I hope to get back to it soon. Thanks.

calvinb commented 5 years ago

I updated the PR with your recommendations. I think it both works and reads better now.