-
Notifications
You must be signed in to change notification settings - Fork 1.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
✨ secret: Add generator for etcd client certs #2029
✨ secret: Add generator for etcd client certs #2029
Conversation
/assign @dlipovetsky |
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.
couple of logic questions around the case statement.
18820d7
to
117317b
Compare
e09ecd1
to
9ea2265
Compare
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.
🙏 thank you for adding this!
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chuckha, randomvariable 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 |
9ea2265
to
83fd1b0
Compare
83fd1b0
to
45d1892
Compare
/hold |
45d1892
to
7e18f75
Compare
Signed-off-by: Naadir Jeewa <jeewan@vmware.com>
7e18f75
to
209e87c
Compare
/unhold |
@@ -215,10 +232,11 @@ func (c Certificates) Generate() error { | |||
continue | |||
case ServiceAccount: | |||
generator = generateServiceAccountKeys | |||
case ClusterAPIEtcdClient: // Depends on the etcd CA, so needs to be handled separately |
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.
I found the Generate
hard to understand; good candidate for a refactor in the future, I think.
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.
Yeah, it's painful/very organic.
CommonName: cfg.CommonName, | ||
Organization: cfg.Organization, | ||
}, | ||
NotBefore: now.Add(time.Minute * -5), |
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.
Clever way to handle potential clock drift between management cluster and target host 😄
Organization: cfg.Organization, | ||
}, | ||
NotBefore: now.Add(time.Minute * -5), | ||
NotAfter: now.Add(time.Hour * 24 * 365 * 10), // 10 years |
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.
Worth defining as a constant somewhere?
Off-topic thought: I don't think we support user-defined expiration in general, but it might be a nice feature
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.
I see we define this in line here:
cluster-api/util/secret/certificates.go
Line 443 in f248367
NotAfter: now.Add(time.Hour * 24 * 365 * 10), // 10 years |
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.
+1 for consts
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.
+100
now := time.Now().UTC() | ||
|
||
tmpl := x509.Certificate{ | ||
SerialNumber: new(big.Int).SetInt64(0), |
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 may be pedantic...
The serial number MUST be a positive integer assigned by the CA to
each certificate. It MUST be unique for each certificate issued by a
given CA (i.e., the issuer name and serial number identify a unique
certificate). CAs MUST force the serialNumber to be a non-negative
integer.
-- https://tools.ietf.org/html/rfc3280#section-4.1.2.2
If we fix this, we could also fix it here:
cluster-api/util/secret/certificates.go
Line 437 in f248367
SerialNumber: new(big.Int).SetInt64(0), |
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.
It looked wrong to me, but I didn't think to question it further.
The comments on this suggest it might as well be refactored now, and verified against the spec. I'm going to close for the moment, and make sure this works with downstream consumption before reopening. |
/close |
@randomvariable: Closed this PR. In response to this:
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. |
What this PR does / why we need it:
Adds a generator for etcd client certificates for use by the control plane controller to connect into the workload cluster's etcd endpoint.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Part of #1902