feat: encrypt oidc and git auth tokens in the database #7959

kylecarbs · 2023-06-12T03:25:56Z

No description provided.

matifali · 2023-06-12T08:37:36Z

does this resolve #7640?

kylecarbs · 2023-06-12T14:19:05Z

Yup!

deansheather

This doesn't seem to support upgrading non-encrypted secrets to encrypted on startup, and it doesn't support rotating the encryption key.

deansheather · 2023-06-18T19:03:34Z

cryptorand/cipher.go

@@ -0,0 +1,48 @@
+package cryptorand


IMO this doesn't belong in cryptorand package because it's not random, it should go in the dbcrypt package or it's own package.

Agreed. Will move

deansheather · 2023-06-18T19:04:09Z

cryptorand/cipher.go

+
+// CipherAES256 returns a new AES-256 cipher.
+func CipherAES256(key []byte) (Cipher, error) {
+	block, err := aes.NewCipher(key)


the key should be verified to ensure it's 32 bytes for AES-256

aes.NewCipher throws an error if it's not

aes.NewCipher accepts smaller key sizes for less secure AES variants, we should just enforce 32 bytes for AES-256 which is the most secure algo supported

Ahh, good catch

deansheather · 2023-06-18T19:05:19Z

cryptorand/cipher.go

+	if err != nil {
+		return nil, err
+	}
+	return a.aead.Seal(nonce, nonce, plaintext, nil), nil


This should write to a new buffer instead of reusing the nonce buffer, and the nonce should be copied

deansheather · 2023-06-18T19:09:31Z

cryptorand/cipher_test.go

+	"github.com/coder/coder/cryptorand"
+)
+
+func TestCipherAES256(t *testing.T) {


Should add a test to ensure a unique nonce is being generated each time we encrypt something new, and another to ensure that changing the nonce causes decryption to fail

Should add another test to ensure that the format doesn't change in a backwards incompatible way. Hardcode a key, encrypt something once and then hardcode the encrypted result to ensure it gets decrypted successfully and matches the same input string.

deansheather · 2023-06-18T19:13:39Z

codersdk/deployment.go

-
+		{
+			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.",


This should mention that it needs to be 32 bytes.

And should provide a bash one liner to generate a secure random key with e.g. openssl or just plain /dev/random

deansheather · 2023-06-18T19:13:58Z

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) {


Suggested change

if errors.Is(err, sql.ErrNoRows) {

if xerrors.Is(err, sql.ErrNoRows) {

deansheather · 2023-06-18T19:14:33Z

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;


Suggested change

DELETE FROM git_auth_links WHERE provider_id = $1 AND user_id = $2;

DELETE FROM

git_auth_links

WHERE

provider_id = $1 AND

user_id = $2;

deansheather · 2023-06-18T21:16:27Z

coderd/database/dbcrypt/dbcrypt.go

+				// If we have a magic prefix but encryption is disabled,
+				// we should delete the row.
+				return delete("encryption disabled")


What happens when the key is accidentally not specified during coder startup? All rows will start being deleted when they're queried? This seems like a really bad idea

All data encrypted at rest is intentionally temporary. We will not do this for any data that must persist.

This should be written in a very prominent comment at the top of the file or something then, because I can definitely see this being used for e.g. gitauth by another engineer who doesn't know about this limitation

Agreed will add

All data encrypted at rest is intentionally temporary. We will not do this for any data that must persist.

What about ssh keys that should use encryption at rest ?

@Adphi that isn't in this PR yet, but I'm not sure what we should do in that case. Maybe stop the server from starting with a warning that is dismissable?

If I may, perhaps when the encryption key is wrong, the program should crash loudly to preserve the integrity of the secrets.
And maybe add a command line flag or environment variable to allow all encrypted data to be erased, recovery style.

This commit builds upon the previous work in #7959: - Moved dbcrypt package to enterprise/dbcrypt - Modified original dbcrypt behaviour to not delete un-decryptable rows. - Added a table dbcrypt_sentinel used to determine database encryption status NOTE: This is part 1 of a 2-part PR. This PR focuses mainly on the dbcrypt and database packages. A separate PR will add the required plumbing to integrate this into enterprise/coderd properly. Co-authored-by: Kyle Carberry <kyle@coder.com>

This commit builds upon the previous work in #7959: - Moved dbcrypt package to enterprise/dbcrypt - Modified original dbcrypt behaviour to not delete un-decryptable rows. - Added a table dbcrypt_sentinel used to determine database encryption status. - Added support for multiple encryption keys in dbcrypt. NOTE: This is part 1 of a 2-part PR. This PR focuses mainly on the dbcrypt and database packages. A separate PR will add the required plumbing to integrate this into enterprise/coderd properly. Co-authored-by: Kyle Carberry <kyle@coder.com>

kylecarbs added 3 commits May 30, 2023 16:26

feat: encrypt oidc and git auth tokens in the database

6651fe1

Fix dbcrypt

b9251fd

Automatically delete rows when not encrypted

deb577b

kylecarbs self-assigned this Jun 12, 2023

kylecarbs added 3 commits June 12, 2023 16:28

gen

faa20ad

Merge branch 'main' into dbcrypt

2e19360

Merge branch 'main' into dbcrypt

614065b

kylecarbs marked this pull request as ready for review June 18, 2023 19:01

kylecarbs requested a review from deansheather June 18, 2023 19:01

deansheather reviewed Jun 18, 2023

View reviewed changes

github-actions bot added stale This issue is like stale bread. and removed stale This issue is like stale bread. labels Jun 30, 2023

github-actions bot added the stale This issue is like stale bread. label Jul 14, 2023

github-actions bot closed this Jul 17, 2023

johnstcn mentioned this pull request Aug 21, 2023

Encrypt OIDC access token in database #7640

Closed

johnstcn mentioned this pull request Aug 29, 2023

[WIP] feat(enterprise): encrypt external access tokens (oidc, git auth) in the database #9339

Closed

6 tasks

johnstcn mentioned this pull request Aug 30, 2023

feat(coderd): add dbcrypt package #9421

Closed

github-actions bot deleted the dbcrypt branch December 19, 2023 00:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: encrypt oidc and git auth tokens in the database #7959

feat: encrypt oidc and git auth tokens in the database #7959

kylecarbs commented Jun 12, 2023

matifali commented Jun 12, 2023

kylecarbs commented Jun 12, 2023

deansheather left a comment

deansheather Jun 18, 2023

kylecarbs Jun 18, 2023

deansheather Jun 18, 2023

kylecarbs Jun 18, 2023

deansheather Jun 18, 2023

kylecarbs Jun 18, 2023

deansheather Jun 18, 2023

deansheather Jun 18, 2023

deansheather Jun 19, 2023

deansheather Jun 18, 2023 •

edited

Loading

deansheather Jun 19, 2023

deansheather Jun 18, 2023

deansheather Jun 18, 2023

deansheather Jun 18, 2023

kylecarbs Jun 19, 2023

deansheather Jun 19, 2023

kylecarbs Jun 19, 2023

Adphi Jun 22, 2023

kylecarbs Jul 2, 2023

Adphi Jul 2, 2023

kylecarbs Jul 2, 2023

	if errors.Is(err, sql.ErrNoRows) {
	if xerrors.Is(err, sql.ErrNoRows) {

feat: encrypt oidc and git auth tokens in the database #7959

feat: encrypt oidc and git auth tokens in the database #7959

Conversation

kylecarbs commented Jun 12, 2023

matifali commented Jun 12, 2023

kylecarbs commented Jun 12, 2023

deansheather left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deansheather Jun 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deansheather Jun 18, 2023 •

edited

Loading