security: parse certificates at load time and export expiration time.

[mberhault]

Work towards #15027

Parse certificates at load time. Any failures are persisted for clarity.
For now, only the expiration time is used (in cockroach cert list and metrics) but debug pages will make use of the parsed certificates.

[cockroach-teamcity]

This change is 

[mberhault]

Second commit adds metrics for ca/node expiration times.

[a-robinson]

---

Reviewed 3 of 3 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

pkg/security/certificate_loader.go, line 371 at r1 (raw file):

	// PEM-decode the file.
	derCerts, err := PEMToCertificates(ci.FileContents)

Not important, but what does the "der" in derCerts mean?

---

pkg/security/certificate_manager.go, line 41 at r2 (raw file):

	metaNodeExpiration = metric.Metadata{
		Name: "security.certificate.expiration.node",
		Help: "Expiration timemestamp for the node certificate. 0 means no certificate or error."}

s/timemestamp/timestamp

---

pkg/security/certificate_manager.go, line 248 at r2 (raw file):

func (cm *CertificateManager) updateMetricsLocked() {
	// Metrics could be nil if we're inside a manually-constructed CertificateManager (eg: tests).
	if cm.caCert != nil && cm.caCert.Error == nil && cm.certMetrics.CAExpiration != nil {

Should we explicitly Update the value to 0 if cm.caCert or cm.caCert.Error is nil? That'll be necessary if it's possible for a process to have had a valid certificate and then only have invalid certificates after reloading them.

Ditto for the node cert metric.

---

Comments from Reviewable

[mberhault]

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

pkg/security/certificate_loader.go, line 371 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Not important, but what does the "der" in derCerts mean?

it's the encoding format for the certificate. not to be confused with PEM encoding which is more of an envelope.
And in case that wasn't clear, here's the snippet from wikipedia:
DER is a restricted variant of BER for producing unequivocal transfer syntax for data structures described by ASN.1. Like CER, DER encodings are valid BER encodings. DER is the same thing as BER with all but one sender's options removed.

---

pkg/security/certificate_manager.go, line 41 at r2 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

s/timemestamp/timestamp

Done.

---

pkg/security/certificate_manager.go, line 248 at r2 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Should we explicitly Update the value to 0 if cm.caCert or cm.caCert.Error is nil? That'll be necessary if it's possible for a process to have had a valid certificate and then only have invalid certificates after reloading them.

Ditto for the node cert metric.

True. It's currently not possible (I bail early from the caller if the cert goes from good to bad), but it doesn't hurt.

---

Comments from Reviewable

[a-robinson]

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[petermattis]

Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

pkg/security/certificate_loader.go, line 130 at r3 (raw file):

	Name string

	// Parsed certificates. This is used to debugging/printing/monitoring only,

s/to/by/g

---

pkg/security/certificate_loader.go, line 136 at r3 (raw file):

	ParsedCertificates []*x509.Certificate

	// Expiration time is the latest "Not After" date across all parsed certificates.

Should this be the earliest Not After date across all parsed certificates? Note that the code matches the comment. The question is about what the correct behavior should be. I have no idea what it means for the parsed certificates to have different Not After dates.

---

Comments from Reviewable

[mberhault]

Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

pkg/security/certificate_loader.go, line 130 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

s/to/by/g

Done.

---

pkg/security/certificate_loader.go, line 136 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Should this be the earliest Not After date across all parsed certificates? Note that the code matches the comment. The question is about what the correct behavior should be. I have no idea what it means for the parsed certificates to have different Not After dates.

Only CA cert files should have more than one certificate (it's technically possible to have more than one in non-CA cert files, but it won't do anything).

When your CA cert is about to expire (ideally months before), you should generate a new CA certificate and add it to the same file, using all CAs in your pool. This means that you may have one cert about to expire (or already expired) and another expiring in X years. Assuming the rest of your certificate rollout is done properly, your deadline is expiration date of the longest-lived CA.

---

Comments from Reviewable

[mberhault]

Review status: 4 of 6 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

---

pkg/security/certificate_loader.go, line 136 at r3 (raw file):

Previously, mberhault (marc) wrote…

Only CA cert files should have more than one certificate (it's technically possible to have more than one in non-CA cert files, but it won't do anything).

When your CA cert is about to expire (ideally months before), you should generate a new CA certificate and add it to the same file, using all CAs in your pool. This means that you may have one cert about to expire (or already expired) and another expiring in X years. Assuming the rest of your certificate rollout is done properly, your deadline is expiration date of the longest-lived CA.

There is one caveat but it's on the node cert, not the CA cert:
if the node certificate expires in 3 years but was signed with a CA cert expiring in 2 years, we should really compute the earliest expiration date in the chain and report that. I've added a TODO. The tricky part here is building the certificate chain.
Some of this trickery can be addressed by documentation on how to properly renew certificates, but we should be as accurate as we can here as well.

---

Comments from Reviewable

[mberhault]

The need to monitor certificate chains is alleviated a bit by #16055. However, someone not using the cockroach cli to generate certs could still have mismatched node/CA expirations.

[bdarnell]

---

Reviewed 2 of 3 files at r1, 2 of 3 files at r2, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

Comments from Reviewable