NotificationsYou must be signed in to change notification settings
Fork1.1k
Star11.4k

feat: add schema for key rotation#14662

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

sreya merged 13 commits intomainfromjon/keyschema

Sep 17, 2024

Merged

feat: add schema for key rotation#14662

sreya merged 13 commits intomainfromjon/keyschema

Sep 17, 2024

Conversation

Copy link

Collaborator

sreya commentedSep 13, 2024•
edited
Loading

This is the first delivery for an implementation for rotating our internal keys. This is mainly just schema related work. I opted forcrypto_keys instead ofkeys since it felt a little too generic and we already have existing key types (such asapi_keys).

Relates tocoder/internal#52

github-actionsbot assignedsreya

Sep 13, 2024

sreya added2 commits

September 13, 2024 00:55

feat: add schema for key rotation

03262de

add some more dbcrypt tests

04de868

sreya force-pushed thejon/keyschema branch fromc793b6f to04de868Compare

September 13, 2024 01:18

sreya added2 commits

September 13, 2024 01:47

add dbauthz tests

561e11f

fixture

c913091

sreya requested review fromdeansheather andspikecurtis

September 13, 2024 02:08

deansheather approved these changes

Sep 13, 2024

View reviewed changes

coderd/database/dbgen/dbgen.go OutdatedShow resolvedHide resolved

coderd/database/queries/crypto_keys.sqlShow resolvedHide resolved

coderd/database/queries/crypto_keys.sql OutdatedShow resolvedHide resolved

spikecurtis reviewed

Sep 13, 2024

View reviewed changes

coderd/database/migrations/000250_crypto_keys.up.sql OutdatedShow resolvedHide resolved

coderd/database/dbauthz/dbauthz.go Outdated

		}

		func (q*querier)DeleteCryptoKey(ctx context.Context,arg database.DeleteCryptoKeyParams) (database.CryptoKey,error) {
		iferr:=q.authorizeContext(ctx,policy.ActionDelete,rbac.ResourceSystem);err!=nil {

Copy link

Contributor

spikecurtisSep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We should have a new object type for CryptoKeys

Copy link

CollaboratorAuthor

sreyaSep 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I went ahead and did this but I'm not sure what adding an object type buys us? I don't believe there's any intent to expose this to users so distinguishing it from other "system resources" doesn't seem to buy us anything.

Copy link

Contributor

spikecurtisSep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

A couple things:

Clarity and understandability. "System resources" is currently a dumping ground of a bunch of misc stuff that doesn't actually make sense together. Let's not make it worse. Crypto keys are a specific distinct resource and our code should reflect that.
Limited permissions scope. You're about to write a new subcomponent that interacts with crypto keys. What permissions should it have? Answer: crypto keys and nothing else. Impossible to do if you lump crypto keys in with a bunch of other stuff.

coderd/database/queries/crypto_keys.sql

		-- name: GetLatestCryptoKeyByFeature :one
		SELECT*
		FROM crypto_keys
		WHERE feature= $1

Copy link

Contributor

spikecurtisSep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We have to be a little careful here.

One reason you might want to get the latest key is that you're looking for the current signing key. In that case you need to excludesecret IS NULL.

Another reason is that you might be inserting a new key. In that case you need to include all keys, even ifsecret is NULL.

Might need 2 queries, unless you're depending onGetCryptoKeys to find the signing key.

Copy link

CollaboratorAuthor

sreyaSep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yeah the plan is to provide an abstraction like we talked that would wrap this and filter out invalid keys so callers don't have to worry about the details.