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

Support Allow/Deny IP Subnets #2462

Closed
Tracked by #2250
arkodg opened this issue Jan 17, 2024 · 17 comments · Fixed by #3399
Closed
Tracked by #2250

Support Allow/Deny IP Subnets #2462

arkodg opened this issue Jan 17, 2024 · 17 comments · Fixed by #3399
Assignees
Labels
Milestone

Comments

@arkodg
Copy link
Contributor

arkodg commented Jan 17, 2024

Description:

Describe the desired behavior, what scenario it enables and how it
would be used.

As a user, I would like to limit the client IP address to a few subnets for some cases as well as also deny specific subnets

@arkodg arkodg added triage area/policy road-to-ga area/api API-related issues help wanted Extra attention is needed labels Jan 17, 2024
@arkodg
Copy link
Contributor Author

arkodg commented Jan 17, 2024

Envoy supports this https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/rbac/v3/rbac.proto#envoy-v3-api-msg-config-rbac-v3-principal

does this feature belong in ClientTrafficPolicy or SecurityPolicy ?

@arkodg arkodg added this to the v1.0.0-rc1 milestone Jan 17, 2024
@tmsnan
Copy link
Contributor

tmsnan commented Jan 18, 2024

Vote for SecurityPolicy, access control is reasonable as a security policy.

@arkodg
Copy link
Contributor Author

arkodg commented Jan 18, 2024

if the SecurityPolicy with ip subnet info is applied at the Gateway level, it can be overridden at the route level if another SecurityPolicy is applied at route level

@tmsnan
Copy link
Contributor

tmsnan commented Jan 19, 2024

Is it possible to add a disclaimer stating that the use of routing override is not recommended?

@zhaohuabing
Copy link
Member

zhaohuabing commented Jan 23, 2024

Should RBAC #2250 be the right home for this?

@zhaohuabing
Copy link
Member

zhaohuabing commented Jan 23, 2024

https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/rbac/v3/rbac.proto#envoy-v3-api-msg-config-rbac-v3-principal

if the SecurityPolicy with ip subnet info is applied at the Gateway level, it can be overridden at the route level if another SecurityPolicy is applied at route level

I didn't get it. A route level SecurityPolicy without Allow/Deny IP setting won't override the Allow/Deny IP setting in Gateway level. "Allow/Deny IP Subnets" should be only configured at the gateway level?

for _, http := range ir.HTTP {
for _, r := range http.Routes {
// Apply if not already set
if r.CORS == nil {
r.CORS = cors
}
if r.JWT == nil {
r.JWT = jwt
}
if r.OIDC == nil {
r.OIDC = oidc
}
if r.BasicAuth == nil {
r.BasicAuth = basicAuth
}
}

@zetaab
Copy link
Contributor

zetaab commented Jan 23, 2024

well at least I have use case that same gateway should be apple to serve clients from the internet, but some routes need to have allow/deny ip setting.

@arkodg
Copy link
Contributor Author

arkodg commented Jan 23, 2024

@zhaohuabing A route level SecurityPolicy without Allow/Deny IP setting will override the Allow/Deny IP setting in Gateway level, until we implement #1934
Can you share why rbac is a better home ? If we took that approach, config would look like

jwt:
   .........
acl:
   jwt:
     claims: ......   

vs

jwt:
  .....
  claims: .......

@arkodg
Copy link
Contributor Author

arkodg commented Jan 23, 2024

thanks for sharing your use case @zetaab, based on community feedback sounds like SecurityPolicy is the way to go here

@arkodg arkodg removed the triage label Jan 23, 2024
@zhaohuabing
Copy link
Member

zhaohuabing commented Jan 24, 2024

@zhaohuabing A route level SecurityPolicy without Allow/Deny IP setting will override the Allow/Deny IP setting in Gateway level, until we implement #1934

This may not be intended, but the current implementation of SecurityPolicy is actually a Merge logic. For example, if a route level SecurityPolicy doesn't has OIDC configuration, but a Gateway level SecurityPolicy has, the Gateway level OIDC applies. As the below code shows:

for _, http := range ir.HTTP {
for _, r := range http.Routes {
// Apply if not already set
if r.CORS == nil {
r.CORS = cors
}
if r.JWT == nil {
r.JWT = jwt
}
if r.OIDC == nil {
r.OIDC = oidc
}
if r.BasicAuth == nil {
r.BasicAuth = basicAuth
}
}

@arkodg
Copy link
Contributor Author

arkodg commented Jan 24, 2024

@zhaohuabing you are right, this needs to be fixed, being tracked with #2055 which needs improvements in this logic

for _, r := range http.Routes {

@zinuga
Copy link

zinuga commented Feb 25, 2024

@arkodg so this wont be in GA release?

@zetaab
Copy link
Contributor

zetaab commented Feb 25, 2024

@zinuga I have been implementing the api design for this in #2652 its pretty fast implement after the api design is accepted.

@arkodg
Copy link
Contributor Author

arkodg commented Feb 27, 2024

thanks for driving this @zetaab, the API is close to getting merged, if the implementation does complete by rc (March 1st-4th), we should be able to support it in v1.0

@zirain
Copy link
Contributor

zirain commented Feb 29, 2024

#2652

@arkodg arkodg removed the help wanted Extra attention is needed label Mar 2, 2024
Copy link

github-actions bot commented Apr 1, 2024

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment