[Networking] Add unicast stream pre-authorization by sender/receiver role#8524
Merged
peterargue merged 10 commits intoApr 8, 2026
Merged
Conversation
Contributor
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 |
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR introduces role-based unicast stream authorization to the network layer. The Changes
Sequence DiagramsequenceDiagram
actor Sender as Sender<br/>(with role)
participant NetLayer as Network Layer<br/>(Receiver)
participant Auth as Authorization<br/>Service
actor Receiver as Receiver<br/>(with role)
Sender->>NetLayer: Initiate unicast stream
NetLayer->>NetLayer: Accept incoming stream
NetLayer->>NetLayer: Resolve remote peer identity
NetLayer->>Auth: IsAuthorizedUnicastSender<br/>(senderRole, receiverRole)
alt Authorized
Auth-->>NetLayer: true
NetLayer->>NetLayer: Proceed with stream<br/>reading/handling
NetLayer->>Receiver: ✓ Stream active
else Unauthorized
Auth-->>NetLayer: false
NetLayer->>NetLayer: Log violation
NetLayer->>Receiver: Report OnUnauthorizedUnicastOnChannel
NetLayer->>Sender: ✗ Reject stream
end
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Found 24 test failures on Blacksmith runners: Failures
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
AGENTS.md (1)
89-96: Use a markdown subsection heading for abbreviations.Line 89 currently uses plain text (
Abbreviations:). Consider### Abbreviationsto match the document’s heading structure and improve scanability.Proposed doc-only tweak
-Abbreviations: +### Abbreviations - **AN**: Access Node - **LN**: Collection Node - **SN**: Consensus Node - **EN**: Execution Node - **VN**: Verification Node - **ON**: Observer Node🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 89 - 96, Replace the plain text "Abbreviations:" heading with a markdown subsection heading to match the document structure — change the line containing Abbreviations: to a header like "### Abbreviations" so the abbreviations block (AN, LN, SN, EN, VN, ON) is a proper subsection and improves scanability.engine/testutil/mocklocal/local.go (1)
23-29: Don't makeRoleConsensusthe implicit mock role.
RoleConsensusis the most permissive sender in the new unicast matrix. Making it the silent default means role-sensitive tests can pass without ever exercising the intended role. Prefer taking the role inNewMockLocal(or adding an explicit setter) so tests have to opt into the behavior they want.Suggested change
-func NewMockLocal(sk crypto.PrivateKey, id flow.Identifier, t mock.TestingT) *MockLocal { +func NewMockLocal(sk crypto.PrivateKey, id flow.Identifier, role flow.Role, t mock.TestingT) *MockLocal { return &MockLocal{ sk: sk, t: t, id: id, - role: flow.RoleConsensus, + role: role, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/testutil/mocklocal/local.go` around lines 23 - 29, NewMockLocal currently sets MockLocal.role to flow.RoleConsensus by default which hides role-sensitive bugs; change NewMockLocal to accept a role parameter (e.g., NewMockLocal(sk, id, t, role flow.Role) or add an explicit SetRole method on MockLocal) and remove the implicit flow.RoleConsensus assignment in the constructor, update all callers to pass the intended role (or call SetRole) so tests must opt into permissive consensus behavior; ensure the MockLocal struct's role field is initialized only from the provided argument or setter.network/internal/testutils/testUtil.go (1)
222-222: Keep the production pre-authorizer as the fixture default.Hardwiring an allow-all function here makes every test built on
NetworkConfigFixturesilently bypass the new stream gate, so regressions in the default authorization path will only show up if a caller explicitly overrides this back. Prefer leaving this unset in the shared fixture and only installing the permissive authorizer in the specific tests that need role-agnostic wiring.Suggested change
- UnicastStreamAuthorizer: func(_, _ flow.Role) bool { return true },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@network/internal/testutils/testUtil.go` at line 222, The fixture currently hardwires an allow-all UnicastStreamAuthorizer (UnicastStreamAuthorizer: func(_, _ flow.Role) bool { return true }) which bypasses the stream gate for every test; remove or leave UnicastStreamAuthorizer unset/default in NetworkConfigFixture so the production pre-authorizer is used by default, and instead install the permissive authorizer only in individual tests that need it (set the authorizer explicitly in those tests). Ensure references to UnicastStreamAuthorizer in NetworkConfigFixture are cleared or nil so default authorization behavior applies.
🤖 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 1001-1013: The code currently reports a channel-scoped violation
via n.slashingViolationsConsumer.OnUnauthorizedUnicastOnChannel when
n.unicastStreamAuthorizer rejects the stream before any channel/frame exists and
without setting stable identity fields; instead, call the
pre-auth/unauthorized-sender handler (e.g., a method like
OnUnauthorizedUnicastPreAuth or a non-channel-specific consumer API) or populate
the network.Violation with stable identity fields (set OriginID from
remoteIdentity.NodeID, fill Identity/PeerID/Protocol and Err with
ErrUnauthorizedUnicastSender) before emitting; update the call site that
currently invokes OnUnauthorizedUnicastOnChannel to use the appropriate pre-auth
consumer method or construct the full network.Violation including OriginID so
the slashing/audit path has a complete record.
- Around line 1017-1024: The receive-side timeout is hardcoded to
DefaultUnicastTimeout causing inbound deadlines to ignore the configured
NetworkConfig.UnicastMessageTimeout; update the code that sets unicastTimeout
(the variable used to create ctx via context.WithTimeout with n.ctx) to default
to the configured value (NetworkConfig.UnicastMessageTimeout on your node config
object) and only override it to LargeMsgUnicastTimeout when the
n.me.Role()/remoteIdentity.Role branch selects large messages, so inbound
deadlines match the configured unicast timeout used elsewhere.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 89-96: Replace the plain text "Abbreviations:" heading with a
markdown subsection heading to match the document structure — change the line
containing Abbreviations: to a header like "### Abbreviations" so the
abbreviations block (AN, LN, SN, EN, VN, ON) is a proper subsection and improves
scanability.
In `@engine/testutil/mocklocal/local.go`:
- Around line 23-29: NewMockLocal currently sets MockLocal.role to
flow.RoleConsensus by default which hides role-sensitive bugs; change
NewMockLocal to accept a role parameter (e.g., NewMockLocal(sk, id, t, role
flow.Role) or add an explicit SetRole method on MockLocal) and remove the
implicit flow.RoleConsensus assignment in the constructor, update all callers to
pass the intended role (or call SetRole) so tests must opt into permissive
consensus behavior; ensure the MockLocal struct's role field is initialized only
from the provided argument or setter.
In `@network/internal/testutils/testUtil.go`:
- Line 222: The fixture currently hardwires an allow-all UnicastStreamAuthorizer
(UnicastStreamAuthorizer: func(_, _ flow.Role) bool { return true }) which
bypasses the stream gate for every test; remove or leave UnicastStreamAuthorizer
unset/default in NetworkConfigFixture so the production pre-authorizer is used
by default, and instead install the permissive authorizer only in individual
tests that need it (set the authorizer explicitly in those tests). Ensure
references to UnicastStreamAuthorizer in NetworkConfigFixture are cleared or nil
so default authorization behavior applies.
🪄 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: c83a67b8-f420-4d5b-9b5c-a34080839445
📒 Files selected for processing (12)
AGENTS.mdengine/testutil/mocklocal/local.gomodule/local.gomodule/local/me.gomodule/local/me_nokey.gomodule/mock/local.gonetwork/internal/testutils/testUtil.gonetwork/message/authorization.gonetwork/message/authorization_test.gonetwork/test/cohort1/network_test.gonetwork/test/cohort2/unicast_authorization_test.gonetwork/underlay/network.go
Comment on lines
+1001
to
+1013
| if !n.unicastStreamAuthorizer(remoteIdentity.Role, n.me.Role()) { | ||
| log.Warn(). | ||
| Str("remote_peer", remotePeer.String()). | ||
| Str("remote_role", remoteIdentity.Role.String()). | ||
| Str("local_role", n.me.Role().String()). | ||
| Bool(logging.KeySuspicious, true). | ||
| Msg("rejecting unicast stream from unauthorized sender role") | ||
| n.slashingViolationsConsumer.OnUnauthorizedUnicastOnChannel(&network.Violation{ | ||
| Identity: remoteIdentity, | ||
| PeerID: p2plogging.PeerId(remotePeer), | ||
| Protocol: message.ProtocolTypeUnicast, | ||
| Err: ErrUnauthorizedUnicastSender, | ||
| }) |
Contributor
There was a problem hiding this comment.
Don't report a channel-scoped violation before any channel exists.
This branch rejects the stream before a frame is read, so Channel and MsgType are unknowable here, but it still goes through OnUnauthorizedUnicastOnChannel. The emitted network.Violation also drops OriginID even though remoteIdentity.NodeID is available, which leaves the slashing/audit path with a partial record. Please route this through a pre-auth/unauthorized-sender violation instead, or at least populate the stable identity fields here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@network/underlay/network.go` around lines 1001 - 1013, The code currently
reports a channel-scoped violation via
n.slashingViolationsConsumer.OnUnauthorizedUnicastOnChannel when
n.unicastStreamAuthorizer rejects the stream before any channel/frame exists and
without setting stable identity fields; instead, call the
pre-auth/unauthorized-sender handler (e.g., a method like
OnUnauthorizedUnicastPreAuth or a non-channel-specific consumer API) or populate
the network.Violation with stable identity fields (set OriginID from
remoteIdentity.NodeID, fill Identity/PeerID/Protocol and Err with
ErrUnauthorizedUnicastSender) before emitting; update the call site that
currently invokes OnUnauthorizedUnicastOnChannel to use the appropriate pre-auth
consumer method or construct the full network.Violation including OriginID so
the slashing/audit path has a complete record.
peterargue
approved these changes
Apr 8, 2026
…unicast-stream-authorization-public-network
…unicast-stream-authorization-public-network
…unicast-stream-authorization-public-network
fxamacker
approved these changes
Apr 8, 2026
…tion-public-network Override role based unicast stream authorization on public network
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, the networking layer will perform authorization of unicast messages only after fully receiving the message and parsing it's type. This mean unauthorized peers are able to tie up resources of peers that they have no business sending unicast messages to.
This PR adds an initial coarse grained authorization based on role before reading any data from the stream.
https://github.com/onflow/flow-go-internal/pull/7178
Summary by CodeRabbit
Release Notes
New Features
Tests