security: allow separate CA to verify client certificates.

[mberhault]

Part of #26630.

This PR adds the following optional files in the certs directory:

• client.node.crt (and associated .key): client certificate for the
  node user
• ca.client.crt: certificate to verify client certificates

This allows for split server/client certificates signed by different
CAs.

The presence of client.node.crt indicates whether we want to use a
separate CA for client certificates.
If the file is present, the following happens:

• client.node.crt is used by nodes in their client TLS config
• ca.client.crt is used by nodes as the ClientCA in the server TLS
  config. ca.crt is NOT used as a ClientCA.

Other bits in this PR:

• add cockroach cert create-client-ca command
• use client CA to sign client certs if present
• show client CA on cockroach cert list
• show client CA in debug page
• metric for client CA expiration time

Release note (general change): allow separate CA for client certificates

[cockroach-teamcity]

This change is 

[bdarnell]

Reviewed 15 of 15 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/security/certificate_manager.go, line 64 at r1 (raw file):

// - ca.crt             main CA certificate.
//                      Used to verify everything unless overridden by more specifica CAs.
// - ca.client.crt      CA certificate to verify client certificates.

ca.client.crt is confusingly similar to client.ca.crt. Maybe we should call this ca-client.crt instead?

---

pkg/security/certificate_manager.go, line 402 at r1 (raw file):

// cm.mu must be held.
func (cm *CertificateManager) getClientCACertLocked() (*CertInfo, error) {
	if cm.nodeClientCert == nil {

I think if ca.client.crt exists, it should be used regardless of whether we have a client.node.crt. Therefore, if we have ca.client.crt, node.crt must be valid according to both ca.crt and ca.client.crt (is this a useful configuration? I'm not sure, but it's more logically consistent than ignoring ca.client.crt because there is no separate node client cert).

---

pkg/security/certs_test.go, line 15 at r1 (raw file):

// permissions and limitations under the License.

// +build !windows

The part of this test that needs sys/unix should be moved to a separate file so we don't lose the rest of these tests on windows. (or better, introduce a "trigger reload" hook so we don't need sys/unix at all)

---

pkg/security/certs_test.go, line 409 at r1 (raw file):

	}

	// Test a SQL client with correct certs usage.

s/correct/correct and incorrect/

[mberhault]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/security/certificate_manager.go, line 64 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

ca.client.crt is confusingly similar to client.ca.crt. Maybe we should call this ca-client.crt instead?

Sure. It feels a little odd to move away from . based delimiters. The idea behind the naming was that the first part indicated the major category: ca vs node (aka: server certificate) vs client. It doesn't have the dynamic property of client.<anything>.crt so I'm fine calling it a separate prefix. The handling is different enough after all.

---

pkg/security/certificate_manager.go, line 402 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I think if ca.client.crt exists, it should be used regardless of whether we have a client.node.crt. Therefore, if we have ca.client.crt, node.crt must be valid according to both ca.crt and ca.client.crt (is this a useful configuration? I'm not sure, but it's more logically consistent than ignoring ca.client.crt because there is no separate node client cert).

I'm not too crazy about it either. The main reason to use this is to allow missing CA files to mean "use the system pool".
This means we need something to switch on that isn't the CA. I don't want to try to hack something using filenames or contents (eg: empty file, or token in the filename). Flags could be used but are heavier (eg: --use-system-pool-for-client-ca and same for node/ui CAs).

Alternatively, we could always use ca-client.crt as the client CA and take the missing file to mean system pool. This would require an upgrade step. We probably can't do it ourselves as we may not have write access to the directory. This would also get confusing when using a client CA as the file would already exist.

Finally, we could stick to the current method and say you always need a CA certificate, even if it's a public one. That may be fine for the client CA where it will be internally-controlled (and hopefully easily available) but the UI CA will be more problematic (eg: there's no single file you can use for Let's Encrypt).

---

pkg/security/certs_test.go, line 15 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

The part of this test that needs sys/unix should be moved to a separate file so we don't lose the rest of these tests on windows. (or better, introduce a "trigger reload" hook so we don't need sys/unix at all)

Split into two tests: one with full certs, one with ca-client.crt deleted after signing client certs. And removed build tag.

---

pkg/security/certs_test.go, line 409 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

