Revise LUKS API to expect key slot and salt when escrowing successfully #23952
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23952 +/- ##
==========================================
+ Coverage 63.16% 63.19% +0.03%
==========================================
Files 1561 1563 +2
Lines 148732 148840 +108
Branches 3788 3788
==========================================
+ Hits 93945 94058 +113
+ Misses 47365 47359 -6
- Partials 7422 7423 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
|
||
| func Up_20241116233322(tx *sql.Tx) error { | ||
| _, err := tx.Exec(`ALTER TABLE host_disk_encryption_keys | ||
| ADD COLUMN base64_encrypted_salt VARCHAR(255) NOT NULL DEFAULT '' AFTER base64_encrypted, |
There was a problem hiding this comment.
[...] CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci
There was a problem hiding this comment.
Of note, we could save storage here by keeping the charset as-is, because the column should never be joined or sorted, and since this is base64'd even ASCII would represent this fine. But might as well be consistent.
|
|
||
| func Up_20241116233322(tx *sql.Tx) error { | ||
| _, err := tx.Exec(`ALTER TABLE host_disk_encryption_keys | ||
| ADD COLUMN base64_encrypted_salt VARCHAR(255) NOT NULL DEFAULT '' AFTER base64_encrypted, |
There was a problem hiding this comment.
What's the reason to use AFTER?
There was a problem hiding this comment.
Slight cleanliness thing when I (or others) use the mysql command line. Figured I might as well put encrypted/encryption related columns visually next to each other.
server/service/orbit.go
Outdated
| func (svc *Service) validateAndEncrypt(ctx context.Context, passphrase string, slotKey string) (string, string, error) { | ||
| if passphrase == "" { | ||
| return "", "", badRequest("Blank passphrase provided") | ||
| func (svc *Service) validateAndEncrypt(ctx context.Context, passphrase string, salt string, keySlot *uint) (string, string, uint, error) { |
There was a problem hiding this comment.
Nit, when having many same-typed return types, it's a good practice to use named returns:
(string, string, uint, error) -> (encryptedPassphrase string, encryptedSalt string, keySlot uint, err error)
There was a problem hiding this comment.
Thanks for the call-out. Will fix.
| func (ds *Datastore) SaveLUKSData(ctx context.Context, hostID uint, encryptedBase64Passphrase string, encryptedBase64SlotKey string) error { | ||
| if encryptedBase64Passphrase == "" { // should have been caught at service level | ||
| return errors.New("blank encrypted passphrase") | ||
| func (ds *Datastore) SaveLUKSData(ctx context.Context, hostID uint, encryptedBase64Passphrase string, encryptedBase64Salt string, keySlot uint) error { |
There was a problem hiding this comment.
Nit: Should keySlot be *uint to map cleanly to the underlying NULLable type type.
There was a problem hiding this comment.
Nope. Only reason key slot is nullable in the DB is we use this table for BitLocker/FileVault, which (from our perspective at least) have neither key slot nor salt. Per the Slack convo earlier today, we're guaranteed to get key slot if we have valid LUKS data, and this method is only called when we have valid LUKS data, so we can tighten the parameter type.
Likewise we're guaranteed to have passphrase and salt when persisting LUKS, but I can't enforce that easily in the type system, hence the runtime check.
#23584
Checklist for submitter
If some of the following don't apply, delete the relevant line.
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)- [ ] Manual QA for all new/changed functionalityWill be tested E2E