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.

Sign up for GitHub

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

Jump to bottom

Add rejectStatusCode option to IPAllowList middleware #10130

Merged
merged 1 commit into from Jan 9, 2024
Merged

Add rejectStatusCode option to IPAllowList middleware #10130

merged 1 commit into from Jan 9, 2024

Conversation

jfly
Copy link
Contributor

@jfly jfly commented Sep 17, 2023

What does this PR do?

Add rejectStatusCode option to IPAllowList middleware. If not specified, this defaults to the old behavior (returning a HTTP 403 Forbidden).

Motivation

Sometimes it's preferable to return a 404 rather than a 403 when rejecting a request. For example, you may not want to leak the fact that a page even exists.

I see other people have asked for this. See:

More

  • Added/updated tests
    • I tried to fix the failing tests. Please let me know if I should add a new test for this new functionality.
  • Added/updated documentation

@ldez
Copy link
Member

ldez commented Sep 17, 2023 •
edited

Hello,

can you rebase your PR onto the branch v3.0?

This will be completely off-topic.

I'm happy to talk to a cuber who helps with software around cubing, I'm also a cuber (not as speedy as young people).

@jfly jfly force-pushed the custom-status-code-for-ip-list-middleware branch from bfb94fe to eca58f3 Compare September 17, 2023 18:36
@jfly jfly changed the base branch from master to v3.0 September 17, 2023 18:37
@jfly
Copy link
Contributor Author

jfly commented Sep 17, 2023

can you rebase your PR onto the branch v3.0?

@ldez, done! For the record, I based this branch off of master because of this documentation:

Enhancements: [...] for Traefik v3: use branch master

Should that documentation be updated? Or is this PR not an "Enhancement"?

I'm also a cuber (not as speedy as young people).

Ha! Yeah, the young people are quite fast (so many of them were born after my first competition 😲!).

@ldez
Copy link
Member

ldez commented Sep 17, 2023

Should that documentation be updated? Or is this PR not an "Enhancement"?

It's more complex: currently, the v3 is in beta so we continue integrating enhancements and breaking changes. It was not the initial plan but it's the current status.

@ldez ldez modified the milestones: 2.10, 3.0 Sep 17, 2023
jfly added a commit to jfly/traefik that referenced this pull request Sep 17, 2023
@jfly
Instruct people to send enhancements to the v3 branch
0c97a95 
Per
traefik#10130 (comment):

> It's more complex: currently, the v3 is in beta so we continue
> integrating enhancements and breaking changes. It was not the initial
> plan but it's the current status.
@jfly jfly mentioned this pull request Sep 17, 2023
Merged
@jfly
Copy link
Contributor Author

jfly commented Sep 17, 2023

Sounds good. I've submitted #10131 to update the PR template accordingly.

@jfly
Copy link
Contributor Author

jfly commented Nov 12, 2023

@ldez any chance of getting this merged up?

geraldcroes
geraldcroes approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@geraldcroes geraldcroes left a comment

Choose a reason for hiding this comment

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

LGTM, the feature is similar to #10147

@jfly
Copy link
Contributor Author

jfly commented Nov 22, 2023

@geraldcroes, thanks for the approval. What's next in terms of getting this merged?

@geraldcroes
Copy link
Contributor

We need 2 other reviews. I'll ask during triage tomorrow if others can tackle it quickly since the feature is straightforward

rtribotte
rtribotte requested changes Nov 23, 2023
View reviewed changes
Copy link
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

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

Hello @jfly,

Thanks for this contribution!

There are a few things missing, and I left a comment for that on the ipallowlist.md.
There is also an update of the dynamic configuration reference files that is missing (file.toml, file.yaml, docker-labels.yaml, etc...) in docs/content/reference/dynamic-configuration directory.

docs/content/middlewares/http/ipallowlist.md Show resolved Hide resolved
@jfly jfly requested a review from rtribotte November 27, 2023 02:08
@jfly
Copy link
Contributor Author

jfly commented Nov 27, 2023

@rtribotte, thanks for the review! I've updated the PR in light of your feedback.

@jfly jfly force-pushed the custom-status-code-for-ip-list-middleware branch 4 times, most recently from 6811662 to df222d9 Compare November 27, 2023 03:10
@rtribotte
Copy link
Member

Hello @jfly,

Thanks for the recent changes!
The last thing I would suggest to do is to test this new option in ip_allowlist_test.go.

mmatur
mmatur requested changes Nov 27, 2023
View reviewed changes
Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

Thanks @jfly for this PR.

Could you please add tests and ensure that invalid status codes are handled correctly?

pkg/middlewares/ipallowlist/ip_allowlist.go Outdated Show resolved Hide resolved
@rtribotte
Copy link
Member

Adding the waiting-for-corrections label while waiting for the unit tests to be updated.

@jfly jfly force-pushed the custom-status-code-for-ip-list-middleware branch 3 times, most recently from b91a0a2 to f198c97 Compare December 15, 2023 10:58
@jfly
Copy link
Contributor Author

jfly commented Dec 15, 2023

@rtribotte, @mmatur, I've added the requested tests to ip_allowlist_test.go.

CI failed on TestServiceHealthChecker_Launch/healthy_grpc_server_becoming_sick, but I don't think that's my fault, as the same test passes locally.

@jfly jfly requested a review from mmatur December 15, 2023 11:09
@jfly
Copy link
Contributor Author

jfly commented Dec 29, 2023

@mmatur, @rtribotte, could I get another review here? Thanks!

mmatur
mmatur approved these changes Jan 5, 2024
View reviewed changes
Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

LGTM

@nmengin nmengin added the priority/P2 need to be fixed in the future label Jan 8, 2024
rtribotte
rtribotte approved these changes Jan 9, 2024
View reviewed changes
Copy link
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@jfly @mmatur
Add rejectStatusCode option to IPAllowList middleware
3847a84 
If not specified, this defaults to the old behavior (returning a HTTP
403 Forbidden).

Sometimes it's preferable to return a 404 rather than a 403 when
rejecting a request. For example, you may not want to leak the fact that
a page even exists.

I see some other people have asked for this. See:
- https://www.reddit.com/r/Traefik/comments/xkcxhb/hide_forbidden_pages_from_public_view/
- traefik#2039: It's not clear to me
  if the OP was requesting something specific to the `IPAllowList` or
  not, but there are definitely some people asking for this in the
  thread.
@mmatur mmatur force-pushed the custom-status-code-for-ip-list-middleware branch from f198c97 to 3847a84 Compare January 9, 2024 17:00
@traefiker traefiker merged commit ccf3a99 into traefik:v3.0 Jan 9, 2024
9 checks passed
@jfly
Copy link
Contributor Author

jfly commented Jan 9, 2024

Thanks, @rtribotte!

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

@geraldcroes geraldcroes geraldcroes approved these changes

@mmatur mmatur mmatur approved these changes

@rtribotte rtribotte rtribotte approved these changes

Assignees
No one assigned
Labels
area/middleware kind/enhancement a new or improved feature. priority/P2 need to be fixed in the future size/S
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jfly @ldez @geraldcroes @rtribotte @mmatur @nmengin @traefiker