add SMS signing keys at the DB layer #1649
Conversation
googlebot
added
the
cla: yes
pkg/database/managed_key.go
Outdated
| // IsActive() returns true if this key is active | ||
| IsActive() bool | ||
|
|
||
| SetRealmID(id uint) |
There was a problem hiding this comment.
I think this function should be a separate interface, since some managed keys will not be realm specific, right? (Like system keys?) Or do you plan to use a realmID == 0 for those cases like we do for audit logs?
There was a problem hiding this comment.
Introduced RealmManagedKey - moved SetRealmID there.
this opens up extending ManagedKey to token signing keys in a follow up.
pkg/database/migrations.go
Outdated
| deleted_at TIMESTAMP WITH TIME ZONE, | ||
| realm_id INTEGER, | ||
| key_id TEXT, | ||
| active BOOLEAN, |
There was a problem hiding this comment.
We should create an index that ensures only one value for active can exist in the scope of a realm_id (there's some other tables that have this, it's a conditional index)
There was a problem hiding this comment.
please double check my index statement
pkg/database/realm.go
Outdated
| return r.createdManagedSigningKey(ctx, db, r.SMSSigningKeyID(), "sms_signing_keys", "SMS", newKey) | ||
| } | ||
|
|
||
| func (r *Realm) createdManagedSigningKey(ctx context.Context, db *Database, keyID, table, purpose string, signingKey ManagedKey) (string, error) { |
There was a problem hiding this comment.
Instead of passing in purpose, you can use %T on the signingKey param in the errors. It seems that's the only purpose for passing in purpose.
There was a problem hiding this comment.
I moved Table() and Purpose() to methods on the ManagedKey interface.
pkg/database/sms_signing_key.go
Outdated
|
|
||
| // GetKID returns the 'kid' field value to use in signing JWTs. | ||
| func (s *SMSSigningKey) GetKID() string { | ||
| return fmt.Sprintf("r%dv%d", s.RealmID, s.ID) |
There was a problem hiding this comment.
This doesn't seem to match realm.SMSSigningKeyID()?
There was a problem hiding this comment.
SMSSigningKeyID is the KMS key - this is the KID for public consumption
| s.Active = active | ||
| } | ||
|
|
||
| func (s *SigningKey) Table() string { |
There was a problem hiding this comment.
Does passing in as Model not work? It might not, I've never tried.
There was a problem hiding this comment.
will mess w/ that separate
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mikehelmick, sethvargo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
towards #1640
Proposed Changes
Release Note