-
Notifications
You must be signed in to change notification settings - Fork 97
Add support for name constraints #433
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
base: main
Are you sure you want to change the base?
Conversation
…ests. Signed-off-by: Nick Perry <[email protected]>
sync from upstream
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| // This assumes only one custom extension is present. If support | ||
| // for additional extensions via apiPassthrough is added, this | ||
| // test will need to be updated. |
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.
Can we future proof this test?
| Extensions: &acmpcatypes.Extensions{ | ||
| CustomExtensions: []acmpcatypes.CustomExtension{ | ||
| { | ||
| ObjectIdentifier: aws.String("2.5.29.30"), // OID for Name Constraints | ||
| Value: aws.String(base64.StdEncoding.EncodeToString(nameConstraints)), | ||
| Critical: aws.Bool(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.
Can we create a function to generate the extensions blob? This way in the future someone just has to add to that function instead of changing all this logic around (and by extensions the tests).
We can say something like "if list empty return nil", which will make the api passthrough blob look like {subject: nil, extensions:nil}, which acts as if the object is not present.
Picturing something like
if nameConstraints {
add custom extension for name constraints to list
}
if otherExtension {
addCustomExtensions
}
return list.empty() ? nil : extensions { list }
Issue # (if applicable)
Closes #428
Reason for this change
aws-privateca-issuer does not currently honor X.509 name constraints present in CertificateRequests.
cert-manager itself does support name constraints. However, when aws-privateca-issuer processes a CertificateRequest with name constraints, they are silently ignored and the certificate is issued without the specified constraints.
Description of changes
If name constraints are present on a certificate, we extract them and build them into an apiPassthrough variable. We then pass that to the PCA API.
For this feature to be useful, the user needs to be able to select an APIPassthrough PCA template. The ability to do that is implemented in a separate PR #432.
It is technically possible to use Name Constraints without THIS PR and just use the ExplicitTemplateSelection feature from #432 with a CSRPassthrough template. However, I think this explicit Name Constraints feature is very worthwhile. It will allow aws-privateca-issuer administrators to let users issue certs with Name Constraints, without having to expose the full flexibility of CSRPassthrough templates.
I suggest getting #432 merged first. With that template selection feature locked in, it will be easier to refine the documentation of this name constraints feature.
Describe any new or updated permissions being added
None
Description of how you validated changes
Unit tests are included.