From 1d923c4940236bfa5d50ca1a3f1f207516cf1a23 Mon Sep 17 00:00:00 2001 From: Don Browne Date: Fri, 17 May 2024 10:11:48 +0100 Subject: [PATCH] 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 | 7 +- .../000059_encrypted_token_nullable.down.sql | 19 ++++ .../000059_encrypted_token_nullable.up.sql | 19 ++++ database/query/provider_access_tokens.sql | 13 ++- database/query/session_store.sql | 2 +- internal/config/server/crypto.go | 2 +- internal/controlplane/handlers_oauth.go | 19 ++-- internal/crypto/algorithms/aes256cfb.go | 8 +- internal/crypto/algorithms/aes256cfb_test.go | 101 ++++++++++++++++++ internal/crypto/algorithms/aes256gcm.go | 28 ++++- internal/crypto/algorithms/aes256gcm_test.go | 23 ++-- internal/crypto/engine.go | 2 +- internal/crypto/engine_test.go | 56 ++++++++-- internal/crypto/keystores/keystore.go | 14 ++- internal/crypto/testdata/test_encryption_key | 2 +- internal/db/models.go | 2 +- internal/db/provider_access_tokens.sql.go | 15 ++- internal/db/provider_access_tokens_test.go | 4 - internal/db/session_store.sql.go | 4 +- internal/engine/executor_test.go | 11 +- internal/providers/dockerhub/manager.go | 4 +- internal/providers/github/manager/manager.go | 4 +- internal/providers/github/service/service.go | 7 +- .../providers/github/service/service_test.go | 2 - internal/providers/providers.go | 4 +- 25 files changed, 293 insertions(+), 79 deletions(-) create mode 100644 database/migrations/000059_encrypted_token_nullable.down.sql create mode 100644 database/migrations/000059_encrypted_token_nullable.up.sql create mode 100644 internal/crypto/algorithms/aes256cfb_test.go diff --git a/config/server-config.yaml.example b/config/server-config.yaml.example index 20d8241b66..d5869a1dd0 100644 --- a/config/server-config.yaml.example +++ b/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 \ No newline at end of file + algorithm: aes-256-gcm + fallback: + algorithms: + - aes-256-cfb \ No newline at end of file diff --git a/database/migrations/000059_encrypted_token_nullable.down.sql b/database/migrations/000059_encrypted_token_nullable.down.sql new file mode 100644 index 0000000000..f7481c5cc9 --- /dev/null +++ b/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; diff --git a/database/migrations/000059_encrypted_token_nullable.up.sql b/database/migrations/000059_encrypted_token_nullable.up.sql new file mode 100644 index 0000000000..7e12fc00d7 --- /dev/null +++ b/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; diff --git a/database/query/provider_access_tokens.sql b/database/query/provider_access_tokens.sql index 4ab033b5ea..949d085f91 100644 --- a/database/query/provider_access_tokens.sql +++ b/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 *; diff --git a/database/query/session_store.sql b/database/query/session_store.sql index ee7160dcc6..1aa912ece2 100644 --- a/database/query/session_store.sql +++ b/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; diff --git a/internal/config/server/crypto.go b/internal/config/server/crypto.go index 6f57b2c382..187b48bd15 100644 --- a/internal/config/server/crypto.go +++ b/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 diff --git a/internal/controlplane/handlers_oauth.go b/internal/controlplane/handlers_oauth.go index d4e4bf7e38..ee153cc27e 100644 --- a/internal/controlplane/handlers_oauth.go +++ b/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 { diff --git a/internal/crypto/algorithms/aes256cfb.go b/internal/crypto/algorithms/aes256cfb.go index b28da434af..16d7b6e10f 100644 --- a/internal/crypto/algorithms/aes256cfb.go +++ b/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 { diff --git a/internal/crypto/algorithms/aes256cfb_test.go b/internal/crypto/algorithms/aes256cfb_test.go new file mode 100644 index 0000000000..f59c88bf0c --- /dev/null +++ b/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{} +) diff --git a/internal/crypto/algorithms/aes256gcm.go b/internal/crypto/algorithms/aes256gcm.go index 13f9161804..ec0d30d879 100644 --- a/internal/crypto/algorithms/aes256gcm.go +++ b/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 +} diff --git a/internal/crypto/algorithms/aes256gcm_test.go b/internal/crypto/algorithms/aes256gcm_test.go index 7bda9aaab5..f24193b477 100644 --- a/internal/crypto/algorithms/aes256gcm_test.go +++ b/internal/crypto/algorithms/aes256gcm_test.go @@ -31,9 +31,15 @@ func TestGCMEncrypt(t *testing.T) { Plaintext []byte ExpectedError string }{ + { + Name: "GCM Encrypt rejects key which cannot be base64 decoded", + Key: []byte{0xFF, 0xFF, 0xFF, 0xFF}, + Plaintext: []byte(plaintext), + ExpectedError: "unable to base64 decode the encryption key", + }, { Name: "GCM Encrypt rejects short key", - Key: []byte{0xFF}, + Key: []byte{0x41, 0x42, 0x43, 0x44}, Plaintext: []byte(plaintext), ExpectedError: "invalid key size", }, @@ -79,9 +85,15 @@ func TestGCMDecrypt(t *testing.T) { ExpectedError string }{ { - Name: "GCM Decrypt rejects short key", + Name: "GCM Decrypt rejects key which cannot be base64 decoded", Key: []byte{0xFF}, Ciphertext: []byte(plaintext), + ExpectedError: "unable to base64 decode the encryption key", + }, + { + Name: "GCM Decrypt rejects short key", + Key: []byte{0xa}, + Ciphertext: []byte(plaintext), ExpectedError: "invalid key size", }, { @@ -91,7 +103,7 @@ func TestGCMDecrypt(t *testing.T) { ExpectedError: "message authentication failed", }, { - Name: "GCM Decrypt rejects undersized key", + Name: "GCM Decrypt rejects undersized ciphertext", Key: key, Ciphertext: []byte{0xFF}, ExpectedError: "malformed ciphertext", @@ -109,10 +121,7 @@ func TestGCMDecrypt(t *testing.T) { } var ( - key = []byte{ - 0x5, 0x94, 0x74, 0xfd, 0xb7, 0xf9, 0x85, 0x9, 0x67, 0x8, 0x2D, 0xe8, 0x46, 0x8c, 0x76, 0xe2, - 0x7a, 0x85, 0x7f, 0xed, 0x67, 0xd4, 0xd5, 0x2c, 0x46, 0x00, 0xba, 0x44, 0x8d, 0x54, 0x20, 0xf1, - } + key = []byte("2hcGLimy2i7LAknby2AFqYx87CaaCAtjxDiorRxYq8Q=") gcm = algorithms.AES256GCMAlgorithm{} ) diff --git a/internal/crypto/engine.go b/internal/crypto/engine.go index a0a2a447e8..ab9df0a6fb 100644 --- a/internal/crypto/engine.go +++ b/internal/crypto/engine.go @@ -79,7 +79,7 @@ func NewEngineFromConfig(config *serverconfig.Config) (Engine, error) { keystore, err := keystores.NewKeyStoreFromConfig(cryptoCfg) if err != nil { - return nil, fmt.Errorf("failed to read token key file: %s", err) + return nil, fmt.Errorf("unable to create keystore: %s", err) } defaultAlgorithm, err := algorithms.TypeFromString(cryptoCfg.Default.Algorithm) diff --git a/internal/crypto/engine_test.go b/internal/crypto/engine_test.go index 789e91a2e9..a83b0504eb 100644 --- a/internal/crypto/engine_test.go +++ b/internal/crypto/engine_test.go @@ -50,7 +50,7 @@ func TestNewFromCryptoConfig(t *testing.T) { require.NoError(t, err) } -func TestNewKeyLoadFail(t *testing.T) { +func TestNewKeystoreFail(t *testing.T) { t.Parallel() config := &server.Config{ @@ -59,10 +59,10 @@ func TestNewKeyLoadFail(t *testing.T) { }, } _, err := NewEngineFromConfig(config) - require.ErrorContains(t, err, "failed to read token key file") + require.ErrorContains(t, err, "unable to create keystore") } -func TestNewKeyRejectsEmptyConfig(t *testing.T) { +func TestNewRejectsEmptyConfig(t *testing.T) { t.Parallel() config := &server.Config{} @@ -70,7 +70,7 @@ func TestNewKeyRejectsEmptyConfig(t *testing.T) { require.ErrorContains(t, err, "no encryption keys configured") } -func TestNewKeyRejectsBadAlgo(t *testing.T) { +func TestNewRejectsBadAlgo(t *testing.T) { t.Parallel() config := &server.Config{ @@ -91,7 +91,7 @@ func TestNewKeyRejectsBadAlgo(t *testing.T) { require.ErrorIs(t, err, algorithms.ErrUnknownAlgorithm) } -func TestNewKeyRejectsBadFallbackAlgo(t *testing.T) { +func TestNewRejectsBadFallbackAlgo(t *testing.T) { t.Parallel() config := &server.Config{ @@ -128,14 +128,52 @@ func TestEncryptDecryptBytes(t *testing.T) { assert.Equal(t, sampleData, decrypted) } -func TestEncryptTooLarge(t *testing.T) { +func TestFallbackDecrypt(t *testing.T) { t.Parallel() + // instantiate engine with CFB as default algorithm engine, err := NewEngineFromConfig(config) require.NoError(t, err) - large := make([]byte, 34000000) // More than 32 MB - _, err = engine.EncryptString(string(large)) - assert.ErrorIs(t, err, algorithms.ErrExceedsMaxSize) + + // encrypt data with old config + const sampleData = "Hello world!" + encrypted, err := engine.EncryptString(sampleData) + require.NoError(t, err) + + // Create new config where we introduce a new default algorithm + // and make CFB the fallback. + newConfig := &server.Config{ + Crypto: server.CryptoConfig{ + KeyStore: server.KeyStoreConfig{ + Type: "local", + Config: map[string]any{ + "key_dir": "./testdata", + }, + }, + Default: server.DefaultCrypto{ + KeyID: "test_encryption_key", + Algorithm: string(algorithms.Aes256Gcm), + }, + Fallback: server.FallbackCrypto{ + Algorithms: []string{string(algorithms.Aes256Cfb)}, + }, + }, + } + + // instantiate new engine + newEngine, err := NewEngineFromConfig(newConfig) + require.NoError(t, err) + + // decrypt data from old engine with new engine + // this validates that the fallback works as expected + decrypted, err := newEngine.DecryptString(encrypted) + assert.Nil(t, err) + assert.Equal(t, sampleData, decrypted) + + // encrypt the data with the new engine - assert it is not the same as the old result + newEncrypted, err := newEngine.EncryptString(sampleData) + require.NoError(t, err) + require.NotEqualf(t, newEncrypted, encrypted, "two encrypted values expected to be different but are not") } func TestEncryptDecryptOAuthToken(t *testing.T) { diff --git a/internal/crypto/keystores/keystore.go b/internal/crypto/keystores/keystore.go index 7f15b1e8b5..655134c028 100644 --- a/internal/crypto/keystores/keystore.go +++ b/internal/crypto/keystores/keystore.go @@ -100,10 +100,18 @@ func (l *localFileKeyStore) GetKey(id string) ([]byte, error) { func readKey(keyDir string, keyFilename string) ([]byte, error) { keyPath := filepath.Join(keyDir, keyFilename) cleanPath := filepath.Clean(keyPath) - data, err := os.ReadFile(cleanPath) + + // NOTE: Minder reads the base64 encoded key PLUS line feed character and + // uses it as the key without decoding. The CFB algorithm expects the line + // feed, and stripping it out will break existing secrets. The GCM + // algorithm will base64 decode the key and get rid of the newline. + // + // If we get rid of the CFB cipher in future, we should base64 decode the + // string here and always use the decoded bytes minus linefeed as the key + // in the algorithms. + key, err := os.ReadFile(cleanPath) if err != nil { return nil, fmt.Errorf("failed to read key: %w", err) } - - return data, nil + return key, nil } diff --git a/internal/crypto/testdata/test_encryption_key b/internal/crypto/testdata/test_encryption_key index 30d74d2584..f65596926d 100644 --- a/internal/crypto/testdata/test_encryption_key +++ b/internal/crypto/testdata/test_encryption_key @@ -1 +1 @@ -test \ No newline at end of file +2hcGLimy2i7LAknby2AFqYx87CaaCAtjxDiorRxYq8Q= \ No newline at end of file diff --git a/internal/db/models.go b/internal/db/models.go index 7b88f1c5ed..ab6bb9b2ad 100644 --- a/internal/db/models.go +++ b/internal/db/models.go @@ -552,7 +552,7 @@ type ProviderAccessToken struct { Provider string `json:"provider"` ProjectID uuid.UUID `json:"project_id"` OwnerFilter sql.NullString `json:"owner_filter"` - EncryptedToken string `json:"encrypted_token"` + EncryptedToken sql.NullString `json:"encrypted_token"` ExpirationTime time.Time `json:"expiration_time"` CreatedAt time.Time `json:"created_at"` UpdatedAt time.Time `json:"updated_at"` diff --git a/internal/db/provider_access_tokens.sql.go b/internal/db/provider_access_tokens.sql.go index 00767d68f5..5f794e4e32 100644 --- a/internal/db/provider_access_tokens.sql.go +++ b/internal/db/provider_access_tokens.sql.go @@ -136,17 +136,16 @@ func (q *Queries) GetAccessTokenSinceDate(ctx context.Context, arg GetAccessToke const upsertAccessToken = `-- 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 id, provider, project_id, owner_filter, encrypted_token, expiration_time, created_at, updated_at, enrollment_nonce, encrypted_access_token ` @@ -154,7 +153,6 @@ RETURNING id, provider, project_id, owner_filter, encrypted_token, expiration_ti type UpsertAccessTokenParams struct { ProjectID uuid.UUID `json:"project_id"` Provider string `json:"provider"` - EncryptedToken string `json:"encrypted_token"` ExpirationTime time.Time `json:"expiration_time"` OwnerFilter sql.NullString `json:"owner_filter"` EnrollmentNonce sql.NullString `json:"enrollment_nonce"` @@ -165,7 +163,6 @@ func (q *Queries) UpsertAccessToken(ctx context.Context, arg UpsertAccessTokenPa row := q.db.QueryRowContext(ctx, upsertAccessToken, arg.ProjectID, arg.Provider, - arg.EncryptedToken, arg.ExpirationTime, arg.OwnerFilter, arg.EnrollmentNonce, diff --git a/internal/db/provider_access_tokens_test.go b/internal/db/provider_access_tokens_test.go index 5b1982217b..163d8ca19f 100644 --- a/internal/db/provider_access_tokens_test.go +++ b/internal/db/provider_access_tokens_test.go @@ -40,7 +40,6 @@ func TestUpsertProviderAccessToken(t *testing.T) { tok, err := testQueries.UpsertAccessToken(context.Background(), UpsertAccessTokenParams{ ProjectID: project.ID, Provider: prov.Name, - EncryptedToken: "abc", EncryptedAccessToken: serialized, OwnerFilter: sql.NullString{}, }) @@ -52,7 +51,6 @@ func TestUpsertProviderAccessToken(t *testing.T) { require.NotEmpty(t, tok.UpdatedAt) require.Equal(t, project.ID, tok.ProjectID) require.Equal(t, prov.Name, tok.Provider) - require.Equal(t, "abc", tok.EncryptedToken) require.Equal(t, secret, deserializeSecret(t, tok.EncryptedAccessToken)) require.Equal(t, sql.NullString{}, tok.OwnerFilter) @@ -61,7 +59,6 @@ func TestUpsertProviderAccessToken(t *testing.T) { tokUpdate, err := testQueries.UpsertAccessToken(context.Background(), UpsertAccessTokenParams{ ProjectID: project.ID, Provider: prov.Name, - EncryptedToken: "def", EncryptedAccessToken: newSerialized, OwnerFilter: sql.NullString{}, }) @@ -69,7 +66,6 @@ func TestUpsertProviderAccessToken(t *testing.T) { require.NoError(t, err) require.Equal(t, project.ID, tokUpdate.ProjectID) require.Equal(t, prov.Name, tokUpdate.Provider) - require.Equal(t, "def", tokUpdate.EncryptedToken) require.Equal(t, newSecret, deserializeSecret(t, tokUpdate.EncryptedAccessToken)) require.Equal(t, sql.NullString{}, tokUpdate.OwnerFilter) require.Equal(t, tok.ID, tokUpdate.ID) diff --git a/internal/db/session_store.sql.go b/internal/db/session_store.sql.go index e2b8bf06eb..3065d0de3a 100644 --- a/internal/db/session_store.sql.go +++ b/internal/db/session_store.sql.go @@ -14,7 +14,7 @@ import ( ) const createSessionState = `-- 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 id, provider, project_id, port, owner_filter, session_state, created_at, redirect_url, remote_user, encrypted_redirect, provider_config +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 id, provider, project_id, port, owner_filter, session_state, created_at, redirect_url, remote_user, encrypted_redirect, provider_config ` type CreateSessionStateParams struct { @@ -24,7 +24,6 @@ type CreateSessionStateParams struct { SessionState string `json:"session_state"` OwnerFilter sql.NullString `json:"owner_filter"` ProviderConfig []byte `json:"provider_config"` - RedirectUrl sql.NullString `json:"redirect_url"` EncryptedRedirect pqtype.NullRawMessage `json:"encrypted_redirect"` } @@ -36,7 +35,6 @@ func (q *Queries) CreateSessionState(ctx context.Context, arg CreateSessionState arg.SessionState, arg.OwnerFilter, arg.ProviderConfig, - arg.RedirectUrl, arg.EncryptedRedirect, ) var i SessionStore diff --git a/internal/engine/executor_test.go b/internal/engine/executor_test.go index 31688a5ccd..5145a92740 100644 --- a/internal/engine/executor_test.go +++ b/internal/engine/executor_test.go @@ -56,7 +56,7 @@ const ( fakeTokenKey = "foo-bar" ) -func generateFakeAccessToken(t *testing.T, cryptoEngine crypto.Engine) string { +func generateFakeAccessToken(t *testing.T, cryptoEngine crypto.Engine) pqtype.NullRawMessage { t.Helper() ftoken := &oauth2.Token{ @@ -70,7 +70,12 @@ func generateFakeAccessToken(t *testing.T, cryptoEngine crypto.Engine) string { // encrypt token encryptedToken, err := cryptoEngine.EncryptOAuthToken(ftoken) require.NoError(t, err) - return encryptedToken.EncodedData + serialized, err := encryptedToken.Serialize() + require.NoError(t, err) + return pqtype.NullRawMessage{ + RawMessage: serialized, + Valid: true, + } } func TestExecutor_handleEntityEvent(t *testing.T) { @@ -142,7 +147,7 @@ func TestExecutor_handleEntityEvent(t *testing.T) { ProjectID: projectID, }). Return(db.ProviderAccessToken{ - EncryptedToken: authtoken, + EncryptedAccessToken: authtoken, }, nil) // list one profile diff --git a/internal/providers/dockerhub/manager.go b/internal/providers/dockerhub/manager.go index 542821cdd3..310f028787 100644 --- a/internal/providers/dockerhub/manager.go +++ b/internal/providers/dockerhub/manager.go @@ -101,8 +101,10 @@ func (m *providerClassManager) getProviderCredentials( if err != nil { return nil, err } + } else if encToken.EncryptedToken.Valid { + encryptedData = crypto.NewBackwardsCompatibleEncryptedData(encToken.EncryptedToken.String) } else { - encryptedData = crypto.NewBackwardsCompatibleEncryptedData(encToken.EncryptedToken) + return nil, fmt.Errorf("no secret found for provider %s", encToken.Provider) } decryptedToken, err := m.crypteng.DecryptOAuthToken(encryptedData) if err != nil { diff --git a/internal/providers/github/manager/manager.go b/internal/providers/github/manager/manager.go index c6c7dbc080..6cb24625a7 100644 --- a/internal/providers/github/manager/manager.go +++ b/internal/providers/github/manager/manager.go @@ -203,8 +203,10 @@ func (g *githubProviderManager) createProviderWithAccessToken( if err != nil { return nil, err } + } else if encToken.EncryptedToken.Valid { + encryptedData = crypto.NewBackwardsCompatibleEncryptedData(encToken.EncryptedToken.String) } else { - encryptedData = crypto.NewBackwardsCompatibleEncryptedData(encToken.EncryptedToken) + return nil, fmt.Errorf("no secret found for provider %s", encToken.Provider) } decryptedToken, err := g.crypteng.DecryptOAuthToken(encryptedData) if err != nil { diff --git a/internal/providers/github/service/service.go b/internal/providers/github/service/service.go index db6509c2e4..1e90c2c95d 100644 --- a/internal/providers/github/service/service.go +++ b/internal/providers/github/service/service.go @@ -194,10 +194,9 @@ func (p *ghProviderService) CreateGitHubOAuthProvider( } _, err = qtx.UpsertAccessToken(ctx, db.UpsertAccessTokenParams{ - ProjectID: stateData.ProjectID, - Provider: providerName, - EncryptedToken: encryptedToken.EncodedData, - OwnerFilter: stateData.OwnerFilter, + ProjectID: stateData.ProjectID, + Provider: providerName, + OwnerFilter: stateData.OwnerFilter, EnrollmentNonce: sql.NullString{ Valid: true, String: state, diff --git a/internal/providers/github/service/service_test.go b/internal/providers/github/service/service_test.go index 6285bb2211..189309577b 100644 --- a/internal/providers/github/service/service_test.go +++ b/internal/providers/github/service/service_test.go @@ -197,7 +197,6 @@ func TestProviderService_CreateGitHubOAuthProvider(t *testing.T) { deserialized, err := crypto.DeserializeEncryptedData(dbToken[0].EncryptedAccessToken.RawMessage) require.NoError(t, err) require.Equal(t, deserialized, encryptedToken) - require.Equal(t, dbToken[0].EncryptedToken, encryptedToken.EncodedData) require.Equal(t, dbToken[0].OwnerFilter, sql.NullString{String: "testorg", Valid: true}) require.Equal(t, dbToken[0].EnrollmentNonce, sql.NullString{String: stateNonce, Valid: true}) @@ -238,7 +237,6 @@ func TestProviderService_CreateGitHubOAuthProvider(t *testing.T) { deserialized, err = crypto.DeserializeEncryptedData(dbToken[0].EncryptedAccessToken.RawMessage) require.NoError(t, err) require.Equal(t, deserialized, encryptedToken) - require.Equal(t, dbTokenUpdate[0].EncryptedToken, encryptedToken.EncodedData) require.Equal(t, dbTokenUpdate[0].OwnerFilter, sql.NullString{String: "testorg", Valid: true}) require.Equal(t, dbTokenUpdate[0].EnrollmentNonce, sql.NullString{String: stateNonceUpdate, Valid: true}) } diff --git a/internal/providers/providers.go b/internal/providers/providers.go index c70396719c..64823d8d3f 100644 --- a/internal/providers/providers.go +++ b/internal/providers/providers.go @@ -126,8 +126,10 @@ func getCredentialForProvider( if err != nil { return nil, err } + } else if encToken.EncryptedToken.Valid { + encryptedData = crypto.NewBackwardsCompatibleEncryptedData(encToken.EncryptedToken.String) } else { - encryptedData = crypto.NewBackwardsCompatibleEncryptedData(encToken.EncryptedToken) + return nil, fmt.Errorf("no secret found for provider %s", encToken.Provider) } decryptedToken, err := crypteng.DecryptOAuthToken(encryptedData) if err != nil {