feat(rp-registration): add manager_key_dedicated field and related fu… - #2168
feat(rp-registration): add manager_key_dedicated field and related fu…#2168igorosip0v wants to merge 3 commits into
Conversation
…nctionality - Introduced a new column `manager_key_dedicated` in the `rp_registration` table to indicate if a manager key is dedicated to a specific RP. - Updated GraphQL mutations and queries to include the `manager_key_dedicated` field. - Enhanced the registration flow to handle dedicated and shared manager keys appropriately, including scheduling key deletion only for dedicated keys. - Added tests to ensure correct behavior of the new functionality and its integration with existing flows. This change improves the management of RP registrations by allowing for more granular control over key handling.
Takaros999
left a comment
Takaros999
left a comment
There was a problem hiding this comment.
lets discuss offline, but high level:
manager_key_dedicatedis not a very intuitive name, especially when we havemode: managed | self-managedcolumn. Thoughts onis_shared_manager_key?
| // manager_key_dedicated is written together with the manager key at the end | ||
| // of this flow. If the migration adding it has not been applied yet, fail | ||
| // here rather than after the on-chain transaction has been submitted. | ||
| try { | ||
| await getVerifySchemaSdk(client).VerifyManagerKeySchema({ | ||
| rp_id: rpIdString, | ||
| }); |
There was a problem hiding this comment.
Im pretty sure the hasura migration runs before the web app deployment, I'm wondering if this case is needed.
| * Uses the shared key from RP_REGISTRY_MANAGER_KMS_KEY_ID when set; | ||
| * otherwise creates a dedicated per-RP key. | ||
| */ | ||
| export async function resolveManagerKey( |
There was a problem hiding this comment.
nit. If we create the shared kms key in both environments this would be simplified a lot. I also think we might be mixing the concerns between the 2 files (kms-eth.ts and rp-registration-flow.ts)
I see your need about having a mechanism for smooth migration, what if we had a ENABLE_SHARED_KEY_RP_REGISTRATION or similar boolean flag, and use that in the rp-registration-flows to determine which key to use for the first rp registration
There was a problem hiding this comment.
ENABLE_SHARED_KEY_RP_REGISTRATION or similar boolean flag
Plan is to test shared-key registration on staging only first, without touching prod deploys. So for now we only wire RP_REGISTRY_MANAGER_KMS_KEY_ID in staging; its presence is the switch. A separate boolean would matter if the key already existed in both envs before we were ready to use it.
There was a problem hiding this comment.
Actually, updated
Agree the name isn't great. I'd keep this polarity though: column default is |
…gistration logic - Removed the resolveManagerKey function, which previously handled both shared and dedicated key resolution. - Updated submitManagedRpRegistration to use createManagerKey and getEthAddressFromKMS based on the new ENABLE_SHARED_KEY_RP_REGISTRATION environment variable. - Adjusted tests to reflect the removal of resolveManagerKey and ensure proper handling of key creation and retrieval logic.
2 participants
PR Type
Description
New managed RPs use a shared KMS manager key when
RP_REGISTRY_MANAGER_KMS_KEY_IDis set, and the database now tracks whether an RP's key is dedicated to it, so the shared key is never scheduled for deletion.manager_key_dedicatedtorp_registration, backfilled totruefor rows that already have amanager_kms_key_id.resolveManagerKey(): reuses the key fromRP_REGISTRY_MANAGER_KMS_KEY_ID(full ARN) when set, otherwise creates a dedicated per-RP key as before.scheduleKeyDeletion()for dedicated keys, in both the registration rollback andswitch-to-self-managed.No behavior change while the variable is unset. The key exists only in staging (worldcoin/world-id-deploy#552).
Checklist