network: Limit message length based on Tag #5388

iansuvak · 2023-05-15T21:35:44Z

Summary

This PR implements specific message length limits based on the protocol Tag of the message being currently read in the wsPeer.

The new <TypeName>MaxSize() int methods are being generated by the msgp library based on the supplied allocbounds and the new struct annotation totalallocbound used to limit the total number of bytes encoded in a collection type.

New constants are defined and Tag.MaxSize() method switching between them is used when reseting LimitedReaderSlurper. These don't override the global maxAllocation setting so if max message size is larger than the maxAllocation we will still error out with the old limit.

TODOs:

Fix allocbounds/totalallocbounds on StateProofs that are currently blowing up the maxsize of transactions.
~~Write a method calculating maxSize for TS TopicMsgRes tags~~ We decided to keep TS and VB at existing maximum message size since their theoretical limits weren't meaningfully different.
~~Implement actual enforcement of limits on topic messages (MI, TS, UE) since those are not read using LimitedReadSlurper~~ This statement was false. They are read using the slurper.

Test Plan

Add new tests to make sure that constants are defined for each Tag
Add tests to make sure that custom calculated constants are correct
Write new tests for the slurper to make sure that the per message limits work

catchup/universalFetcher_test.go

agreement/types.go

go.mod

go.sum

network/limited_reader_slurper.go

node/node_test.go

protocol/codec_tester.go

network/wsNetwork.go

protocol/txntype.go

data/transactions/signedtxn.go

data/transactions/teal.go

protocol/tags.go

…posals which have a hard limit of config.MaxTxnBytesPerBlock

Co-authored-by: Pavel Zbitskiy <[email protected]>

Changes after applying msgp alises fixes

algonautshant · 2023-06-20T19:37:13Z

crypto/merklearray/proof.go

+// proof size for individual message types we have smaller valid bounds.
+// Exported because it's used in the stateproof package for ensuring that SigPartProof constant is correct size.
+func ProofMaxSizeByElements(n int) (s int) {
+	s = 1 + 4


I am very worried about having such fine-grained calculations done manually.
The tradeoff we need to consider is:
what is the risk of allowing loose bounds versus having such fine-grained calculations done manually.

Still considering this, and making sure the calculations are correct.

It's actually not fully manual. It's a modification of the autogenerated test: here https://github.com/algorand/go-algorand/pull/5388/files#diff-add6a5ef46c2629eb7484bf3113df670d70cfdb6661ca69e236a778a7e1cb4c4R334

and we have a test here https://github.com/algorand/go-algorand/pull/5388/files#diff-181a1af1a38a976d2ddce9a94f778520e2ad787079c3cc99dbd1b7483386aa73R248 that ensures that that auto-generated ProofMaxSize and and ProofMaxSizeByElements output the same value when the allocbound is used as the input

The test is very good and reliable. However, the mapping of the function SingleLeafProofMaxSize to SingleLeafProof is not obvious for someone who is not aware of this arrangement.

In this particular case, this function is not needed. It can be implemented by setting a constant bound directly to the Proof field in:

Signature struct { _struct struct{} `codec:",omitempty,omitemptyarray"` Signature crypto.FalconSignature `codec:"sig"` VectorCommitmentIndex uint64 `codec:"idx"` Proof merklearray.SingleLeafProof `codec:"prf"`

Yeah but we do have a pattern for doing this manual implementation already for basics.MicroAlgos and this solution will continue working if we ever use SingleLeafProof elsewhere. Finding it's maxtotalbytes bound and using it is more implicit IMO.

crypto/merklearray/proof.go

This reverts commit e52b870.

Co-authored-by: Shant Karakashian <[email protected]>

crypto/merklearray/proof.go

algonautshant · 2023-06-22T21:13:39Z

crypto/merklearray/proof.go

+// proof size for individual message types we have smaller valid bounds.
+// Exported because it's used in the stateproof package for ensuring that SigPartProof constant is correct size.
+func ProofMaxSizeByElements(n int) (s int) {
+	s = 1 + 4


The test is very good and reliable. However, the mapping of the function SingleLeafProofMaxSize to SingleLeafProof is not obvious for someone who is not aware of this arrangement.

In this particular case, this function is not needed. It can be implemented by setting a constant bound directly to the Proof field in:

Signature struct { _struct struct{} `codec:",omitempty,omitemptyarray"` Signature crypto.FalconSignature `codec:"sig"` VectorCommitmentIndex uint64 `codec:"idx"` Proof merklearray.SingleLeafProof `codec:"prf"`

algonautshant · 2023-06-22T21:20:44Z

crypto/vrf.go

+// VRFVerifierMaxSize forwards to base implementation since it's expected by the msgp generated MaxSize functions
+func VRFVerifierMaxSize() int {
+	return VrfPubkeyMaxSize()
+}


Why is this needed? The bound can be set like elsewhere in the struct field where this is used.

because of type alias. This type is on TODO for removal.

algonautshant · 2023-06-22T21:30:24Z

network/msgOfInterest.go

+
+// MessageOfInterestMaxSize returns the maximum size of a MI message sent over the network
+// by encoding all of the tags currenttly in use.
+func MessageOfInterestMaxSize() int {


This should be in the test file, since it is only used from the test.

I can move it but would have to move it to node_test.go which seems like an odd place for it and it would break symmetry since all other MaxSize() functions are available in non-test code.

algonautshant · 2023-06-22T21:35:16Z

data/basics/units.go

@@ -108,6 +108,12 @@ func (a MicroAlgos) MsgIsZero() bool {
 	return a.Raw == 0
 }

+// MicroAlgosMaxSize returns maximum possible msgp encoded size of MicroAlgos in bytes.
+// It is expected by msgp generated MaxSize functions
+func MicroAlgosMaxSize() (s int) {


Why is this needed? MicroAlgos has one field of type uint64.

Same reason as the previous conversations we've had: msgp when it sees a type defined outside of the current file it's parsing it assumes that it has this function defined. MicroAlgos is an example of a struct that we generate fully manually and don't use msgp for but we still need this method and the ones above for the rest of the msgp generated functions to work since they call them when they are Marshalling/Unmarshalling themselves or checking their own MaxSize

protocol/tags_test.go

algorandskiy · 2023-06-22T21:55:43Z

crypto/vrf.go

+// VRFVerifierMaxSize forwards to base implementation since it's expected by the msgp generated MaxSize functions
+func VRFVerifierMaxSize() int {
+	return VrfPubkeyMaxSize()
+}


because of type alias. This type is on TODO for removal.

Co-authored-by: Pavel Zbitskiy <[email protected]>

AlgoAxel

I reviewed primarily the Network and Tags modules, and looked lightly at the rest.

AlgoAxel · 2023-06-23T18:12:54Z

ledger/testing/randomAccounts.go

@@ -100,7 +100,7 @@ func RandomAssetParams() basics.AssetParams {
 		DefaultFrozen: crypto.RandUint64()%2 == 0,
 	}
 	if crypto.RandUint64()%5 != 0 {
-		ap.UnitName = fmt.Sprintf("un%x", uint32(crypto.RandUint64()%0x7fffffff))
+		ap.UnitName = fmt.Sprintf("un%x", uint32(crypto.RandUint64()%0x7fffff))


I know you did not write the conditional, but do you happen to know why we check if the random int is divisible by 5?

network/wsPeer.go

AlgoAxel · 2023-06-23T18:18:54Z

network/wsPeer_test.go

 		for _, spec := range genDecl.Specs {
 			if valueSpec, ok := spec.(*ast.ValueSpec); ok {
 				for _, n := range valueSpec.Names {
-					if n.Name == "TagLength" {
+					if strings.HasSuffix(n.Name, "MaxSize") || n.Name == "TagLength" {
 						continue
 					}
 					declaredTags = append(declaredTags, n.Name)


if I'm reading this block right, we're trying to iterate over the valueSpec of a given tag, and through excluding MaxSize and TagLength, we expect that the only remaining thing is the declared tag's name? This seems fragile if/when more is added to the spec. Since it's just a test I don't know that it's worth pursuing.

Yeah it is fragile as you can see since both myself and Shant had to add exclusions here for MaxSize and TagLength respectively. Hopefully we won't be adding a lot more specs to protocol.Tags

protocol/tags.go

iansuvak added 13 commits May 15, 2023 16:52

WIP pre-rangexpr

a9131bb

rename to static method

6ce8cea

using 'static' methods

94dbf71

Added almost all bounds, missing pp, ue, ts

0d17dc6

UE = 67

73be798

Add totalallocbound for the infiniteloop

70ee7e3

add comments to exported constants and function

8153660

remove stateproofs from innertxns in proposal payload

67d89db

Fix some tests, add checking to limitedSlurper

a181944

Add test ensuring non-zero MaxSize is defined for each Tag

5ea2771

Cleanup

03df5fe

Add missing comment

0646f7f

more missing comments and cleanup

50a267d

iansuvak requested a review from algorandskiy May 15, 2023 21:36

iansuvak marked this pull request as draft May 15, 2023 21:36

updated msgp + uint64 casting

d6e6366

algorandskiy reviewed May 17, 2023

View reviewed changes

iansuvak added 2 commits May 17, 2023 11:29

Fix Stateproof and PP issues

ecda744

Add new AST test and more comments throughout

f7b369e

algorandskiy assigned iansuvak May 19, 2023

algorandskiy added Enhancement Team Carbon-11 labels May 19, 2023

iansuvak added 3 commits May 31, 2023 09:44

Merge remote-tracking branch 'upstream/master' into ian/max-message-size

4c25f21

Add special handling for TS, UE, VB

64288a5

Reference the reason for UE having separate testing method

66cbf61

algorandskiy reviewed Jun 7, 2023

View reviewed changes

data/transactions/signedtxn.go Outdated Show resolved Hide resolved

data/transactions/teal.go Outdated Show resolved Hide resolved

protocol/tags.go Outdated Show resolved Hide resolved

iansuvak and others added 4 commits June 7, 2023 11:42

Remove unnecessary SignedTxnWithADMaxSize because it was used for pro…

f82c316

…posals which have a hard limit of config.MaxTxnBytesPerBlock

Update protocol/tags.go

4580863

Co-authored-by: Pavel Zbitskiy <[email protected]>

Changes after applying msgp alises fixes

45b0d65

Merge pull request #3 from algorandskiy/ian/max-message-size

e9ba543

Changes after applying msgp alises fixes

iansuvak added 3 commits June 21, 2023 09:27

more comments

a500239

Unexport MakeBlockRequestTopics

4bd07f5

Add comment for alias-base type compat test

e52b870

algonautshant reviewed Jun 21, 2023

View reviewed changes

iansuvak and others added 6 commits June 21, 2023 12:06

Revert "Add comment for alias-base type compat test"

03f4b01

This reverts commit e52b870.

Update crypto/merklearray/proof.go

e563c69

Co-authored-by: Shant Karakashian <[email protected]>

Remove race from TestDiscardUnrequestedBlockResponse

02364e1

gofmt shenanigans

289b062

bump msgp version

e0cc3ee

Merge remote-tracking branch 'upstream/master' into ian/max-message-size

3e84468

algonautshant reviewed Jun 22, 2023

View reviewed changes

run go mod tidy in blockgenerator folder

5de753b

iansuvak marked this pull request as ready for review June 22, 2023 21:43

iansuvak changed the title ~~WIP: network: Limit message length based on Tag~~ network: Limit message length based on Tag Jun 22, 2023

make sanity

9db27dd

algorandskiy reviewed Jun 22, 2023

View reviewed changes

iansuvak and others added 3 commits June 23, 2023 05:21

move msgp directive outside of the doc-comment region

7d31ac7

Update protocol/tags_test.go

b64ca5b

Co-authored-by: Pavel Zbitskiy <[email protected]>

%s/returnNames/namesOnly

ffcf43e

algorandskiy previously approved these changes Jun 23, 2023

View reviewed changes

add limitedReadSlurper test

52cc1d8

iansuvak dismissed algorandskiy’s stale review via 52cc1d8 June 23, 2023 16:56

AlgoAxel reviewed Jun 23, 2023

View reviewed changes

add a scary comment

036f012

algorandskiy approved these changes Jun 23, 2023

View reviewed changes

AlgoAxel approved these changes Jun 23, 2023

View reviewed changes

algorandskiy merged commit c223dbc into algorand:master Jun 23, 2023

Algo-devops-service mentioned this pull request Jul 11, 2023

go-algorand 3.17.0-beta Release PR #5541

Merged

onetechnical mentioned this pull request Jul 24, 2023

go-algorand 3.17.0-beta Release PR #5601

Merged

Algo-devops-service mentioned this pull request Aug 3, 2023

go-algorand 3.17.0-stable Release PR #5633

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

network: Limit message length based on Tag #5388

network: Limit message length based on Tag #5388

iansuvak commented May 15, 2023 •

edited

Loading

algonautshant Jun 20, 2023

iansuvak Jun 21, 2023

algonautshant Jun 22, 2023

iansuvak Jun 23, 2023

algonautshant Jun 22, 2023

algonautshant Jun 22, 2023

algorandskiy Jun 22, 2023

algonautshant Jun 22, 2023

iansuvak Jun 23, 2023

algonautshant Jun 22, 2023

iansuvak Jun 22, 2023

algorandskiy Jun 22, 2023

AlgoAxel left a comment

AlgoAxel Jun 23, 2023

AlgoAxel Jun 23, 2023

iansuvak Jun 23, 2023

network: Limit message length based on Tag #5388

network: Limit message length based on Tag #5388

Conversation

iansuvak commented May 15, 2023 • edited Loading

Summary

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlgoAxel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iansuvak commented May 15, 2023 •

edited

Loading