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.

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

feat: Increase the rsa key size #1433

Merged
merged 16 commits into from
Sep 17, 2024
4 changes: 2 additions & 2 deletions config/manager/manager.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ spec:
httpGet:
path: /healthz
port: 8081
initialDelaySeconds: 15
initialDelaySeconds: 30
periodSeconds: 20
readinessProbe:
httpGet:
Expand All @@ -60,7 +60,7 @@ spec:
periodSeconds: 10
resources:
limits:
cpu: 100m
cpu: 300m
memory: 384Mi
requests:
cpu: 5m
Expand Down
3 changes: 2 additions & 1 deletion internal/webhookcert/ca_cert_generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
)

const (
rsaKeySize = 2048
rsaKeySize = 4096
duration365d = time.Hour * 24 * 365
caCertMaxAge = duration365d
)
Expand Down Expand Up @@ -61,6 +61,7 @@ func (g *caCertGeneratorImpl) generateCertInternal() (*x509.Certificate, *rsa.Pr
}

certDERBytes, err := x509.CreateCertificate(crand.Reader, &caCertTemplate, &caCertTemplate, caKey.Public(), caKey)

if err != nil {
return nil, nil, fmt.Errorf("failed to create x509 cert: %w", err)
}
Expand Down
31 changes: 19 additions & 12 deletions internal/webhookcert/ca_cert_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,10 @@ type caCertGenerator interface {
}

type caCertProviderImpl struct {
client client.Client
expiryChecker certExpiryChecker
generator caCertGenerator
client client.Client
expiryChecker certExpiryChecker
keyLengthChecker caKeyLengthChecker
generator caCertGenerator
}

func newCACertProvider(client client.Client) *caCertProviderImpl {
Expand All @@ -39,6 +40,7 @@ func newCACertProvider(client client.Client) *caCertProviderImpl {
generator: &caCertGeneratorImpl{
clock: clock,
},
keyLengthChecker: &caKeyLengthCheckerImpl{},
}
}

Expand Down Expand Up @@ -75,31 +77,36 @@ func (p *caCertProviderImpl) provideCert(ctx context.Context, caSecretName types
}

func (p *caCertProviderImpl) checkCASecret(ctx context.Context, caSecret *corev1.Secret) bool {
rakesh-garimella marked this conversation as resolved.
Show resolved Hide resolved
caCertPEM, err := p.fetchCACert(caSecret)
caCertPEM, caKeyPEM, err := p.fetchCACertAndKey(caSecret)
if err != nil {
logf.FromContext(ctx).Error(err, "Invalid ca secret. Creating a new one",
"secretName", caSecret.Name,
"secretNamespace", caSecret.Namespace)
return false
}

certValid, checkErr := p.expiryChecker.checkExpiry(ctx, caCertPEM)
return checkErr == nil && certValid
certValid, checkCertErr := p.expiryChecker.checkExpiry(ctx, caCertPEM)
rakesh-garimella marked this conversation as resolved.
Show resolved Hide resolved

keyLengthValid, checkKeyErr := p.keyLengthChecker.checkKeyLength(ctx, caKeyPEM)

return checkCertErr == nil && certValid && checkKeyErr == nil && keyLengthValid
}

func (p *caCertProviderImpl) fetchCACert(caSecret *corev1.Secret) ([]byte, error) {
var caCertPEM []byte
func (p *caCertProviderImpl) fetchCACertAndKey(caSecret *corev1.Secret) ([]byte, []byte, error) {
var caCertPEM, caKeyPEM []byte
if val, found := caSecret.Data[caCertFile]; found {
caCertPEM = val
} else {
return nil, fmt.Errorf("key not found: %v", caCertFile)
return nil, nil, fmt.Errorf("caCert not found: %v", caCertFile)
rakesh-garimella marked this conversation as resolved.
Show resolved Hide resolved
}

if _, found := caSecret.Data[caKeyFile]; !found {
return nil, fmt.Errorf("key not found: %v", caKeyFile)
if val, found := caSecret.Data[caKeyFile]; found {
caKeyPEM = val
} else {
return nil, nil, fmt.Errorf("caKey not found: %v", caKeyFile)
rakesh-garimella marked this conversation as resolved.
Show resolved Hide resolved
}

return caCertPEM, nil
return caCertPEM, caKeyPEM, nil
}

func makeCASecret(certificate []byte, key []byte, name types.NamespacedName) corev1.Secret {
Expand Down
64 changes: 55 additions & 9 deletions internal/webhookcert/ca_cert_provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,14 @@ func (g *mockCACertGenerator) generateCert() ([]byte, []byte, error) {
return g.cert, g.key, nil
}

type mockCertKeyLengthChecker struct {
keyLenValid bool
}
rakesh-garimella marked this conversation as resolved.
Show resolved Hide resolved

func (c *mockCertKeyLengthChecker) checkKeyLength(ctx context.Context, keyPEM []byte) (bool, error) {
return c.keyLenValid, nil
}

func TestProvideCACert(t *testing.T) {
t.Run("should generate new ca cert if no secret found", func(t *testing.T) {
fakeClient := fake.NewClientBuilder().Build()
Expand Down Expand Up @@ -105,9 +113,10 @@ func TestProvideCACert(t *testing.T) {
},
}).Build()
sut := caCertProviderImpl{
client: fakeClient,
expiryChecker: &mockCertExpiryChecker{certValid: false},
generator: &mockCACertGenerator{cert: fakeNewCertPEM, key: fakeNewKeyPEM},
client: fakeClient,
expiryChecker: &mockCertExpiryChecker{certValid: false},
generator: &mockCACertGenerator{cert: fakeNewCertPEM, key: fakeNewKeyPEM},
keyLengthChecker: &mockCertKeyLengthChecker{keyLenValid: true},
}

secretName := types.NamespacedName{Namespace: "default", Name: "ca-cert"}
Expand Down Expand Up @@ -139,9 +148,10 @@ func TestProvideCACert(t *testing.T) {
},
}).Build()
sut := caCertProviderImpl{
client: fakeClient,
expiryChecker: &mockCertExpiryChecker{err: errors.New("failed")},
generator: &mockCACertGenerator{cert: fakeNewCertPEM, key: fakeNewKeyPEM},
client: fakeClient,
expiryChecker: &mockCertExpiryChecker{err: errors.New("failed")},
generator: &mockCACertGenerator{cert: fakeNewCertPEM, key: fakeNewKeyPEM},
keyLengthChecker: &mockCertKeyLengthChecker{keyLenValid: true},
}

secretName := types.NamespacedName{Namespace: "default", Name: "ca-cert"}
Expand Down Expand Up @@ -173,9 +183,44 @@ func TestProvideCACert(t *testing.T) {
},
}).Build()
sut := caCertProviderImpl{
client: fakeClient,
expiryChecker: &mockCertExpiryChecker{certValid: true},
generator: &mockCACertGenerator{cert: fakeCertPEM, key: fakeKeyPEM},
client: fakeClient,
expiryChecker: &mockCertExpiryChecker{certValid: true},
generator: &mockCACertGenerator{cert: fakeCertPEM, key: fakeKeyPEM},
keyLengthChecker: &mockCertKeyLengthChecker{keyLenValid: true},
}

