From 548ae2e0173b222e59a117e8347ffaac6acefa5d Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Mon, 9 Nov 2020 17:31:21 -0500 Subject: [PATCH] Add descriptions and missing validation for service-intentions (#385) * Add descriptions and missing validation for service-intentions Co-authored-by: Iryna Shustava Co-authored-by: Luke Kysow --- api/v1alpha1/serviceintentions_types.go | 153 ++++++++++--- api/v1alpha1/serviceintentions_types_test.go | 216 ++++++++++++++++-- ...onsul.hashicorp.com_serviceintentions.yaml | 25 +- 3 files changed, 349 insertions(+), 45 deletions(-) diff --git a/api/v1alpha1/serviceintentions_types.go b/api/v1alpha1/serviceintentions_types.go index 3cca51bf38..1c52d032a7 100644 --- a/api/v1alpha1/serviceintentions_types.go +++ b/api/v1alpha1/serviceintentions_types.go @@ -2,6 +2,7 @@ package v1alpha1 import ( "encoding/json" + "net/http" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" @@ -18,12 +19,22 @@ import ( // ServiceIntentionsSpec defines the desired state of ServiceIntentions type ServiceIntentionsSpec struct { - Destination Destination `json:"destination,omitempty"` - Sources SourceIntentions `json:"sources,omitempty"` + // Destination is the intention destination that will have the authorization granted to. + Destination Destination `json:"destination,omitempty"` + // Sources is the list of all intention sources and the authorization granted to those sources. + // The order of this list does not matter, but out of convenience Consul will always store this + // reverse sorted by intention precedence, as that is the order that they will be evaluated at enforcement time. + Sources SourceIntentions `json:"sources,omitempty"` } type Destination struct { - Name string `json:"name,omitempty"` + // Name is the destination of all intentions defined in this config entry. + // This may be set to the wildcard character (*) to match + // all services that don't otherwise have intentions defined. + Name string `json:"name,omitempty"` + // Namespace specifies the namespace the config entry will apply to. + // This may be set to the wildcard character (*) to match all services + // in all namespaces that don't otherwise have intentions defined. Namespace string `json:"namespace,omitempty"` } @@ -32,36 +43,63 @@ type IntentionPermissions []*IntentionPermission type IntentionHTTPHeaderPermissions []IntentionHTTPHeaderPermission type SourceIntention struct { - Name string `json:"name,omitempty"` - Namespace string `json:"namespace,omitempty"` - Action IntentionAction `json:"action,omitempty"` + // Name is the source of the intention. This is the name of a + // Consul service. The service doesn't need to be registered. + Name string `json:"name,omitempty"` + // Namespace is the namespace for the Name parameter. + Namespace string `json:"namespace,omitempty"` + // Action is required for an L4 intention, and should be set to one of + // "allow" or "deny" for the action that should be taken if this intention matches a request. + Action IntentionAction `json:"action,omitempty"` + // Permissions is the list of all additional L7 attributes that extend the intention match criteria. + // Permission precedence is applied top to bottom. For any given request the first permission to match + // in the list is terminal and stops further evaluation. As with L4 intentions, traffic that fails to + // match any of the provided permissions in this intention will be subject to the default intention + // behavior is defined by the default ACL policy. This should be omitted for an L4 intention + // as it is mutually exclusive with the Action field. Permissions IntentionPermissions `json:"permissions,omitempty"` - Description string `json:"description,omitempty"` + // Description for the intention. This is not used by Consul, but is presented in API responses to assist tooling. + Description string `json:"description,omitempty"` } type IntentionPermission struct { - Action IntentionAction `json:"action,omitempty"` - HTTP *IntentionHTTPPermission `json:"http,omitempty"` + // Action is one of "allow" or "deny" for the action that + // should be taken if this permission matches a request. + Action IntentionAction `json:"action,omitempty"` + // HTTP is a set of HTTP-specific authorization criteria. + HTTP *IntentionHTTPPermission `json:"http,omitempty"` } type IntentionHTTPPermission struct { - PathExact string `json:"pathExact,omitempty"` + // PathExact is the exact path to match on the HTTP request path. + PathExact string `json:"pathExact,omitempty"` + // PathPrefix is the path prefix to match on the HTTP request path. PathPrefix string `json:"pathPrefix,omitempty"` - PathRegex string `json:"pathRegex,omitempty"` - + // PathRegex is the regular expression to match on the HTTP request path. + PathRegex string `json:"pathRegex,omitempty"` + // Header is a set of criteria that can match on HTTP request headers. + // If more than one is configured all must match for the overall match to apply. Header IntentionHTTPHeaderPermissions `json:"header,omitempty"` - + // Methods is a list of HTTP methods for which this match applies. If unspecified + // all HTTP methods are matched. If provided the names must be a valid method. Methods []string `json:"methods,omitempty"` } type IntentionHTTPHeaderPermission struct { - Name string `json:"name,omitempty"` - Present bool `json:"present,omitempty"` - Exact string `json:"exact,omitempty"` - Prefix string `json:"prefix,omitempty"` - Suffix string `json:"suffix,omitempty"` - Regex string `json:"regex,omitempty"` - Invert bool `json:"invert,omitempty"` + // Name is the name of the header to match. + Name string `json:"name,omitempty"` + // Present matches if the header with the given name is present with any value. + Present bool `json:"present,omitempty"` + // Exact matches if the header with the given name is this value. + Exact string `json:"exact,omitempty"` + // Prefix matches if the header with the given name has this prefix. + Prefix string `json:"prefix,omitempty"` + // Suffix matches if the header with the given name has this suffix. + Suffix string `json:"suffix,omitempty"` + // Regex matches if the header with the given name matches this pattern. + Regex string `json:"regex,omitempty"` + // Invert inverts the logic of the match. + Invert bool `json:"invert,omitempty"` } // IntentionAction is the action that the intention represents. This @@ -196,13 +234,13 @@ func (in IntentionPermissions) toConsul() []*capi.IntentionPermission { for _, permission := range in { consulIntentionPermissions = append(consulIntentionPermissions, &capi.IntentionPermission{ Action: permission.Action.toConsul(), - HTTP: permission.HTTP.ToConsul(), + HTTP: permission.HTTP.toConsul(), }) } return consulIntentionPermissions } -func (in *IntentionHTTPPermission) ToConsul() *capi.IntentionHTTPPermission { +func (in *IntentionHTTPPermission) toConsul() *capi.IntentionHTTPPermission { if in == nil { return nil } @@ -296,11 +334,64 @@ func (in IntentionPermissions) validate(path *field.Path) field.ErrorList { func (in *IntentionHTTPPermission) validate(path *field.Path) field.ErrorList { var errs field.ErrorList - if invalidPathPrefix(in.PathPrefix) { - errs = append(errs, field.Invalid(path.Child("pathPrefix"), in.PathPrefix, `must begin with a '/'`)) + pathParts := 0 + if in.PathRegex != "" { + pathParts++ + } + if in.PathPrefix != "" { + pathParts++ + if invalidPathPrefix(in.PathPrefix) { + errs = append(errs, field.Invalid(path.Child("pathPrefix"), in.PathPrefix, `must begin with a '/'`)) + } + } + if in.PathExact != "" { + pathParts++ + if invalidPathPrefix(in.PathExact) { + errs = append(errs, field.Invalid(path.Child("pathExact"), in.PathExact, `must begin with a '/'`)) + } + } + if pathParts > 1 { + asJSON, _ := json.Marshal(in) + errs = append(errs, field.Invalid(path, string(asJSON), `at most only one of pathExact, pathPrefix, or pathRegex may be configured.`)) + } + + found := make(map[string]struct{}) + for i, method := range in.Methods { + methods := []string{ + http.MethodGet, + http.MethodHead, + http.MethodPost, + http.MethodPut, + http.MethodPatch, + http.MethodDelete, + http.MethodConnect, + http.MethodOptions, + http.MethodTrace, + } + if !sliceContains(methods, method) { + errs = append(errs, field.Invalid(path.Child("methods").Index(i), method, notInSliceMessage(methods))) + } + if _, ok := found[method]; ok { + errs = append(errs, field.Invalid(path.Child("methods").Index(i), method, `method listed more than once.`)) + } + found[method] = struct{}{} } - if invalidPathPrefix(in.PathExact) { - errs = append(errs, field.Invalid(path.Child("pathExact"), in.PathExact, `must begin with a '/'`)) + errs = append(errs, in.Header.validate(path.Child("header"))...) + return errs +} + +func (in IntentionHTTPHeaderPermissions) validate(path *field.Path) field.ErrorList { + var errs field.ErrorList + for i, permission := range in { + hdrParts := 0 + if permission.Present { + hdrParts++ + } + hdrParts += numNotEmpty(permission.Exact, permission.Regex, permission.Prefix, permission.Suffix) + if hdrParts > 1 { + asJson, _ := json.Marshal(in[i]) + errs = append(errs, field.Invalid(path.Index(i), string(asJson), `at most only one of exact, prefix, suffix, regex, or present may be configured.`)) + } } return errs } @@ -355,3 +446,13 @@ type ServiceIntentionsList struct { func init() { SchemeBuilder.Register(&ServiceIntentions{}, &ServiceIntentionsList{}) } + +func numNotEmpty(ss ...string) int { + count := 0 + for _, s := range ss { + if s != "" { + count++ + } + } + return count +} diff --git a/api/v1alpha1/serviceintentions_types_test.go b/api/v1alpha1/serviceintentions_types_test.go index 831db4e7f2..a37d848f3b 100644 --- a/api/v1alpha1/serviceintentions_types_test.go +++ b/api/v1alpha1/serviceintentions_types_test.go @@ -564,17 +564,11 @@ func TestServiceIntentions_Validate(t *testing.T) { { Action: "allow", HTTP: &IntentionHTTPPermission{ - PathExact: "/foo", - PathPrefix: "/bar", - PathRegex: "/baz", + PathExact: "/foo", Header: IntentionHTTPHeaderPermissions{ { Name: "header", Present: true, - Exact: "exact", - Prefix: "prefix", - Suffix: "suffix", - Regex: "regex", Invert: true, }, }, @@ -617,18 +611,12 @@ func TestServiceIntentions_Validate(t *testing.T) { { Action: "allow", HTTP: &IntentionHTTPPermission{ - PathExact: "/foo", - PathPrefix: "/bar", - PathRegex: "/baz", + PathRegex: "/baz", Header: IntentionHTTPHeaderPermissions{ { - Name: "header", - Present: true, - Exact: "exact", - Prefix: "prefix", - Suffix: "suffix", - Regex: "regex", - Invert: true, + Name: "header", + Regex: "regex", + Invert: true, }, }, Methods: []string{ @@ -719,6 +707,102 @@ func TestServiceIntentions_Validate(t *testing.T) { `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0].pathPrefix: Invalid value: "bar": must begin with a '/'`, }, }, + "invalid permissions.http pathPrefix,pathExact specified": { + input: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-matter", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "dest-service", + Namespace: "namespace", + }, + Sources: SourceIntentions{ + { + Name: "svc-2", + Namespace: "bar", + Permissions: IntentionPermissions{ + { + Action: "allow", + HTTP: &IntentionHTTPPermission{ + PathPrefix: "/bar", + PathExact: "/foo", + }, + }, + }, + }, + }, + }, + }, + namespacesEnabled: true, + expectedErrMsgs: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: "{\"pathExact\":\"/foo\",\"pathPrefix\":\"/bar\"}": at most only one of pathExact, pathPrefix, or pathRegex may be configured.`, + }, + }, + "invalid permissions.http pathPrefix,pathRegex specified": { + input: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-matter", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "dest-service", + Namespace: "namespace", + }, + Sources: SourceIntentions{ + { + Name: "svc-2", + Namespace: "bar", + Permissions: IntentionPermissions{ + { + Action: "allow", + HTTP: &IntentionHTTPPermission{ + PathPrefix: "/bar", + PathRegex: "foo", + }, + }, + }, + }, + }, + }, + }, + namespacesEnabled: true, + expectedErrMsgs: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: "{\"pathPrefix\":\"/bar\",\"pathRegex\":\"foo\"}": at most only one of pathExact, pathPrefix, or pathRegex may be configured.`, + }, + }, + "invalid permissions.http pathRegex,pathExact specified": { + input: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-matter", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "dest-service", + Namespace: "namespace", + }, + Sources: SourceIntentions{ + { + Name: "svc-2", + Namespace: "bar", + Permissions: IntentionPermissions{ + { + Action: "allow", + HTTP: &IntentionHTTPPermission{ + PathRegex: "bar", + PathExact: "/foo", + }, + }, + }, + }, + }, + }, + }, + namespacesEnabled: true, + expectedErrMsgs: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: "{\"pathExact\":\"/foo\",\"pathRegex\":\"bar\"}": at most only one of pathExact, pathPrefix, or pathRegex may be configured.`, + }, + }, "invalid permissions.http.pathExact": { input: &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ @@ -750,6 +834,104 @@ func TestServiceIntentions_Validate(t *testing.T) { `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0].pathExact: Invalid value: "bar": must begin with a '/'`, }, }, + "invalid permissions.http.methods": { + input: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-matter", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "dest-service", + Namespace: "namespace", + }, + Sources: SourceIntentions{ + { + Name: "svc-2", + Namespace: "bar", + Permissions: IntentionPermissions{ + { + Action: "allow", + HTTP: &IntentionHTTPPermission{ + Methods: []string{ + "FOO", + "GET", + "BAR", + "GET", + "POST", + }, + }, + }, + }, + }, + }, + }, + }, + namespacesEnabled: true, + expectedErrMsgs: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: [spec.sources[0].permissions[0].methods[0]: Invalid value: "FOO": must be one of "GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "CONNECT", "OPTIONS", "TRACE", spec.sources[0].permissions[0].methods[2]: Invalid value: "BAR": must be one of "GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "CONNECT", "OPTIONS", "TRACE", spec.sources[0].permissions[0].methods[3]: Invalid value: "GET": method listed more than once.`, + }, + }, + "invalid permissions.http.header": { + input: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-matter", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "dest-service", + Namespace: "namespace", + }, + Sources: SourceIntentions{ + { + Name: "svc-2", + Namespace: "bar", + Permissions: IntentionPermissions{ + { + Action: "allow", + HTTP: &IntentionHTTPPermission{ + Header: IntentionHTTPHeaderPermissions{ + { + Name: "exact-present", + Present: true, + Exact: "foobar", + }, + { + Name: "prefix-exact", + Exact: "foobar", + Prefix: "barfood", + }, + { + Name: "suffix-prefix", + Prefix: "foo", + Suffix: "bar", + }, + { + Name: "suffix-regex", + Suffix: "bar", + Regex: "foo", + }, + { + Name: "regex-present", + Present: true, + Regex: "foobar", + }, + }, + }, + }, + }, + }, + }, + }, + }, + namespacesEnabled: true, + expectedErrMsgs: []string{ + `spec.sources[0].permissions[0].header[0]: Invalid value: "{\"name\":\"exact-present\",\"present\":true,\"exact\":\"foobar\"}": at most only one of exact, prefix, suffix, regex, or present may be configured.`, + `spec.sources[0].permissions[0].header[1]: Invalid value: "{\"name\":\"prefix-exact\",\"exact\":\"foobar\",\"prefix\":\"barfood\"}": at most only one of exact, prefix, suffix, regex, or present may be configured.`, + `spec.sources[0].permissions[0].header[2]: Invalid value: "{\"name\":\"suffix-prefix\",\"prefix\":\"foo\",\"suffix\":\"bar\"}": at most only one of exact, prefix, suffix, regex, or present may be configured.`, + `spec.sources[0].permissions[0].header[3]: Invalid value: "{\"name\":\"suffix-regex\",\"suffix\":\"bar\",\"regex\":\"foo\"}": at most only one of exact, prefix, suffix, regex, or present may be configured.`, + `spec.sources[0].permissions[0].header[4]: Invalid value: "{\"name\":\"regex-present\",\"present\":true,\"regex\":\"foobar\"}": at most only one of exact, prefix, suffix, regex, or present may be configured.`, + }, + }, "invalid permissions.action": { input: &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ diff --git a/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml b/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml index db544e096c..f5aaa2180d 100644 --- a/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml +++ b/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml @@ -42,60 +42,81 @@ spec: description: ServiceIntentionsSpec defines the desired state of ServiceIntentions properties: destination: + description: Destination is the intention destination that will have the authorization granted to. properties: name: + description: Name is the destination of all intentions defined in this config entry. This may be set to the wildcard character (*) to match all services that don't otherwise have intentions defined. type: string namespace: + description: Namespace specifies the namespace the config entry will apply to. This may be set to the wildcard character (*) to match all services in all namespaces that don't otherwise have intentions defined. type: string type: object sources: + description: Sources is the list of all intention sources and the authorization granted to those sources. The order of this list does not matter, but out of convenience Consul will always store this reverse sorted by intention precedence, as that is the order that they will be evaluated at enforcement time. items: properties: action: - description: IntentionAction is the action that the intention represents. This can be "allow" or "deny" to allowlist or denylist intentions. + description: Action is required for an L4 intention, and should be set to one of "allow" or "deny" for the action that should be taken if this intention matches a request. type: string description: + description: Description for the intention. This is not used by Consul, but is presented in API responses to assist tooling. type: string name: + description: Name is the source of the intention. This is the name of a Consul service. The service doesn't need to be registered. type: string namespace: + description: Namespace is the namespace for the Name parameter. type: string permissions: + description: Permissions is the list of all additional L7 attributes that extend the intention match criteria. Permission precedence is applied top to bottom. For any given request the first permission to match in the list is terminal and stops further evaluation. As with L4 intentions, traffic that fails to match any of the provided permissions in this intention will be subject to the default intention behavior is defined by the default ACL policy. This should be omitted for an L4 intention as it is mutually exclusive with the Action field. items: properties: action: - description: IntentionAction is the action that the intention represents. This can be "allow" or "deny" to allowlist or denylist intentions. + description: Action is one of "allow" or "deny" for the action that should be taken if this permission matches a request. type: string http: + description: HTTP is a set of HTTP-specific authorization criteria. properties: header: + description: Header is a set of criteria that can match on HTTP request headers. If more than one is configured all must match for the overall match to apply. items: properties: exact: + description: Exact matches if the header with the given name is this value. type: string invert: + description: Invert inverts the logic of the match. type: boolean name: + description: Name is the name of the header to match. type: string prefix: + description: Prefix matches if the header with the given name has this prefix. type: string present: + description: Present matches if the header with the given name is present with any value. type: boolean regex: + description: Regex matches if the header with the given name matches this pattern. type: string suffix: + description: Suffix matches if the header with the given name has this suffix. type: string type: object type: array methods: + description: Methods is a list of HTTP methods for which this match applies. If unspecified all HTTP methods are matched. If provided the names must be a valid method. items: type: string type: array pathExact: + description: PathExact is the exact path to match on the HTTP request path. type: string pathPrefix: + description: PathPrefix is the path prefix to match on the HTTP request path. type: string pathRegex: + description: PathRegex is the regular expression to match on the HTTP request path. type: string type: object type: object