refactor: Initial db table and api for identity keys #2771
Conversation
gak
requested review from
a team and
deniseli
and removed request for
a team
gak
changed the title
|
|
||
| type EncryptedIdentity = sql.GetOnlyIdentityKeyRow | ||
|
|
||
| func (d *DAL) GetOnlyIdentityKey(ctx context.Context) (*EncryptedIdentity, error) { |
There was a problem hiding this comment.
Return a value here rather than a pointer so you're using the same kind consistently (eg. a value is used below).
| return keyPair, nil | ||
| } | ||
|
|
||
| const verificationText = "My voice is my passport, verify me." |
| return fmt.Errorf("failed to write keyset: %w", err) | ||
| } | ||
| encryptedIdentityColumn := api.EncryptedIdentityColumn{} | ||
| encryptedIdentityColumn.Set(buf.Bytes()) |
There was a problem hiding this comment.
Is this encrypted or plain text? If it's plain text it shouldn't be in an EncryptedIdentityColumn?
There was a problem hiding this comment.
buf.Bytes() is actually encrypted already!
There was a problem hiding this comment.
Tink strongly prefers (and thus makes it the default) to export / write keysets in encrypted form only. The Write operation on line 162 takes the AEAD primitive and uses it to encrypt the keyset. This encrypted keyset is FLE encrypted again via our EncryptedColumen abtraction
| } | ||
|
|
||
| func (d *DAL) CreateOnlyIdentityKey(ctx context.Context, e EncryptedIdentity) error { | ||
| if err := d.db.CreateOnlyIdentityKey(ctx, e.Private, e.Public, e.VerifySignature); err != nil { |
There was a problem hiding this comment.
Should this be protected from inserting a second identity key?
There was a problem hiding this comment.
Should already be protected. It's inside a transaction and other instances will fail if there are two rows anyway.
|
|
||
| CREATE TABLE identity_keys ( | ||
| id BIGINT NOT NULL GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY, | ||
| private encrypted_identity NOT NULL, |
There was a problem hiding this comment.
This is KMS encrypted right?
There was a problem hiding this comment.
It is encrypted the same way as timeline/async. The root key is encrypted by KMS and left unencrypted in memory. The id key is encrypted using a subkey.
| private encrypted_identity NOT NULL, | ||
| public BYTEA NOT NULL, | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT (NOW() AT TIME ZONE 'utc'), | ||
| verify_signature BYTEA NOT NULL |
There was a problem hiding this comment.
What is this signature?
There was a problem hiding this comment.
It's a sanity check signature done on the first generation of the key. It is checked on startup to make sure the key wasn't changed. Mostly to stop bugs and users doing silly things.
|
|
||
| _, err = s.dal.GetOnlyIdentityKey(ctx) | ||
| if err != nil { | ||
| if !errors.Is(err, libdal.ErrNotFound) { |
There was a problem hiding this comment.
ensureIdentity generates a new identity if one isn't found in the database. This is keyed off the error returned from GetONlyIdentityKey.
There was a problem hiding this comment.
Yeah it should only trigger when there are no rows from inside GetOnlyIdentityKey.
| } | ||
|
|
||
| // For total sanity, verify immediately | ||
| if err = verifier.Verify(*signed); err != nil { |
There was a problem hiding this comment.
Ye with little faith ;)
| return fmt.Errorf("failed to write keyset: %w", err) | ||
| } | ||
| encryptedIdentityColumn := api.EncryptedIdentityColumn{} | ||
| encryptedIdentityColumn.Set(buf.Bytes()) |
There was a problem hiding this comment.
Tink strongly prefers (and thus makes it the default) to export / write keysets in encrypted form only. The Write operation on line 162 takes the AEAD primitive and uses it to encrypt the keyset. This encrypted keyset is FLE encrypted again via our EncryptedColumen abtraction
|
|
||
| var _ Signer = &TinkSigner{} | ||
|
|
||
| type TinkSigner struct { |
There was a problem hiding this comment.
Why wrap the tink abstractions again?
There was a problem hiding this comment.
Originally it was because of having a NoOpEncryptor equivalent, but now probably will be dropping NoOp as well as this abstraction.
continuation from #2771 for #2595 --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Alec Thomas <[email protected]>
A WIP for #2595