-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Added URI Subject Alternative Names to secrets/pki #4675
Added URI Subject Alternative Names to secrets/pki #4675
Conversation
@nicholasjackson Thanks for this! I think, similarly to allowed_oid_sans, it should be a |
Cool, will update tomorrow morning |
f7cc12c
to
a5750c0
Compare
a5750c0
to
8db7d94
Compare
Updated code to change |
builtin/logical/pki/fields.go
Outdated
@@ -39,6 +39,12 @@ pkcs8 instead. Defaults to "der".`, | |||
comma-delimited list`, | |||
} | |||
|
|||
fields["uri_sans"] = &framework.FieldSchema{ | |||
Type: framework.TypeString, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be TypeCommaStringSlice
builtin/logical/pki/cert_util.go
Outdated
} | ||
} | ||
|
||
for _, v := range strings.Split(uriAlt, ",") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than do this, please use TypeCommaStringSlice for the parameter which will return you a []string already.
@@ -908,6 +920,7 @@ $ curl \ | |||
"data": { | |||
"allow_any_name": false, | |||
"allow_ip_sans": true, | |||
"allow_uri_sans": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this was missed in the bool -> slice change. Should be allowed_uri_sans: "spiffe://*"
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also from below:
Only valid if the role allows URI SANs (which is the default).
We should probably mention that default here too as it's non-obvious that it's only mentioned in other fields that relate to this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, will get on this first thing tomorrow morning.
Added new parameter
uri_sans
when generating x.509 certificatesAdded new parameter
allow_uri_sans
to roleUpdated documentation
@jefferai I am not sure which test to modify to validate this change (have tested manually for now),
ip_sans
testing seems to be in the OID test however I think I should probably create a separate test for this?In addition to this, should I also modify the UI to add the new parameter?