From 6651fe1bd2735e406fe9fc2c97c516374fa54e1d Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Tue, 30 May 2023 16:26:29 +0000 Subject: [PATCH 1/4] feat: encrypt oidc and git auth tokens in the database --- coderd/authorize.go | 2 +- coderd/database/dbcrypt/dbcrypt.go | 164 +++++++++++++++ coderd/database/dbcrypt/dbcrypt_test.go | 196 ++++++++++++++++++ coderd/database/dbgen/generator.go | 4 +- codersdk/deployment.go | 12 +- cryptorand/cipher.go | 48 +++++ cryptorand/cipher_test.go | 30 +++ enterprise/cli/server.go | 12 ++ enterprise/coderd/coderd.go | 38 +++- .../coderd/coderdenttest/coderdenttest.go | 2 +- enterprise/coderd/licenses.go | 4 +- 11 files changed, 497 insertions(+), 15 deletions(-) create mode 100644 coderd/database/dbcrypt/dbcrypt.go create mode 100644 coderd/database/dbcrypt/dbcrypt_test.go create mode 100644 cryptorand/cipher.go create mode 100644 cryptorand/cipher_test.go diff --git a/coderd/authorize.go b/coderd/authorize.go index 9dcc7e411298e..ab0042603fac8 100644 --- a/coderd/authorize.go +++ b/coderd/authorize.go @@ -75,7 +75,7 @@ func (h *HTTPAuthorizer) Authorize(r *http.Request, action rbac.Action, object r } // Log information for debugging. This will be very helpful // in the early days - logger.Warn(r.Context(), "unauthorized", + logger.Debug(r.Context(), "unauthorized", slog.F("roles", roles.Actor.SafeRoleNames()), slog.F("actor_id", roles.Actor.ID), slog.F("actor_name", roles.ActorName), diff --git a/coderd/database/dbcrypt/dbcrypt.go b/coderd/database/dbcrypt/dbcrypt.go new file mode 100644 index 0000000000000..e579903288b87 --- /dev/null +++ b/coderd/database/dbcrypt/dbcrypt.go @@ -0,0 +1,164 @@ +package dbcrypt + +import ( + "context" + "database/sql" + "strings" + "sync/atomic" + + "golang.org/x/xerrors" + + "github.com/coder/coder/coderd/database" + "github.com/coder/coder/cryptorand" +) + +// MagicPrefix is prepended to all encrypted values in the database. +// This is used to determine if a value is encrypted or not. +// If it is encrypted but a key is not provided, an error is returned. +const MagicPrefix = "dbcrypt-" + +// ErrInvalidCipher is returned when an invalid cipher is provided +// for the encrypted data. +var ErrInvalidCipher = xerrors.New("an invalid encryption cipher was provided for the encrypted data") + +type Options struct { + // ExternalTokenCipher is an optional cipher that is used + // to encrypt/decrypt user link and git auth link tokens. If this is nil, + // then no encryption/decryption will be performed. + ExternalTokenCipher *atomic.Pointer[cryptorand.Cipher] +} + +// New creates a database.Store wrapper that encrypts/decrypts values +// stored at rest in the database. +func New(db database.Store, options *Options) database.Store { + return &dbCrypt{ + Options: options, + Store: db, + } +} + +type dbCrypt struct { + *Options + database.Store +} + +func (db *dbCrypt) InTx(function func(database.Store) error, txOpts *sql.TxOptions) error { + return db.Store.InTx(func(s database.Store) error { + return function(&dbCrypt{ + Options: db.Options, + Store: s, + }) + }, txOpts) +} + +func (db *dbCrypt) GetUserLinkByLinkedID(ctx context.Context, linkedID string) (database.UserLink, error) { + link, err := db.Store.GetUserLinkByLinkedID(ctx, linkedID) + if err != nil { + return database.UserLink{}, err + } + return link, db.decryptFields(&link.OAuthAccessToken, &link.OAuthRefreshToken) +} + +func (db *dbCrypt) GetUserLinkByUserIDLoginType(ctx context.Context, params database.GetUserLinkByUserIDLoginTypeParams) (database.UserLink, error) { + link, err := db.Store.GetUserLinkByUserIDLoginType(ctx, params) + if err != nil { + return database.UserLink{}, err + } + return link, db.decryptFields(&link.OAuthAccessToken, &link.OAuthRefreshToken) +} + +func (db *dbCrypt) InsertUserLink(ctx context.Context, params database.InsertUserLinkParams) (database.UserLink, error) { + err := db.encryptFields(¶ms.OAuthAccessToken, ¶ms.OAuthRefreshToken) + if err != nil { + return database.UserLink{}, err + } + return db.Store.InsertUserLink(ctx, params) +} + +func (db *dbCrypt) UpdateUserLink(ctx context.Context, params database.UpdateUserLinkParams) (database.UserLink, error) { + err := db.encryptFields(¶ms.OAuthAccessToken, ¶ms.OAuthRefreshToken) + if err != nil { + return database.UserLink{}, err + } + return db.Store.UpdateUserLink(ctx, params) +} + +func (db *dbCrypt) InsertGitAuthLink(ctx context.Context, params database.InsertGitAuthLinkParams) (database.GitAuthLink, error) { + err := db.encryptFields(¶ms.OAuthAccessToken, ¶ms.OAuthRefreshToken) + if err != nil { + return database.GitAuthLink{}, err + } + return db.Store.InsertGitAuthLink(ctx, params) +} + +func (db *dbCrypt) GetGitAuthLink(ctx context.Context, params database.GetGitAuthLinkParams) (database.GitAuthLink, error) { + link, err := db.Store.GetGitAuthLink(ctx, params) + if err != nil { + return database.GitAuthLink{}, err + } + return link, db.decryptFields(&link.OAuthAccessToken, &link.OAuthRefreshToken) +} + +func (db *dbCrypt) UpdateGitAuthLink(ctx context.Context, params database.UpdateGitAuthLinkParams) (database.GitAuthLink, error) { + err := db.encryptFields(¶ms.OAuthAccessToken, ¶ms.OAuthRefreshToken) + if err != nil { + return database.GitAuthLink{}, err + } + return db.Store.UpdateGitAuthLink(ctx, params) +} + +func (db *dbCrypt) encryptFields(fields ...*string) error { + cipherPtr := db.ExternalTokenCipher.Load() + // If no cipher is loaded, then we don't need to encrypt or decrypt anything! + if cipherPtr == nil { + return nil + } + cipher := *cipherPtr + for _, field := range fields { + if field == nil { + continue + } + + encrypted, err := cipher.Encrypt([]byte(*field)) + if err != nil { + return err + } + *field = MagicPrefix + string(encrypted) + } + return nil +} + +// decryptFields decrypts the given fields in place. +// If the value fails to decrypt, sql.ErrNoRows will be returned. +func (db *dbCrypt) decryptFields(fields ...*string) error { + cipherPtr := db.ExternalTokenCipher.Load() + // If no cipher is loaded, then we don't need to encrypt or decrypt anything! + if cipherPtr == nil { + for _, field := range fields { + if field == nil { + continue + } + if strings.HasPrefix(*field, MagicPrefix) { + return ErrInvalidCipher + } + } + return nil + } + + cipher := *cipherPtr + for _, field := range fields { + if field == nil { + continue + } + if len(*field) < len(MagicPrefix) || !strings.HasPrefix(*field, MagicPrefix) { + continue + } + + decrypted, err := cipher.Decrypt([]byte((*field)[len(MagicPrefix):])) + if err != nil { + return xerrors.Errorf("%w: %s", ErrInvalidCipher, err) + } + *field = string(decrypted) + } + return nil +} diff --git a/coderd/database/dbcrypt/dbcrypt_test.go b/coderd/database/dbcrypt/dbcrypt_test.go new file mode 100644 index 0000000000000..8529f457bb1cb --- /dev/null +++ b/coderd/database/dbcrypt/dbcrypt_test.go @@ -0,0 +1,196 @@ +package dbcrypt_test + +import ( + "context" + "crypto/rand" + "io" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/coderd/database" + "github.com/coder/coder/coderd/database/dbcrypt" + "github.com/coder/coder/coderd/database/dbfake" + "github.com/coder/coder/coderd/database/dbgen" + "github.com/coder/coder/cryptorand" +) + +func TestUserLinks(t *testing.T) { + t.Parallel() + ctx := context.Background() + + t.Run("InsertUserLink", func(t *testing.T) { + t.Parallel() + db, crypt, cipher := setup(t) + initCipher(t, cipher) + link := dbgen.UserLink(t, crypt, database.UserLink{ + OAuthAccessToken: "access", + OAuthRefreshToken: "refresh", + }) + link, err := db.GetUserLinkByLinkedID(ctx, link.LinkedID) + require.NoError(t, err) + requireEncryptedEquals(t, cipher, link.OAuthAccessToken, "access") + requireEncryptedEquals(t, cipher, link.OAuthRefreshToken, "refresh") + }) + + t.Run("UpdateUserLink", func(t *testing.T) { + t.Parallel() + db, crypt, cipher := setup(t) + initCipher(t, cipher) + link := dbgen.UserLink(t, crypt, database.UserLink{}) + _, err := crypt.UpdateUserLink(ctx, database.UpdateUserLinkParams{ + OAuthAccessToken: "access", + OAuthRefreshToken: "refresh", + UserID: link.UserID, + LoginType: link.LoginType, + }) + require.NoError(t, err) + link, err = db.GetUserLinkByLinkedID(ctx, link.LinkedID) + require.NoError(t, err) + requireEncryptedEquals(t, cipher, link.OAuthAccessToken, "access") + requireEncryptedEquals(t, cipher, link.OAuthRefreshToken, "refresh") + }) + + t.Run("GetUserLinkByLinkedID", func(t *testing.T) { + t.Parallel() + db, crypt, cipher := setup(t) + initCipher(t, cipher) + link := dbgen.UserLink(t, crypt, database.UserLink{ + OAuthAccessToken: "access", + OAuthRefreshToken: "refresh", + }) + link, err := db.GetUserLinkByLinkedID(ctx, link.LinkedID) + require.NoError(t, err) + requireEncryptedEquals(t, cipher, link.OAuthAccessToken, "access") + requireEncryptedEquals(t, cipher, link.OAuthRefreshToken, "refresh") + + // Reset the key and empty values should be returned! + initCipher(t, cipher) + + link, err = crypt.GetUserLinkByLinkedID(ctx, link.LinkedID) + require.ErrorIs(t, err, dbcrypt.ErrInvalidCipher) + }) + + t.Run("GetUserLinkByUserIDLoginType", func(t *testing.T) { + t.Parallel() + db, crypt, cipher := setup(t) + initCipher(t, cipher) + link := dbgen.UserLink(t, crypt, database.UserLink{ + OAuthAccessToken: "access", + OAuthRefreshToken: "refresh", + }) + link, err := db.GetUserLinkByUserIDLoginType(ctx, database.GetUserLinkByUserIDLoginTypeParams{ + UserID: link.UserID, + LoginType: link.LoginType, + }) + require.NoError(t, err) + requireEncryptedEquals(t, cipher, link.OAuthAccessToken, "access") + requireEncryptedEquals(t, cipher, link.OAuthRefreshToken, "refresh") + + // Reset the key and empty values should be returned! + initCipher(t, cipher) + + link, err = crypt.GetUserLinkByUserIDLoginType(ctx, database.GetUserLinkByUserIDLoginTypeParams{ + UserID: link.UserID, + LoginType: link.LoginType, + }) + require.ErrorIs(t, err, dbcrypt.ErrInvalidCipher) + }) +} + +func TestGitAuthLinks(t *testing.T) { + t.Parallel() + ctx := context.Background() + + t.Run("InsertGitAuthLink", func(t *testing.T) { + t.Parallel() + db, crypt, cipher := setup(t) + initCipher(t, cipher) + link := dbgen.GitAuthLink(t, crypt, database.GitAuthLink{ + OAuthAccessToken: "access", + OAuthRefreshToken: "refresh", + }) + link, err := db.GetGitAuthLink(ctx, database.GetGitAuthLinkParams{ + ProviderID: link.ProviderID, + UserID: link.UserID, + }) + require.NoError(t, err) + requireEncryptedEquals(t, cipher, link.OAuthAccessToken, "access") + requireEncryptedEquals(t, cipher, link.OAuthRefreshToken, "refresh") + }) + + t.Run("UpdateGitAuthLink", func(t *testing.T) { + t.Parallel() + db, crypt, cipher := setup(t) + initCipher(t, cipher) + link := dbgen.GitAuthLink(t, crypt, database.GitAuthLink{}) + _, err := crypt.UpdateGitAuthLink(ctx, database.UpdateGitAuthLinkParams{ + ProviderID: link.ProviderID, + UserID: link.UserID, + OAuthAccessToken: "access", + OAuthRefreshToken: "refresh", + }) + require.NoError(t, err) + link, err = db.GetGitAuthLink(ctx, database.GetGitAuthLinkParams{ + ProviderID: link.ProviderID, + UserID: link.UserID, + }) + require.NoError(t, err) + requireEncryptedEquals(t, cipher, link.OAuthAccessToken, "access") + requireEncryptedEquals(t, cipher, link.OAuthRefreshToken, "refresh") + }) + + t.Run("GetGitAuthLink", func(t *testing.T) { + t.Parallel() + db, crypt, cipher := setup(t) + initCipher(t, cipher) + link := dbgen.GitAuthLink(t, crypt, database.GitAuthLink{ + OAuthAccessToken: "access", + OAuthRefreshToken: "refresh", + }) + link, err := db.GetGitAuthLink(ctx, database.GetGitAuthLinkParams{ + UserID: link.UserID, + ProviderID: link.ProviderID, + }) + require.NoError(t, err) + requireEncryptedEquals(t, cipher, link.OAuthAccessToken, "access") + requireEncryptedEquals(t, cipher, link.OAuthRefreshToken, "refresh") + + // Reset the key and empty values should be returned! + initCipher(t, cipher) + + link, err = crypt.GetGitAuthLink(ctx, database.GetGitAuthLinkParams{ + UserID: link.UserID, + ProviderID: link.ProviderID, + }) + require.ErrorIs(t, err, dbcrypt.ErrInvalidCipher) + }) +} + +func requireEncryptedEquals(t *testing.T, cipher *atomic.Pointer[cryptorand.Cipher], value, expected string) { + t.Helper() + c := (*cipher.Load()) + got, err := c.Decrypt([]byte(value[len(dbcrypt.MagicPrefix):])) + require.NoError(t, err) + require.Equal(t, expected, string(got)) +} + +func initCipher(t *testing.T, cipher *atomic.Pointer[cryptorand.Cipher]) { + t.Helper() + key := make([]byte, 32) // AES-256 key size is 32 bytes + _, err := io.ReadFull(rand.Reader, key) + require.NoError(t, err) + c, err := cryptorand.CipherAES256(key) + require.NoError(t, err) + cipher.Store(&c) +} + +func setup(t *testing.T) (db, cryptodb database.Store, cipher *atomic.Pointer[cryptorand.Cipher]) { + t.Helper() + rawDB := dbfake.New() + cipher = &atomic.Pointer[cryptorand.Cipher]{} + return rawDB, dbcrypt.New(rawDB, &dbcrypt.Options{ + ExternalTokenCipher: cipher, + }), cipher +} diff --git a/coderd/database/dbgen/generator.go b/coderd/database/dbgen/generator.go index b53a749aa0f49..9d735698f3889 100644 --- a/coderd/database/dbgen/generator.go +++ b/coderd/database/dbgen/generator.go @@ -385,7 +385,7 @@ func UserLink(t testing.TB, db database.Store, orig database.UserLink) database. LoginType: takeFirst(orig.LoginType, database.LoginTypeGithub), LinkedID: takeFirst(orig.LinkedID), OAuthAccessToken: takeFirst(orig.OAuthAccessToken, uuid.NewString()), - OAuthRefreshToken: takeFirst(orig.OAuthAccessToken, uuid.NewString()), + OAuthRefreshToken: takeFirst(orig.OAuthRefreshToken, uuid.NewString()), OAuthExpiry: takeFirst(orig.OAuthExpiry, database.Now().Add(time.Hour*24)), }) @@ -398,7 +398,7 @@ func GitAuthLink(t testing.TB, db database.Store, orig database.GitAuthLink) dat ProviderID: takeFirst(orig.ProviderID, uuid.New().String()), UserID: takeFirst(orig.UserID, uuid.New()), OAuthAccessToken: takeFirst(orig.OAuthAccessToken, uuid.NewString()), - OAuthRefreshToken: takeFirst(orig.OAuthAccessToken, uuid.NewString()), + OAuthRefreshToken: takeFirst(orig.OAuthRefreshToken, uuid.NewString()), OAuthExpiry: takeFirst(orig.OAuthExpiry, database.Now().Add(time.Hour*24)), CreatedAt: takeFirst(orig.CreatedAt, database.Now()), UpdatedAt: takeFirst(orig.UpdatedAt, database.Now()), diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 84c295aefee7f..b9b7e527cd49e 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -46,6 +46,7 @@ const ( FeatureAppearance FeatureName = "appearance" FeatureAdvancedTemplateScheduling FeatureName = "advanced_template_scheduling" FeatureWorkspaceProxy FeatureName = "workspace_proxy" + FeatureExternalTokenEncryption FeatureName = "external_token_encryption" ) // FeatureNames must be kept in-sync with the Feature enum above. @@ -61,6 +62,7 @@ var FeatureNames = []FeatureName{ FeatureAppearance, FeatureAdvancedTemplateScheduling, FeatureWorkspaceProxy, + FeatureExternalTokenEncryption, } // Humanize returns the feature name in a human-readable format. @@ -146,6 +148,7 @@ type DeploymentValues struct { AgentFallbackTroubleshootingURL clibase.URL `json:"agent_fallback_troubleshooting_url,omitempty" typescript:",notnull"` BrowserOnly clibase.Bool `json:"browser_only,omitempty" typescript:",notnull"` SCIMAPIKey clibase.String `json:"scim_api_key,omitempty" typescript:",notnull"` + ExternalTokenEncryptionKey clibase.String `json:"external_token_encryption_key"` Provisioner ProvisionerConfig `json:"provisioner,omitempty" typescript:",notnull"` RateLimit RateLimitConfig `json:"rate_limit,omitempty" typescript:",notnull"` Experiments clibase.StringArray `json:"experiments,omitempty" typescript:",notnull"` @@ -1374,7 +1377,14 @@ when required by your organization's security policy.`, Annotations: clibase.Annotations{}.Mark(annotationEnterpriseKey, "true").Mark(annotationSecretKey, "true"), Value: &c.SCIMAPIKey, }, - + { + Name: "External Token Encryption Key", + Description: "Encrypt OIDC and Git authentication tokens with AES-256-GCM in the database. The value must be a base64-encoded key.", + Flag: "external-token-encryption-key", + Env: "CODER_EXTERNAL_TOKEN_ENCRYPTION_KEY", + Annotations: clibase.Annotations{}.Mark(annotationEnterpriseKey, "true").Mark(annotationSecretKey, "true"), + Value: &c.ExternalTokenEncryptionKey, + }, { Name: "Disable Path Apps", Description: "Disable workspace apps that are not served from subdomains. Path-based apps can make requests to the Coder API and pose a security risk when the workspace serves malicious JavaScript. This is recommended for security purposes if a --wildcard-access-url is configured.", diff --git a/cryptorand/cipher.go b/cryptorand/cipher.go new file mode 100644 index 0000000000000..4be6c1917d0d3 --- /dev/null +++ b/cryptorand/cipher.go @@ -0,0 +1,48 @@ +package cryptorand + +import ( + "crypto/aes" + "crypto/cipher" + "crypto/rand" + "io" + + "golang.org/x/xerrors" +) + +type Cipher interface { + Encrypt([]byte) ([]byte, error) + Decrypt([]byte) ([]byte, error) +} + +// CipherAES256 returns a new AES-256 cipher. +func CipherAES256(key []byte) (Cipher, error) { + block, err := aes.NewCipher(key) + if err != nil { + return nil, err + } + aead, err := cipher.NewGCM(block) + if err != nil { + return nil, err + } + return &aes256{aead}, nil +} + +type aes256 struct { + aead cipher.AEAD +} + +func (a *aes256) Encrypt(plaintext []byte) ([]byte, error) { + nonce := make([]byte, a.aead.NonceSize()) + _, err := io.ReadFull(rand.Reader, nonce) + if err != nil { + return nil, err + } + return a.aead.Seal(nonce, nonce, plaintext, nil), nil +} + +func (a *aes256) Decrypt(ciphertext []byte) ([]byte, error) { + if len(ciphertext) < a.aead.NonceSize() { + return nil, xerrors.Errorf("ciphertext too short") + } + return a.aead.Open(nil, ciphertext[:a.aead.NonceSize()], ciphertext[a.aead.NonceSize():], nil) +} diff --git a/cryptorand/cipher_test.go b/cryptorand/cipher_test.go new file mode 100644 index 0000000000000..ff0943dc4f7e7 --- /dev/null +++ b/cryptorand/cipher_test.go @@ -0,0 +1,30 @@ +package cryptorand_test + +import ( + "bytes" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/cryptorand" +) + +func TestCipherAES256(t *testing.T) { + t.Parallel() + key := bytes.Repeat([]byte{'a'}, 32) + cipher, err := cryptorand.CipherAES256(key) + require.NoError(t, err) + + output, err := cipher.Encrypt([]byte("hello world")) + require.NoError(t, err) + + response, err := cipher.Decrypt(output) + require.NoError(t, err) + require.Equal(t, "hello world", string(response)) + + t.Run("InvalidInput", func(t *testing.T) { + t.Parallel() + _, err := cipher.Decrypt(bytes.Repeat([]byte{'a'}, 100)) + require.NoError(t, err) + }) +} diff --git a/enterprise/cli/server.go b/enterprise/cli/server.go index d26f9e1b64618..5ccb29c1c02e3 100644 --- a/enterprise/cli/server.go +++ b/enterprise/cli/server.go @@ -5,6 +5,7 @@ package cli import ( "context" "database/sql" + "encoding/base64" "errors" "io" "net/url" @@ -66,6 +67,17 @@ func (r *RootCmd) server() *clibase.Cmd { Options: options, } + if options.DeploymentValues.ExternalTokenEncryptionKey.Value() != "" { + key, err := base64.StdEncoding.DecodeString(options.DeploymentValues.ExternalTokenEncryptionKey.String()) + if err != nil { + return nil, nil, xerrors.Errorf("decode external-token-encryption-key: %w", err) + } + o.ExternalTokenEncryption, err = cryptorand.CipherAES256(key) + if err != nil { + return nil, nil, xerrors.Errorf("create external-token-encryption-key cipher: %w", err) + } + } + api, err := coderd.New(ctx, o) if err != nil { return nil, nil, err diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index ff44aed60c676..93301bdb4a550 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -7,6 +7,7 @@ import ( "crypto/x509" "net/http" "sync" + "sync/atomic" "time" "golang.org/x/xerrors" @@ -18,11 +19,13 @@ import ( "cdr.dev/slog" "github.com/coder/coder/coderd" agplaudit "github.com/coder/coder/coderd/audit" + "github.com/coder/coder/coderd/database/dbcrypt" "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpmw" "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/coderd/schedule" "github.com/coder/coder/codersdk" + "github.com/coder/coder/cryptorand" "github.com/coder/coder/enterprise/coderd/license" "github.com/coder/coder/enterprise/coderd/proxyhealth" "github.com/coder/coder/enterprise/derpmesh" @@ -39,8 +42,8 @@ func New(ctx context.Context, options *Options) (*API, error) { if options.EntitlementsUpdateInterval == 0 { options.EntitlementsUpdateInterval = 10 * time.Minute } - if options.Keys == nil { - options.Keys = Keys + if options.LicenseKeys == nil { + options.LicenseKeys = Keys } if options.Options == nil { options.Options = &coderd.Options{} @@ -52,9 +55,16 @@ func New(ctx context.Context, options *Options) (*API, error) { options.Options.Authorizer = rbac.NewCachingAuthorizer(options.PrometheusRegistry) } ctx, cancelFunc := context.WithCancel(ctx) + + externalTokenCipher := &atomic.Pointer[cryptorand.Cipher]{} + options.Database = dbcrypt.New(options.Database, &dbcrypt.Options{ + ExternalTokenCipher: externalTokenCipher, + }) + api := &API{ - ctx: ctx, - cancel: cancelFunc, + ctx: ctx, + cancel: cancelFunc, + externalTokenCipher: externalTokenCipher, AGPL: coderd.New(options.Options), Options: options, @@ -274,8 +284,9 @@ type Options struct { RBAC bool AuditLogging bool // Whether to block non-browser connections. - BrowserOnly bool - SCIMAPIKey []byte + BrowserOnly bool + SCIMAPIKey []byte + ExternalTokenEncryption cryptorand.Cipher // Used for high availability. DERPServerRelayAddress string @@ -283,7 +294,7 @@ type Options struct { EntitlementsUpdateInterval time.Duration ProxyHealthInterval time.Duration - Keys map[string]ed25519.PublicKey + LicenseKeys map[string]ed25519.PublicKey } type API struct { @@ -295,6 +306,8 @@ type API struct { ctx context.Context cancel context.CancelFunc + externalTokenCipher *atomic.Pointer[cryptorand.Cipher] + // Detects multiple Coder replicas running at the same time. replicaManager *replicasync.Manager // Meshes DERP connections from multiple replicas. @@ -319,13 +332,14 @@ func (api *API) updateEntitlements(ctx context.Context) error { entitlements, err := license.Entitlements( ctx, api.Database, - api.Logger, len(api.replicaManager.All()), len(api.GitAuthConfigs), api.Keys, map[codersdk.FeatureName]bool{ + api.Logger, len(api.replicaManager.All()), len(api.GitAuthConfigs), api.LicenseKeys, map[codersdk.FeatureName]bool{ codersdk.FeatureAuditLog: api.AuditLogging, codersdk.FeatureBrowserOnly: api.BrowserOnly, codersdk.FeatureSCIM: len(api.SCIMAPIKey) != 0, codersdk.FeatureHighAvailability: api.DERPServerRelayAddress != "", codersdk.FeatureMultipleGitAuth: len(api.GitAuthConfigs) > 1, codersdk.FeatureTemplateRBAC: api.RBAC, + codersdk.FeatureExternalTokenEncryption: api.ExternalTokenEncryption != nil, codersdk.FeatureExternalProvisionerDaemons: true, codersdk.FeatureAdvancedTemplateScheduling: true, codersdk.FeatureWorkspaceProxy: true, @@ -396,6 +410,14 @@ func (api *API) updateEntitlements(ctx context.Context) error { } } + if changed, enabled := featureChanged(codersdk.FeatureExternalTokenEncryption); changed { + if enabled { + api.externalTokenCipher.Store(&api.ExternalTokenEncryption) + } else { + api.externalTokenCipher.Store(nil) + } + } + if changed, enabled := featureChanged(codersdk.FeatureHighAvailability); changed { coordinator := agpltailnet.NewCoordinator(api.Logger) if enabled { diff --git a/enterprise/coderd/coderdenttest/coderdenttest.go b/enterprise/coderd/coderdenttest/coderdenttest.go index 40f819ea67507..3026230507b99 100644 --- a/enterprise/coderd/coderdenttest/coderdenttest.go +++ b/enterprise/coderd/coderdenttest/coderdenttest.go @@ -74,7 +74,7 @@ func NewWithAPI(t *testing.T, options *Options) (*codersdk.Client, io.Closer, *c DERPServerRegionID: oop.DERPMap.RegionIDs()[0], Options: oop, EntitlementsUpdateInterval: options.EntitlementsUpdateInterval, - Keys: Keys, + LicenseKeys: Keys, ProxyHealthInterval: options.ProxyHealthInterval, }) assert.NoError(t, err) diff --git a/enterprise/coderd/licenses.go b/enterprise/coderd/licenses.go index 24085ee9a7bea..7ca65667cfc2a 100644 --- a/enterprise/coderd/licenses.go +++ b/enterprise/coderd/licenses.go @@ -82,7 +82,7 @@ func (api *API) postLicense(rw http.ResponseWriter, r *http.Request) { return } - rawClaims, err := license.ParseRaw(addLicense.License, api.Keys) + rawClaims, err := license.ParseRaw(addLicense.License, api.LicenseKeys) if err != nil { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Invalid license", @@ -100,7 +100,7 @@ func (api *API) postLicense(rw http.ResponseWriter, r *http.Request) { } expTime := time.Unix(int64(exp), 0) - claims, err := license.ParseClaims(addLicense.License, api.Keys) + claims, err := license.ParseClaims(addLicense.License, api.LicenseKeys) if err != nil { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Invalid license", From b9251fd68acf00c2aca3426a927f08514f50d829 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Sun, 11 Jun 2023 17:41:45 +0000 Subject: [PATCH 2/4] Fix dbcrypt --- coderd/database/dbauthz/querier.go | 13 +++++++++ coderd/database/dbcrypt/dbcrypt.go | 36 ++++++++++++++++++------- coderd/database/dbcrypt/dbcrypt_test.go | 7 ++--- coderd/database/dbfake/databasefake.go | 33 +++++++++++++++++++++++ coderd/database/dbmock/store.go | 28 +++++++++++++++++++ coderd/database/querier.go | 2 ++ coderd/database/queries.sql.go | 26 ++++++++++++++++++ coderd/database/queries/gitauth.sql | 3 +++ coderd/database/queries/user_links.sql | 6 +++++ site/src/api/typesGenerated.ts | 3 +++ 10 files changed, 144 insertions(+), 13 deletions(-) diff --git a/coderd/database/dbauthz/querier.go b/coderd/database/dbauthz/querier.go index 1f3a5b81663b9..1d869c1eb31ab 100644 --- a/coderd/database/dbauthz/querier.go +++ b/coderd/database/dbauthz/querier.go @@ -1113,6 +1113,10 @@ func (q *querier) InsertUserLink(ctx context.Context, arg database.InsertUserLin return q.db.InsertUserLink(ctx, arg) } +func (q *querier) DeleteUserLinkByLinkedID(ctx context.Context, linkedID string) error { + return deleteQ(q.log, q.auth, q.db.GetUserLinkByLinkedID, q.db.DeleteUserLinkByLinkedID)(ctx, linkedID) +} + func (q *querier) SoftDeleteUserByID(ctx context.Context, id uuid.UUID) error { deleteF := func(ctx context.Context, id uuid.UUID) error { return q.db.UpdateUserDeletedByID(ctx, database.UpdateUserDeletedByIDParams{ @@ -1201,6 +1205,15 @@ func (q *querier) GetGitAuthLink(ctx context.Context, arg database.GetGitAuthLin return fetch(q.log, q.auth, q.db.GetGitAuthLink)(ctx, arg) } +func (q *querier) DeleteGitAuthLink(ctx context.Context, arg database.DeleteGitAuthLinkParams) error { + return deleteQ(q.log, q.auth, func(ctx context.Context, arg database.DeleteGitAuthLinkParams) (database.GitAuthLink, error) { + return q.db.GetGitAuthLink(ctx, database.GetGitAuthLinkParams{ + ProviderID: arg.ProviderID, + UserID: arg.UserID, + }) + }, q.db.DeleteGitAuthLink)(ctx, arg) +} + func (q *querier) InsertGitAuthLink(ctx context.Context, arg database.InsertGitAuthLinkParams) (database.GitAuthLink, error) { return insert(q.log, q.auth, rbac.ResourceUserData.WithOwner(arg.UserID.String()).WithID(arg.UserID), q.db.InsertGitAuthLink)(ctx, arg) } diff --git a/coderd/database/dbcrypt/dbcrypt.go b/coderd/database/dbcrypt/dbcrypt.go index e579903288b87..ffdca87c2b8b9 100644 --- a/coderd/database/dbcrypt/dbcrypt.go +++ b/coderd/database/dbcrypt/dbcrypt.go @@ -17,10 +17,6 @@ import ( // If it is encrypted but a key is not provided, an error is returned. const MagicPrefix = "dbcrypt-" -// ErrInvalidCipher is returned when an invalid cipher is provided -// for the encrypted data. -var ErrInvalidCipher = xerrors.New("an invalid encryption cipher was provided for the encrypted data") - type Options struct { // ExternalTokenCipher is an optional cipher that is used // to encrypt/decrypt user link and git auth link tokens. If this is nil, @@ -56,7 +52,9 @@ func (db *dbCrypt) GetUserLinkByLinkedID(ctx context.Context, linkedID string) ( if err != nil { return database.UserLink{}, err } - return link, db.decryptFields(&link.OAuthAccessToken, &link.OAuthRefreshToken) + return link, db.decryptFields(func() error { + return db.Store.DeleteUserLinkByLinkedID(ctx, linkedID) + }, &link.OAuthAccessToken, &link.OAuthRefreshToken) } func (db *dbCrypt) GetUserLinkByUserIDLoginType(ctx context.Context, params database.GetUserLinkByUserIDLoginTypeParams) (database.UserLink, error) { @@ -64,7 +62,9 @@ func (db *dbCrypt) GetUserLinkByUserIDLoginType(ctx context.Context, params data if err != nil { return database.UserLink{}, err } - return link, db.decryptFields(&link.OAuthAccessToken, &link.OAuthRefreshToken) + return link, db.decryptFields(func() error { + return db.Store.DeleteUserLinkByLinkedID(ctx, link.LinkedID) + }, &link.OAuthAccessToken, &link.OAuthRefreshToken) } func (db *dbCrypt) InsertUserLink(ctx context.Context, params database.InsertUserLinkParams) (database.UserLink, error) { @@ -96,7 +96,12 @@ func (db *dbCrypt) GetGitAuthLink(ctx context.Context, params database.GetGitAut if err != nil { return database.GitAuthLink{}, err } - return link, db.decryptFields(&link.OAuthAccessToken, &link.OAuthRefreshToken) + return link, db.decryptFields(func() error { + return db.Store.DeleteGitAuthLink(ctx, database.DeleteGitAuthLinkParams{ + ProviderID: params.ProviderID, + UserID: params.UserID, + }) + }, &link.OAuthAccessToken, &link.OAuthRefreshToken) } func (db *dbCrypt) UpdateGitAuthLink(ctx context.Context, params database.UpdateGitAuthLinkParams) (database.GitAuthLink, error) { @@ -130,7 +135,15 @@ func (db *dbCrypt) encryptFields(fields ...*string) error { // decryptFields decrypts the given fields in place. // If the value fails to decrypt, sql.ErrNoRows will be returned. -func (db *dbCrypt) decryptFields(fields ...*string) error { +func (db *dbCrypt) decryptFields(deleteFn func() error, fields ...*string) error { + delete := func() error { + err := deleteFn() + if err != nil { + return xerrors.Errorf("delete encrypted row: %w", err) + } + return sql.ErrNoRows + } + cipherPtr := db.ExternalTokenCipher.Load() // If no cipher is loaded, then we don't need to encrypt or decrypt anything! if cipherPtr == nil { @@ -139,7 +152,9 @@ func (db *dbCrypt) decryptFields(fields ...*string) error { continue } if strings.HasPrefix(*field, MagicPrefix) { - return ErrInvalidCipher + // If we have a magic prefix but encryption is disabled, + // we should delete the row. + return delete() } } return nil @@ -156,7 +171,8 @@ func (db *dbCrypt) decryptFields(fields ...*string) error { decrypted, err := cipher.Decrypt([]byte((*field)[len(MagicPrefix):])) if err != nil { - return xerrors.Errorf("%w: %s", ErrInvalidCipher, err) + // If the encryption key changed, we should delete the row. + return delete() } *field = string(decrypted) } diff --git a/coderd/database/dbcrypt/dbcrypt_test.go b/coderd/database/dbcrypt/dbcrypt_test.go index 8529f457bb1cb..fe7aaef7270f9 100644 --- a/coderd/database/dbcrypt/dbcrypt_test.go +++ b/coderd/database/dbcrypt/dbcrypt_test.go @@ -3,6 +3,7 @@ package dbcrypt_test import ( "context" "crypto/rand" + "database/sql" "io" "sync/atomic" "testing" @@ -69,7 +70,7 @@ func TestUserLinks(t *testing.T) { initCipher(t, cipher) link, err = crypt.GetUserLinkByLinkedID(ctx, link.LinkedID) - require.ErrorIs(t, err, dbcrypt.ErrInvalidCipher) + require.ErrorIs(t, err, sql.ErrNoRows) }) t.Run("GetUserLinkByUserIDLoginType", func(t *testing.T) { @@ -95,7 +96,7 @@ func TestUserLinks(t *testing.T) { UserID: link.UserID, LoginType: link.LoginType, }) - require.ErrorIs(t, err, dbcrypt.ErrInvalidCipher) + require.ErrorIs(t, err, sql.ErrNoRows) }) } @@ -164,7 +165,7 @@ func TestGitAuthLinks(t *testing.T) { UserID: link.UserID, ProviderID: link.ProviderID, }) - require.ErrorIs(t, err, dbcrypt.ErrInvalidCipher) + require.ErrorIs(t, err, sql.ErrNoRows) }) } diff --git a/coderd/database/dbfake/databasefake.go b/coderd/database/dbfake/databasefake.go index 41800dbcd9e74..d7689d5030b24 100644 --- a/coderd/database/dbfake/databasefake.go +++ b/coderd/database/dbfake/databasefake.go @@ -4380,6 +4380,24 @@ func (q *fakeQuerier) UpdateGroupByID(_ context.Context, arg database.UpdateGrou return database.Group{}, sql.ErrNoRows } +func (q *fakeQuerier) DeleteGitAuthLink(_ context.Context, arg database.DeleteGitAuthLinkParams) error { + q.mutex.Lock() + defer q.mutex.Unlock() + + for index, link := range q.gitAuthLinks { + if link.ProviderID != arg.ProviderID { + continue + } + if link.UserID != arg.UserID { + continue + } + q.gitAuthLinks[index] = q.gitAuthLinks[len(q.gitAuthLinks)-1] + q.gitAuthLinks = q.gitAuthLinks[:len(q.gitAuthLinks)-1] + return nil + } + return sql.ErrNoRows +} + func (q *fakeQuerier) DeleteGitSSHKey(_ context.Context, userID uuid.UUID) error { q.mutex.Lock() defer q.mutex.Unlock() @@ -4911,6 +4929,21 @@ func (q *fakeQuerier) DeleteReplicasUpdatedBefore(_ context.Context, before time return nil } +func (q *fakeQuerier) DeleteUserLinkByLinkedID(ctx context.Context, linkedID string) error { + q.mutex.Lock() + defer q.mutex.Unlock() + + for index, link := range q.userLinks { + if link.LinkedID != linkedID { + continue + } + q.userLinks[index] = q.userLinks[len(q.userLinks)-1] + q.userLinks = q.userLinks[:len(q.userLinks)-1] + return nil + } + return sql.ErrNoRows +} + func (q *fakeQuerier) InsertReplica(_ context.Context, arg database.InsertReplicaParams) (database.Replica, error) { if err := validateDatabaseType(arg); err != nil { return database.Replica{}, err diff --git a/coderd/database/dbmock/store.go b/coderd/database/dbmock/store.go index 5fe83e6f514d6..09e672f9d911c 100644 --- a/coderd/database/dbmock/store.go +++ b/coderd/database/dbmock/store.go @@ -110,6 +110,20 @@ func (mr *MockStoreMockRecorder) DeleteApplicationConnectAPIKeysByUserID(arg0, a return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteApplicationConnectAPIKeysByUserID", reflect.TypeOf((*MockStore)(nil).DeleteApplicationConnectAPIKeysByUserID), arg0, arg1) } +// DeleteGitAuthLink mocks base method. +func (m *MockStore) DeleteGitAuthLink(arg0 context.Context, arg1 database.DeleteGitAuthLinkParams) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteGitAuthLink", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeleteGitAuthLink indicates an expected call of DeleteGitAuthLink. +func (mr *MockStoreMockRecorder) DeleteGitAuthLink(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteGitAuthLink", reflect.TypeOf((*MockStore)(nil).DeleteGitAuthLink), arg0, arg1) +} + // DeleteGitSSHKey mocks base method. func (m *MockStore) DeleteGitSSHKey(arg0 context.Context, arg1 uuid.UUID) error { m.ctrl.T.Helper() @@ -237,6 +251,20 @@ func (mr *MockStoreMockRecorder) DeleteReplicasUpdatedBefore(arg0, arg1 interfac return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteReplicasUpdatedBefore", reflect.TypeOf((*MockStore)(nil).DeleteReplicasUpdatedBefore), arg0, arg1) } +// DeleteUserLinkByLinkedID mocks base method. +func (m *MockStore) DeleteUserLinkByLinkedID(arg0 context.Context, arg1 string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteUserLinkByLinkedID", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeleteUserLinkByLinkedID indicates an expected call of DeleteUserLinkByLinkedID. +func (mr *MockStoreMockRecorder) DeleteUserLinkByLinkedID(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteUserLinkByLinkedID", reflect.TypeOf((*MockStore)(nil).DeleteUserLinkByLinkedID), arg0, arg1) +} + // GetAPIKeyByID mocks base method. func (m *MockStore) GetAPIKeyByID(arg0 context.Context, arg1 string) (database.APIKey, error) { m.ctrl.T.Helper() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index e30207c7ba44e..17a01dc0e3de4 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -29,6 +29,7 @@ type sqlcQuerier interface { DeleteAPIKeyByID(ctx context.Context, id string) error DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error DeleteApplicationConnectAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error + DeleteGitAuthLink(ctx context.Context, arg DeleteGitAuthLinkParams) error DeleteGitSSHKey(ctx context.Context, userID uuid.UUID) error DeleteGroupByID(ctx context.Context, id uuid.UUID) error DeleteGroupMemberFromGroup(ctx context.Context, arg DeleteGroupMemberFromGroupParams) error @@ -40,6 +41,7 @@ type sqlcQuerier interface { DeleteOldWorkspaceAgentStats(ctx context.Context) error DeleteParameterValueByID(ctx context.Context, id uuid.UUID) error DeleteReplicasUpdatedBefore(ctx context.Context, updatedAt time.Time) error + DeleteUserLinkByLinkedID(ctx context.Context, linkedID string) error GetAPIKeyByID(ctx context.Context, id string) (APIKey, error) // there is no unique constraint on empty token names GetAPIKeyByName(ctx context.Context, arg GetAPIKeyByNameParams) (APIKey, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 9955deb439ffd..c8347b41b49b9 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -792,6 +792,20 @@ func (q *sqlQuerier) InsertFile(ctx context.Context, arg InsertFileParams) (File return i, err } +const deleteGitAuthLink = `-- name: DeleteGitAuthLink :exec +DELETE FROM git_auth_links WHERE provider_id = $1 AND user_id = $2 +` + +type DeleteGitAuthLinkParams struct { + ProviderID string `db:"provider_id" json:"provider_id"` + UserID uuid.UUID `db:"user_id" json:"user_id"` +} + +func (q *sqlQuerier) DeleteGitAuthLink(ctx context.Context, arg DeleteGitAuthLinkParams) error { + _, err := q.db.ExecContext(ctx, deleteGitAuthLink, arg.ProviderID, arg.UserID) + return err +} + const getGitAuthLink = `-- name: GetGitAuthLink :one SELECT provider_id, user_id, created_at, updated_at, oauth_access_token, oauth_refresh_token, oauth_expiry FROM git_auth_links WHERE provider_id = $1 AND user_id = $2 ` @@ -4741,6 +4755,18 @@ func (q *sqlQuerier) InsertTemplateVersionVariable(ctx context.Context, arg Inse return i, err } +const deleteUserLinkByLinkedID = `-- name: DeleteUserLinkByLinkedID :exec +DELETE FROM + user_links +WHERE + linked_id = $1 +` + +func (q *sqlQuerier) DeleteUserLinkByLinkedID(ctx context.Context, linkedID string) error { + _, err := q.db.ExecContext(ctx, deleteUserLinkByLinkedID, linkedID) + return err +} + const getUserLinkByLinkedID = `-- name: GetUserLinkByLinkedID :one SELECT user_id, login_type, linked_id, oauth_access_token, oauth_refresh_token, oauth_expiry diff --git a/coderd/database/queries/gitauth.sql b/coderd/database/queries/gitauth.sql index a35de98a08908..c2cd6853f7bcd 100644 --- a/coderd/database/queries/gitauth.sql +++ b/coderd/database/queries/gitauth.sql @@ -27,3 +27,6 @@ UPDATE git_auth_links SET oauth_refresh_token = $5, oauth_expiry = $6 WHERE provider_id = $1 AND user_id = $2 RETURNING *; + +-- name: DeleteGitAuthLink :exec +DELETE FROM git_auth_links WHERE provider_id = $1 AND user_id = $2; diff --git a/coderd/database/queries/user_links.sql b/coderd/database/queries/user_links.sql index 2390cb9782b30..d7e35ae677f19 100644 --- a/coderd/database/queries/user_links.sql +++ b/coderd/database/queries/user_links.sql @@ -44,3 +44,9 @@ SET oauth_expiry = $3 WHERE user_id = $4 AND login_type = $5 RETURNING *; + +-- name: DeleteUserLinkByLinkedID :exec +DELETE FROM + user_links +WHERE + linked_id = $1; diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 768c74f8f640e..3688e0a953364 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -362,6 +362,7 @@ export interface DeploymentValues { readonly agent_fallback_troubleshooting_url?: string readonly browser_only?: boolean readonly scim_api_key?: string + readonly external_token_encryption_key: string readonly provisioner?: ProvisionerConfig readonly rate_limit?: RateLimitConfig // This is likely an enum in an external package ("github.com/coder/coder/cli/clibase.StringArray") @@ -1395,6 +1396,7 @@ export type FeatureName = | "audit_log" | "browser_only" | "external_provisioner_daemons" + | "external_token_encryption" | "high_availability" | "multiple_git_auth" | "scim" @@ -1407,6 +1409,7 @@ export const FeatureNames: FeatureName[] = [ "audit_log", "browser_only", "external_provisioner_daemons", + "external_token_encryption", "high_availability", "multiple_git_auth", "scim", From deb577b6bab23bac01103303d3eeef7860caa23a Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Mon, 12 Jun 2023 03:24:19 +0000 Subject: [PATCH 3/4] Automatically delete rows when not encrypted --- coderd/database/dbcrypt/dbcrypt.go | 28 +++++++++++++++++++------ coderd/database/dbcrypt/dbcrypt_test.go | 8 ++++++- coderd/httpmw/apikey.go | 6 ++++++ enterprise/coderd/coderd.go | 1 + 4 files changed, 36 insertions(+), 7 deletions(-) diff --git a/coderd/database/dbcrypt/dbcrypt.go b/coderd/database/dbcrypt/dbcrypt.go index ffdca87c2b8b9..2d1fd7d8b6096 100644 --- a/coderd/database/dbcrypt/dbcrypt.go +++ b/coderd/database/dbcrypt/dbcrypt.go @@ -3,9 +3,12 @@ package dbcrypt import ( "context" "database/sql" + "encoding/base64" + "runtime" "strings" "sync/atomic" + "cdr.dev/slog" "golang.org/x/xerrors" "github.com/coder/coder/coderd/database" @@ -22,6 +25,7 @@ type Options struct { // to encrypt/decrypt user link and git auth link tokens. If this is nil, // then no encryption/decryption will be performed. ExternalTokenCipher *atomic.Pointer[cryptorand.Cipher] + Logger slog.Logger } // New creates a database.Store wrapper that encrypts/decrypts values @@ -128,7 +132,8 @@ func (db *dbCrypt) encryptFields(fields ...*string) error { if err != nil { return err } - *field = MagicPrefix + string(encrypted) + // Base64 is used to support UTF-8 encoding in PostgreSQL. + *field = MagicPrefix + base64.StdEncoding.EncodeToString(encrypted) } return nil } @@ -136,11 +141,16 @@ func (db *dbCrypt) encryptFields(fields ...*string) error { // decryptFields decrypts the given fields in place. // If the value fails to decrypt, sql.ErrNoRows will be returned. func (db *dbCrypt) decryptFields(deleteFn func() error, fields ...*string) error { - delete := func() error { + delete := func(reason string) error { err := deleteFn() if err != nil { return xerrors.Errorf("delete encrypted row: %w", err) } + pc, _, _, ok := runtime.Caller(2) + details := runtime.FuncForPC(pc) + if ok && details != nil { + db.Logger.Debug(context.Background(), "deleted row", slog.F("reason", reason), slog.F("caller", details.Name())) + } return sql.ErrNoRows } @@ -154,7 +164,7 @@ func (db *dbCrypt) decryptFields(deleteFn func() error, fields ...*string) error if strings.HasPrefix(*field, MagicPrefix) { // If we have a magic prefix but encryption is disabled, // we should delete the row. - return delete() + return delete("encryption disabled") } } return nil @@ -166,13 +176,19 @@ func (db *dbCrypt) decryptFields(deleteFn func() error, fields ...*string) error continue } if len(*field) < len(MagicPrefix) || !strings.HasPrefix(*field, MagicPrefix) { + // We do not force encryption of unencrypted rows. This could be damaging + // to the deployment, and admins can always manually purge data. continue } - - decrypted, err := cipher.Decrypt([]byte((*field)[len(MagicPrefix):])) + data, err := base64.StdEncoding.DecodeString((*field)[len(MagicPrefix):]) + if err != nil { + // If it's not base64 with the prefix, we should delete the row. + return delete("stored value was not base64 encoded") + } + decrypted, err := cipher.Decrypt(data) if err != nil { // If the encryption key changed, we should delete the row. - return delete() + return delete("encryption key changed") } *field = string(decrypted) } diff --git a/coderd/database/dbcrypt/dbcrypt_test.go b/coderd/database/dbcrypt/dbcrypt_test.go index fe7aaef7270f9..8c8a5b14ad8f5 100644 --- a/coderd/database/dbcrypt/dbcrypt_test.go +++ b/coderd/database/dbcrypt/dbcrypt_test.go @@ -4,10 +4,13 @@ import ( "context" "crypto/rand" "database/sql" + "encoding/base64" "io" "sync/atomic" "testing" + "cdr.dev/slog" + "cdr.dev/slog/sloggers/slogtest" "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/database" @@ -172,7 +175,9 @@ func TestGitAuthLinks(t *testing.T) { func requireEncryptedEquals(t *testing.T, cipher *atomic.Pointer[cryptorand.Cipher], value, expected string) { t.Helper() c := (*cipher.Load()) - got, err := c.Decrypt([]byte(value[len(dbcrypt.MagicPrefix):])) + data, err := base64.StdEncoding.DecodeString(value[len(dbcrypt.MagicPrefix):]) + require.NoError(t, err) + got, err := c.Decrypt(data) require.NoError(t, err) require.Equal(t, expected, string(got)) } @@ -193,5 +198,6 @@ func setup(t *testing.T) (db, cryptodb database.Store, cipher *atomic.Pointer[cr cipher = &atomic.Pointer[cryptorand.Cipher]{} return rawDB, dbcrypt.New(rawDB, &dbcrypt.Options{ ExternalTokenCipher: cipher, + Logger: slogtest.Make(t, nil).Leveled(slog.LevelDebug), }), cipher } diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go index 509ce4a82a24a..25d1bf3ab97a7 100644 --- a/coderd/httpmw/apikey.go +++ b/coderd/httpmw/apikey.go @@ -229,6 +229,12 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon UserID: key.UserID, LoginType: key.LoginType, }) + if errors.Is(err, sql.ErrNoRows) { + return optionalWrite(http.StatusUnauthorized, codersdk.Response{ + Message: SignedOutErrorMessage, + Detail: "You must re-authenticate with the login provider.", + }) + } if err != nil { return write(http.StatusInternalServerError, codersdk.Response{ Message: "A database error occurred", diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 93301bdb4a550..00a6a6928d95e 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -59,6 +59,7 @@ func New(ctx context.Context, options *Options) (*API, error) { externalTokenCipher := &atomic.Pointer[cryptorand.Cipher]{} options.Database = dbcrypt.New(options.Database, &dbcrypt.Options{ ExternalTokenCipher: externalTokenCipher, + Logger: options.Logger.Named("dbcrypt"), }) api := &API{ From faa20ad5426b787c11fdb8f12241d3247f7f79d7 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Mon, 12 Jun 2023 16:28:18 +0000 Subject: [PATCH 4/4] gen --- coderd/apidoc/docs.go | 3 +++ coderd/apidoc/swagger.json | 3 +++ docs/api/general.md | 1 + docs/api/schemas.md | 3 +++ docs/cli/server.md | 9 +++++++++ 5 files changed, 19 insertions(+) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index bb2f2b602621c..1c4b1b9c52460 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -7373,6 +7373,9 @@ const docTemplate = `{ "type": "string" } }, + "external_token_encryption_key": { + "type": "string" + }, "git_auth": { "$ref": "#/definitions/clibase.Struct-array_codersdk_GitAuthConfig" }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index ba4da7aa72b43..ad12fd3bb4f86 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -6575,6 +6575,9 @@ "type": "string" } }, + "external_token_encryption_key": { + "type": "string" + }, "git_auth": { "$ref": "#/definitions/clibase.Struct-array_codersdk_GitAuthConfig" }, diff --git a/docs/api/general.md b/docs/api/general.md index b1b713a66bc6f..008d9f853218b 100644 --- a/docs/api/general.md +++ b/docs/api/general.md @@ -196,6 +196,7 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \ "disable_path_apps": true, "disable_session_expiry_refresh": true, "experiments": ["string"], + "external_token_encryption_key": "string", "git_auth": { "value": [ { diff --git a/docs/api/schemas.md b/docs/api/schemas.md index e428c8bc2f9c3..e58e797e61c51 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -1894,6 +1894,7 @@ CreateParameterRequest is a structure used to create a new parameter value for a "disable_path_apps": true, "disable_session_expiry_refresh": true, "experiments": ["string"], + "external_token_encryption_key": "string", "git_auth": { "value": [ { @@ -2239,6 +2240,7 @@ CreateParameterRequest is a structure used to create a new parameter value for a "disable_path_apps": true, "disable_session_expiry_refresh": true, "experiments": ["string"], + "external_token_encryption_key": "string", "git_auth": { "value": [ { @@ -2429,6 +2431,7 @@ CreateParameterRequest is a structure used to create a new parameter value for a | `disable_path_apps` | boolean | false | | | | `disable_session_expiry_refresh` | boolean | false | | | | `experiments` | array of string | false | | | +| `external_token_encryption_key` | string | false | | | | `git_auth` | [clibase.Struct-array_codersdk_GitAuthConfig](#clibasestruct-array_codersdk_gitauthconfig) | false | | | | `http_address` | string | false | | Http address is a string because it may be set to zero to disable. | | `in_memory_database` | boolean | false | | | diff --git a/docs/cli/server.md b/docs/cli/server.md index 3cbcafe9bbc35..095766c389846 100644 --- a/docs/cli/server.md +++ b/docs/cli/server.md @@ -233,6 +233,15 @@ Expose the swagger endpoint via /swagger. Enable one or more experiments. These are not ready for production. Separate multiple experiments with commas, or enter '\*' to opt-in to all available experiments. +### --external-token-encryption-key + +| | | +| ----------- | ------------------------------------------------- | +| Type | string | +| Environment | $CODER_EXTERNAL_TOKEN_ENCRYPTION_KEY | + +Encrypt OIDC and Git authentication tokens with AES-256-GCM in the database. The value must be a base64-encoded key. + ### --provisioner-force-cancel-interval | | |