secretName := types.NamespacedName{Namespace: "default", Name: "ca-cert"}
_, _, err := sut.provideCert(context.Background(), secretName)
require.NoError(t, err)

var secret corev1.Secret
fakeClient.Get(context.Background(), secretName, &secret)
require.NotNil(t, secret.Data)
require.Contains(t, secret.Data, "ca.crt")
require.Contains(t, secret.Data, "ca.key")
require.Equal(t, secret.Data["ca.crt"], fakeCertPEM)
require.Equal(t, secret.Data["ca.key"], fakeKeyPEM)
})

t.Run("Should generate new cert if keylength is 2048", func(t *testing.T) {

fakeCertPEM := []byte{1, 2, 3}
fakeKeyPEM := []byte{4, 5, 6}
fakeClient := fake.NewClientBuilder().WithObjects(&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "ca-cert",
Namespace: "default",
},
Data: map[string][]byte{
"ca.crt": fakeCertPEM,
"ca.key": fakeKeyPEM,
},
}).Build()
sut := caCertProviderImpl{
client: fakeClient,
generator: &mockCACertGenerator{cert: fakeCertPEM, key: fakeKeyPEM},
expiryChecker: &mockCertExpiryChecker{certValid: true},
keyLengthChecker: &mockCertKeyLengthChecker{keyLenValid: false},
}

secretName := types.NamespacedName{Namespace: "default", Name: "ca-cert"}
Expand All @@ -189,6 +234,7 @@ func TestProvideCACert(t *testing.T) {
require.Contains(t, secret.Data, "ca.key")
require.Equal(t, secret.Data["ca.crt"], fakeCertPEM)
require.Equal(t, secret.Data["ca.key"], fakeKeyPEM)

})
}

Expand Down
34 changes: 34 additions & 0 deletions internal/webhookcert/ca_key_length_checker.go
rakesh-garimella marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package webhookcert

import (
"context"
"fmt"

logf "sigs.k8s.io/controller-runtime/pkg/log"
)

const desiredRSAKeyLength = 4096
rakesh-garimella marked this conversation as resolved.
Show resolved Hide resolved

type caKeyLengthChecker interface {
checkKeyLength(ctx context.Context, keyPEM []byte) (bool, error)
}

type caKeyLengthCheckerImpl struct {
}

// checkKeyLength checks if the provided PEM-encoded key has the desired length
func (c *caKeyLengthCheckerImpl) checkKeyLength(ctx context.Context, keyPEM []byte) (bool, error) {
key, err := parseKeyPEM(keyPEM)
if err != nil {
return false, fmt.Errorf("failed to parse key PEM: %w", err)
}

if key.N.BitLen() != desiredRSAKeyLength {
logf.FromContext(ctx).Info("CA key length check failed",
"currentLength", key.N.BitLen(),
"desiredLength", desiredRSAKeyLength)
return false, nil
}

return true, nil
}
44 changes: 44 additions & 0 deletions internal/webhookcert/ca_key_length_checker_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package webhookcert

import (
"context"
crand "crypto/rand"
"crypto/rsa"
"testing"

"github.com/stretchr/testify/require"
)

func TestCheckKeyLength(t *testing.T) {
tt := []struct {
name string
key []byte
expected bool
}{
{
name: "key with desired length",
key: generateKey(t, 4096),
expected: true,
}, {
name: "key with undesired length",
key: generateKey(t, 2048),
expected: false,
},
}
for _, tc := range tt {
t.Run(tc.name, func(t *testing.T) {
checker := &caKeyLengthCheckerImpl{}
result, err := checker.checkKeyLength(context.TODO(), tc.key)
require.NoError(t, err, "failed to check key length")
require.Equal(t, tc.expected, result)
})
}
}

func generateKey(t *testing.T, keyLen int) []byte {
key, err := rsa.GenerateKey(crand.Reader, keyLen)
require.NoError(t, err, "failed to generate key")
encodedKey, err := encodeKeyPEM(key)
require.NoError(t, err, "failed to encode key")
return encodedKey
}
Loading