Skip to content
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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 96 additions & 2 deletions util/secret/certificates.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ func NewCertificatesForInitialControlPlane(config *v1beta1.ClusterConfiguration)
return certificates
}

// NewCertificatesForJoiningControlPlane gets any certs that exist and writes them to disk
// NewCertificatesForJoiningControlPlane returns a list of certificates configured for a joining control plane node
func NewCertificatesForJoiningControlPlane() Certificates {
return Certificates{
&Certificate{
Expand Down Expand Up @@ -150,6 +150,23 @@ func NewCertificatesForWorker(caCertPath string) Certificates {
}
}

// NewCertificatesForEtcdClient returns an initialised but empty set of certificates needed to communicate
// with etcd
func NewCertificatesForEtcdClient() Certificates {
return Certificates{
&Certificate{
Purpose: EtcdCA,
CertFile: filepath.Join(defaultCertificatesDir, "etcd", "ca.crt"),
KeyFile: filepath.Join(defaultCertificatesDir, "etcd", "ca.key"),
},
&Certificate{
Purpose: ClusterAPIEtcdClient,
CertFile: "",
KeyFile: "",
},
}
}

// GetByPurpose returns a certificate by the given name.
// This could be removed if we use a map instead of a slice to hold certificates, however other code becomes more complex.
func (c Certificates) GetByPurpose(purpose Purpose) *Certificate {
Expand Down Expand Up @@ -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
Copy link
Contributor

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.

Copy link
Member Author

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.

continue
default:
generator = generateCACert
}

kp, err := generator()
if err != nil {
return err
Expand All @@ -227,6 +245,30 @@ func (c Certificates) Generate() error {
certificate.Generated = true
}
}
return c.generateEtcdClientCertificate()
}

func (c Certificates) generateEtcdClientCertificate() error {
for _, certificate := range c {
if !(certificate.KeyPair == nil && certificate.Purpose == ClusterAPIEtcdClient) {
continue
}
etcdCA := c.GetByPurpose(EtcdCA)
if etcdCA == nil || etcdCA.KeyPair == nil {
return errors.Wrapf(
ErrMissingCertificate,
"for certificate: %s, required to generate %s certificate",
EtcdCA,
ClusterAPIEtcdClient,
)
}
kp, err := generateClientCert("cluster-api.x-k8s.io", etcdCA.KeyPair)
if err != nil {
return err
}
certificate.KeyPair = kp
certificate.Generated = true
}
return nil
}

Expand Down Expand Up @@ -410,6 +452,29 @@ func generateServiceAccountKeys() (*certs.KeyPair, error) {
}, nil
}

func generateClientCert(cn string, ca *certs.KeyPair) (*certs.KeyPair, error) {
privKey, err := certs.NewPrivateKey()
if err != nil {
return nil, err
}
caCert, err := certs.DecodeCertPEM(ca.Cert)
if err != nil {
return nil, err
}
caKey, err := certs.DecodePrivateKeyPEM(ca.Key)
if err != nil {
return nil, err
}
x509Cert, err := newClientCert(cn, caCert, privKey, caKey)
if err != nil {
return nil, err
}
return &certs.KeyPair{
Cert: certs.EncodeCertPEM(x509Cert),
Key: certs.EncodePrivateKeyPEM(privKey),
}, nil
}

// newCertificateAuthority creates new certificate and private key for the certificate authority
func newCertificateAuthority() (*x509.Certificate, *rsa.PrivateKey, error) {
key, err := certs.NewPrivateKey()
Expand Down Expand Up @@ -456,3 +521,32 @@ func newSelfSignedCACert(key *rsa.PrivateKey) (*x509.Certificate, error) {
c, err := x509.ParseCertificate(b)
return c, errors.WithStack(err)
}

func newClientCert(cn string, caCert *x509.Certificate, key *rsa.PrivateKey, caKey *rsa.PrivateKey) (*x509.Certificate, error) {
cfg := certs.Config{
CommonName: cn,
}

now := time.Now().UTC()

tmpl := x509.Certificate{
SerialNumber: new(big.Int).SetInt64(0),
Copy link
Contributor

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:

SerialNumber: new(big.Int).SetInt64(0),

Copy link
Member Author

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.

Subject: pkix.Name{
CommonName: cfg.CommonName,
Organization: cfg.Organization,
},
NotBefore: now.Add(time.Minute * -5),
Copy link
Contributor

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 😄

NotAfter: now.Add(time.Hour * 24 * 365 * 10), // 10 years
Copy link
Contributor

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

Copy link
Contributor

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:

NotAfter: now.Add(time.Hour * 24 * 365 * 10), // 10 years

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for consts

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+100

KeyUsage: x509.KeyUsageDigitalSignature,
ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
}

b, err := x509.CreateCertificate(rand.Reader, &tmpl, caCert, key.Public(), caKey)
if err != nil {
return nil, errors.Wrapf(err, "failed to create signed client certificate: %+v", tmpl)
}

c, err := x509.ParseCertificate(b)
return c, errors.WithStack(err)

}
3 changes: 3 additions & 0 deletions util/secret/consts.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,7 @@ const (

// APIServerEtcdClient is the secret name of user-supplied secret containing the apiserver-etcd-client key/cert
APIServerEtcdClient Purpose = "apiserver-etcd-client"

// ClusterAPIEtcdClient is the secret name of the Cluster API etcd client
ClusterAPIEtcdClient Purpose = "cluster-api-etcd-client"
)