feat: add two-step external user registration (#134)#154
feat: add two-step external user registration (#134)#154salindne merged 6 commits intofeature/111-c-http-apifrom
Conversation
Summary of ChangesHello, 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 robust two-step external user registration process, enabling non-custodial user onboarding by allowing clients to sign topology transactions with their own keys. This change enhances security and decentralization by delegating key management to the user, while ensuring proper integration with the Canton ledger through a carefully designed API and internal service logic. The new flow is supported by updates to the Canton SDK integration, a dedicated in-memory cache for pending topologies, and comprehensive safety checks. 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. Changelog
Activity
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 two-step registration flow for external (non-custodial) users, with logic split across the Canton client, user service, and API layers, and includes security considerations like public key consistency checks. However, a high-severity security vulnerability has been identified: the registration whitelist is bypassed during the second step of the external registration process. Specifically, the RegisterWeb3User function returns early for external registrations before performing the whitelist check, and the subsequent registerExternalWeb3User function does not re-verify the whitelist status of the EVM address, allowing an attacker to register unauthorized addresses. Additionally, there is a critical lack of unit tests for this new, complex registration flow, and the use of an in-memory TopologyCache introduces a scalability bottleneck that will cause issues in a multi-instance deployment.
| storeMock.EXPECT().UserExists(ctx, evmAddress).Return(true, nil).Once() | ||
|
|
||
| svc := NewService(storeMock, nil, nil, zap.NewNop(), false) | ||
| svc := NewService(storeMock, nil, nil, zap.NewNop(), false, nil) |
There was a problem hiding this comment.
This pull request introduces a critical and complex feature for two-step external user registration, but lacks corresponding unit tests. The modifications in this test file only ensure compilation. To ensure the correctness and security of the new flow, please add comprehensive unit tests for PrepareExternalRegistration and registerExternalWeb3User in pkg/user/service/service.go.
These tests should cover:
- The happy path for the full two-step registration.
- Failure scenarios such as invalid signatures, non-whitelisted users, and already-registered users.
- Security checks, especially the public key consistency validation between step 1 and 2.
- Edge cases with the
TopologyCache, like using an expired, invalid, or already-used token.
| if req.KeyMode == user.KeyModeExternal { | ||
| return s.registerExternalWeb3User(ctx, evmAddress, req) | ||
| } |
There was a problem hiding this comment.
The RegisterWeb3User function handles both custodial and external (non-custodial) registration. For external registration (req.KeyMode == user.KeyModeExternal), it calls registerExternalWeb3User and returns immediately on line 123. However, the whitelist check on line 136 occurs after this call, meaning it is bypassed for external registration. Furthermore, registerExternalWeb3User (lines 313-397) does not perform its own whitelist check.
While the first step of external registration (PrepareExternalRegistration) does check the whitelist, an attacker with one whitelisted address can obtain a registration_token and then use it to register a different, non-whitelisted address in the second step. This is because registerExternalWeb3User does not verify that the evmAddress being registered matches the one that was whitelisted in the first step, nor does it re-check the whitelist for the new address.
func (s *registrationService) RegisterWeb3User(
ctx context.Context,
req *user.RegisterRequest,
) (*user.RegisterResponse, error) {
// Verify EVM signature
recoveredAddr, err := auth.VerifyEIP191Signature(req.Message, req.Signature)
if err != nil {
return nil, apperrors.BadRequestError(err, "invalid signature")
}
evmAddress := auth.NormalizeAddress(recoveredAddr.Hex())
s.logger.Info("Web3 registration initiated",
zap.String("evm_address", evmAddress),
zap.String("key_mode", req.KeyMode))
// Check whitelist before proceeding with any registration type
whitelisted, err := s.store.IsWhitelisted(ctx, evmAddress)
if err != nil {
return nil, fmt.Errorf("failed to check whitelist: %w", err)
}
if !whitelisted {
return nil, apperrors.ForbiddenError(ErrNotWhitelisted, "address not whitelisted for registration")
}
// External (non-custodial) registration: second step of two-step flow
if req.KeyMode == user.KeyModeExternal {
return s.registerExternalWeb3User(ctx, evmAddress, req)
}
// Check if user already exists
exists, err := s.store.UserExists(ctx, evmAddress)
if err != nil {
return nil, fmt.Errorf("failed to check user existence: %w", err)
}
if exists {
return nil, apperrors.ConflictError(ErrUserAlreadyRegistered, "user already registered")
}
// ... rest of the function ...| // TopologyCache stores pending topology data for two-step external user registration. | ||
| type TopologyCache struct { | ||
| mu sync.RWMutex | ||
| entries map[string]*pendingTopology | ||
| ttl time.Duration | ||
| } |
There was a problem hiding this comment.
The new TopologyCache is implemented as an in-memory cache local to a single server instance. This creates a significant scalability limitation. If this service is deployed with multiple instances behind a load balancer, the two-step registration flow will be unreliable, as step 1 and step 2 of a registration may be handled by different instances. The instance handling step 2 would not have the pending topology information in its memory, causing the registration to fail.
For a scalable architecture, consider replacing this with a distributed cache solution (e.g., Redis, Memcached). If this service is intended to always run as a single instance, this constraint should be clearly documented.
salindne
force-pushed
the
feature/111-c-http-api
branch
from
8b68b03 to
5e10849
Compare
salindne
force-pushed
the
feature/111-d-registration
branch
from
469603b to
080da58
Compare
salindne
force-pushed
the
feature/111-c-http-api
branch
from
5e10849 to
90ad5d9
Compare
salindne
force-pushed
the
feature/111-d-registration
branch
from
080da58 to
61570d1
Compare
salindne
force-pushed
the
feature/111-c-http-api
branch
from
90ad5d9 to
7c80a3f
Compare
salindne
force-pushed
the
feature/111-d-registration
branch
from
61570d1 to
cb733e9
Compare
Support non-custodial (external) user registration via two HTTP calls: 1. POST /register/prepare-topology — verify EIP-191 signature, validate whitelist, derive SPKI public key from compressed secp256k1 key, call Canton GenerateExternalPartyTopology, return topology hash + fingerprint + registration token for client-side signing. 2. POST /register (key_mode=external) — retrieve pending topology from cache, verify SPKI key consistency between steps, call Canton AllocateExternalPartyWithSignature with client's DER signature, create fingerprint mapping, save user as key_mode=external. Canton SDK: split AllocateExternalParty into GenerateExternalPartyTopology and AllocateExternalPartyWithSignature for the two-step flow. Add TopologyCache (in-memory, 5-min TTL) for pending registrations. Add PrepareTopologyResponse and external fields to RegisterRequest. Extend logging decorator with PrepareExternalRegistration wrapper.
The whitelist check was after the KeyModeExternal early return, meaning external registrations bypassed it. While PrepareExternalRegistration also checks the whitelist, defense-in-depth requires the check at both entry points.
Integration test script (test-prepare-execute.go) covering: - Happy path: register external user, mint, prepare, sign, execute - Expired transfer (past TTL → 410) - Replay prevention (double execute → 404) - Wrong fingerprint (→ 403) - Custodial rejection (→ 400) Add docs/non-custodial-snap-testing.md with complete Snap integration guide: cryptographic requirements, API reference, error tables, RPC methods, JavaScript dapp example, and curl testing commands. Update e2e-local config with test user addresses.
…ment Update mock expectations in registration tests to match the new whitelist-before-UserExists ordering. Also remove no-op `_ = hash` in test script.
salindne
force-pushed
the
feature/111-c-http-api
branch
from
7c80a3f to
75e867d
Compare
salindne
force-pushed
the
feature/111-d-registration
branch
from
cb733e9 to
23d9cb0
Compare
feat: add non-custodial integration test and Snap docs (#135)
1 participant
Summary
Sub-issue D (#134) of the non-custodial signing feature (#111).
POST /register/prepare-topology— verify EIP-191 sig, validate whitelist, derive SPKI public key from compressed secp256k1, call CantonGenerateExternalPartyTopology, return topology hash + fingerprint + registration tokenPOST /register(withkey_mode=external) — retrieve pending topology from cache, verify SPKI key consistency between steps, call CantonAllocateExternalPartyWithSignature, create fingerprint mapping, save user askey_mode=externalAllocateExternalPartyintoGenerateExternalPartyTopologyandAllocateExternalPartyWithSignatureRegisterRequest,PrepareTopologyResponsetype,KeyModetoRegisterResponseTest plan
go build ./...— compilesgo test ./pkg/user/service/...— registration tests passgo test ./...— all tests passmake lint— passesStacked PR 3/4 for #111 — depends on #153