s/correct/correct and incorrect/

Done.

[mberhault]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/security/certificate_manager.go, line 402 at r1 (raw file):

Previously, mberhault (marc) wrote…

I'm not too crazy about it either. The main reason to use this is to allow missing CA files to mean "use the system pool".
This means we need something to switch on that isn't the CA. I don't want to try to hack something using filenames or contents (eg: empty file, or token in the filename). Flags could be used but are heavier (eg: --use-system-pool-for-client-ca and same for node/ui CAs).

Alternatively, we could always use ca-client.crt as the client CA and take the missing file to mean system pool. This would require an upgrade step. We probably can't do it ourselves as we may not have write access to the directory. This would also get confusing when using a client CA as the file would already exist.

Finally, we could stick to the current method and say you always need a CA certificate, even if it's a public one. That may be fine for the client CA where it will be internally-controlled (and hopefully easily available) but the UI CA will be more problematic (eg: there's no single file you can use for Let's Encrypt).

@bdarnell: thoughts on this bit?

[bdarnell]

Reviewed 7 of 7 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/security/certificate_manager.go, line 402 at r1 (raw file):

Previously, mberhault (marc) wrote…

@bdarnell: thoughts on this bit?

We don't currently support the system pool, right? The system pool seems like an odd fit since we generally wouldn't want to trust public web CAs.

I'd rather do something explicit for the system pool instead of switching based on having a separate node client cert. Either separate flags or a marker file in the certs directory.

[mberhault]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/security/certificate_manager.go, line 402 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

We don't currently support the system pool, right? The system pool seems like an odd fit since we generally wouldn't want to trust public web CAs.

I'd rather do something explicit for the system pool instead of switching based on having a separate node client cert. Either separate flags or a marker file in the certs directory.

We don't right now, partially because you'd have a very hard time finding a CA that will sign our certificates. But with the separate client and UI CAs, it's perfectly reasonable to start using the system pool, either for things like Let's Encrypt for the Admin UI, or organization CAs pre-installed on the machines.

The Admin UI alone pretty much requires public CAs, or you'll have a hard time getting rid of the browser certificate warnings.

I'm not a big fan of magic files in the certs directory, or magic file contents. I suppose a flag along the lines of --use-system-ca-pool-for-admin-ui (and ditto for nodes and clients. The question now becomes whether to ignore a CA certificate if present.

[bdarnell]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/security/certificate_manager.go, line 402 at r1 (raw file):

Previously, mberhault (marc) wrote…

We don't right now, partially because you'd have a very hard time finding a CA that will sign our certificates. But with the separate client and UI CAs, it's perfectly reasonable to start using the system pool, either for things like Let's Encrypt for the Admin UI, or organization CAs pre-installed on the machines.

The Admin UI alone pretty much requires public CAs, or you'll have a hard time getting rid of the browser certificate warnings.

I'm not a big fan of magic files in the certs directory, or magic file contents. I suppose a flag along the lines of --use-system-ca-pool-for-admin-ui (and ditto for nodes and clients. The question now becomes whether to ignore a CA certificate if present.

I'd never ignore a CA file that exists. Either error out or allow both if there's a CA and a flag indicating the system pool should be used.

[mberhault]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/security/certificate_manager.go, line 402 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I'd never ignore a CA file that exists. Either error out or allow both if there's a CA and a flag indicating the system pool should be used.

Ok, I can do something along the lines of --use-system-ca-pool-if-ui-ca-missing. When false (the default), we would fall back on ca.crt. If ca.crt is missing and its flag is set then we can in turn fall back on the system pool. Anyway, I'll switch the current PR to using ca-client.crt when present and use client.node.crt if present, regardless of the presence of the ca.

[mberhault]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/security/certificate_manager.go, line 402 at r1 (raw file):

Previously, mberhault (marc) wrote…

Ok, I can do something along the lines of --use-system-ca-pool-if-ui-ca-missing. When false (the default), we would fall back on ca.crt. If ca.crt is missing and its flag is set then we can in turn fall back on the system pool. Anyway, I'll switch the current PR to using ca-client.crt when present and use client.node.crt if present, regardless of the presence of the ca.

Done.

[bdarnell]

Reviewed 9 of 9 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained

[mberhault]

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[mberhault]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)