From 8067293ec909505e4b18ba97855c1fc5ab2762b4 Mon Sep 17 00:00:00 2001 From: Patryk Kalinowski Date: Fri, 18 Oct 2024 14:56:02 +0200 Subject: [PATCH 1/3] rpc: reject RegisterSession when creating an account for existing user --- data/account.go | 19 +++++++++++++++++++ rpc/sessions.go | 14 ++++++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/data/account.go b/data/account.go index 81e3bab3..032abb70 100644 --- a/data/account.go +++ b/data/account.go @@ -128,6 +128,25 @@ func (t *AccountTable) Get(ctx context.Context, projectID uint64, identity proto return &acct, true, nil } +func (t *AccountTable) ExistsByUserID(ctx context.Context, userID string) (bool, error) { + input := &dynamodb.QueryInput{ + TableName: &t.tableARN, + IndexName: &t.indices.ByUserID, + KeyConditionExpression: aws.String("UserID = :userID"), + ExpressionAttributeValues: map[string]types.AttributeValue{ + ":userID": &types.AttributeValueMemberS{Value: userID}, + }, + Limit: aws.Int32(1), + } + + out, err := t.db.Query(ctx, input) + if err != nil { + return false, fmt.Errorf("Query: %w", err) + } + + return len(out.Items) > 0, nil +} + // ListByUserID returns all Accounts of a given user. // // TODO: implement pagination. diff --git a/rpc/sessions.go b/rpc/sessions.go index 4f986947..a4fba97b 100644 --- a/rpc/sessions.go +++ b/rpc/sessions.go @@ -119,6 +119,16 @@ func (s *RPC) RegisterSession( } if !accountFound { + userID := fmt.Sprintf("%d|%s", tntData.ProjectID, sessionHash) + + userExists, err := s.Accounts.ExistsByUserID(ctx, userID) + if err != nil { + return nil, nil, proto.ErrWebrpcInternalError.WithCausef("failed to check if user exists: %w", err) + } + if userExists { + return nil, nil, proto.ErrWebrpcBadRequest.WithCausef("user already exists") + } + if !intentTyped.Data.ForceCreateAccount && ident.Email != "" { accs, err := s.Accounts.ListByEmail(ctx, tntData.ProjectID, ident.Email) if err != nil { @@ -135,7 +145,7 @@ func (s *RPC) RegisterSession( accData := &proto.AccountData{ ProjectID: tntData.ProjectID, - UserID: fmt.Sprintf("%d|%s", tntData.ProjectID, sessionHash), + UserID: userID, Identity: ident.String(), CreatedAt: time.Now(), } @@ -264,7 +274,7 @@ func (s *RPC) initiateAuth( _, err = s.Wallets.InitiateAuth(waasapi.Context(ctx), waasapi.ConvertToAPIIntent(intent.ToIntent()), answer, challenge) if err != nil { - return fmt.Errorf("initiating auth with WaaS API: %m", err) + return fmt.Errorf("initiating auth with WaaS API: %w", err) } encryptedKey, algorithm, ciphertext, err := crypto.EncryptData(ctx, att, tnt.KMSKeys[0], verifCtx) From a757546a8c8735912a7ccc643909f634ea6d7ba3 Mon Sep 17 00:00:00 2001 From: Patryk Kalinowski Date: Fri, 18 Oct 2024 15:50:10 +0200 Subject: [PATCH 2/3] add more explanation to the RegisterSession flow --- rpc/sessions.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/rpc/sessions.go b/rpc/sessions.go index a4fba97b..2f3cc09e 100644 --- a/rpc/sessions.go +++ b/rpc/sessions.go @@ -118,9 +118,15 @@ func (s *RPC) RegisterSession( return nil, nil, proto.ErrWebrpcInternalError.WithCausef("failed to retrieve account: %w", err) } + // If there's no account for this identity then we know it's used for the first time. Prepare the account to be + // created at the end of the process. if !accountFound { + // The user ID is deterministic and derived from the first session ever used by the user. userID := fmt.Sprintf("%d|%s", tntData.ProjectID, sessionHash) + // If the user already has an account (of a different identity), we need to reject this intent. Otherwise, it + // would result in an accidental account federation as a new identity is connected to an existing user through + // unintended method. userExists, err := s.Accounts.ExistsByUserID(ctx, userID) if err != nil { return nil, nil, proto.ErrWebrpcInternalError.WithCausef("failed to check if user exists: %w", err) @@ -129,6 +135,10 @@ func (s *RPC) RegisterSession( return nil, nil, proto.ErrWebrpcBadRequest.WithCausef("user already exists") } + // Warn the user if another account already exists with the same email address. This allows them to go back and + // sign in using the other identity and then use account federation to add this one. + // Otherwise, this would result in a creation of a new user and thus separate wallet that's very unlikely to be + // the user's intent. if !intentTyped.Data.ForceCreateAccount && ident.Email != "" { accs, err := s.Accounts.ListByEmail(ctx, tntData.ProjectID, ident.Email) if err != nil { @@ -154,6 +164,7 @@ func (s *RPC) RegisterSession( return nil, nil, proto.ErrWebrpcInternalError.WithCausef("encrypting account data: %w", err) } + // This account is inserted to the DB, once the WaaS API returns successfully. account = &data.Account{ ProjectID: tntData.ProjectID, Identity: data.Identity(ident), @@ -167,11 +178,17 @@ func (s *RPC) RegisterSession( } } + // This calls the Sequence WaaS API. No changes to the DB were done yet up to this point, and we can only execute + // them if the call is successful. + // Note that if we return an error *after* this and *before* the DB is updated, we risk having data desync between + // the enclave and the guard. This is a dangerous state to be in, so this call is expected -- and assumed -- to be + // idempotent. Retrying it with the same input is safe. res, err := s.Wallets.RegisterSession(waasapi.Context(ctx), account.UserID, waasapi.ConvertToAPIIntent(intent)) if err != nil { return nil, nil, proto.ErrWebrpcInternalError.WithCausef("registering session with WaaS API: %w", err) } + // Insert an account if it's new OR update it with a fresh email if it differs from what we have in the DB. if !accountFound || (ident.Email != "" && account.Email != ident.Email) { account.Email = ident.Email if err := s.Accounts.Put(ctx, account); err != nil { From 179fa10b8009b461d10e5ab21565b4fc93d48a30 Mon Sep 17 00:00:00 2001 From: Patryk Kalinowski Date: Fri, 18 Oct 2024 15:52:51 +0200 Subject: [PATCH 3/3] small comment grammar fixes --- rpc/sessions.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rpc/sessions.go b/rpc/sessions.go index 2f3cc09e..0242ede8 100644 --- a/rpc/sessions.go +++ b/rpc/sessions.go @@ -137,8 +137,8 @@ func (s *RPC) RegisterSession( // Warn the user if another account already exists with the same email address. This allows them to go back and // sign in using the other identity and then use account federation to add this one. - // Otherwise, this would result in a creation of a new user and thus separate wallet that's very unlikely to be - // the user's intent. + // Otherwise, this would result in a creation of a new user and thus a separate wallet and that's very unlikely + // to be the user's intent. if !intentTyped.Data.ForceCreateAccount && ident.Email != "" { accs, err := s.Accounts.ListByEmail(ctx, tntData.ProjectID, ident.Email) if err != nil { @@ -164,7 +164,7 @@ func (s *RPC) RegisterSession( return nil, nil, proto.ErrWebrpcInternalError.WithCausef("encrypting account data: %w", err) } - // This account is inserted to the DB, once the WaaS API returns successfully. + // This account is inserted to the DB later once the WaaS API returns successfully. account = &data.Account{ ProjectID: tntData.ProjectID, Identity: data.Identity(ident),