From ce6cc094a11e787e3662e966d76e876ed43c9fa8 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 14 May 2020 21:10:03 -0400 Subject: [PATCH] intentions: fix a bug in Intention.SetHash Found using staticcheck. binary.Write does not accept int types without a size. The error from binary.Write was ignored, so we never saw this error. Casting the data to uint64 produces a correct hash. Also deprecate the Default{Addr,Port} fields, and prevent them from being encoded. These fields will always be empty and are not used. Removing these would break backwards compatibility, so they are left in place for now. Co-authored-by: Hans Hasselberg --- agent/consul/intention_endpoint.go | 4 +- agent/http_decode_test.go | 2 - agent/structs/intention.go | 104 +++++++++--------- agent/structs/intention_test.go | 25 +++++ agent/structs/structs_filtering_test.go | 10 -- api/connect_intention.go | 10 +- ui-v2/app/models/intention.js | 5 - website/pages/api-docs/connect/intentions.mdx | 10 -- 8 files changed, 82 insertions(+), 88 deletions(-) diff --git a/agent/consul/intention_endpoint.go b/agent/consul/intention_endpoint.go index 676632c4f0..5560a778e9 100644 --- a/agent/consul/intention_endpoint.go +++ b/agent/consul/intention_endpoint.go @@ -86,7 +86,7 @@ func (s *Intention) prepareApplyCreate(ident structs.ACLIdentity, authz acl.Auth } // make sure we set the hash prior to raft application - args.Intention.SetHash(true) + args.Intention.SetHash() return nil } @@ -145,7 +145,7 @@ func (s *Intention) prepareApplyUpdate(ident structs.ACLIdentity, authz acl.Auth } // make sure we set the hash prior to raft application - args.Intention.SetHash(true) + args.Intention.SetHash() return nil } diff --git a/agent/http_decode_test.go b/agent/http_decode_test.go index 9993614439..32f56c9db3 100644 --- a/agent/http_decode_test.go +++ b/agent/http_decode_test.go @@ -1996,8 +1996,6 @@ func TestDecodeCatalogRegister(t *testing.T) { // DestinationName string // SourceType structs.IntentionSourceType // Action structs.IntentionAction -// DefaultAddr string -// DefaultPort int // Meta map[string]string // Precedence int // CreatedAt time.Time mapstructure:'-' diff --git a/agent/structs/intention.go b/agent/structs/intention.go index 9406a46726..84a3101e5d 100644 --- a/agent/structs/intention.go +++ b/agent/structs/intention.go @@ -55,10 +55,12 @@ type Intention struct { // Action is whether this is an allowlist or denylist intention. Action IntentionAction - // DefaultAddr, DefaultPort of the local listening proxy (if any) to - // make this connection. - DefaultAddr string - DefaultPort int + // DefaultAddr is not used. + // Deprecated: DefaultAddr is not used and may be removed in a future version. + DefaultAddr string `bexpr:"-" codec:",omitempty"` + // DefaultPort is not used. + // Deprecated: DefaultPort is not used and may be removed in a future version. + DefaultPort int `bexpr:"-" codec:",omitempty"` // Meta is arbitrary metadata associated with the intention. This is // opaque to Consul but is served in API responses. @@ -103,55 +105,47 @@ func (t *Intention) UnmarshalJSON(data []byte) (err error) { return nil } -// nolint: staticcheck // should be fixed in https://github.com/hashicorp/consul/pull/7900 -func (x *Intention) SetHash(force bool) []byte { - if force || x.Hash == nil { - hash, err := blake2b.New256(nil) - if err != nil { - panic(err) - } - - // Any non-immutable "content" fields should be involved with the - // overall hash. The IDs are immutable which is why they aren't here. - // The raft indices are metadata similar to the hash which is why they - // aren't incorporated. CreateTime is similarly immutable - // - // The Hash is really only used for replication to determine if a token - // has changed and should be updated locally. - - // Write all the user set fields - hash.Write([]byte(x.ID)) - hash.Write([]byte(x.Description)) - hash.Write([]byte(x.SourceNS)) - hash.Write([]byte(x.SourceName)) - hash.Write([]byte(x.DestinationNS)) - hash.Write([]byte(x.DestinationName)) - hash.Write([]byte(x.SourceType)) - hash.Write([]byte(x.Action)) - hash.Write([]byte(x.DefaultAddr)) - binary.Write(hash, binary.LittleEndian, x.DefaultPort) - binary.Write(hash, binary.LittleEndian, x.Precedence) - - // hashing the metadata - var keys []string - for k := range x.Meta { - keys = append(keys, k) - } - - // keep them sorted to ensure hash stability - sort.Strings(keys) - - for _, k := range keys { - hash.Write([]byte(k)) - hash.Write([]byte(x.Meta[k])) - } - - // Finalize the hash - hashVal := hash.Sum(nil) - - x.Hash = hashVal +// SetHash calculates Intention.Hash from any mutable "content" fields. +// +// The Hash is primarily used for replication to determine if a token +// has changed and should be updated locally. +// +// TODO: move to agent/consul where it is called +func (x *Intention) SetHash() { + hash, err := blake2b.New256(nil) + if err != nil { + panic(err) } - return x.Hash + + // Write all the user set fields + hash.Write([]byte(x.ID)) + hash.Write([]byte(x.Description)) + hash.Write([]byte(x.SourceNS)) + hash.Write([]byte(x.SourceName)) + hash.Write([]byte(x.DestinationNS)) + hash.Write([]byte(x.DestinationName)) + hash.Write([]byte(x.SourceType)) + hash.Write([]byte(x.Action)) + // hash.Write can not return an error, so the only way for binary.Write to + // error is to pass it data with an invalid data type. Doing so would be a + // programming error, so panic in that case. + if err := binary.Write(hash, binary.LittleEndian, uint64(x.Precedence)); err != nil { + panic(err) + } + + // sort keys to ensure hash stability when meta is stored later + var keys []string + for k := range x.Meta { + keys = append(keys, k) + } + sort.Strings(keys) + + for _, k := range keys { + hash.Write([]byte(k)) + hash.Write([]byte(x.Meta[k])) + } + + x.Hash = hash.Sum(nil) } // Validate returns an error if the intention is invalid for inserting @@ -337,9 +331,9 @@ func (x *Intention) String() string { // EstimateSize returns an estimate (in bytes) of the size of this structure when encoded. func (x *Intention) EstimateSize() int { - // 60 = 36 (uuid) + 16 (RaftIndex) + 4 (Precedence) + 4 (DefaultPort) - size := 60 + len(x.Description) + len(x.SourceNS) + len(x.SourceName) + len(x.DestinationNS) + - len(x.DestinationName) + len(x.SourceType) + len(x.Action) + len(x.DefaultAddr) + // 56 = 36 (uuid) + 16 (RaftIndex) + 4 (Precedence) + size := 56 + len(x.Description) + len(x.SourceNS) + len(x.SourceName) + len(x.DestinationNS) + + len(x.DestinationName) + len(x.SourceType) + len(x.Action) for k, v := range x.Meta { size += len(k) + len(v) diff --git a/agent/structs/intention_test.go b/agent/structs/intention_test.go index fa0cbb75a6..6665ef43c3 100644 --- a/agent/structs/intention_test.go +++ b/agent/structs/intention_test.go @@ -334,3 +334,28 @@ func TestIntentionPrecedenceSorter(t *testing.T) { }) } } + +func TestIntention_SetHash(t *testing.T) { + i := Intention{ + ID: "the-id", + Description: "the-description", + SourceNS: "source-ns", + SourceName: "source-name", + DestinationNS: "dest-ns", + DestinationName: "dest-name", + SourceType: "source-type", + Action: "action", + Precedence: 123, + Meta: map[string]string{ + "meta1": "one", + "meta2": "two", + }, + } + i.SetHash() + expected := []byte{ + 0x20, 0x89, 0x55, 0xdb, 0x69, 0x34, 0xce, 0x89, 0xd8, 0xb9, 0x2e, 0x3a, + 0x85, 0xb6, 0xea, 0x43, 0xb2, 0x23, 0x16, 0x93, 0x94, 0x13, 0x2a, 0xe4, + 0x81, 0xfe, 0xe, 0x34, 0x91, 0x99, 0xe9, 0x8d, + } + require.Equal(t, expected, i.Hash) +} diff --git a/agent/structs/structs_filtering_test.go b/agent/structs/structs_filtering_test.go index ad981b8bd2..da63441118 100644 --- a/agent/structs/structs_filtering_test.go +++ b/agent/structs/structs_filtering_test.go @@ -566,16 +566,6 @@ var expectedFieldConfigIntention bexpr.FieldConfigurations = bexpr.FieldConfigur CoerceFn: bexpr.CoerceString, SupportedOperations: []bexpr.MatchOperator{bexpr.MatchEqual, bexpr.MatchNotEqual, bexpr.MatchIn, bexpr.MatchNotIn, bexpr.MatchMatches, bexpr.MatchNotMatches}, }, - "DefaultAddr": &bexpr.FieldConfiguration{ - StructFieldName: "DefaultAddr", - CoerceFn: bexpr.CoerceString, - SupportedOperations: []bexpr.MatchOperator{bexpr.MatchEqual, bexpr.MatchNotEqual, bexpr.MatchIn, bexpr.MatchNotIn, bexpr.MatchMatches, bexpr.MatchNotMatches}, - }, - "DefaultPort": &bexpr.FieldConfiguration{ - StructFieldName: "DefaultPort", - CoerceFn: bexpr.CoerceInt, - SupportedOperations: []bexpr.MatchOperator{bexpr.MatchEqual, bexpr.MatchNotEqual}, - }, "Precedence": &bexpr.FieldConfiguration{ StructFieldName: "Precedence", CoerceFn: bexpr.CoerceInt, diff --git a/api/connect_intention.go b/api/connect_intention.go index 3db177c7b4..c7c36b6d27 100644 --- a/api/connect_intention.go +++ b/api/connect_intention.go @@ -36,10 +36,12 @@ type Intention struct { // Action is whether this is an allowlist or denylist intention. Action IntentionAction - // DefaultAddr, DefaultPort of the local listening proxy (if any) to - // make this connection. - DefaultAddr string - DefaultPort int + // DefaultAddr is not used. + // Deprecated: DefaultAddr is not used and may be removed in a future version. + DefaultAddr string `json:",omitempty"` + // DefaultPort is not used. + // Deprecated: DefaultPort is not used and may be removed in a future version. + DefaultPort int `json:",omitempty"` // Meta is arbitrary metadata associated with the intention. This is // opaque to Consul but is served in API responses. diff --git a/ui-v2/app/models/intention.js b/ui-v2/app/models/intention.js index 6d0e782350..1e18ff43af 100644 --- a/ui-v2/app/models/intention.js +++ b/ui-v2/app/models/intention.js @@ -14,11 +14,6 @@ export default Model.extend({ Precedence: attr('number'), SourceType: attr('string', { defaultValue: 'consul' }), Action: attr('string', { defaultValue: 'deny' }), - // These are in the API response but up until now - // aren't used for anything - DefaultAddr: attr('string'), - DefaultPort: attr('number'), - // Meta: attr(), SyncTime: attr('number'), Datacenter: attr('string'), diff --git a/website/pages/api-docs/connect/intentions.mdx b/website/pages/api-docs/connect/intentions.mdx index 372bba61ca..a863f11852 100644 --- a/website/pages/api-docs/connect/intentions.mdx +++ b/website/pages/api-docs/connect/intentions.mdx @@ -145,8 +145,6 @@ $ curl \ "DestinationName": "db", "SourceType": "consul", "Action": "allow", - "DefaultAddr": "", - "DefaultPort": 0, "Meta": {}, "Precedence": 9, "CreatedAt": "2018-05-21T16:41:27.977155457Z", @@ -208,8 +206,6 @@ $ curl \ "DestinationName": "db", "SourceType": "consul", "Action": "allow", - "DefaultAddr": "", - "DefaultPort": 0, "Meta": {}, "Precedence": 9, "CreatedAt": "2018-05-21T16:41:27.977155457Z", @@ -228,8 +224,6 @@ the following selectors and filter operations being supported: | Selector | Supported Operations | | ----------------- | -------------------------------------------------- | | `Action` | Equal, Not Equal, In, Not In, Matches, Not Matches | -| `DefaultAddr` | Equal, Not Equal, In, Not In, Matches, Not Matches | -| `DefaultPort` | Equal, Not Equal | | `Description` | Equal, Not Equal, In, Not In, Matches, Not Matches | | `DestinationNS` | Equal, Not Equal, In, Not In, Matches, Not Matches | | `DestinationName` | Equal, Not Equal, In, Not In, Matches, Not Matches | @@ -450,8 +444,6 @@ $ curl \ "DestinationName": "db", "SourceType": "consul", "Action": "deny", - "DefaultAddr": "", - "DefaultPort": 0, "Meta": {}, "CreatedAt": "2018-05-21T16:41:33.296693825Z", "UpdatedAt": "2018-05-21T16:41:33.296694288Z", @@ -467,8 +459,6 @@ $ curl \ "DestinationName": "*", "SourceType": "consul", "Action": "allow", - "DefaultAddr": "", - "DefaultPort": 0, "Meta": {}, "CreatedAt": "2018-05-21T16:41:27.977155457Z", "UpdatedAt": "2018-05-21T16:41:27.977157724Z",