Skip to content
Open
Show file tree
Hide file tree
Changes from all 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: 3 additions & 1 deletion api/v1alpha1/authorization_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,9 @@ type AuthorizationHeaderMatch struct {
type JWTPrincipal struct {
// Provider is the name of the JWT provider that used to verify the JWT token.
// In order to use JWT claims for authorization, you must configure the JWT
// authentication with the same provider in the same `SecurityPolicy`.
// authentication with the same provider in the same `SecurityPolicy`, or,
// when `mergeType` is set, in the parent `SecurityPolicy` this policy is
// merged into.
//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
Expand Down
2 changes: 1 addition & 1 deletion api/v1alpha1/securitypolicy_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ type SecurityPolicy struct {
// +kubebuilder:validation:XValidation:rule="has(self.targetRefs) ? self.targetRefs.all(ref, ref.group == 'gateway.networking.k8s.io') : true ", message="this policy can only have a targetRefs[*].group of gateway.networking.k8s.io"
// +kubebuilder:validation:XValidation:rule="has(self.targetRefs) ? self.targetRefs.all(ref, ref.kind in ['Gateway', 'ListenerSet', 'HTTPRoute', 'GRPCRoute', 'TCPRoute']) : true ", message="this policy can only have a targetRefs[*].kind of Gateway/ListenerSet/HTTPRoute/GRPCRoute/TCPRoute"
// +kubebuilder:validation:XValidation:rule="!has(self.mergeType) || ((!has(self.targetRef) || self.targetRef.kind in ['HTTPRoute', 'GRPCRoute', 'TCPRoute']) && (!has(self.targetRefs) || self.targetRefs.all(ref, ref.kind in ['HTTPRoute', 'GRPCRoute', 'TCPRoute'])) && (!has(self.targetSelectors) || self.targetSelectors.all(sel, sel.kind in ['HTTPRoute', 'GRPCRoute', 'TCPRoute'])))", message="mergeType can only be used with xRoute targets"
// +kubebuilder:validation:XValidation:rule="(has(self.authorization) && has(self.authorization.rules) && self.authorization.rules.exists(r, has(r.principal) ? has(r.principal.jwt) : false)) ? has(self.jwt) : true", message="if authorization.rules.principal.jwt is used, jwt must be defined"
// +kubebuilder:validation:XValidation:rule="(!has(self.mergeType) && has(self.authorization) && has(self.authorization.rules) && self.authorization.rules.exists(r, has(r.principal) ? has(r.principal.jwt) : false)) ? has(self.jwt) : true", message="if authorization.rules.principal.jwt is used, jwt must be defined, unless mergeType is set and the jwt providers are inherited from the parent policy"
type SecurityPolicySpec struct {
PolicyTargetReferences `json:",inline"`

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,9 @@ spec:
description: |-
Provider is the name of the JWT provider that used to verify the JWT token.
In order to use JWT claims for authorization, you must configure the JWT
authentication with the same provider in the same `SecurityPolicy`.
authentication with the same provider in the same `SecurityPolicy`, or,
when `mergeType` is set, in the parent `SecurityPolicy` this policy is
merged into.
maxLength: 253
minLength: 1
type: string
Expand Down Expand Up @@ -8114,9 +8116,11 @@ spec:
|| self.targetRefs.all(ref, ref.kind in [''HTTPRoute'', ''GRPCRoute'',
''TCPRoute''])) && (!has(self.targetSelectors) || self.targetSelectors.all(sel,
sel.kind in [''HTTPRoute'', ''GRPCRoute'', ''TCPRoute''])))'
- message: if authorization.rules.principal.jwt is used, jwt must be defined
rule: '(has(self.authorization) && has(self.authorization.rules) &&
self.authorization.rules.exists(r, has(r.principal) ? has(r.principal.jwt)
- message: if authorization.rules.principal.jwt is used, jwt must be defined,
unless mergeType is set and the jwt providers are inherited from the
parent policy
rule: '(!has(self.mergeType) && has(self.authorization) && has(self.authorization.rules)
&& self.authorization.rules.exists(r, has(r.principal) ? has(r.principal.jwt)
: false)) ? has(self.jwt) : true'
status:
description: Status defines the current status of SecurityPolicy.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,9 @@ spec:
description: |-
Provider is the name of the JWT provider that used to verify the JWT token.
In order to use JWT claims for authorization, you must configure the JWT
authentication with the same provider in the same `SecurityPolicy`.
authentication with the same provider in the same `SecurityPolicy`, or,
when `mergeType` is set, in the parent `SecurityPolicy` this policy is
merged into.
maxLength: 253
minLength: 1
type: string
Expand Down Expand Up @@ -8113,9 +8115,11 @@ spec:
|| self.targetRefs.all(ref, ref.kind in [''HTTPRoute'', ''GRPCRoute'',
''TCPRoute''])) && (!has(self.targetSelectors) || self.targetSelectors.all(sel,
sel.kind in [''HTTPRoute'', ''GRPCRoute'', ''TCPRoute''])))'
- message: if authorization.rules.principal.jwt is used, jwt must be defined
rule: '(has(self.authorization) && has(self.authorization.rules) &&
self.authorization.rules.exists(r, has(r.principal) ? has(r.principal.jwt)
- message: if authorization.rules.principal.jwt is used, jwt must be defined,
unless mergeType is set and the jwt providers are inherited from the
parent policy
rule: '(!has(self.mergeType) && has(self.authorization) && has(self.authorization.rules)
&& self.authorization.rules.exists(r, has(r.principal) ? has(r.principal.jwt)
: false)) ? has(self.jwt) : true'
status:
description: Status defines the current status of SecurityPolicy.
Expand Down
75 changes: 61 additions & 14 deletions internal/gatewayapi/securitypolicy.go
Original file line number Diff line number Diff line change
Expand Up @@ -511,28 +511,30 @@ func (t *Translator) processSecurityPolicyForRoute(
return
}

