Support certificate policies in the pki backend #4125
Conversation
…with custom certificate policies.\nAdd basic constraint CA:FALSE by default in certificate extensions.
builtin/logical/pki/cert_util.go
Outdated
| @@ -1240,6 +1259,9 @@ func signCertificate(data *dataBundle) (*certutil.ParsedCertBundle, error) { | |||
| if certTemplate.MaxPathLen == 0 { | |||
| certTemplate.MaxPathLenZero = true | |||
| } | |||
| } else { | |||
| certTemplate.BasicConstraintsValid = true | |||
There was a problem hiding this comment.
What's the reason for this change? Also, IsCA wouldn't need to be set explicitly false, and if BasicConstraintsValid should be true in either case it should be pulled out of the if condition.
There was a problem hiding this comment.
client/server certificates should have this constraint explicitly set. Some old tools/browser refuse to connect to web servers with a certificate without the basicConstraint CA:False
To not change the default behaviour, I updated the patch to add an option in the pki role for that.
…A:False in issued certificates (generated or signed)
builtin/logical/pki/cert_util.go
Outdated
| // addPolicyIdentifiers adds certificate policies extension | ||
| // | ||
| func addPolicyIdentifiers(data *dataBundle, certTemplate *x509.Certificate) { | ||
| if len(data.params.PolicyIdentifiers) > 0 { |
There was a problem hiding this comment.
Can this if condition can be removed?
| func addPolicyIdentifiers(data *dataBundle, certTemplate *x509.Certificate) { | ||
| for _, oidstr := range data.params.PolicyIdentifiers { | ||
| oid, err := stringToOid(oidstr) | ||
| if err == nil { |
There was a problem hiding this comment.
I think we should return an error if we can't parse an OID instead of silently swallowing the error.
There was a problem hiding this comment.
Do you think we should do that at submission time, sanity-checking the role data that was provided? It's safer to do in both places but not sure it's really necessary.
builtin/logical/pki/path_roles.go
Outdated
| @@ -262,6 +262,16 @@ for "generate_lease".`, | |||
| Default: true, | |||
| Description: `If set to false, makes the 'common_name' field optional while generating a certificate.`, | |||
| }, | |||
| "policy_identifiers": &framework.FieldSchema{ | |||
| Type: framework.TypeCommaStringSlice, | |||
| Default: []string{}, | |||
There was a problem hiding this comment.
This default value is not required.
builtin/logical/pki/path_roles.go
Outdated
| }, | ||
| "basic_constraints_valid_for_non_ca": &framework.FieldSchema{ | ||
| Type: framework.TypeBool, | ||
| Default: false, |
There was a problem hiding this comment.
This default value is not required.
|
Going to merge this, will address Chris' concern separately. |
|
Thanks @remip2 ! |
Add policy_identifiers field to the pki role to generate certificate with custom certificate policies.
Add basic constraint CA:FALSE by default in certificate extensions.