Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure RSA keys are at least 2048 bits in length #17911

Merged
merged 8 commits into from
Jun 28, 2023
Merged

Ensure RSA keys are at least 2048 bits in length #17911

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
93ccfe4
Ensure RSA keys are at least 2048 bits in length
jm96441n Jun 27, 2023
e8209ad
Merge branch 'main' into NET-4173-rsa-key-length-inline-certs
jm96441n Jun 27, 2023
f03248a
Add changelog
jm96441n Jun 27, 2023
8ba9e20
update key length check for FIPS compliance
jm96441n Jun 27, 2023
31c65cb
Fix no new variables error and failing to return when error exists from
jm96441n Jun 27, 2023
1854949
clean up code for better readability
jm96441n Jun 28, 2023
c7c2c07
Merge branch 'main' into NET-4173-rsa-key-length-inline-certs
jm96441n Jun 28, 2023
c9599af
actually return value
jm96441n Jun 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions agent/structs/config_entry_inline_certificate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ import (
"errors"
"fmt"

"github.com/hashicorp/consul/acl"
"github.com/miekg/dns"

"github.com/hashicorp/consul/acl"
)

// InlineCertificateConfigEntry manages the configuration for an inline certificate
Expand Down Expand Up @@ -42,8 +43,13 @@ func (e *InlineCertificateConfigEntry) GetEnterpriseMeta() *acl.EnterpriseMeta {
}
func (e *InlineCertificateConfigEntry) GetRaftIndex() *RaftIndex { return &e.RaftIndex }

// Envoy will silently reject any keys that are less than 2048 bytes long
// https://github.com/envoyproxy/envoy/blob/main/source/extensions/transport_sockets/tls/context_impl.cc#L238
const MinKeyLength = 2048
Copy link
Contributor

@andrewstucki andrewstucki Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given the FIPS work on enterprise, wondering based on the conditional a couple lines below the linked envoy line:

        if (rsa_key_length != 2048 && rsa_key_length != 3072 && rsa_key_length != 4096) {
          throw EnvoyException(
              fmt::format("Failed to load certificate chain from {}, only RSA certificates with "
                          "2048-bit, 3072-bit or 4096-bit keys are supported in FIPS mode",
                          ctx.cert_chain_file_path_));
        }

We may want to call:

consul/version/fips.go

Line 5 in b76c4d7

