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

Add reverse proxy configuration support for remote IP address #14959

Merged
merged 7 commits into from
Mar 15, 2021

Conversation

lafriks
Copy link
Member

@lafriks lafriks commented Mar 11, 2021

By default trust single reverse proxy from loopback IP addresses (127.0.0.0/8, ::1/128)

In containerized environments by default trust requests from all IP addresses

EDIT: It is breaking change for users that use more then single reverse proxy to serve Gitea content

@lafriks lafriks added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Mar 11, 2021
@lafriks lafriks added this to the 1.14.0 milestone Mar 11, 2021
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 11, 2021
@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

how exactly is this supposed to work?
how would I set a trusted thing if Gitea listens for proxied traffic on a unix socket (I actually do that)?

@lafriks
Copy link
Member Author

lafriks commented Mar 11, 2021

I think for socket remote address should be 127.0.0.1

@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

I think for socket remote address should be 127.0.0.1

so they're considered as being one and the same here?

@lafriks
Copy link
Member Author

lafriks commented Mar 11, 2021

I think yes, but that should probably needs to be tested

@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

I think yes, but that should probably needs to be tested

would this be sort of activated by default if I build a new version?
because then tests for this could be nice prior to merge :D
if it doesn't do anything unless I have it in the config that's better but it'd still be great to have the behaviour tested and understood.

@lafriks
Copy link
Member Author

lafriks commented Mar 11, 2021

I think yes, but that should probably needs to be tested

would this be sort of activated by default if I build a new version?
because then tests for this could be nice prior to merge :D
if it doesn't do anything unless I have it in the config that's better but it'd still be great to have the behaviour tested and understood.

yes, it will be activated by default otherwise it will be insecure. By default it will trust single proxy server from localhost.
Fixed library to correctly handle unix socket address that was @ now it will be treated as 127.0.0.1:0 so trusted by default.

@lafriks lafriks added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Mar 11, 2021
@lafriks lafriks changed the title Add reverse proxy configuration support for remote IP address validation Add reverse proxy configuration support for remote IP address Mar 11, 2021
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 15, 2021
@techknowlogick
Copy link
Member

@lafriks can I have access to your fork so I can "update branch" as needed?

@lafriks
Copy link
Member Author

lafriks commented Mar 15, 2021

@techknowlogick done

@lafriks
Copy link
Member Author

lafriks commented Mar 15, 2021

🚀

@lafriks lafriks merged commit 044cd4d into go-gitea:master Mar 15, 2021
@lafriks lafriks deleted the fix/reverse_proxy_headers branch March 15, 2021 22:27
@lafriks lafriks mentioned this pull request Mar 15, 2021
1 task
@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! topic/security Something leaks user information or is otherwise vulnerable. Should be fixed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants