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

Switch to AES-256-GCM encryption for secrets #3356

Merged
merged 2 commits into from
May 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions config/server-config.yaml.example
Original file line number Diff line number Diff line change
Expand Up @@ -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
19 changes: 19 additions & 0 deletions database/migrations/000059_encrypted_token_nullable.down.sql
Original file line number Diff line number Diff line change
@@ -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;
19 changes: 19 additions & 0 deletions database/migrations/000059_encrypted_token_nullable.up.sql
Original file line number Diff line number Diff line change
@@ -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;
13 changes: 6 additions & 7 deletions database/query/provider_access_tokens.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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 *;

Expand Down
2 changes: 1 addition & 1 deletion database/query/session_store.sql
Original file line number Diff line number Diff line change
@@ -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;
Expand Down
2 changes: 1 addition & 1 deletion internal/config/server/crypto.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 8 additions & 11 deletions internal/controlplane/handlers_oauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() != "" {
Expand All @@ -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
Expand All @@ -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,
})
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down
8 changes: 3 additions & 5 deletions internal/crypto/algorithms/aes256cfb.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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 {
Expand Down
101 changes: 101 additions & 0 deletions internal/crypto/algorithms/aes256cfb_test.go
Original file line number Diff line number Diff line change
@@ -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{}
)
28 changes: 26 additions & 2 deletions internal/crypto/algorithms/aes256gcm.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ import (
"crypto/aes"
"crypto/cipher"
"crypto/rand"
"encoding/base64"
"errors"
"fmt"
"io"
)

Expand All @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}
Loading