-
Notifications
You must be signed in to change notification settings - Fork 189
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
AUTH-296: certificate chains init refactoring #939
AUTH-296: certificate chains init refactoring #939
Conversation
The failures are appearing because the binaries are still being built with go 1.17, whereas my PR is using generics at a single spot, and go 1.17 does not know what to do about it, obviously. |
f17fca9
to
363be63
Compare
363be63
to
9ba225d
Compare
We allow specifying the certificate information using Go structures instead of additional code that goes a certificate signing at a time and checking for errors. All of the certificate chains can be created by using the builder patter of the "certificateChains", where all the signing information can be injected, and once everything is done, calling "Complete()" attempts to build all the certificate chains specified. The "CertificateChains" structure, which is a result of the previous chain completion allows querying for certificate PEMs in a directory-like manner. Each of the signers provides information about its directly-signed certificates and its sub-CAs. Signers don't allow retrieving their keys directly as those should never be needed to be extracted anywhere in the code.
9ba225d
to
d82a9f5
Compare
I've had a look at this PR. I'd need a bit more of context from OpenShift cert management, but in general it looks good to me. I understand it generalizes the build of signers and client certificates. It works in my dev environment. @ibihim is referenced in the PR, are we expecting him to review it too? |
I'm ok with a single person review, I'd like to move on to using this code in new PRs. |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: oglok, stlaz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
/test periodics-images |
/test e2e-openshift-conformance-sig-node |
@stlaz: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
This PR adds a refactoring of the current certificate initialization code.
The new code allows describing certificate chains by Go structures, feeding them into a builder object, and then creating them all at once once the developer is happy with what they have.
The new structure currently only allows to retrieve leaf certificate certs/key PEMs in a path-like manner, but might be extensible to perform queries on *x509.Certificate objects in the future, so as to allow for checking expirations and could be thus fed to a controller that is capable of handling rotations over the structure.
/cc @ibihim