server: add basic TLS support to tenant HTTP server

[dhartunian]

Previously, the tenant HTTP server only served non-TLS connections by
default. This change will use the TLS config when it's present and serve
the HTTP server over TLS on the tenant. As on dedicated nodes, the
configuration will pick up the optional UI cert or the node cert as a
fallback.

This change also adds TLS to all other HTTP servers on the tenant server
including the _status/vars endpoint and pprof endpoints.

Resolves #69927

Release justification: fixing high-priority bug in existing
functionality.

Release note (security update): sql tenant servers will now use a TLS
certificate for their HTTP server when it's present. Previously this
server never used TLS.

[cockroach-teamcity]

This change is 

[chrisseto]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz)

[knz]

Reviewed all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian)

---

pkg/server/tenant.go, line 191 at r1 (raw file):

	// Additionally, all tenant HTTPS access will be via proxy.
	// TODO(davidh): Use a UI certificate here instead of node cert.
	serverTLSConfig, err := args.rpcContext.GetServerTLSConfig()

I think most unit tests expect to use the UI certs. It will make your life more complicated to make this choice here.

[dhartunian]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz)

---

pkg/server/tenant.go, line 191 at r1 (raw file):

Previously, knz (kena) wrote…

I think most unit tests expect to use the UI certs. It will make your life more complicated to make this choice here.

Good call. I modified the logic in GetUIServerTLSConfig instead.

[knz]

Reviewed 2 of 4 files at r2, 3 of 3 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dhartunian)

---

pkg/rpc/tls.go, line 205 at r4 (raw file):

// config. When called on a tenant instance, will fall back to returning
// the server TLS config for the node if the UI Server Config cannot be
// found. This choice was made to allow for simpler configuration

simpler simpler

---

pkg/rpc/tls.go, line 224 at r4 (raw file):

	tlsCfg, err := cm.GetUIServerTLSConfig()
	if tlsCfg == nil && ctx.tenID != roachpb.SystemTenantID {
		ctx := context.Background()

sorry can you spell out for me why you need this fallback? It looks to me that the remainder of the PR is sufficient to resolve the linked issue, without this fallback.

[dhartunian]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @chrisseto and @knz)

---

pkg/rpc/tls.go, line 205 at r4 (raw file):

Previously, knz (kena) wrote…

simpler simpler

done.

---

pkg/rpc/tls.go, line 224 at r4 (raw file):

Previously, knz (kena) wrote…

sorry can you spell out for me why you need this fallback? It looks to me that the remainder of the PR is sufficient to resolve the linked issue, without this fallback.

The fallback was requested from @chrisseto because we do not currently generate any UI certs for tenants and doing so would require many more changes on the Intrusion side. Since the UI/HTTP will never be accessed directly from the public internet and only via our own proxy, it seems reasonable to use the node server cert so we could support TLS between the proxy and the tenant.

[dhartunian]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @chrisseto and @knz)

---

pkg/rpc/tls.go, line 224 at r4 (raw file):

Previously, dhartunian (David Hartunian) wrote…

The fallback was requested from @chrisseto because we do not currently generate any UI certs for tenants and doing so would require many more changes on the Intrusion side. Since the UI/HTTP will never be accessed directly from the public internet and only via our own proxy, it seems reasonable to use the node server cert so we could support TLS between the proxy and the tenant.

Actually, I now realize there is already a fallback elsewhere in the code. Let me test without the fallback, I think you're correct @knz.

cockroach/pkg/security/certificate_manager.go

Lines 898 to 910 in 60dba10

	// getUICertLocked returns the UI certificate if present, otherwise returns
	// the node certificate.
	// cm.mu must be held.
	func (cm *CertificateManager) getUICertLocked() (*CertInfo, error) {
	if cm.uiCert == nil {
	// No UI certificate: use node certificate.
	return cm.getNodeCertLocked()
	}
	if err := checkCertIsValid(cm.uiCert); err != nil {
	return nil, makeError(err, "problem with UI certificate")
	}
	return cm.uiCert, nil
	}

[dhartunian]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @chrisseto and @knz)

---

pkg/rpc/tls.go, line 224 at r4 (raw file):

Previously, dhartunian (David Hartunian) wrote…

Actually, I now realize there is already a fallback elsewhere in the code. Let me test without the fallback, I think you're correct @knz.

cockroach/pkg/security/certificate_manager.go

Lines 898 to 910 in 60dba10

	// getUICertLocked returns the UI certificate if present, otherwise returns
	// the node certificate.
	// cm.mu must be held.
	func (cm *CertificateManager) getUICertLocked() (*CertInfo, error) {
	if cm.uiCert == nil {
	// No UI certificate: use node certificate.
	return cm.getNodeCertLocked()
	}
	if err := checkCertIsValid(cm.uiCert); err != nil {
	return nil, makeError(err, "problem with UI certificate")
	}
	return cm.uiCert, nil
	}

I confirmed that the fallback is unnecessary, the node cert will be used automatically since the UI cert is already optional. Thanks for the nudge @knz, all modifications to tls.go and its test have been removed from this PR.

[knz]

thank you!

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @chrisseto and @knz)

[dhartunian]

TFTR!

bors r=knz,chrisseto

[craig]

Build failed:

• GitHub CI (Cockroach)

[dhartunian]

bors r-

[dhartunian]

bors r=knz,chrisseto

[craig]

Build succeeded:

• GitHub CI (Cockroach)