Vault: canonicalize owner at storage and validate Ethereum addresses#22828
Vault: canonicalize owner at storage and validate Ethereum addresses#22828prashantkumar1982 wants to merge 2 commits into
Conversation
… into fix/vault-owner-address-validation
product-security-plaid-production
Bot
requested review from
bolekk and
justinkaseman
|
✅ No conflicts with other open PRs targeting |
|
I see you updated files related to
|
|
|
@prashantkumar1982 Is L3 safe to do? This is effectively going to modify all requests made to the gateway so that addresses are fetched under the checksummed representation; but what if the data isn't stored in that format? |
| if !isValidIDComponent(idKey) || !isValidIDComponent(idOwner) || (idNamespace != "" && !isValidIDComponent(idNamespace)) { | ||
| return errors.New("key, owner and namespace must only contain alphanumeric characters") | ||
| } | ||
| if !common.IsHexAddress(idOwner) { |
There was a problem hiding this comment.
See my comment on the overall PR: the fact that this doesn't exist already worries me a little. Might be worth adding a feature flag around this is case it causes issue with the rollout
There was a problem hiding this comment.
I'm a little uncomfortable with some of these changes: KeyFor + CanonicalSecretIdentifier both have the possibility of causing some non-determinism issues depending on what format the address is actually in today.
Is it worth being more cautious with the rollout and putting this behind a feature flag?
I also think it would be better if we handled the transformation to a checksummed addresses in a central place -- for example before adding the message to the incoming queue, rather than forcing the change throughout the plugin. That makes any bugs a lot harder to track and reason about.
There was a problem hiding this comment.
I don't think we realistically have to worry about hitting non-determinism. If you created a secret with a lowercase address it would get stored with that lowercase address and then be unreadable because all of the workflows send the checksummed address. So we can be reasonably sure only people not using the CLI could cause a non-determinism issue on rollout.
With that said, we also agreed we want to put everything behind flags to be cautious so +1 for a feature flag and +1 for centralizing it
3 participants
Summary
This PR hardens vault secret identifier handling around workflow owner addresses (M2/L3 follow-up).
Defense in depth (M2):
ValidateSecretIdentifiernow requiresownerto be a valid Ethereum address (common.IsHexAddress). List requests validate owner with a placeholder key instead of incorrectly passing owner as the key field (which broke once addresses became 42-byte strings).Storage-only canonicalization (L3): Owner addresses are normalized to EIP-55 checksum form at the storage boundary only:
vaultutils.KeyForandCanonicalSecretIdentifierfor secret/metadata keysRequest payloads are not rewritten in gateway handlers. We explicitly avoided stamping owner onto fan-out requests, because mutating params after user-gateway auth would break vault-node JWT re-auth (request digest includes params).
Refactor: Utility functions moved out of
vaulttypesintovaultutils:identifier.go— namespace/owner normalization,KeyFor,CanonicalSecretIdentifiersignatures.go—ValidateSignatures(OCR signed response verification)No migration or dual-read: Secrets written with non-canonical owner casing were already unreadable from all primary read paths — workflows (
normalizeOwner), confidential relay, and confidential-compute (util.HexToAddress(...).String()inexecutor.go) all canonicalize owner before calling vault. A lowercase write therefore never matched a checksum lookup on read. We are not adding a migration or dual-key fallback for legacy OCR state.No feature flag: We do not believe any current production use case writes secrets with non-canonical owner strings (authorized owner binding already uses canonical addresses from allowlist/JWT). The KV store changes align writes with existing read behavior and should be a no-op for correctly written data.