Merge pull request #3119 from jrvaldes/WINC-1406

WINC-1406: Adds validation logic in the signer.go to trigger a warning if a weak public key is used

Author: openshift-merge-bot[bot]

controllers/secret_controller.go

@@ -183,8 +183,16 @@ func (r *SecretReconciler) reconcileUserDataSecret(ctx context.Context) error {
 		}
 		return fmt.Errorf("unable to get secret %s: %w", secrets.PrivateKeySecret, err)
 	}
+
+	// get the public key from the signer
+	publicKey := keySigner.PublicKey()
+	if err := signer.ValidatePublicKey(publicKey); err != nil {
+		r.log.Info("Warning: A weak private key is being used for userdata generation. "+
+			"It is strongly recommended to use a more secure key.", "details", err)
+	}
+
 	// Generate expected userData based on the existing private key
-	validUserData, err := secrets.GenerateUserData(r.platform, keySigner.PublicKey())
+	validUserData, err := secrets.GenerateUserData(r.platform, publicKey)
 	if err != nil {
 		return fmt.Errorf("error generating %s secret: %w", secrets.UserDataSecret, err)
 	}

pkg/signer/signer.go

@@ -2,6 +2,10 @@ package signer
 
 import (
 	"context"
+	"crypto/dsa"
+	"crypto/ecdsa"
+	"crypto/ed25519"
+	"crypto/rsa"
 	"fmt"
 
 	"golang.org/x/crypto/ssh"
@@ -11,6 +15,9 @@ import (
 	"github.com/openshift/windows-machine-config-operator/pkg/secrets"
 )
 
+// minRSABitLen is the minimum RSA key size recommended for security.
+const minRSABitLen = 2048
+
 // Create creates a signer using the private key data
 func Create(ctx context.Context, secret kubeTypes.NamespacedName, c client.Client) (ssh.Signer, error) {
 	privateKey, err := secrets.GetPrivateKey(ctx, secret, c)
@@ -23,3 +30,50 @@ func Create(ctx context.Context, secret kubeTypes.NamespacedName, c client.Clien
 	}
 	return signer, nil
 }
+
+// ValidatePublicKey checks if the given public key meets security standards.
+// It returns an error if the key is weak.
+func ValidatePublicKey(pubKey ssh.PublicKey) error {
+	return validate(pubKey)
+}
+
+// validate checks the provided ssh.PublicKey for cryptographic strength and compliance with modern security standards.
+// It performs the following checks:
+//  1. Ensures the key implements ssh.CryptoPublicKey, which exposes the underlying crypto.PublicKey.
+//  2. For RSA keys: verifies the modulus bit length is at least minRSABitLen (2048 bits), rejecting weak keys.
+//  3. For DSA keys: rejects all, as DSA is deprecated and considered insecure.
+//  4. For ECDSA keys: checks the curve used; specifically rejects P-224 as too weak
+//  5. For Ed25519 keys: accepts as secure.
+//  6. For unknown or unsupported key types: rejects with an error.
+//
+// Returns nil if the key is considered secure, or an error describing the weakness otherwise.
+func validate(pubKey ssh.PublicKey) error {
+	cryptoPubKey, ok := pubKey.(ssh.CryptoPublicKey)
+	if !ok {
+		// This case should ideally not be hit with standard SSH keys.
+		return fmt.Errorf("invalid key type: %s", pubKey.Type())
+	}
+
+	switch key := cryptoPubKey.CryptoPublicKey().(type) {
+	case *rsa.PublicKey:
+		if key.N.BitLen() < minRSABitLen {
+			return fmt.Errorf("RSA key size is %d bits, which is considered weak. Use %d or greater",
+				key.N.BitLen(), minRSABitLen)
+		}
+	case *dsa.PublicKey:
+		return fmt.Errorf("DSA keys are deprecated and considered weak. Please use RSA, ECDSA, or Ed25519")
+	case *ecdsa.PublicKey:
+		curveName := key.Curve.Params().Name
+		// P‑224 is deprecated, too small (~112‑bit) for modern standards and should be phased out by 2030
+		if curveName == "P‑224" {
+			return fmt.Errorf("found ECDSA key with small curve %s. Use P-256, P-384, P-521 or larger", curveName)
+		}
+	case ed25519.PublicKey:
+		// Ed25519 is a secure algorithm
+	default:
+		return fmt.Errorf("unknown or unsupported public key type: %T", key)
+	}
+
+	// the key is not weak
+	return nil
+}

pkg/signer/signer_test.go

@@ -0,0 +1,93 @@
+package signer
+
+import (
+	"crypto/dsa"
+	"crypto/ecdsa"
+	"crypto/ed25519"
+	"crypto/elliptic"
+	"crypto/rand"
+	"crypto/rsa"
+	"testing"
+
+	"golang.org/x/crypto/ssh"
+)
+
+func TestValidatePublicKey(t *testing.T) {
+	// Strong RSA (2048-bit)
+	strongRSAKey, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		t.Fatalf("Failed to generate strong RSA key: %v", err)
+	}
+	strongRSAPub, err := ssh.NewPublicKey(&strongRSAKey.PublicKey)
+	if err != nil {
+		t.Fatalf("Failed to create SSH public key from strong RSA key: %v", err)
+	}
+
+	// Weak RSA (1024-bit)
+	weakRSAKey, err := rsa.GenerateKey(rand.Reader, 1024)
+	if err != nil {
+		t.Fatalf("Failed to generate weak RSA key: %v", err)
+	}
+	weak1024RSAPub, err := ssh.NewPublicKey(&weakRSAKey.PublicKey)
+	if err != nil {
+		t.Fatalf("Failed to create SSH public key from weak RSA key: %v", err)
+	}
+
+	// Strong curve ECDSA (P-256)
+	strongECDSAKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatalf("Failed to generate strong ECDSA key: %v", err)
+	}
+	strongECDSAPub, err := ssh.NewPublicKey(&strongECDSAKey.PublicKey)
+	if err != nil {
+		t.Fatalf("Failed to create SSH public key from strong ECDSA key: %v", err)
+	}
+
+	// Strong Ed25519
+	ed25519Pub, ed25519Priv, err := ed25519.GenerateKey(rand.Reader)
+	if err != nil {
+		t.Fatalf("Failed to generate Ed25519 key: %v", err)
+	}
+	ed25519SSHPub, err := ssh.NewPublicKey(ed25519Pub)
+	if err != nil {
+		t.Fatalf("Failed to create SSH public key from Ed25519 key: %v", err)
+	}
+	_ = ed25519Priv // Suppress unused variable warning
+
+	// Weak DSA
+	params := new(dsa.Parameters)
+	if err := dsa.GenerateParameters(params, rand.Reader, dsa.L1024N160); err != nil {
+		t.Fatalf("Failed to generate DSA params: %v", err)
+	}
+	dsaKey := new(dsa.PrivateKey)
+	dsaKey.Parameters = *params
+	if err := dsa.GenerateKey(dsaKey, rand.Reader); err != nil {
+		t.Fatalf("Failed to generate DSA key: %v", err)
+	}
+	dsaPub, err := ssh.NewPublicKey(&dsaKey.PublicKey)
+	if err != nil {
+		t.Fatalf("Failed to create SSH public key from DSA key: %v", err)
+	}
+
+	testCases := []struct {
+		name    string
+		key     ssh.PublicKey
+		wantErr bool
+	}{
+		{"Strong RSA", strongRSAPub, false},
+		{"Weak RSA", weak1024RSAPub, true},
+		{"Strong ECDSA", strongECDSAPub, false},
+		{"Ed25519", ed25519SSHPub, false},
+		{"DSA", dsaPub, true},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := ValidatePublicKey(tc.key)
+			if (err != nil) != tc.wantErr {
+				t.Errorf("ValidatePublicKey() error = %v, wantErr %v", err, tc.wantErr)
+				return
+			}
+		})
+	}
+}