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

Set AllowCreate default value to true #91

Closed

Conversation

remilapeyre
Copy link
Contributor

The SAML2Core spec says for AllowCreate:

A Boolean value used to indicate whether the requester grants to the identity provider, in the
course of fulfilling the request, permission to create a new identifier or to associate an existing
identifier representing the principal with the relying party. Defaults to "false"if not present or the entire
element is omitted.

[...]

Requesters that do not make specific use of this attribute SHOULD generally set it to “true” to
maximize interoperability.

The SAML2Core spec says for AllowCreate:

	A Boolean value used to indicate whether the requester grants to the identity provider, in the
	course of fulfilling the request, permission to create a new identifier or to associate an existing
	identifier representing the principal with the relying party. Defaults to "false"if not present or the entire
	element is omitted.

	[...]

	Requesters that do not make specific use of this attribute SHOULD generally set it to “true” to
	maximize interoperability.
Copy link
Collaborator

@hcjulz hcjulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@remilapeyre thx for the PR.
In general this looks good to me, but I went through the specs again: OASISCoreSpec and SAML2Int.

In SAML2Int they say:

[SDP-SP04]
The <samlp:AuthnRequest> message MUST either omit the <samlp:NameIDPolicy> element (RECOMMENDED), or the element MUST contain an AllowCreate attribute of "true" and MUST NOT contain a Format attribute.

In OASIDCore they say about the NameIDPolicy element:

[3.4.1 Element <AuthnRequest>]
... If omitted, then any type of identifier supported by the identity provider for the requested subject can be used, constrained by any relevant deployment-specific policies, with respect to privacy, for example.

Then on the AllowCreate option they say it should generally be set to true if a NameIDPolicy element is set.

So this means, if no NameIDFormat element is set, the requester can use any supported identifier supported by the IDP. Which is why SAML2Int recommends this setting. Because of that I'd suggest we default to false and no NameIDPolicy element (as recommended). Does that make sense?

@@ -45,10 +45,10 @@ func getAuthnRequestOptions(opt ...Option) authnRequestOptions {

// AllowCreate is a Boolean value used to indicate whether the identity provider is allowed, in the course
// of fulfilling the request, to create a new identifier to represent the principal.
func AllowCreate() Option {
func AllowCreate(allow bool) Option {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this is a flag, we might come around it without a bool param. Something linke DisableAllowCreate().

@hcjulz hcjulz deleted the branch hashicorp:saml-lib September 22, 2023 05:05
@hcjulz hcjulz closed this Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants