[Networking] GossipSub Publish Message Validation (Unknown Topic Spam)

[kc1116]

This PR adds RPC Publish messages validation to the control message validation inspector. When a node receives messages via a direct RPC from a remote peer, it processes them sequentially. However, if a message is on a topic the node has not subscribed, it is discarded. While this may seem harmless, an attacker can exploit this behavior to stage a Denial-of-Service attack by spamming a victim with a high volume of messages on unknown/invalid topics. This forces the victim to consume resources processing each message only to discard them. The control message validation inspector will now inspect a sample of the RPC publish messages and disseminate an invalid control message notification when the number of invalid messages exceeds the configured threshold.

This PR

• Adds 2 new configuration values and corresponding CLI Flags
  • RPCMessageMaxSampleSize gossipsub-rpc-message-max-sample-size : The max sample size used for RPC message validation. If the total number of RPC messages exceeds this value a sample will be taken but messages will not be truncated
  • RPCMessageErrorThreshold gossipsub-rpc-message-error-threshold: The threshold at which an error will be returned if the number of invalid RPC messages exceeds this value
• Inspects a sample of RPC publish messages after all control message validation has succeeded
  • Disseminates a single invalid control message notification for all errors encountered during validation

ref: https://github.com/dapperlabs/flow-go/issues/6798

[codecov-commenter]

Codecov Report

Attention: 52 lines in your changes are missing coverage. Please review.

Comparison is base (d86b4f4) 52.96% compared to head (8890d75) 55.86%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4720      +/-   ##
==========================================
+ Coverage   52.96%   55.86%   +2.90%     
==========================================
  Files         754      945     +191     
  Lines       68045    88259   +20214     
==========================================
+ Hits        36037    49304   +13267     
- Misses      29289    35243    +5954     
- Partials     2719     3712     +993     

Flag	Coverage Δ
unittests	55.86% <52.29%> (+2.90%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
network/p2p/inspector/validation/errors.go	95.91% <100.00%> (+0.68%)	⬆️
network/p2p/scoring/registry.go	90.12% <100.00%> (+0.18%)	⬆️
network/p2p/test/fixtures.go	35.92% <0.00%> (+0.70%)	⬆️
network/netconf/flags.go	38.02% <40.00%> (+0.04%)	⬆️
utils/unittest/unittest.go	8.67% <0.00%> (-0.31%)	⬇️
...validation/control_message_validation_inspector.go	77.39% <78.84%> (+0.12%)	⬆️
utils/unittest/fixtures.go	0.00% <0.00%> (ø)

... and 199 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[yhassanzadeh13]

Looks good overall, I have a suggestion about the dependency inversion that is imposed through SetSubscriptions. Please find my comment here.

[yhassanzadeh13]

⚠️ I recommend not passing a p2p.Subscriptions object to the GossipSub RPC inspector. Doing so violates the Dependency Inversion Principle and the Interface Segregation Principle. The p2p.Subscriptions interface is higher-level and should not be depended upon by a lower-level component like the RPC inspector. This design choice risks circular dependencies and also breaks encapsulation, as methods irrelevant to the RPC inspector get exposed.

Moreover, using p2p.Subscription to query active subscriptions is unreliable, as it queries the Flow node than the GossipSub router; it's more accurate to consult the underlying pubsub router to avoid any false-positive due to glitches. To address this, I suggest refactoring to use the PubSubAdapter.GetTopics() method.

Dependency injection is required between the RPC inspector and the GossipSub adapter. To solve this, inject gossipSub.GetTopics() as a topic oracle within the RPC inspector by adding a method SetTopicOracle(topicOracle func()[]string) to the GossipSubInspectorSuite interface:

type GossipSubInspectorSuite interface {
	// existing methods ...

	// SetTopicOracle sets the topic oracle of the gossipsub inspector suite.
	// The topic oracle is used to determine the list of topics that the node is subscribed to.
	// If an oracle is not set, the node will not be able to determine the list of topics that the node is subscribed to.
	// If an oracle is already set, a fatal error is logged.
	SetTopicOracle(topicOracle func()[]string)
}

Inject this dependency in the codebase like so:

	gossipSub, err := g.gossipSubFactory(ctx, g.logger, g.h, gossipSubConfigs, inspectorSuite)
	if err != nil {
		return nil, fmt.Errorf("could not create gossipsub: %w", err)
	}
	inspectorSuite.SetTopicOracle(gossipSub.GetTopics())

[kc1116]

d4245da

[yhassanzadeh13]

Thanks for applying the comment. However, there is still a minor concern. In reviewing the interfaces GossipSubInspectorSuite and InspectorTopicOracle, it's apparent that the encapsulation of SetTopicOracle within a separate interface does not provide a coherent or meaningful abstraction.

The InspectorTopicOracle has a single method SetTopicOracle, which is more of a configuration aspect rather than a functional capability deserving a separate interface. This violates the Single Responsibility Principle as it doesn’t represent a distinct responsibility that warrants a separate abstraction. Also, the name InspectorTopicOracle implies a broader functionality than merely setting a topic oracle. However, the sole method it encapsulates doesn't justify the semantics of an oracle. In this case, the oracle interface generally implies a source of information or decision-making capability, however, the capability this interface abstracts does not behave as a source of information.

My suggestion is to directly integrate the SetTopicOracle method into GossipSubInspectorSuite, which makes the design becomes more intuitive:

type GossipSubInspectorSuite interface {
	// existing methods ...

	// SetTopicOracle sets the topic oracle of the gossipsub inspector suite.
	// The topic oracle is used to determine the list of topics that the node is subscribed to.
	// If an oracle is not set, the node will not be able to determine the list of topics that the node is subscribed to.
	// If an oracle is already set, a fatal error is logged.
	SetTopicOracle(topicOracle func()[]string)
}

[kc1116]

4afb6a9

[yhassanzadeh13]

Looks very good, please address this comment before merging.

[gomisha]

Thanks for addressing all my comments.