Skip to content

Commit

Permalink
server: remove TLS cert data retrieval over HTTP
Browse files Browse the repository at this point in the history
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
  • Loading branch information
knz committed Jul 6, 2022
1 parent 8b6e3c3 commit 8b07764
Show file tree
Hide file tree
Showing 4 changed files with 2 additions and 26 deletions.
1 change: 0 additions & 1 deletion docs/generated/http/full.md
Original file line number Diff line number Diff line change
Expand Up @@ -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) |


Expand Down
4 changes: 1 addition & 3 deletions pkg/server/serverpb/status.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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 ];
}

Expand Down
3 changes: 1 addition & 2 deletions pkg/server/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
20 changes: 0 additions & 20 deletions pkg/server/status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -1320,37 +1318,19 @@ 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]
if a, e := cert.Type, serverpb.CertificateDetails_NODE; 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, nodeFile; !bytes.Equal(a, e) {
t.Errorf("mismatched contents: %s vs %s", a, e)
}
}

Expand Down

0 comments on commit 8b07764

Please sign in to comment.