From 8b0776410b74e9449461c856439aa773e47c3529 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Wed, 6 Jul 2022 17:40:37 +0200 Subject: [PATCH] server: remove TLS cert data retrieval over HTTP Back in CockroachDB v1.1 (v17.2 in the new calver scheme), we introduced a certificate rotation mechanism. To help teach/troubleshoot that feature, we also provided a way for the operator to view the certificate details in the DB Console (expiration time, addresses, etc.) This work was done in PR #16087, to solve issues #15027/#1674. However, as part of that PR, the implementation of the back-end API also included the *data* of the cert (including the cert signature and the signature chain) in the response payload. This additional payload was never used in a user-facing feature: the DB Console does not display it nor does it contain a link to "download the cert file". The back-end API is not public either, so we are not expecting end-users to have legitimate uses for this feature. Meanwhile, leaking cert data through an API runs dangerously close to violating PCI guidelines (not quite, since keys are not exposed, but still...). So in order to avoid a remark on this during PCI review cycles, and to remove the chance this will be misused, this patch removes the data payload from the cert response. The DB Console screen corresponding to the original work remains unaffected. Release note: None --- docs/generated/http/full.md | 1 - pkg/server/serverpb/status.proto | 4 +--- pkg/server/status.go | 3 +-- pkg/server/status_test.go | 20 -------------------- 4 files changed, 2 insertions(+), 26 deletions(-) diff --git a/docs/generated/http/full.md b/docs/generated/http/full.md index 95c2a0076530..e12c69bb6f52 100644 --- a/docs/generated/http/full.md +++ b/docs/generated/http/full.md @@ -50,7 +50,6 @@ Support status: [reserved](#support-status) | ----- | ---- | ----- | ----------- | -------------- | | type | [CertificateDetails.CertificateType](#cockroach.server.serverpb.CertificatesResponse-cockroach.server.serverpb.CertificateDetails.CertificateType) | | | [reserved](#support-status) | | error_message | [string](#cockroach.server.serverpb.CertificatesResponse-string) | | "error_message" and "data" are mutually exclusive. | [reserved](#support-status) | -| data | [bytes](#cockroach.server.serverpb.CertificatesResponse-bytes) | | data is the raw file contents of the certificate. This means PEM-encoded DER data. | [reserved](#support-status) | | fields | [CertificateDetails.Fields](#cockroach.server.serverpb.CertificatesResponse-cockroach.server.serverpb.CertificateDetails.Fields) | repeated | | [reserved](#support-status) | diff --git a/pkg/server/serverpb/status.proto b/pkg/server/serverpb/status.proto index 9388bbc3f10d..e982fbc55801 100644 --- a/pkg/server/serverpb/status.proto +++ b/pkg/server/serverpb/status.proto @@ -73,9 +73,7 @@ message CertificateDetails { CertificateType type = 1; // "error_message" and "data" are mutually exclusive. string error_message = 2; - // data is the raw file contents of the certificate. This means PEM-encoded - // DER data. - bytes data = 3; + reserved 3; repeated Fields fields = 4 [ (gogoproto.nullable) = false ]; } diff --git a/pkg/server/status.go b/pkg/server/status.go index 7dd8efbebf02..5d269a36db63 100644 --- a/pkg/server/status.go +++ b/pkg/server/status.go @@ -938,8 +938,7 @@ func (s *statusServer) Certificates( } if cert.Error == nil { - details.Data = cert.FileContents - if err := extractCertFields(details.Data, &details); err != nil { + if err := extractCertFields(cert.FileContents, &details); err != nil { details.ErrorMessage = err.Error() } } else { diff --git a/pkg/server/status_test.go b/pkg/server/status_test.go index 4e486e14ebff..f90b1c7d70b8 100644 --- a/pkg/server/status_test.go +++ b/pkg/server/status_test.go @@ -38,8 +38,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/rpc" - "github.com/cockroachdb/cockroach/pkg/security" - "github.com/cockroachdb/cockroach/pkg/security/securitytest" "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/server/diagnostics/diagnosticspb" "github.com/cockroachdb/cockroach/pkg/server/serverpb" @@ -1320,28 +1318,12 @@ func TestCertificatesResponse(t *testing.T) { t.Errorf("expected %d certificates, found %d", e, a) } - // Read the certificates from the embedded assets. - caPath := filepath.Join(security.EmbeddedCertsDir, security.EmbeddedCACert) - nodePath := filepath.Join(security.EmbeddedCertsDir, security.EmbeddedNodeCert) - - caFile, err := securitytest.EmbeddedAssets.ReadFile(caPath) - if err != nil { - t.Fatal(err) - } - - nodeFile, err := securitytest.EmbeddedAssets.ReadFile(nodePath) - if err != nil { - t.Fatal(err) - } - // The response is ordered: CA cert followed by node cert. cert := response.Certificates[0] if a, e := cert.Type, serverpb.CertificateDetails_CA; a != e { t.Errorf("wrong type %s, expected %s", a, e) } else if cert.ErrorMessage != "" { t.Errorf("expected cert without error, got %v", cert.ErrorMessage) - } else if a, e := cert.Data, caFile; !bytes.Equal(a, e) { - t.Errorf("mismatched contents: %s vs %s", a, e) } cert = response.Certificates[1] @@ -1349,8 +1331,6 @@ func TestCertificatesResponse(t *testing.T) { t.Errorf("wrong type %s, expected %s", a, e) } else if cert.ErrorMessage != "" { t.Errorf("expected cert without error, got %v", cert.ErrorMessage) - } else if a, e := cert.Data, nodeFile; !bytes.Equal(a, e) { - t.Errorf("mismatched contents: %s vs %s", a, e) } }