// Protocol-specific validation: pick the appropriate validator and message,
// then run it once to keep the flow linear and easier to read.
// Protocol-specific validation: pick the appropriate validator and message.
// A merging policy is only complete once it has been merged - its authorization
// rules may reference a JWT provider that the parent policy defines - so each
// branch below validates at the point where the policy is whole.
validator := validateSecurityPolicy
errMsg := "invalid SecurityPolicy"
if currTarget.Kind == resource.KindTCPRoute {
validator = validateSecurityPolicyForTCP
errMsg = "invalid SecurityPolicy for TCP route"
}
if err := validator(policy); err != nil {
status.SetTranslationErrorForPolicyAncestors(&policy.Status,
ancestorRefs,
t.GatewayControllerName,
policy.Generation,
status.Error2ConditionMsg(fmt.Errorf("%s: %w", errMsg, err)),
)

return
}

// Check if merging is enabled
if policy.Spec.MergeType == nil {
// No merging - use existing translation logic
// No merging - validate and translate the policy as it was written.
if err := validator(policy); err != nil {
status.SetTranslationErrorForPolicyAncestors(&policy.Status,
ancestorRefs,
t.GatewayControllerName,
policy.Generation,
status.Error2ConditionMsg(fmt.Errorf("%s: %w", errMsg, err)),
)
return
}

if err := t.translateSecurityPolicyForRoute(policy, &securityPolicyOwners{}, targetedRoute, currTarget, resources, xdsIR, nil); err != nil {
status.SetTranslationErrorForPolicyAncestors(&policy.Status,
ancestorRefs,
Expand Down Expand Up @@ -589,7 +591,20 @@ func (t *Translator) processSecurityPolicyForRoute(
}

if parentPolicy == nil {
// No parent policy found, fall back to current policy
// No parent policy found, fall back to the current policy. Nothing was
// inherited, so validate the policy exactly as it was written.
if err := validator(policy); err != nil {
status.SetConditionForPolicyAncestor(&policy.Status,
&ancestorRef,
t.GatewayControllerName,
gwapiv1.PolicyConditionAccepted, metav1.ConditionFalse,
egv1a1.PolicyReasonInvalid,
status.Error2ConditionMsg(err),
policy.Generation,
)
continue
Comment on lines +596 to +605

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Fail closed when no JWT provider can be inherited

When a merging route policy uses JWT authorization but its listener has no parent SecurityPolicy (the newly added httproute-no-parent-policy fixture exercises this), validation fails here and continue skips translation entirely. The generated route consequently has no security stanza or direct response, so traffic is unrestricted even though the policy specifies defaultAction: Deny; this can also happen transiently if the expected parent policy is removed. Preserve a fail-closed route/direct-response marker while reporting the policy as invalid.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the behavior is right. with no parent policy, validation fails and translation is skipped.

that said, this is not specific to this PR. Every validateSecurityPolicy have same issue.
so I'd like to raise issue and handle follow up PRs.

}

if err := t.translateSecurityPolicyForRoute(policy, &securityPolicyOwners{}, targetedRoute, currTarget, resources, xdsIR, listener); err != nil {
status.SetConditionForPolicyAncestor(&policy.Status,
&ancestorRef,
Expand Down Expand Up @@ -968,9 +983,41 @@ func validateSecurityPolicy(p *egv1a1.SecurityPolicy) error {
return err
}
}

if err := validateAuthorizationJWTProviders(p); err != nil {
return err
}
return nil
}

// validateAuthorizationJWTProviders ensures every JWT principal in the authorization
// rules references a JWT provider that the policy defines.
func validateAuthorizationJWTProviders(p *egv1a1.SecurityPolicy) error {
if p.Spec.Authorization == nil {
return nil
}

providers := sets.New[string]()
if p.Spec.JWT != nil {
for _, provider := range p.Spec.JWT.Providers {
providers.Insert(provider.Name)
}
}

var errs []error
for i, rule := range p.Spec.Authorization.Rules {
if rule.Principal == nil || rule.Principal.JWT == nil {
continue
}
if !providers.Has(rule.Principal.JWT.Provider) {
errs = append(errs, fmt.Errorf(
"authorization rule %d: jwt provider %q is not defined in jwt.providers",
i, rule.Principal.JWT.Provider))
}
}
return errors.Join(errs...)
}

// validateSecurityPolicyForTCP ensures SecurityPolicy usage on TCP is compatible.
//
// TCP supports Authorization with ClientCIDRs ONLY.
Expand Down
80 changes: 80 additions & 0 deletions internal/gatewayapi/securitypolicy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,86 @@ func Test_OIDC_PassThroughAuthHeader(t *testing.T) {
}
}

func TestValidateAuthorizationJWTProviders(t *testing.T) {
jwtWith := func(names ...string) *egv1a1.JWT {
providers := make([]egv1a1.JWTProvider, 0, len(names))
for _, name := range names {
providers = append(providers, egv1a1.JWTProvider{Name: name})
}
return &egv1a1.JWT{Providers: providers}
}
authorizationWith := func(providers ...string) *egv1a1.Authorization {
rules := make([]egv1a1.AuthorizationRule, 0, len(providers))
for _, provider := range providers {
rules = append(rules, egv1a1.AuthorizationRule{
Action: egv1a1.AuthorizationActionAllow,
Principal: &egv1a1.Principal{JWT: &egv1a1.JWTPrincipal{Provider: provider}},
})
}
return &egv1a1.Authorization{Rules: rules}
}

tests := []struct {
name string
jwt *egv1a1.JWT
authorization *egv1a1.Authorization
wantError bool
}{
{
name: "provider defined in the same policy",
jwt: jwtWith("example"),
authorization: authorizationWith("example"),
},
{
name: "no authorization rules",
jwt: jwtWith("example"),
authorization: nil,
},
{
name: "principal without a jwt is ignored",
authorization: &egv1a1.Authorization{
Rules: []egv1a1.AuthorizationRule{
{
Action: egv1a1.AuthorizationActionAllow,
Principal: &egv1a1.Principal{ClientCIDRs: []egv1a1.CIDR{"10.0.0.0/8"}},
},
},
},
},
{
name: "unknown provider",
jwt: jwtWith("example"),
authorization: authorizationWith("does-not-exist"),
wantError: true,
},
{
name: "no jwt providers at all",
authorization: authorizationWith("example"),
wantError: true,
},
{
name: "one known and one unknown provider",
jwt: jwtWith("example"),
authorization: authorizationWith("example", "does-not-exist"),
wantError: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
policy := &egv1a1.SecurityPolicy{
Spec: egv1a1.SecurityPolicySpec{
JWT: tt.jwt,
Authorization: tt.authorization,
},
}

err := validateAuthorizationJWTProviders(policy)
require.Equal(t, tt.wantError, err != nil, "validateAuthorizationJWTProviders() error = %v", err)
})
}
}

func ToPointer[T any](v T) *T {
return &v
}
Expand Down
Loading
Loading