-
Notifications
You must be signed in to change notification settings - Fork 104
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
Fixed issue PKI: Support explicit Basic Constraints isCA=False #81 #201
Fixed issue PKI: Support explicit Basic Constraints isCA=False #81 #201
Conversation
8e89d79
to
997c641
Compare
Hi @naphelps, I think I'm done with the current PR (CI tests on going), could you have a look at it and merge it if no remark ? Thanks, |
@DanGhita Do you want to add a changelog entry? Thanks! |
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.
Some general commentary. :-)
Thanks @DanGhita!
builtin/logical/pki/backend_test.go
Outdated
t.Fatalf("expected a single cert, got %d", len(certs)) | ||
} | ||
cert = certs[0] | ||
if math.Abs(float64(time.Now().Add(12*time.Hour).Unix()-cert.NotAfter.Unix())) < 10 { |
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.
IMHO, not worth validating this. :-)
builtin/logical/pki/ca_util.go
Outdated
@@ -314,6 +315,7 @@ func buildSignVerbatimRole(data *framework.FieldData, role *roleEntry) *roleEntr | |||
} | |||
entry.NoStore = role.NoStore | |||
entry.Issuer = role.Issuer | |||
entry.BasicConstraintsValidForNonCA = role.BasicConstraintsValidForNonCA |
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.
Should this be added conditionally on whether the parameter was provided?
If so, I'd do:
if , ok := data.GetOk("basic_constraints_valid_for_non_ca"); !ok {
entry.BasicConstraintsValidForNonCA = role.BasicConstraintsValidForNonCA
}
That way, if a role requires it, but the API call explicitly disables it, you respect that explicit disabling. My 2c.
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.
Very nice, thanks @DanGhita!
Hi @naphelps, Alex approved the PR, I think it can me merged to the main. Thanks! |
…o#81 Signed-off-by: DanGhita <dan.ghita@viaccess-orca.com>
Signed-off-by: DanGhita <dan.ghita@viaccess-orca.com>
…rovided in the API call, it takes priority over the value set in the role. Signed-off-by: DanGhita <dan.ghita@viaccess-orca.com>
5f34da1
to
491b677
Compare
When issuing certificates with /pki_int/sign-verbatim, if the initial CSR contains the extension basicConstraints=CA:FALSE, the issued certificate must include this extension.
Resolves #
#81
Target Release
1.14.7