status-im
/
consul
mirror of https://github.com/status-im/consul.git
2
0
Fork 0
Code Issues Projects Releases Wiki Activity

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 <me@hans.io>
This commit is contained in:
Daniel Nephin 2020-05-14 21:10:03 -04:00
parent fed7489a37
commit ce6cc094a1
8 changed files with 82 additions and 88 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
agent/consul/intention_endpoint.go
View File

@ -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
}

2
agent/http_decode_test.go
View File

@ -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:'-'

104
agent/structs/intention.go
View File

@ -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)

25
agent/structs/intention_test.go
View File

@ -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)
}

10
agent/structs/structs_filtering_test.go
View File

@ -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,

10
api/connect_intention.go
View File

@ -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.

5
ui-v2/app/models/intention.js
View File

@ -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'),

10
website/pages/api-docs/connect/intentions.mdx
View File

@ -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",