Use AES-256-GCM encryption by default

Fixes #3317

Use GCM encryption as the default encryption algorithm for Minder. At this
point, we now have a hard requirement to use the new EncryptedData structure in
the database - the old columns (for the access token, and for the redirect URL)
do not track the algorithm used. As part of the migration away from the old
columns, I have made the following changes in this PR:

1. Make the old columns nullable
2. Stop writing to the old columns
3. Change all unit tests which relied on the old columns

config/server-config.yaml.example

@@ -106,10 +106,13 @@ authz:
 #    name: healthcheck
 
 crypto:
-    keystore:
+    key_store:
         type: local
         config:
             key_dir: "./.ssh"
     default:
         key_id:    token_key_passphrase
-        algorithm: aes-256-cfb
+        algorithm: aes-256-gcm
+    fallback:
+        algorithms:
+            - aes-256-cfb

database/migrations/000059_encrypted_token_nullable.down.sql

@@ -0,0 +1,19 @@
+-- Copyright 2024 Stacklok, Inc
+--
+-- Licensed under the Apache License, Version 2.0 (the "License");
+-- you may not use this file except in compliance with the License.
+-- You may obtain a copy of the License at
+--
+--      http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+BEGIN;
+
+ALTER TABLE provider_access_tokens ALTER COLUMN encrypted_token SET NOT NULL;
+
+COMMIT;

database/migrations/000059_encrypted_token_nullable.up.sql

@@ -0,0 +1,19 @@
+-- Copyright 2024 Stacklok, Inc
+--
+-- Licensed under the Apache License, Version 2.0 (the "License");
+-- you may not use this file except in compliance with the License.
+-- You may obtain a copy of the License at
+--
+--      http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+BEGIN;
+
+ALTER TABLE provider_access_tokens ALTER COLUMN encrypted_token DROP NOT NULL;
+
+COMMIT;

database/query/provider_access_tokens.sql

@@ -9,17 +9,16 @@ SELECT * FROM provider_access_tokens WHERE provider = $1 AND project_id = $2 AND
 
 -- name: UpsertAccessToken :one
 INSERT INTO provider_access_tokens
-(project_id, provider, encrypted_token, expiration_time, owner_filter, enrollment_nonce, encrypted_access_token)
+(project_id, provider, expiration_time, owner_filter, enrollment_nonce, encrypted_access_token)
 VALUES
-    ($1, $2, $3, $4, $5, $6, $7)
+    ($1, $2, $3, $4, $5, $6)
 ON CONFLICT (project_id, provider)
     DO UPDATE SET
-                  encrypted_token = $3,
-                  expiration_time = $4,
-                  owner_filter = $5,
-                  enrollment_nonce = $6,
+                  expiration_time = $3,
+                  owner_filter = $4,
+                  enrollment_nonce = $5,
                   updated_at = NOW(),
-                  encrypted_access_token = $7
+                  encrypted_access_token = $6
 WHERE provider_access_tokens.project_id = $1 AND provider_access_tokens.provider = $2
 RETURNING *;
 

database/query/session_store.sql

@@ -1,5 +1,5 @@
 -- name: CreateSessionState :one
-INSERT INTO session_store (provider, project_id, remote_user, session_state, owner_filter, provider_config, redirect_url, encrypted_redirect) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) RETURNING *;
+INSERT INTO session_store (provider, project_id, remote_user, session_state, owner_filter, provider_config, encrypted_redirect) VALUES ($1, $2, $3, $4, $5, $6, $7) RETURNING *;
 
 -- name: GetProjectIDBySessionState :one
 SELECT provider, project_id, remote_user, owner_filter, provider_config, redirect_url, encrypted_redirect FROM session_store WHERE session_state = $1;

internal/config/server/crypto.go

@@ -31,7 +31,7 @@ type KeyStoreConfig struct {
 // DefaultCrypto defines the default crypto to be used for new data
 type DefaultCrypto struct {
 	KeyID     string `mapstructure:"key_id"`
-	Algorithm string `mapstructure:"algorithm"`
+	Algorithm string `mapstructure:"algorithm" default:"aes-256-gcm"`
 }
 
 // FallbackCrypto defines the optional list of keys and algorithms to fall

internal/controlplane/handlers_oauth.go

@@ -116,7 +116,6 @@ func (s *Server) GetAuthorizationURL(ctx context.Context,
 		String: req.GetOwner(),
 	}
 
-	var redirectUrl sql.NullString
 	var encryptedBytes pqtype.NullRawMessage
 	// Empty redirect URL means null string (default condition)
 	if req.GetRedirectUrl() != "" {
@@ -132,7 +131,6 @@ func (s *Server) GetAuthorizationURL(ctx context.Context,
 			RawMessage: serialized,
 			Valid:      true,
 		}
-		redirectUrl = sql.NullString{Valid: true, String: encryptedRedirectURL.EncodedData}
 	}
 
 	var confBytes []byte
@@ -151,7 +149,6 @@ func (s *Server) GetAuthorizationURL(ctx context.Context,
 		RemoteUser:        sql.NullString{Valid: user != "", String: user},
 		SessionState:      state,
 		OwnerFilter:       owner,
-		RedirectUrl:       redirectUrl,
 		ProviderConfig:    confBytes,
 		EncryptedRedirect: encryptedBytes,
 	})