func IsFIPS() bool {

and do some additional constraint checks and only allow you to create InlineCertificates with those exact bit lengths if built with FIPS... since in the FIPS-compliant envoy build (and due to the movement towards packaging envoy FIPS alongside dataplane we should be able to detect since we require the whole stack including the Consul servers to be FIPS-compliant at that point) any other key length will result in the same broken envoy instances... WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a good call out! lemme modify this a bit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other thing to call out is that you'll want to also do this in the k8s side of things on the gateway validation itself, somewhere like this:

https://github.com/hashicorp/consul-k8s/blob/d3f9b670ab8055f0fc8ea4061c2d3c40abeb047f/control-plane/api-gateway/binding/validation.go#L207-L213

Since we don't do any sort of status syncing to Kubernetes secret objects, invalid certificates referenced by a gateway get statuses reflected back only on the gateway. Rather than attempting a sync and having it be silently rejected by Consul, we should just skip the attempt at InlineCertificate creation and set a status on the Gateway object in Kubernetes saying, "the cert you're referencing has an invalid length" to inform the user.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I've got some code over in the ParseCertificates function currently that I'm gonna push up after this merges so implementations are similar-ish and will have mostly been code reviewed already


func (e *InlineCertificateConfigEntry) Validate() error {
if err := validateConfigEntryMeta(e.Meta); err != nil {
err := validateConfigEntryMeta(e.Meta)
if err != nil {
return err
}

Expand All @@ -52,13 +58,25 @@ func (e *InlineCertificateConfigEntry) Validate() error {
return errors.New("failed to parse private key PEM")
}

if privateKeyBlock.Type == "RSA PRIVATE KEY" {
key, err := x509.ParsePKCS1PrivateKey(privateKeyBlock.Bytes)
if err != nil {
return err
}

// ensure private key is of the correct length
if key.N.BitLen() < MinKeyLength {
return errors.New("key length must be at least 2048 bits")
}
}

certificateBlock, _ := pem.Decode([]byte(e.Certificate))
if certificateBlock == nil {
return errors.New("failed to parse certificate PEM")
}

// make sure we have a valid x509 certificate
_, err := x509.ParseCertificate(certificateBlock.Bytes)
_, err = x509.ParseCertificate(certificateBlock.Bytes)
if err != nil {
return fmt.Errorf("failed to parse certificate: %w", err)
}
Expand Down
24 changes: 24 additions & 0 deletions agent/structs/config_entry_inline_certificate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,21 @@ NtyHRuD+KYRmjXtyX1yHNqfGN3vOQmwavHq2R8wHYuBSc6LAHHV9vG+j0VsgMELO
qwxn8SmLkSKbf2+MsQVzLCXXN5u+D8Yv+4py+oKP4EQ5aFZuDEx+r/G/31rTthww
AAJAMaoXmoYVdgXV+CPuBb2M4XCpuzLu3bcA2PXm5ipSyIgntMKwXV7r
-----END CERTIFICATE-----`
tooShortPrivateKey = `-----BEGIN RSA PRIVATE KEY-----
MIICXAIBAAKBgQCtmK1VjmXJ7vm4CZkkOSjc+kjGNMlyce5rXxwlDRz9LcGGc3Tg
kwUJesyBpDtxLLVHXQIPr5mWYbX/W/ezQ9sntxrATbDek8pBgoOlARebwkD2ivVW
BWfVhlryVihWlXApKiJ2n3i0m+OVtdrceC9Bv2hEMhYVOwzxtb3O0YFkbwIDAQAB
AoGAIxgnipFUEKPIRiVimUkY8ruCdNd9Fi7kNT6wEOl6v9A9PHIg4bm3Hfh+WYMb
JUEVkMzDuuoUEavFQE+WXt5L8oE1lEBmN2++FQsvllN+MRBTRg2sfw4mUWDI6S4r
h8+XNTzTIg2sUd2J3o2qNmQoOheYb+iuYDj76IFoEdwwZ0kCQQDYKKs5HAbnrLj1
UrOp8TyHdFf0YNw5tGdbNTbffq4rlBD6SW70+Sj624i2UqdnYwRiWzdXv3zN08aI
Vfoh2cGlAkEAzZe5B6BhiX/PcIYutMtuT3K+mysFNlowrutXWoQOpR7gGAkgEt6e
oCDgx1QJRjsp6NFQxKc6l034Hzs17gqJgwJAcu9U873aUg9+HTuHOoKB28haCCAE
mU46cr3d2oKCW7uUN3EaZXmid5iJneBfENMOfrnfuHGiC9NiShXlNWCS3QJAO5Ne
w83+1ahaxUGs4SkeExmuECrcPM7P0rBRxOIFmGWlDHIAgFdQYhiE6l34vghA8b1O
CV5oRRYL84jl7M/S3wJBALDfL5YXcc8P6scLJJ1biqhLYppvGN5CUwbsJsluvHCW
XCTVIbPOaS42A0xUfpoiTcdbNSFRvdCzPR5nsGy8Y7g=
-----END RSA PRIVATE KEY-----`
)

func TestInlineCertificate(t *testing.T) {
Expand All @@ -140,6 +155,15 @@ func TestInlineCertificate(t *testing.T) {
},
validateErr: "failed to parse certificate PEM",
},
"invalid private key length": {
entry: &InlineCertificateConfigEntry{
Kind: InlineCertificate,
Name: "cert-two",
PrivateKey: tooShortPrivateKey,
Certificate: "foo",
},
validateErr: "key length must be at least 2048 bits",
},
"mismatched certificate": {
entry: &InlineCertificateConfigEntry{
Kind: InlineCertificate,
Expand Down