Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add CORS to SecurityPolicy #2065

Merged
merged 11 commits into from
Oct 25, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions api/v1alpha1/envoyproxy_metric_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ type ProxyPrometheusProvider struct {
}

// Match defines the stats match configuration.
type Match struct {
type Match struct { // TODO: zhaohuabing this type should be renamed to StatsMatch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you open an issue to track this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// MatcherType defines the stats matcher type
//
// +kubebuilder:validation:Enum=RegularExpression;Prefix;Suffix
Expand All @@ -70,7 +70,7 @@ type Match struct {

type MatcherType string

const (
const ( // TODO: zhaohuabing the const types should be prefixed with StatsMatch
Prefix MatcherType = "Prefix"
RegularExpression MatcherType = "RegularExpression"
Suffix MatcherType = "Suffix"
Expand Down
2 changes: 1 addition & 1 deletion api/v1alpha1/ratelimitfilter_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ type SourceMatch struct {
}

// HeaderMatch defines the match attributes within the HTTP Headers of the request.
type HeaderMatch struct {
type HeaderMatch struct { // TODO: zhaohuabing this type could be replaced with a general purpose StringMatch type.
// Type specifies how to match against the value of the header.
//
// +optional
Expand Down
66 changes: 66 additions & 0 deletions api/v1alpha1/securitypolicy_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,74 @@ type SecurityPolicySpec struct {
// for this Policy to have effect and be applied to the Gateway.
// TargetRef
TargetRef gwapiv1a2.PolicyTargetReferenceWithSectionName `json:"targetRef"`

// CORS defines the configuration for Cross-Origin Resource Sharing (CORS).
CORS *CORS `json:"cors,omitempty"`
}

// CORS defines the configuration for Cross-Origin Resource Sharing (CORS).
type CORS struct {
// AllowOrigins defines the origins that are allowed to make requests.
AllowOrigins []StringMatch `json:"allowOrigins,omitempty" yaml:"allowOrigins,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

min length for AllowOrigins should be 1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an empty AllowOrigins could be useful if the backend service allows CORS but we want to explicitly disable it at the Gateway.

I remember we discussed this at the ir PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not setting CORS disables CORS :)

Copy link
Member Author

@zhaohuabing zhaohuabing Oct 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's a bit tricky. Maybe we should add some comments on the API.

// AllowMethods defines the methods that are allowed to make requests.
AllowMethods []string `json:"allowMethods,omitempty" yaml:"allowMethods,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens when AllowMethods is empty ?
are all requests types allowed or none ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None, just like empty AllowOrigins.

Copy link
Member Author

@zhaohuabing zhaohuabing Oct 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some experiments, the CORS filter won't set "Access-Control-Request-Headers" in the preflight response if AllowMethods is empty. So it means none Method Types are allowed.

curl -H "Origin: http://example.com" \
  -H "Access-Control-Request-Method: GET" \
  -H "Access-Control-Request-Headers: X-Requested-With" \
  -X OPTIONS --verbose \
  http://localhost:8002/cors/open
*   Trying 127.0.0.1:8002...
* Connected to localhost (127.0.0.1) port 8002 (#0)
> OPTIONS /cors/open HTTP/1.1
> Host: localhost:8002
> User-Agent: curl/8.1.2
> Accept: */*
> Origin: http://example.com
> Access-Control-Request-Method: GET
> Access-Control-Request-Headers: X-Requested-With
> 
< HTTP/1.1 200 OK
< access-control-allow-origin: http://example.com
< date: Wed, 25 Oct 2023 17:32:39 GMT
< server: envoy
< content-length: 0
< 
* Connection #0 to host localhost left intact

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, so its opt in, thanks for running the test

// AllowHeaders defines the headers that are allowed to be sent with requests.
AllowHeaders []string `json:"allowHeaders,omitempty" yaml:"allowHeaders,omitempty"`
// ExposeHeaders defines the headers that can be exposed in the responses.
ExposeHeaders []string `json:"exposeHeaders,omitempty" yaml:"exposeHeaders,omitempty"`
// MaxAge defines how long the results of a preflight request can be cached.
MaxAge *metav1.Duration `json:"maxAge,omitempty" yaml:"maxAge,omitempty"`
}

// StringMatch defines how to match any strings.
// This is a general purpose match condition that can be used by other EG APIs
// that need to match against a string.
type StringMatch struct {
// Type specifies how to match against a string.
//
// +optional
// +kubebuilder:default=Exact
Type *MatchType `json:"type,omitempty"`

// Value specifies the string value that the match must have.
//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=1024
Value string `json:"value"`

// IgnoreCase specifies whether the match should be case insensitive.
// This has no effect for the safe_regex match.
// Defaults to false.
// +optional
IgnoreCase bool `json:"caseSensitive,omitempty"`
zhaohuabing marked this conversation as resolved.
Show resolved Hide resolved
}

// MatchType specifies the semantics of how a string value should be compared.
// Valid MatchType values are "Exact", "Prefix", "Suffix", "Contains", "RegularExpression".
//
// +kubebuilder:validation:Enum=Exact;Prefix;Suffix;Contains;RegularExpression
type MatchType string

const (
// MatchExact :the input string must match exactly the match value.
MatchExact MatchType = "Exact"

// MatchPrefix :the input string must start with the match value.
MatchPrefix MatchType = "Prefix"

// MatchSuffix :the input string must end with the match value.
MatchSuffix MatchType = "Suffix"

// MatchContains :the input string must contain the match value.
zhaohuabing marked this conversation as resolved.
Show resolved Hide resolved
MatchContains MatchType = "Contains"

// MatchRegularExpression :The input string must match the regular expression
// specified in the match value.
// The regex string must adhere to the syntax documented in
// https://github.com/google/re2/wiki/Syntax.
MatchRegularExpression MatchType = "RegularExpression"
)

// SecurityPolicyStatus defines the state of SecurityPolicy
type SecurityPolicyStatus struct {
// Conditions describe the current conditions of the SecurityPolicy.
Expand Down
67 changes: 67 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,66 @@ spec:
spec:
description: Spec defines the desired state of SecurityPolicy.
properties:
cors:
description: CORS defines the configuration for Cross-Origin Resource
Sharing (CORS).
properties:
allowHeaders:
description: AllowHeaders defines the headers that are allowed
to be sent with requests.
items:
type: string
type: array
allowMethods:
description: AllowMethods defines the methods that are allowed
to make requests.
items:
type: string
type: array
allowOrigins:
description: AllowOrigins defines the origins that are allowed
to make requests.
items:
description: StringMatch defines how to match any strings. This
is a general purpose match condition that can be used by other
EG APIs that need to match against a string.
properties:
caseSensitive:
description: IgnoreCase specifies whether the match should
be case insensitive. This has no effect for the safe_regex
match. Defaults to false.
type: boolean
type:
default: Exact
description: Type specifies how to match against a string.
enum:
- Exact
- Prefix
- Suffix
- Contains
- RegularExpression
type: string
value:
description: Value specifies the string value that the match
must have.
maxLength: 1024
minLength: 1
type: string
required:
- value
type: object
type: array
exposeHeaders:
description: ExposeHeaders defines the headers that can be exposed
in the responses.
items:
type: string
type: array
maxAge:
description: MaxAge defines how long the results of a preflight
request can be cached.
type: string
type: object
targetRef:
description: TargetRef is the name of the Gateway resource this policy
is being attached to. This Policy and the TargetRef MUST be in the
Expand Down
Loading