feat: Custodial key management for Canton user registration#53
Conversation
- Add pkg/keys for Canton Ed25519 keypair generation and AES-256-GCM encryption - Add pkg/canton/lapi/v2/admin proto bindings for PartyManagementService - Update registration handler to allocate Canton parties via Admin API - Store encrypted Canton keys in PostgreSQL for custodial management - Update demo-activity script to show PROMPT vs DEMO token breakdown - Update configs with v1.2.0 DAML package IDs - Add key_management config section for master key env var This enables the API server to create Canton parties for users during registration and manage their signing keys custodially, allowing EVM wallet signatures to authorize Canton ledger operations.
Points to salindne/remove-issuer-transfer branch which removes the IssuerTransfer choice, now unnecessary with custodial key model.
Summary of ChangesHello @salindne, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant architectural shift by enabling custodial key management for user registrations on Canton. Users can now register and have a unique Canton party and associated keys managed by the API server, enhancing decentralization and user ownership of assets. This change moves the system from an issuer-centric model to one where the API server acts as a custodian, signing transactions on behalf of individual users with their dedicated Canton keys. This improves the security posture and aligns with principles of self-sovereign identity within the Canton ecosystem. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant architectural change by implementing a custodial key management system for Canton user registration. The API server now generates, encrypts, and manages Canton keys on behalf of users, which is a robust approach for simplifying user onboarding. The implementation correctly uses the Canton Admin API for dynamic party allocation and AES-256-GCM for secure key encryption.
While the overall design is solid, I've identified a few critical issues. There are several database queries that omit the demo_balance column, which will lead to incorrect balance data for users. Additionally, the demo token transfer logic in the token service is incomplete, as it fails to increment the recipient's balance in the cache. I've also raised a high-severity concern about the non-atomic nature of user creation and key storage during registration, which could result in inconsistent user states if an error occurs. Finally, I've suggested a refactoring in the database layer to improve maintainability by reducing code duplication.
| SELECT id, evm_address, canton_party, fingerprint, mapping_cid, balance, balance_updated_at, created_at, | ||
| canton_party_id, canton_private_key_encrypted, canton_key_created_at | ||
| FROM users |
There was a problem hiding this comment.
The SELECT query is missing the demo_balance column. This will cause the demo_balance field on the returned User struct to be incorrect (it will default to "0"). The Scan call below also needs to be updated to include a variable for demo_balance.
This same issue is present in GetUserByFingerprint (line 127) and GetUsersWithoutCantonKey (line 708). All three queries should be updated to include demo_balance to prevent data inconsistencies.
| SELECT id, evm_address, canton_party, fingerprint, mapping_cid, balance, balance_updated_at, created_at, | |
| canton_party_id, canton_private_key_encrypted, canton_key_created_at | |
| FROM users | |
| SELECT id, evm_address, canton_party, fingerprint, mapping_cid, balance, demo_balance, balance_updated_at, created_at, | |
| canton_party_id, canton_private_key_encrypted, canton_key_created_at |
| if err := h.db.CreateUser(user); err != nil { | ||
| h.logger.Error("Failed to save user", zap.Error(err)) | ||
| h.writeError(w, http.StatusInternalServerError, "failed to save user") | ||
| return | ||
| } | ||
|
|
||
| // Store the encrypted Canton key | ||
| if h.keyStore != nil { | ||
| if err := h.keyStore.SetUserKey(evmAddress, cantonPartyID, cantonKeyPair.PrivateKey); err != nil { | ||
| h.logger.Error("Failed to store Canton key", | ||
| zap.String("evm_address", evmAddress), | ||
| zap.Error(err)) | ||
| // Don't fail registration - key can be regenerated later | ||
| } | ||
| } |
There was a problem hiding this comment.
The user creation and key storage are performed in two separate, non-atomic database operations: an INSERT via db.CreateUser followed by an UPDATE within keyStore.SetUserKey. If the second operation fails, a user record will exist in the database without an associated key.
The comment on line 197, // Don't fail registration - key can be regenerated later, is only accurate if keys are derived deterministically. Since the default KeyDerivation method is "generate", the key is random and cannot be regenerated. This could leave the user in an inconsistent state, unable to use features requiring their Canton key.
To ensure data integrity, these operations should be performed within a single database transaction.
Co-authored-by: Cursor <cursoragent@cursor.com>
- Refactor balance functions to use TokenType parameter (TokenPrompt, TokenDemo) instead of hardcoded prompt/demo-specific functions - Rename balance column to prompt_balance for clarity - Fix non-atomic user registration by adding cleanup on key storage failure - Add DeleteUser function for registration cleanup - Fix demo-activity.go SQL query for prompt_balance - Update Docker config with correct local Anvil addresses (chain ID 31337) - Add scripts/bootstrap-all.sh for full local environment automation Co-authored-by: Cursor <cursoragent@cursor.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-architected feature for custodial key management in the user registration flow. The changes are extensive, touching the database schema, API handlers, Canton client, and introducing a new pkg/keys for cryptographic operations. The use of a KeyStore interface for abstraction and the robust error handling in the registration handler, including cleanup on failure, are commendable. The documentation additions are also very helpful for understanding the new architecture.
My review has identified one high-severity issue related to signature encoding that needs to be addressed to ensure compatibility with Canton, and a minor formatting issue in the documentation. Overall, this is a solid implementation of a complex feature.
| func (kp *CantonKeyPair) Sign(message []byte) ([]byte, error) { | ||
| // Convert private key bytes to ECDSA private key | ||
| privateKey, err := crypto.ToECDSA(kp.PrivateKey) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to convert private key: %w", err) | ||
| } | ||
|
|
||
| // Hash the message with SHA-256 | ||
| hash := sha256.Sum256(message) | ||
|
|
||
| // Sign with ECDSA | ||
| signature, err := crypto.Sign(hash[:], privateKey) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to sign: %w", err) | ||
| } | ||
|
|
||
| // The crypto.Sign function returns a 65-byte signature [R || S || V] | ||
| // For Canton, we need just the DER-encoded signature without the recovery ID | ||
| // We'll return the R and S values (first 64 bytes) | ||
| return signature[:64], nil | ||
| } |
There was a problem hiding this comment.
The Sign function's implementation returns a raw 64-byte [R || S] signature, but the function's comment and Canton's requirements for SIGNING_ALGORITHM_SPEC_EC_DSA_SHA_256 specify a DER-encoded signature. This will likely cause signature verification failures in Canton. The signature should be encoded in ASN.1 DER format.
You can use ecdsa.SignASN1 from the standard library for this. Note that you will need to add import "crypto/ecdsa".
A similar fix should also be applied to the SignHash function.
// Sign signs a message with the private key using ECDSA with SHA-256
// Returns the signature in DER format (compatible with Canton)
func (kp *CantonKeyPair) Sign(message []byte) ([]byte, error) {
// Convert private key bytes to ECDSA private key
privateKey, err := crypto.ToECDSA(kp.PrivateKey)
if err != nil {
return nil, fmt.Errorf("failed to convert private key: %w", err)
}
// Hash the message with SHA-256
hash := sha256.Sum256(message)
// Sign and encode as ASN.1 DER, which is what Canton expects.
signature, err := ecdsa.SignASN1(rand.Reader, privateKey, hash[:])
if err != nil {
return nil, fmt.Errorf("failed to create DER-encoded signature: %w", err)
}
return signature, nil
}| func (kp *CantonKeyPair) SignHash(hash []byte) ([]byte, error) { | ||
| if len(hash) != 32 { | ||
| return nil, fmt.Errorf("hash must be 32 bytes") | ||
| } | ||
|
|
||
| privateKey, err := crypto.ToECDSA(kp.PrivateKey) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to convert private key: %w", err) | ||
| } | ||
|
|
||
| signature, err := crypto.Sign(hash, privateKey) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to sign: %w", err) | ||
| } | ||
|
|
||
| return signature[:64], nil | ||
| } |
There was a problem hiding this comment.
Similar to the Sign function, SignHash returns a raw signature instead of the required DER-encoded format. This should be updated to use ecdsa.SignASN1 to produce a compliant signature for Canton.
func (kp *CantonKeyPair) SignHash(hash []byte) ([]byte, error) {
if len(hash) != 32 {
return nil, fmt.Errorf("hash must be 32 bytes")
}
privateKey, err := crypto.ToECDSA(kp.PrivateKey)
if err != nil {
return nil, fmt.Errorf("failed to convert private key: %w", err)
}
// Sign and encode as ASN.1 DER.
signature, err := ecdsa.SignASN1(rand.Reader, privateKey, hash)
if err != nil {
return nil, fmt.Errorf("failed to create DER-encoded signature for hash: %w", err)
}
return signature, nil
}Scripts reorganization: - Move setup/bootstrap scripts to scripts/setup/ - Move test/demo scripts to scripts/testing/ - Move bridge operations to scripts/bridge/ - Move diagnostics to scripts/utils/ - Move migration scripts to scripts/archive/ - Delete redundant demo scripts (native-wallet-demo.go, reconcile-demo.go, demo-showcase.sh, interop-demo.sh) Documentation cleanup: - Consolidate architecture docs into ARCHITECTURE.md - Consolidate setup guides into SETUP_AND_TESTING.md - Rename CIP-0086-OVERVIEW2.md to CIP-0086-OVERVIEW.md - Update WAYFINDER_DEPLOYMENT_REQUIREMENTS.md - Delete outdated docs (canton-integration.md, relayer-logic.md, secp256k1-*.md, etc.) - Move demo docs to docs/demo/ (gitignored) Code changes: - Add register-native-user.go for native Canton user registration - Add canton-transfer-demo.go for transfer demonstrations - Add query-canton-holdings.sh and verify-canton-holdings.go utilities - Fix linter error in register-native-user.go (add //go:build ignore) - Remove hardcoded OAuth secrets from archive scripts README update: - Reflect current project structure and script locations - Update documentation links - Modernize quick start instructions Co-authored-by: Cursor <cursoragent@cursor.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-structured feature: custodial key management for Canton users. This allows the API server to manage Canton keys on behalf of users, enabling them to interact with the Canton network using only their EVM wallet. The changes are extensive, touching documentation, configuration, database schema, and core application logic. New packages for key generation/encryption (pkg/keys) and Canton signature authentication (pkg/auth) have been added. The registration flow is significantly updated to allocate unique Canton parties and store encrypted keys, and the token transfer logic is refactored to a more appropriate user-centric model. My review includes suggestions for improving database migration safety, error handling in the registration flow, and clarity in the new cryptography package.
| CREATE TABLE IF NOT EXISTS users ( | ||
| id SERIAL PRIMARY KEY, | ||
| evm_address VARCHAR(42) UNIQUE NOT NULL, | ||
| canton_party VARCHAR(255) NOT NULL, | ||
| canton_party VARCHAR(255) NOT NULL, -- User's Canton party ID | ||
| fingerprint VARCHAR(128) NOT NULL, | ||
| mapping_cid VARCHAR(255), | ||
| balance DECIMAL(38,18) DEFAULT 0, | ||
| demo_balance DECIMAL(38,18) DEFAULT 0, -- DEMO (native) token balance | ||
| prompt_balance DECIMAL(38,18) DEFAULT 0, -- PROMPT (bridged) token balance | ||
| demo_balance DECIMAL(38,18) DEFAULT 0, -- DEMO (native) token balance | ||
| balance_updated_at TIMESTAMP, | ||
| created_at TIMESTAMP DEFAULT NOW() | ||
| created_at TIMESTAMP DEFAULT NOW(), | ||
| -- Custodial Canton key fields | ||
| canton_party_id VARCHAR(255), -- User's own Canton party (same as canton_party for new users) | ||
| canton_private_key_encrypted TEXT, -- AES-256-GCM encrypted Canton private key (base64) | ||
| canton_key_created_at TIMESTAMP -- When the Canton key was generated | ||
| ); |
There was a problem hiding this comment.
The schema for the users table has been updated, including renaming the balance column to prompt_balance and adding new columns for key management. The CREATE TABLE IF NOT EXISTS statement will not apply these changes to an existing database, which will cause errors in existing deployments.
A migration script is needed to safely alter the table for existing installations. You could add a DO ... END block to check for the existence of the old column and rename it, and to add the new columns if they don't exist.
| Fingerprint: fingerprint, | ||
| MappingCID: mappingCID, | ||
| EVMAddress: evmAddress, | ||
| PrivateKey: evmKeyPair.PrivateKeyHex(), // For MetaMask import |
There was a problem hiding this comment.
Returning a private key in an API response is highly sensitive, even for a user import flow. While this enables the user to import the generated EVM identity into MetaMask, it's crucial to ensure this endpoint is well-protected.
The documentation should strongly emphasize that this endpoint MUST be served over TLS and that clients should treat this key as extremely sensitive, clearing it from memory as soon as it's used.
| func tokenColumn(token TokenType) string { | ||
| switch token { | ||
| case TokenPrompt: | ||
| return "prompt_balance" | ||
| case TokenDemo: | ||
| return "demo_balance" | ||
| default: | ||
| return "prompt_balance" // Default to PROMPT for safety | ||
| } |
There was a problem hiding this comment.
The tokenColumn function defaults to prompt_balance for any unknown TokenType. While this provides a safe fallback, it could mask bugs where an invalid or new, unhandled token type is passed to a balance function. This might lead to incorrect balance updates for the default token instead of failing fast.
Consider panicking or returning an error for an unknown token type to make the behavior more explicit and prevent silent failures.
|
|
||
| // Sign signs a message with the private key using ECDSA with SHA-256 | ||
| // Returns the signature in DER format (compatible with Canton) | ||
| func (kp *CantonKeyPair) Sign(message []byte) ([]byte, error) { |
There was a problem hiding this comment.
The comment states that the function returns the signature in DER format. However, the implementation return signature[:64], nil returns the raw R and S values concatenated, which is not a valid DER-encoded signature. This is misleading and could cause confusion or errors if a consumer of this function expects a standard DER signature.
Please update the comment to accurately reflect the returned format (e.g., "64-byte R || S format").
| if h.keyStore != nil { | ||
| if err := h.keyStore.SetUserKey(evmAddress, cantonPartyID, cantonKeyPair.PrivateKey); err != nil { | ||
| h.logger.Error("Failed to store Canton key", | ||
| zap.String("evm_address", evmAddress), | ||
| zap.Error(err)) | ||
| // Cleanup: delete the user we just created to maintain consistency | ||
| if delErr := h.db.DeleteUser(evmAddress); delErr != nil { | ||
| h.logger.Error("Failed to cleanup user after key storage failure", | ||
| zap.String("evm_address", evmAddress), | ||
| zap.Error(delErr)) | ||
| } | ||
| h.writeError(w, http.StatusInternalServerError, "failed to store Canton key") | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
The error handling for keyStore.SetUserKey correctly cleans up the user from the local database. However, the party previously allocated via AllocateParty and the mapping created via RegisterUser on Canton are not rolled back. This could lead to orphaned resources on the Canton ledger.
Consider implementing a compensating action to de-allocate or disable the Canton party if the full registration process fails after it has been created.
3 participants
Summary
AllocateParty)Changes
Requirements for DevNet Deployment
The API server now requires:
CANTON_MASTER_KEYenv var (generate with:openssl rand -base64 32)ParticipantAdminrights for party allocationTest plan