From d7cd4259df42001b1e38b43c85fd00c6b8332657 Mon Sep 17 00:00:00 2001 From: portly-halicore-76 <170707699+portly-halicore-76@users.noreply.github.com> Date: Sat, 23 Nov 2024 17:18:23 +0100 Subject: [PATCH] Fixes #241 * Some minor refactoring in the crypt area Signed-off-by: portly-halicore-76 <170707699+portly-halicore-76@users.noreply.github.com> --- api/v1alpha1/paasconfig_types.go | 4 ++ api/v1alpha1/zz_generated.deepcopy.go | 1 + cmd/crypttool/check_paas.go | 2 +- cmd/crypttool/reencrypt.go | 4 +- cmd/webservice/main.go | 2 +- internal/controller/main.go | 72 ++++++++++++++----- internal/controller/paasconfig_controller.go | 6 +- internal/controller/secret_controller.go | 30 +++++++- internal/crypt/crypt.go | 49 +++++++++++-- internal/crypt/main.go | 4 +- .../cpet.belastingdienst.nl_paasconfig.yaml | 13 ++++ test/e2e/main_test.go | 8 ++- test/e2e/sshsecrets_test.go | 6 +- 13 files changed, 163 insertions(+), 38 deletions(-) diff --git a/api/v1alpha1/paasconfig_types.go b/api/v1alpha1/paasconfig_types.go index 1be3cbf9..28f7f07c 100644 --- a/api/v1alpha1/paasconfig_types.go +++ b/api/v1alpha1/paasconfig_types.go @@ -53,6 +53,10 @@ type PaasConfigSpec struct { // +kubebuilder:validation:Required DecryptKeyPaths []string `json:"decryptKeyPaths"` + // DecryptKeysSecretName is a reference to the secret containing the DecryptKeys + // +kubebuilder:validation:Required + DecryptKeysSecret NamespacedName `json:"decryptKeySecret"` + // Enable debug information generation or not // +kubebuilder:default:=false // +kubebuilder:validation:Optional diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 9f828995..9d524b27 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -433,6 +433,7 @@ func (in *PaasConfigSpec) DeepCopyInto(out *PaasConfigSpec) { *out = make([]string, len(*in)) copy(*out, *in) } + out.DecryptKeysSecret = in.DecryptKeysSecret if in.Capabilities != nil { in, out := &in.Capabilities, &out.Capabilities *out = make(ConfigCapabilities, len(*in)) diff --git a/cmd/crypttool/check_paas.go b/cmd/crypttool/check_paas.go index daaef8ee..19af6a13 100644 --- a/cmd/crypttool/check_paas.go +++ b/cmd/crypttool/check_paas.go @@ -31,7 +31,7 @@ func checkPaasFiles(privateKeyFiles string, files []string) error { } paasName := paas.ObjectMeta.Name - srcCrypt, err := crypt.NewCrypt([]string{privateKeyFiles}, "", paasName) + srcCrypt, err := crypt.NewCryptFromFiles([]string{privateKeyFiles}, "", paasName) if err != nil { return err } diff --git a/cmd/crypttool/reencrypt.go b/cmd/crypttool/reencrypt.go index 8e5b7429..53ed8a78 100644 --- a/cmd/crypttool/reencrypt.go +++ b/cmd/crypttool/reencrypt.go @@ -53,12 +53,12 @@ func reencryptFiles(privateKeyFiles string, publicKeyFile string, outputFormat s } paasName := paas.ObjectMeta.Name - srcCrypt, err := crypt.NewCrypt([]string{privateKeyFiles}, "", paasName) + srcCrypt, err := crypt.NewCryptFromFiles([]string{privateKeyFiles}, "", paasName) if err != nil { return err } - dstCrypt, err := crypt.NewCrypt([]string{}, publicKeyFile, paasName) + dstCrypt, err := crypt.NewCryptFromFiles([]string{}, publicKeyFile, paasName) if err != nil { return err } diff --git a/cmd/webservice/main.go b/cmd/webservice/main.go index 537bba61..70642219 100644 --- a/cmd/webservice/main.go +++ b/cmd/webservice/main.go @@ -43,7 +43,7 @@ func getRsa(paas string) *crypt.Crypt { return c } - c, err := crypt.NewCrypt([]string{config.PrivateKeyPath}, config.PublicKeyPath, paas) + c, err := crypt.NewCryptFromFiles([]string{config.PrivateKeyPath}, config.PublicKeyPath, paas) if err != nil { panic(fmt.Errorf("unable to create a crypt: %w", err)) } diff --git a/internal/controller/main.go b/internal/controller/main.go index bde67b5e..9a6e5d14 100644 --- a/internal/controller/main.go +++ b/internal/controller/main.go @@ -8,9 +8,14 @@ package controller import ( "context" + "crypto/rsa" + "crypto/x509" + "encoding/pem" "fmt" "sync" + v1 "k8s.io/api/core/v1" + "github.com/belastingdienst/opr-paas/api/v1alpha1" "github.com/belastingdienst/opr-paas/internal/crypt" @@ -30,37 +35,66 @@ type PaasConfigStore struct { } var ( - _cnf = &PaasConfigStore{} - _crypt map[string]*crypt.Crypt - debugComponents []string + cnf = &PaasConfigStore{} + // crypts contains a maps of crypt against a Paas name + crypts map[string]*crypt.Crypt + currentDecryptSecretGeneration int64 + decryptSecretPrivateKeys []rsa.PrivateKey + debugComponents []string ) // GetConfig retrieves the current configuration func GetConfig() v1alpha1.PaasConfigSpec { - _cnf.mutex.RLock() - defer _cnf.mutex.RUnlock() - return _cnf.currentConfig + cnf.mutex.RLock() + defer cnf.mutex.RUnlock() + return cnf.currentConfig } // SetConfig updates the current configuration func SetConfig(newConfig v1alpha1.PaasConfig) { - _cnf.mutex.Lock() - defer _cnf.mutex.Unlock() - _cnf.currentConfig = newConfig.Spec + cnf.mutex.Lock() + defer cnf.mutex.Unlock() + cnf.currentConfig = newConfig.Spec +} + +// resetCrypts removes all crypts, decryptSecretPrivateKeys and resets the currentDecryptSecretGeneration +func resetCrypts() { + crypts = nil + decryptSecretPrivateKeys = nil + currentDecryptSecretGeneration = 0 } -func getRsa(paas string) *crypt.Crypt { - config := GetConfig() - if _crypt == nil { - _crypt = make(map[string]*crypt.Crypt) +// getRsa returns a crypt.Crypt for a specified paasName +func getRsa(paasName string, secret v1.Secret) (*crypt.Crypt, error) { + if crypts == nil { + crypts = make(map[string]*crypt.Crypt) } - if c, exists := _crypt[paas]; exists { - return c - } else if c, err := crypt.NewCrypt(config.DecryptKeyPaths, "", paas); err != nil { - panic(fmt.Errorf("could not get a crypt: %w", err)) + // Load secrets + // If one error occurs, all is invalid + if decryptSecretPrivateKeys == nil { + tmpKeys := make([]rsa.PrivateKey, 0) + for _, value := range secret.Data { + if privateKeyBlock, _ := pem.Decode(secret.Data[string(value)]); privateKeyBlock == nil { + return nil, fmt.Errorf("cannot decode private key") + } else if privateKey, err := x509.ParsePKCS1PrivateKey(privateKeyBlock.Bytes); err != nil { + return nil, fmt.Errorf("private key invalid: %w", err) + } else { + tmpKeys = append(tmpKeys, *privateKey) + } + } + decryptSecretPrivateKeys = tmpKeys + currentDecryptSecretGeneration = secret.Generation + } + + if c, exists := crypts[paasName]; exists { + return c, nil } else { - _crypt[paas] = c - return c + if c, err := crypt.NewCryptFromKeys(decryptSecretPrivateKeys, "", paasName); err != nil { + return nil, err + } else { + crypts[paasName] = c + return c, nil + } } } diff --git a/internal/controller/paasconfig_controller.go b/internal/controller/paasconfig_controller.go index 5823e193..bb33a4b2 100644 --- a/internal/controller/paasconfig_controller.go +++ b/internal/controller/paasconfig_controller.go @@ -151,7 +151,7 @@ func (pcr *PaasConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request logger.Err(err).Msg("failed to update PaasConfig status") return errResult, nil } - // don't reconcile this one again as that won't change anything.. I guess. + // don't reconcile this one again as that won't change anything. return ctrl.Result{}, nil } } @@ -169,6 +169,10 @@ func (pcr *PaasConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request } logger.Info().Msg("configuration has changed") + // TODO(portly-halicore-76) is this the correct place and time to change? + if !reflect.DeepEqual(config.Spec.DecryptKeysSecret, GetConfig().DecryptKeysSecret) { + resetCrypts() + } // Update the shared configuration store SetConfig(*config) logger.Debug().Msg("set active PaasConfig successfully") diff --git a/internal/controller/secret_controller.go b/internal/controller/secret_controller.go index 8bcf71b6..6e79ac99 100644 --- a/internal/controller/secret_controller.go +++ b/internal/controller/secret_controller.go @@ -14,6 +14,8 @@ import ( "maps" "strings" + "github.com/belastingdienst/opr-paas/internal/crypt" + "github.com/belastingdienst/opr-paas/api/v1alpha1" "github.com/rs/zerolog/log" @@ -115,12 +117,34 @@ func (r *PaasNSReconciler) backendSecret( return s, nil } +// getSecrets returns a list of Secrets which are desired based on the Paas(Ns) spec func (r *PaasNSReconciler) getSecrets( ctx context.Context, paas *v1alpha1.Paas, paasns *v1alpha1.PaasNS, encryptedSecrets map[string]string, ) (secrets []*corev1.Secret, err error) { + // Only do something when secrets are required + if len(encryptedSecrets) > 0 { + return nil, nil + } + // Get the configured decryptSecret + decryptSecret := &corev1.Secret{} + err = r.Get(ctx, types.NamespacedName{Name: GetConfig().DecryptKeysSecret.Name, Namespace: GetConfig().DecryptKeysSecret.Namespace}, decryptSecret) + if err != nil { + return nil, fmt.Errorf("Unable to get decryptSecret from kubernetes, contact system administrator: %w", err) + } + // If the generation is changed, the secret has changed, reset Crypts. + if decryptSecret.Generation != currentDecryptSecretGeneration { + resetCrypts() + } + + var rsa *crypt.Crypt + rsa, err = getRsa(paas.Name, *decryptSecret) + if err != nil { + return nil, err + } + for url, encryptedSecretData := range encryptedSecrets { namespacedName := types.NamespacedName{ Namespace: paasns.NamespaceName(), @@ -130,16 +154,18 @@ func (r *PaasNSReconciler) getSecrets( if err != nil { return nil, err } - if decrypted, err := getRsa(paasns.Spec.Paas).Decrypt(encryptedSecretData); err != nil { + if decrypted, err := rsa.Decrypt(encryptedSecretData); err != nil { return nil, fmt.Errorf("failed to decrypt secret %s: %s", secret, err.Error()) } else { secret.Data["sshPrivateKey"] = decrypted secrets = append(secrets, secret) } } - return + return secrets, nil } +// BackendSecrets returns a list of kubernetes Secrets which are desired based on the Paas(Ns) spec. +// It returns an error when the secrets cannot be determined. func (r *PaasNSReconciler) BackendSecrets( ctx context.Context, paasns *v1alpha1.PaasNS, diff --git a/internal/crypt/crypt.go b/internal/crypt/crypt.go index 81fc86a7..66a59910 100644 --- a/internal/crypt/crypt.go +++ b/internal/crypt/crypt.go @@ -33,7 +33,10 @@ type cryptPrivateKey struct { privateKey *rsa.PrivateKey } -func NewPrivateKey(privateKeyPath string) (*cryptPrivateKey, error) { +type cryptPrivateKeys []cryptPrivateKey + +// NewPrivateKeyFromFile returns a cryptPrivateKey from a privateKeyFilePath +func NewPrivateKeyFromFile(privateKeyPath string) (*cryptPrivateKey, error) { if privateKeyPath == "" { return nil, fmt.Errorf("cannot get private key without a specified path") } @@ -52,6 +55,15 @@ func NewPrivateKey(privateKeyPath string) (*cryptPrivateKey, error) { } } +// NewPrivateKeyFromRsa returns a cryptPrivateKey from a rsa.PrivateKey +func NewPrivateKeyFromRsa(privateKey rsa.PrivateKey) *cryptPrivateKey { + return &cryptPrivateKey{ + "", + nil, + &privateKey, + } +} + func (pk *cryptPrivateKey) writePrivateKey() error { if pk.privateKeyPath == "" { return fmt.Errorf("cannot write private key without a specified path") @@ -69,17 +81,23 @@ func (pk *cryptPrivateKey) writePrivateKey() error { return nil } +// getPrivateKey returns the rsa.PrivateKey from the provided cryptPrivateKey. If it is not set yet, it will +// try to load it from the specified filePath. It also checks whether it is a valid PrivateKey. func (pk cryptPrivateKey) getPrivateKey() (*rsa.PrivateKey, error) { + // if privateKey is already loaded, return it from the cryptPrivateKey if pk.privateKey != nil { return pk.privateKey, nil } + // if it is not loaded on the pk, we must load it, in this case from a specific path which hopefully is set if pk.privateKeyPath == "" { return nil, fmt.Errorf("cannot get private key without a specified path") } + // if set, read file from set path and try to decode if privateKeyPEM, err := os.ReadFile(pk.privateKeyPath); err != nil { panic(err) } else if privateKeyBlock, _ := pem.Decode(privateKeyPEM); privateKeyBlock == nil { return nil, fmt.Errorf("cannot decode private key") + // sanity check if the privatekey is a valid one } else if privateRsaKey, err := x509.ParsePKCS1PrivateKey(privateKeyBlock.Bytes); err != nil { return nil, fmt.Errorf("private key invalid: %w", err) } else { @@ -88,16 +106,15 @@ func (pk cryptPrivateKey) getPrivateKey() (*rsa.PrivateKey, error) { return pk.privateKey, nil } -type cryptPrivateKeys []cryptPrivateKey - -func NewCrypt(privateKeyPaths []string, publicKeyPath string, encryptionContext string) (*Crypt, error) { +// NewCryptFromFiles returns a Crypt based on the provided privateKeyPaths and publicKeyPath using the encryptionContext +func NewCryptFromFiles(privateKeyPaths []string, publicKeyPath string, encryptionContext string) (*Crypt, error) { var privateKeys cryptPrivateKeys if files, err := utils.PathToFileList(privateKeyPaths); err != nil { return nil, fmt.Errorf("could not find files in '%v': %w", privateKeyPaths, err) } else { for _, file := range files { - if pk, err := NewPrivateKey(file); err != nil { + if pk, err := NewPrivateKeyFromFile(file); err != nil { return nil, fmt.Errorf("invalid private key file %s", file) } else { privateKeys = append(privateKeys, *pk) @@ -119,6 +136,28 @@ func NewCrypt(privateKeyPaths []string, publicKeyPath string, encryptionContext }, nil } +// NewCryptFromKeys returns a Crypt based on the provided privateKeys and publicKey using the encryptionContext +func NewCryptFromKeys(privateKeys []rsa.PrivateKey, publicKeyPath string, encryptionContext string) (*Crypt, error) { + var cryptPrivateKeys cryptPrivateKeys + + for _, key := range privateKeys { + cryptPrivateKeys = append(cryptPrivateKeys, *NewPrivateKeyFromRsa(key)) + } + + if publicKeyPath != "" { + publicKeyPaths := []string{publicKeyPath} + if _, err := utils.PathToFileList(publicKeyPaths); err != nil { + return nil, fmt.Errorf("could not find files in '%v': %w", publicKeyPaths, err) + } + } + + return &Crypt{ + privateKeys: cryptPrivateKeys, + publicKeyPath: publicKeyPath, + encryptionContext: []byte(encryptionContext), + }, nil +} + func NewGeneratedCrypt(privateKeyPath string, publicKeyPath string) (*Crypt, error) { var c Crypt if privateKey, err := rsa.GenerateKey(rand.Reader, 4096); err != nil { diff --git a/internal/crypt/main.go b/internal/crypt/main.go index d3b7eb83..d0051645 100644 --- a/internal/crypt/main.go +++ b/internal/crypt/main.go @@ -13,7 +13,7 @@ import ( ) func encrypt(publicKey string, paasName string, data []byte) error { - if c, err := NewCrypt([]string{}, publicKey, paasName); err != nil { + if c, err := NewCryptFromFiles([]string{}, publicKey, paasName); err != nil { return err } else if encrypted, err := c.Encrypt(data); err != nil { return fmt.Errorf("failed to encrypt: %w", err) @@ -27,7 +27,7 @@ func DecryptFromStdin(privateKeys []string, paasName string) error { if data, err := io.ReadAll(os.Stdin); err != nil { return err } else { - if c, err := NewCrypt(privateKeys, "", paasName); err != nil { + if c, err := NewCryptFromFiles(privateKeys, "", paasName); err != nil { return err } else if encrypted, err := c.Decrypt(string(data)); err != nil { return fmt.Errorf("failed to decrypt: %w", err) diff --git a/manifests/crds/cpet.belastingdienst.nl_paasconfig.yaml b/manifests/crds/cpet.belastingdienst.nl_paasconfig.yaml index 89ec4acf..d9a09d7a 100644 --- a/manifests/crds/cpet.belastingdienst.nl_paasconfig.yaml +++ b/manifests/crds/cpet.belastingdienst.nl_paasconfig.yaml @@ -186,6 +186,18 @@ spec: type: string minItems: 1 type: array + decryptKeySecret: + description: DecryptKeysSecretName is a reference to the secret containing + the DecryptKeys + properties: + name: + type: string + namespace: + type: string + required: + - name + - namespace + type: object exclude_appset_name: description: |- Deprecated: ArgoCD specific code will be removed from the operator @@ -243,6 +255,7 @@ spec: required: - capabilities - decryptKeyPaths + - decryptKeySecret - exclude_appset_name - whitelist type: object diff --git a/test/e2e/main_test.go b/test/e2e/main_test.go index 2a10880c..d8ba161f 100644 --- a/test/e2e/main_test.go +++ b/test/e2e/main_test.go @@ -180,8 +180,11 @@ var examplePaasConfig = v1alpha1.PaasConfig{ }, }, }, - Debug: false, - DecryptKeyPaths: []string{"/tmp/paas-e2e/secrets/priv"}, + Debug: false, + DecryptKeysSecret: v1alpha1.NamespacedName{ + Name: "example-keys", + Namespace: "paas-system", + }, LDAP: v1alpha1.ConfigLdap{ Host: "my-ldap-host", Port: 13, @@ -228,6 +231,7 @@ func TestMain(m *testing.M) { // Global setup testenv.Setup( func(ctx context.Context, cfg *envconf.Config) (context.Context, error) { + // Nasty, required fields (according to CRD) are filled with an 'empty' value, which allows creation. paasconfig := &v1alpha1.PaasConfig{} *paasconfig = examplePaasConfig diff --git a/test/e2e/sshsecrets_test.go b/test/e2e/sshsecrets_test.go index 160b0b75..6efbe405 100644 --- a/test/e2e/sshsecrets_test.go +++ b/test/e2e/sshsecrets_test.go @@ -18,7 +18,7 @@ import ( ) func TestSecrets(t *testing.T) { - c, err := crypt.NewCrypt([]string{"/tmp/paas-e2e/secrets/priv"}, "/tmp/paas-e2e/secrets/pub/publicKey0", "sshpaas") + c, err := crypt.NewCryptFromFiles([]string{"/tmp/paas-e2e/secrets/priv"}, "/tmp/paas-e2e/secrets/pub/publicKey0", "sshpaas") if err != nil { panic(fmt.Errorf("unable to create a crypt: %w", err)) } @@ -82,7 +82,7 @@ func assertSecretCreated(ctx context.Context, t *testing.T, cfg *envconf.Config) } func assertSecretValueUpdated(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context { - c, err := crypt.NewCrypt([]string{"/tmp/paas-e2e/secrets/priv"}, "/tmp/paas-e2e/secrets/pub/publicKey0", "sshpaas") + c, err := crypt.NewCryptFromFiles([]string{"/tmp/paas-e2e/secrets/priv"}, "/tmp/paas-e2e/secrets/pub/publicKey0", "sshpaas") if err != nil { panic(fmt.Errorf("unable to create a crypt: %w", err)) } @@ -138,7 +138,7 @@ func assertSecretValueUpdated(ctx context.Context, t *testing.T, cfg *envconf.Co } func assertSecretKeyUpdated(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context { - c, err := crypt.NewCrypt([]string{"/tmp/paas-e2e/secrets/priv"}, "/tmp/paas-e2e/secrets/pub/publicKey0", "sshpaas") + c, err := crypt.NewCryptFromFiles([]string{"/tmp/paas-e2e/secrets/priv"}, "/tmp/paas-e2e/secrets/pub/publicKey0", "sshpaas") if err != nil { panic(fmt.Errorf("unable to create a crypt: %w", err)) }