-
Notifications
You must be signed in to change notification settings - Fork 26
feat(secret): encrypt secret metadata at rest #277
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(secret): encrypt secret metadata at rest #277
Conversation
|
Thanks for the contribution 🙏 -- there is some upstream refactoring in progress; I'll check this immediately after that. |
v0lkan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for compliance we woud need either unique nonces per field (which is an overkill, or a deterministic nonce-creation (more pragmatist)
I've commented on an option that this can be done.
Thanks for your contributions 🙏 .
| const QuerySecretMetadata = ` | ||
| SELECT current_version, oldest_version, created_time, updated_time, max_versions | ||
| FROM secret_metadata | ||
| SELECT nonce, encrypted_current_version, encrypted_oldest_version, encrypted_created_time, encrypted_updated_time, encrypted_max_versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a single nonce per field would be cryptographically weak and it can cause warnings in security audits.
While I do realize that the data here itself is not super-sensitive, auditors typically want to "check boxes", rather than having a pragmatic perspective on security.
My suggestion is as follows:
Derive per-field nonces from the stored base nonce without changing the schema:
var fieldNonceSalts = map[string][]byte{
"current_version": []byte("current_ver_"), // 12 bytes each
"oldest_version": []byte("oldest_ver__"),
"created_time": []byte("created_tim_"),
// ...
}
func deriveFieldNonce(baseNonce []byte, field string) []byte {
derived := make([]byte, len(baseNonce))
salt := fieldNonceSalts[field]
for i := range baseNonce {
derived[i] = baseNonce[i] ^ salt[i]
}
return derived
}
This gives each field a unique nonce while keeping a single nonce column in the DB. Decryption remains deterministic since the derivation is reversible.
One related question would be "what if an attacker knows the per-field key" (since they have access to the source code, as the code is public).
Kerckhoffs's principle states that "A cryptosystem should be secure even if everything except the key is public knowledge".
In AES-GCM:
- Key → must be secret (this is your root key)
- Nonce → must be unique per encryption, but can be public
The nonce is typically stored in plaintext right next to the ciphertext (which we are already doing). An attacker seeing the nonce doesn't help them—they still can't decrypt without the key.
The per-field salts being in the source code is fine because:
- The base nonce is random (generated via crypto/rand)
- XORing with known constants just ensures each field's nonce differs
- The actual security comes entirely from the encryption key
As an aside, nonces don't need to be secret—they only need to be unique.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
My initial thought was to generate unique nonces per field, but as you mentioned, this would be an overkill, so I opted for the single nonce approach—referring to how policies are encrypted in app/nexus/internal/state/backend/sqlite/persist/policy.go.
I'm also just curious: Is there a specific reason why single nonce per field approach is used in policies? Wouldn't that be cryptographically weak as well?
ab0b7f4 to
0a0d8cf
Compare
|
I will give a bit more context on why I’ve still kept the schema change as is, in my last commit As we discussed earlier, I was not sure how to get spike/app/nexus/internal/state/backend/sqlite/persist/crypto.go Lines 32 to 33 in 41e5ec5
which means this was not the case and encrypted blobs are stored as ciphertext only (no nonce prefix added) To my understanding, there are two options while decrypting the fields:
I picked the first option, using derived per field nonces as you requested, but unfortunately this requires modifying the schema Though, I can't say I understand how XOR'ing with a fixed salt removes the need for a schema change I’m happy to adjust/refactor the code further, so please do let me know @v0lkan |
Fixes: #251