feat: add schema for key rotation #14662
Conversation
c793b6f to
04de868
Compare
coderd/database/dbauthz/dbauthz.go
Outdated
| @@ -1041,6 +1041,13 @@ func (q *querier) DeleteCoordinator(ctx context.Context, id uuid.UUID) error { | |||
| return q.db.DeleteCoordinator(ctx, id) | |||
| } | |||
|
|
|||
| func (q *querier) DeleteCryptoKey(ctx context.Context, arg database.DeleteCryptoKeyParams) (database.CryptoKey, error) { | |||
| if err := q.authorizeContext(ctx, policy.ActionDelete, rbac.ResourceSystem); err != nil { | |||
There was a problem hiding this comment.
We should have a new object type for CryptoKeys
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| -- name: GetLatestCryptoKeyByFeature :one | ||
| SELECT * | ||
| FROM crypto_keys | ||
| WHERE feature = $1 |
There was a problem hiding this comment.
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 exclude secret IS NULL.
Another reason is that you might be inserting a new key. In that case you need to include all keys, even if secret is NULL.
Might need 2 queries, unless you're depending on GetCryptoKeys to find the signing key.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That may be dumb though, might be better to avoid any "cleverness" at all
This is the first delivery for an implementation for rotating our internal keys. This is mainly just schema related work. I opted for
crypto_keysinstead ofkeyssince it felt a little too generic and we already have existing key types (such asapi_keys).Relates to coder/internal#52