-
Notifications
You must be signed in to change notification settings - Fork 360
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
API: authorization api for jwt claims #4009
Conversation
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4009 +/- ##
==========================================
+ Coverage 67.95% 67.96% +0.01%
==========================================
Files 189 189
Lines 23103 23103
==========================================
+ Hits 15699 15703 +4
+ Misses 6281 6279 -2
+ Partials 1123 1121 -2 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good from my perspective
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
// JWT authentication in the same `SecurityPolicy`. | ||
// +optional | ||
// +notImplementedHide | ||
JWT *JWTPrincipal `json:"jwt,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// If the claim is a string type, the specified value must match exactly. | ||
// If the claim is a string array type, the specified value must match one of the values in the array. | ||
// If multiple values are specified, one of the values must match for the rule to match. | ||
Values []string `json:"values"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer using string for bool as "true"/"false" and integer if we need these types in the future. Any is too flexible, as it can represent nested structures besides simple types, which we won't use.
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com> Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com> Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
// +kubebuilder:default=String | ||
// +unionDiscriminator | ||
// +optional | ||
ValueType *JWTClaimValueType `json:"valueType,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to specify the Matcher instead of the ClaimValueType? I think that would bring more flexibility.
i.e.
type JWTClaim struct {
Name string `json:"name"`
ValueMatcher JWTClaimValueMatcher `json:"valueMatcher "`
}
type JWTClaimValueMatcher struct {
equalsAny *JWTClaimValues `json:"valueType,omitempty"`
notEqualsAny *JWTClaimValues `json:"valueType,omitempty"`
includesAll *JWTClaimValues `json:"valueType,omitempty"`
includesAny *JWTClaimValues `json:"valueType,omitempty"`
notIncludesAny *JWTClaimValues `json:"valueType,omitempty"`
}
type JWTClaimValues struct {
Values []string `json:"values"`
}
- equalsAny (assume claim is single string)
- notEqualsAny (assume claim is single string)
- includesAll (assume claim is string array)
- includesAny (assume claim is string array)
- notIncludesAny (assume claim is string array)
- ...
If necessary for a certain Matcher we could specify different properties
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @denniskniep In theory, we could make this API as flexible as the Envoy RBAC API, but I would like to make this API simpler and easier to understand as it's a user-facing API.
I believe the normal use cases for JWT claim based authorization is that we just compare wether the claims equal with any of the specified values (equalsAny). I can't think of any use cases for a substring matching.
Scope comparison is a bit different, all specified scopes must exist in the JWT. It's handled in a dedicated API field.
For the not
use case, it can be accomplished with a Allow
default action and a Deny
rule:
rules:
- default: Allow
- action: Deny
principal:
jwt:
claims:
- name: user
values: [jason, alice]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks Huabing !
|
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
API for #3367
A SecurityPolicy enforcing authz on a Gateway resource based on jwtClaims (sub=jason or sub=alice) and iss=foo.bar.com:
A SecurityPolicy enforcing authz on a HTTPRoute resource based on scope contains "read::message"
Scopes and claims can be used together, they're And-ed if both are specified.
A SecurityPolicy enforcing authz on a HTTPRoute resource based on scope contains "read::message" and a custom claim message_type: