Verifiable payer #544
Verifiable payer #544
Conversation
WalkthroughThis pull request makes extensive changes to envelope creation, validation, and publishing. Several tests are updated to pass a new node identifier parameter and adjust expectations around mismatching originators. The service layer now performs additional checks on envelope originators and recovers signers with the new parameter. A retry mechanism is introduced for publishing envelopes with node selection that respects a ban list. Function signatures in utility packages are modified to incorporate the originator ID, and a new constant is added. There are also routine updates to mock version comments. Changes
Sequence Diagram(s)sequenceDiagram
participant S as Service
participant NS as NodeSelector
participant N as Node
S->>NS: GetNode(topic, banlist...)
NS-->>S: Return available nodeID
S->>N: publishEnvelope(envelopes)
alt Publish fails
N-->>S: Error response
S->>NS: Update banlist & retry GetNode
S->>N: publishEnvelope(envelopes) (Retry)
else Publish succeeds
N-->>S: Success response
end
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24) 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pkg/envelopes/payer.go (1)
14-18: Add documentation for the new TargetOriginator field.While the implementation is correct, adding documentation would help explain the field's purpose in preventing payload stealing.
type PayerEnvelope struct { proto *envelopesProto.PayerEnvelope ClientEnvelope ClientEnvelope + // TargetOriginator is the ID of the node this envelope is intended for, + // preventing other nodes from stealing and republishing the payload TargetOriginator uint32 }pkg/api/message/service.go (1)
410-416: Improve error messages for better debugging.While the validation checks are correct and align with preventing node payload stealing, the error messages could be more descriptive.
Apply this diff to improve error messages:
if payerEnv.TargetOriginator != s.registrant.NodeID() { - return nil, status.Errorf(codes.InvalidArgument, "invalid target originator") + return nil, status.Errorf(codes.InvalidArgument, "target originator %d does not match node ID %d", payerEnv.TargetOriginator, s.registrant.NodeID()) } if _, err = payerEnv.RecoverSigner(); err != nil { - return nil, err + return nil, status.Errorf(codes.InvalidArgument, "failed to recover signer: %v", err) }pkg/api/payer/service.go (2)
168-202: Consider implementing a brief backoff period in the retry loop.
Currently, the loop attempts each retry immediately in quick succession. Introducing an exponential or fixed delay helps avoid potential tight loops and reduces load or noise if a node is persistently down.for retries := 0; retries < 5; retries++ { result, err = s.publishToNodes(ctx, nodeID, indexedEnvelopes) if err == nil { return result, nil } + // Example of a simple backoff + time.Sleep(time.Duration(retries+1) * time.Second) ... }
380-399: Validate the consistency of originator information.
Consider verifying that theoriginatorIDparameter matches the data inclientEnvelope.Aad()to prevent potential spoofing attempts where these might differ.pkg/api/payer/nodeSelector.go (1)
33-85: Implementation of banlist logic works but could benefit from further efficiency consideration.
Flattening the banlist into a map and iterating is clear and correct. If banlists become large or concurrency is introduced, more sophisticated coordination or data structures might be beneficial, but this is sufficient for many use cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
pkg/proto/openapi/xmtpv4/message_api/message_api.swagger.jsonis excluded by!pkg/proto/**pkg/proto/openapi/xmtpv4/message_api/misbehavior_api.swagger.jsonis excluded by!pkg/proto/**pkg/proto/xmtpv4/envelopes/envelopes.pb.gois excluded by!**/*.pb.go,!pkg/proto/**
📒 Files selected for processing (26)
pkg/api/message/publish_test.go(11 hunks)pkg/api/message/service.go(1 hunks)pkg/api/payer/nodeSelector.go(3 hunks)pkg/api/payer/nodeSelector_test.go(1 hunks)pkg/api/payer/publish_test.go(2 hunks)pkg/api/payer/service.go(8 hunks)pkg/constants/constants.go(1 hunks)pkg/envelopes/envelopes_test.go(4 hunks)pkg/envelopes/payer.go(3 hunks)pkg/indexer/e2e_test.go(3 hunks)pkg/indexer/storer/groupMessage_test.go(2 hunks)pkg/mocks/authn/mock_JWTVerifier.go(1 hunks)pkg/mocks/blockchain/mock_ChainClient.go(1 hunks)pkg/mocks/blockchain/mock_IBlockchainPublisher.go(1 hunks)pkg/mocks/indexer/mock_ChainReorgHandler.go(1 hunks)pkg/mocks/indexer/mock_IBlockTracker.go(1 hunks)pkg/mocks/metadata_api/mock_MetadataApiClient.go(1 hunks)pkg/mocks/mls_validationv1/mock_ValidationApiClient.go(1 hunks)pkg/mocks/mlsvalidate/mock_MLSValidationService.go(1 hunks)pkg/mocks/registry/mock_NodeRegistry.go(1 hunks)pkg/mocks/registry/mock_NodesContract.go(1 hunks)pkg/mocks/storer/mock_LogStorer.go(1 hunks)pkg/server/server_test.go(3 hunks)pkg/testutils/envelopes/envelopes.go(7 hunks)pkg/utils/hash.go(1 hunks)pkg/utils/signature.go(1 hunks)
✅ Files skipped from review due to trivial changes (11)
- pkg/mocks/registry/mock_NodesContract.go
- pkg/mocks/blockchain/mock_ChainClient.go
- pkg/mocks/indexer/mock_IBlockTracker.go
- pkg/mocks/mlsvalidate/mock_MLSValidationService.go
- pkg/mocks/indexer/mock_ChainReorgHandler.go
- pkg/mocks/authn/mock_JWTVerifier.go
- pkg/mocks/storer/mock_LogStorer.go
- pkg/mocks/mls_validationv1/mock_ValidationApiClient.go
- pkg/mocks/registry/mock_NodeRegistry.go
- pkg/mocks/blockchain/mock_IBlockchainPublisher.go
- pkg/mocks/metadata_api/mock_MetadataApiClient.go
🧰 Additional context used
🪛 GitHub Check: Lint-Go
pkg/api/payer/nodeSelector_test.go
[failure] 262-262:
ineffectual assignment to reselectedNode (ineffassign)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Test (Node)
🔇 Additional comments (32)
pkg/constants/constants.go (1)
6-6: LGTM! The new constant follows the established naming pattern.The new separation label is consistent with existing domain separation constants and supports the verifiable payer feature.
pkg/indexer/storer/groupMessage_test.go (1)
52-52: LGTM! Test updates align with function signature changes.The removal of the targetOriginator parameter from CreateGroupMessageClientEnvelope calls reflects the updated function signature while maintaining test coverage.
Also applies to: 95-95
pkg/indexer/e2e_test.go (1)
65-67: LGTM! Variable rename improves code clarity.The change from
topictomsgTopicmakes the variable's purpose clearer, and the function call updates align with the new signature.Also applies to: 80-80, 94-94
pkg/envelopes/payer.go (2)
29-34: LGTM! Proper initialization of the TargetOriginator field.The field is correctly initialized from the proto message, maintaining consistency between the struct and protobuf representation.
54-54: LGTM! Enhanced security with TargetOriginator in signature hash.Including TargetOriginator in the hash computation ensures the signature binds the envelope to the intended node, preventing payload stealing as intended in issue #474.
pkg/api/payer/publish_test.go (2)
184-186: LGTM! Mock setup aligns with PR objectives.The mock setup correctly returns a node with ID 100, which is used to verify the target originator in the payer envelope.
211-212: LGTM! Verification of target originator.The test now properly verifies that the target originator in the payer envelope matches the expected node ID, which aligns with the PR objective of preventing node payload theft.
pkg/server/server_test.go (2)
163-164: LGTM! Node ID handling in test cases.The test cases now correctly pass node IDs when creating payer envelopes, which is essential for the new security feature.
Also applies to: 178-179
272-273: LGTM! Simplified envelope structure.The test case correctly removes the redundant target originator from the authenticated data structure while maintaining essential fields.
pkg/utils/hash.go (2)
9-12: LGTM! Enhanced security with originator ID in hash calculation.The function now includes the originator ID in the hash calculation, which is crucial for preventing node payload theft. The use of
binary.BigEndianfor uint32 conversion is correct and consistent with network byte order standards.
13-15: LGTM! Proper domain separation in hash calculation.The hash calculation correctly uses domain separation labels and includes the target originator bytes in the appropriate order.
pkg/utils/signature.go (1)
9-14: LGTM! Enhanced signature security with originator ID.The signature function now correctly incorporates the originator ID in the hash calculation, which is essential for the security enhancement described in issue #474.
pkg/envelopes/envelopes_test.go (3)
22-22: LGTM!The test correctly validates the originator node ID in the payer envelope, which aligns with the PR's objective of preventing node payload stealing.
51-51: LGTM!The test ensures that the envelope serialization works correctly with the new originator node ID field.
141-146: LGTM!The test thoroughly validates the signer recovery functionality with the new originator node ID field, including both valid and invalid signature scenarios.
Also applies to: 164-164
pkg/api/payer/nodeSelector_test.go (1)
230-264: LGTM!The test comprehensively validates the node selection logic with banned nodes, ensuring that:
- Initial node selection works correctly
- Alternative nodes are selected when preferred nodes are banned
- An error is returned when all nodes are banned
This aligns with the PR's objective of allowing the payer to select different nodes if the connection to the preferred node fails.
🧰 Tools
🪛 GitHub Check: Lint-Go
[failure] 262-262:
ineffectual assignment to reselectedNode (ineffassign)pkg/testutils/envelopes/envelopes.go (4)
16-16: LGTM!Good refactoring to extract the hardcoded value into a named constant.
32-32: LGTM!Good use of the new constant to improve code maintainability.
84-107: LGTM!The function has been properly enhanced to:
- Accept a node ID parameter
- Generate a valid signature for testing
- Set the target originator field
This aligns with the PR's objective of preventing node payload stealing.
117-117: LGTM!The functions correctly pass the originator node ID to maintain consistency in test envelope creation.
Also applies to: 142-142
pkg/api/payer/service.go (2)
79-79: No concerns about switching to the new retry method.
InvokingpublishToNodeWithRetryhere aligns with the updated retry logic and looks correct.
335-338: Confirm that ignoring the node cursor error is intended.
By only logging this error rather than returning it, the method could continue operating with incomplete data. Ensure this decision aligns with your reliability/availability requirements.pkg/api/message/publish_test.go (9)
25-28: Creation of payer envelope with default node ID looks fine.
No issues spotted with this usage.
76-79: Payer envelope generation is consistent.
Properly matches the new signature usage.
90-112: Verify that ignoring mismatching AAD originator is acceptable.
This test confirms mismatching AAD originator is allowed. Double-check that your security model permits ignoring such mismatches without side effects.
114-130: Good coverage for mismatching originator scenario.
This test ensures the system properly rejects conflicting originator values.
142-146: Envelope creation for missing topic tests is correct.
No concerns with parameter usage here.
153-196: Key package validation success path appears consistent.
The changes comply with the new node ID handling.
202-237: Key package validation failure path is well-handled.
The updated usage of the node ID parameter is consistent with the rest of the code.
243-253: Usage of the helper with cursor validation is straightforward.
No issues noted with the updated node ID parameter.
284-293: Correctly tests scenario with an unknown originator.
Ensures the function returns an error as expected.pkg/api/payer/nodeSelector.go (1)
13-14: Adding a banlist parameter to the interface is a sensible approach.
It provides flexibility to exclude undesirable nodes.
| @@ -10,7 +10,7 @@ import ( | |||
| ) | |||
|
|
|||
| type NodeSelectorAlgorithm interface { | |||
| GetNode(topic topic.Topic) (uint32, error) | |||
| GetNode(topic topic.Topic, banlist ...[]uint32) (uint32, error) | |||
Uses xmtp/proto#247
This adds a new field to the payer envelope that prevents a node from stealing payloads #474
Additionally it gets rid of old AAD validation #463
The bulk of the changeset allows the payer to pick a different node if the connection to the ideal node has failed. This is obviously not ideal, because it will keep retrying even if the node is down long term, but in the short/medium term this is sufficient.
Fixes #474 #463
Summary by CodeRabbit
New Features
TargetOriginatorin payer envelopes to improve message association.Improvements
Refactoring