Override role based unicast stream authorization on public network#8526
Override role based unicast stream authorization on public network#8526peterargue merged 5 commits intoleo/228-networking-unicast-authorizationonflow/flow-go:leo/228-networking-unicast-authorizationfrom leo/fix-unicast-stream-authorization-public-networkonflow/flow-go:leo/fix-unicast-stream-authorization-public-networkCopy head branch name to clipboard
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request introduces unicast stream authorization to Flow network nodes. It adds role-based authorization functions in the message package, extends the network configuration to support pluggable authorizers, and applies this configuration across access, observer, and follower node builders. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
network/message/authorization_test.go (1)
46-119: Add one underlay-level test for the callback wiring.These tests only exercise the helper functions, and the “cross validation” path effectively treats every role as a possible receiver, so it can still pass even when
NetworkConfig.UnicastStreamAuthorizeris never invoked at runtime. A small integration test aroundunderlay.NewNetworkand incoming unicast handling would catch the regression this PR currently misses.Based on learnings: Ensure network messages are authenticated and validated; treat message handling with the same high-assurance principles as other protocol code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/message/authorization_test.go` around lines 46 - 119, Add an integration test that constructs an underlay network via underlay.NewNetwork using a test NetworkConfig with a stubbed UnicastStreamAuthorizer and a test incoming stream (e.g., a fake net.Conn or in-memory stream) to confirm the authorizer callback and message validation path are invoked; specifically, wire a UnicastStreamAuthorizer spy into NetworkConfig.UnicastStreamAuthorizer, start the network, inject a crafted unicast message on the incoming stream, and assert that the spy was called with the expected sender/receiver roles and that the message handler path validated/authenticated the message (fail the test if the authorizer is never called), so the test verifies callback wiring beyond the helper functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@network/underlay/network.go`:
- Around line 165-175: NetworkConfig.UnicastStreamAuthorizer is never used: add
a field UnicastStreamAuthorizer to the Network struct, initialize it in
NewNetwork from cfg.UnicastStreamAuthorizer (falling back to
message.IsAuthorizedUnicastSenderRole if nil), and in handleIncomingStream
consult that authorizer before reading/processing the unicast stream
(reject/close the stream if the callback returns false). Update references to
use Network.UnicastStreamAuthorizer so public-network overrides are honored.
---
Nitpick comments:
In `@network/message/authorization_test.go`:
- Around line 46-119: Add an integration test that constructs an underlay
network via underlay.NewNetwork using a test NetworkConfig with a stubbed
UnicastStreamAuthorizer and a test incoming stream (e.g., a fake net.Conn or
in-memory stream) to confirm the authorizer callback and message validation path
are invoked; specifically, wire a UnicastStreamAuthorizer spy into
NetworkConfig.UnicastStreamAuthorizer, start the network, inject a crafted
unicast message on the incoming stream, and assert that the spy was called with
the expected sender/receiver roles and that the message handler path
validated/authenticated the message (fail the test if the authorizer is never
called), so the test verifies callback wiring beyond the helper functions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c95c64d2-4d1a-4f62-ab89-6892537cf956
📒 Files selected for processing (6)
cmd/access/node_builder/access_node_builder.gocmd/observer/node_builder/observer_builder.gofollower/follower_builder.gonetwork/message/authorization.gonetwork/message/authorization_test.gonetwork/underlay/network.go
| UnicastStreamAuthorizer func(flow.Role, flow.Role) bool | ||
| } | ||
|
|
||
| // Validate validates the configuration, and sets default values for any missing fields. | ||
| func (cfg *NetworkConfig) Validate() { | ||
| if cfg.UnicastMessageTimeout <= 0 { | ||
| cfg.UnicastMessageTimeout = DefaultUnicastTimeout | ||
| } | ||
| if cfg.UnicastStreamAuthorizer == nil { | ||
| cfg.UnicastStreamAuthorizer = message.IsAuthorizedUnicastSenderRole | ||
| } |
There was a problem hiding this comment.
The new unicast authorizer is never enforced.
Line 165 adds the callback to NetworkConfig, but Network never stores it and neither NewNetwork nor handleIncomingStream calls it. That makes the new public-network overrides a no-op and leaves the default role gate unused as well. Please thread the callback into Network and consult it before reading from an incoming unicast stream.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@network/underlay/network.go` around lines 165 - 175,
NetworkConfig.UnicastStreamAuthorizer is never used: add a field
UnicastStreamAuthorizer to the Network struct, initialize it in NewNetwork from
cfg.UnicastStreamAuthorizer (falling back to
message.IsAuthorizedUnicastSenderRole if nil), and in handleIncomingStream
consult that authorizer before reading/processing the unicast stream
(reject/close the stream if the callback returns false). Update references to
use Network.UnicastStreamAuthorizer so public-network overrides are honored.
…unicast-stream-authorization-public-network
…unicast-stream-authorization-public-network
…unicast-stream-authorization-public-network
Skip role-based checks on the public network.
https://github.com/onflow/flow-go-internal/pull/7185
Summary by CodeRabbit
New Features
Tests