@@ -272,8 +269,7 @@ func (s *Server) processOAuthCallback(ctx context.Context, w http.ResponseWriter
 
 	logger.BusinessRecord(ctx).ProviderID = p.ID
 
-	// Note: right now, both RedirectUrl and EncryptedRedirect should both be valid
-	if stateData.RedirectUrl.Valid {
+	if stateData.RedirectUrl.Valid || stateData.EncryptedRedirect.Valid {
 		redirectURL, err := s.decryptRedirect(&stateData)
 		if err != nil {
 			return fmt.Errorf("unable to decrypt redirect URL: %w", err)
@@ -347,7 +343,7 @@ func (s *Server) processAppCallback(ctx context.Context, w http.ResponseWriter,
 			return fmt.Errorf("error creating GitHub App provider: %w", err)
 		}
 
-		if stateData.RedirectUrl.Valid {
+		if stateData.RedirectUrl.Valid || stateData.EncryptedRedirect.Valid {
 			redirectURL, err := s.decryptRedirect(&stateData)
 			if err != nil {
 				return fmt.Errorf("unable to decrypt redirect URL: %w", err)
@@ -495,10 +491,9 @@ func (s *Server) StoreProviderToken(ctx context.Context,
 	}
 
 	_, err = s.store.UpsertAccessToken(ctx, db.UpsertAccessTokenParams{
-		ProjectID:      projectID,
-		Provider:       provider.Name,
-		EncryptedToken: encryptedToken.EncodedData,
-		OwnerFilter:    owner,
+		ProjectID:   projectID,
+		Provider:    provider.Name,
+		OwnerFilter: owner,
 		EncryptedAccessToken: pqtype.NullRawMessage{
 			RawMessage: serialized,
 			Valid:      true,
@@ -670,8 +665,10 @@ func (s *Server) decryptRedirect(stateData *db.GetProjectIDBySessionStateRow) (*
 		if err != nil {
 			return nil, err
 		}
-	} else {
+	} else if stateData.RedirectUrl.Valid {
 		encryptedData = mcrypto.NewBackwardsCompatibleEncryptedData(stateData.RedirectUrl.String)
+	} else {
+		return nil, fmt.Errorf("no secret found in session data for provider %s", stateData.Provider)
 	}
 	redirectUrl, err := s.cryptoEngine.DecryptString(encryptedData)
 	if err != nil {

internal/crypto/algorithms/aes256cfb.go

@@ -22,8 +22,6 @@ import (
 	"io"
 
 	"golang.org/x/crypto/argon2"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 // AES256CFBAlgorithm implements the AES-256-CFB algorithm
@@ -40,14 +38,14 @@ func (a *AES256CFBAlgorithm) Encrypt(plaintext []byte, key []byte) ([]byte, erro
 	}
 	block, err := aes.NewCipher(a.deriveKey(key))
 	if err != nil {
-		return nil, status.Errorf(codes.Unknown, "failed to create cipher: %s", err)
+		return nil, fmt.Errorf("failed to create cipher: %w", err)
 	}
 
 	// The IV needs to be unique, but not secure. Therefore, it's common to include it at the beginning of the ciphertext.
 	ciphertext := make([]byte, aes.BlockSize+len(plaintext))
 	iv := ciphertext[:aes.BlockSize]
 	if _, err := io.ReadFull(rand.Reader, iv); err != nil {
-		return nil, status.Errorf(codes.Unknown, "failed to read random bytes: %s", err)
+		return nil, fmt.Errorf("failed to read random bytes: %w", err)
 	}
 
 	stream := cipher.NewCFBEncrypter(block, iv)
@@ -60,7 +58,7 @@ func (a *AES256CFBAlgorithm) Encrypt(plaintext []byte, key []byte) ([]byte, erro
 func (a *AES256CFBAlgorithm) Decrypt(ciphertext []byte, key []byte) ([]byte, error) {
 	block, err := aes.NewCipher(a.deriveKey(key))
 	if err != nil {
-		return nil, status.Errorf(codes.Unknown, "failed to create cipher: %s", err)
+		return nil, fmt.Errorf("failed to create cipher: %w", err)
 	}
 
 	if len(ciphertext) < aes.BlockSize {

internal/crypto/algorithms/aes256cfb_test.go

@@ -0,0 +1,101 @@
+// Copyright 2024 Stacklok, Inc
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package algorithms_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/stacklok/minder/internal/crypto/algorithms"
+)
+
+func TestCFBEncrypt(t *testing.T) {
+	t.Parallel()
+
+	scenarios := []struct {
+		Name          string
+		Key           []byte
+		Plaintext     []byte
+		ExpectedError string
+	}{
+		{
+			Name:          "CFB Encrypt rejects oversized plaintext",
+			Key:           key,
+			Plaintext:     make([]byte, 33*1024*1024), // 33MiB
+			ExpectedError: algorithms.ErrExceedsMaxSize.Error(),
+		},
+		{
+			Name:      "CFB encrypts plaintext",
+			Key:       key,
+			Plaintext: []byte(plaintext),
+		},
+	}
+
+	for _, scenario := range scenarios {
+		t.Run(scenario.Name, func(t *testing.T) {
+			t.Parallel()
+
+			result, err := cfb.Encrypt(scenario.Plaintext, scenario.Key)
+			if scenario.ExpectedError == "" {
+				require.NoError(t, err)
+				// validate by decrypting
+				decrypted, err := cfb.Decrypt(result, key)
+				require.NoError(t, err)
+				require.Equal(t, scenario.Plaintext, decrypted)
+			} else {
+				require.ErrorContains(t, err, scenario.ExpectedError)
+			}
+		})
+	}
+}
+
+// This doesn't test decryption - that is tested in the happy path of the encrypt test
+func TestCFBDecrypt(t *testing.T) {
+	t.Parallel()
+
+	scenarios := []struct {
+		Name          string
+		Key           []byte
+		Ciphertext    []byte
+		ExpectedError string
+	}{
+		{
+			Name:          "CFB Decrypt rejects short key",
+			Key:           []byte{0xFF},
+			Ciphertext:    []byte(plaintext),
+			ExpectedError: "ciphertext too short to decrypt",
+		},
+		{
+			Name:          "CFB Decrypt rejects undersized ciphertext",
+			Key:           key,
+			Ciphertext:    []byte{0xFF},
+			ExpectedError: "ciphertext too short to decrypt",
+		},
+	}
+
+	for _, scenario := range scenarios {
+		t.Run(scenario.Name, func(t *testing.T) {
+			t.Parallel()
+
+			_, err := cfb.Decrypt(scenario.Ciphertext, scenario.Key)
+			require.ErrorContains(t, err, scenario.ExpectedError)
+		})
+	}
+}
+
+var (
+	cfb = algorithms.AES256CFBAlgorithm{}
+)

internal/crypto/algorithms/aes256gcm.go

@@ -30,7 +30,9 @@ import (
 	"crypto/aes"
 	"crypto/cipher"
 	"crypto/rand"
+	"encoding/base64"
 	"errors"
+	"fmt"
 	"io"
 )
 
@@ -45,7 +47,12 @@ func (_ *AES256GCMAlgorithm) Encrypt(plaintext []byte, key []byte) ([]byte, erro
 		return nil, ErrExceedsMaxSize
 	}
 
-	block, err := aes.NewCipher(key[:])
+	decodedKey, err := decodeKey(key)
+	if err != nil {
+		return nil, err
+	}
+
+	block, err := aes.NewCipher(decodedKey[:])
 	if err != nil {
 		return nil, err
 	}
@@ -68,7 +75,12 @@ func (_ *AES256GCMAlgorithm) Encrypt(plaintext []byte, key []byte) ([]byte, erro
 // the data and provides a check that it hasn't been altered. Expects input
 // form nonce|ciphertext|tag where '|' indicates concatenation.
 func (_ *AES256GCMAlgorithm) Decrypt(ciphertext []byte, key []byte) ([]byte, error) {
-	block, err := aes.NewCipher(key[:])
+	decodedKey, err := decodeKey(key)
+	if err != nil {
+		return nil, err
+	}
+
+	block, err := aes.NewCipher(decodedKey[:])
 	if err != nil {
 		return nil, err
 	}
@@ -88,3 +100,15 @@ func (_ *AES256GCMAlgorithm) Decrypt(ciphertext []byte, key []byte) ([]byte, err
 		nil,
 	)
 }
+
+func decodeKey(key []byte) ([]byte, error) {
+	// Minder reads its keys as base64 encoded strings. Decode the string before
+	// using it as the key. Ideally, this would be done by the keystore, but there
+	// are some interesting quirks with how CFB uses the keys (see the comment
+	// in keystore.go) so I am doing it here for now.
+	decodedKey, err := base64.StdEncoding.DecodeString(string(key))
+	if err != nil {
+		return nil, fmt.Errorf("unable to base64 decode the encryption key: %w", err)
+	}
+	return decodedKey, nil
+}