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(ip-restriction): Add TCP support (#6679) #10229

Closed

Conversation

merusso
Copy link

@merusso merusso commented Feb 4, 2023

Summary

This change adds TCP support to the ip-restriction plugin by implementing the Stream module's preread function.

When a TCP connection is rejected due to IP restriction rules, a JSON error response is written to the stream and the connection is closed.

Note: This change is based on branch release/2.8.x so it targets this branch. However, since it is a new feature, it would probably be added in a 2.9.x release instead. There isn't currently a release/2.9.x or release/2.x branch.

Checklist

Full changelog

  • Add protocols to schema: "tcp", "tls", "grpc", "grpcs"
  • Implement preread function
  • Make handler logic generic to be shared by HTTP and TCP

Issue reference

Fix #6679

@CLAassistant
Copy link

CLAassistant commented Feb 4, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Michael Russo seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@merusso merusso force-pushed the feat/ip-restriction-support-tcp branch from 6ed6bfa to 43b93a7 Compare February 4, 2023 22:16
end

local status = conf.status or 403
local message = conf.message or "Your IP address is not allowed"
local message = conf.message or string.format("IP address not allowed: %s", ngx.var.remote_addr)
Copy link
Author

Choose a reason for hiding this comment

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

I forgot to note that we updated the rejection message format also. This is causing tests to fail.

Will this string format change be accepted? It is technically unrelated to enabling TCP, but it helps us troubleshoot issues with rejected requests.

@hbagdi
Copy link
Member

hbagdi commented Feb 6, 2023

Can you please rebase this on top of 'master' and target it for 'master'?
New features always go into the mainline development branch (named master) first.

@scrudge
Copy link
Contributor

scrudge commented Feb 7, 2023

Can you please rebase this on top of 'master' and target it for 'master'? New features always go into the mainline development branch (named master) first.

We have a separate PR for master. The code has changed a bit between the 2 versions.

@hbagdi
Copy link
Member

hbagdi commented Feb 7, 2023

In that case, please get the other PR merged first and then we will move forward with this.
We don't ship features in patch releases so I'm not sure if we can even move forward with this or not.
cc @dndx

local binary_remote_addr = ngx.var.binary_remote_addr
if not binary_remote_addr then
return kong.response.error(403, "Cannot identify the client IP address, unix domain sockets are not supported.")
local status = 403
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this line overwrite the conf.status value?

Copy link
Author

Choose a reason for hiding this comment

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

Good question. This preserves the existing behavior which also uses a hardcoded status 403 instead of conf.status.

@merusso
Copy link
Author

merusso commented Feb 13, 2023

In that case, please get the other PR merged first and then we will move forward with this.
We don't ship features in patch releases so I'm not sure if we can even move forward with this or not.

I'm happy to target a 2.9.x or 2.x branch for this feature change if one becomes available.

@hbagdi
Copy link
Member

hbagdi commented Feb 21, 2023

Let's first get the PR targeting the master branch merged. We can then cherry-pick it back.

@merusso
Copy link
Author

merusso commented Feb 23, 2023

Sounds good. We'll continue working on PR #10245.

Resolves Kong#6679

This change adds TCP support to the ip-restriction plugin by
implementing the Stream module's preread function.

When a TCP connection is rejected due to IP restriction rules, a JSON
error response is written to the stream and the connection is closed.
@hanshuebner hanshuebner force-pushed the feat/ip-restriction-support-tcp branch from 43b93a7 to 25266ae Compare March 6, 2023 13:36
@hanshuebner
Copy link
Contributor

Closing as we intend to backport #10245 instead.

@hanshuebner hanshuebner closed this Mar 6, 2023
curiositycasualty pushed a commit that referenced this pull request Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants