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

Update nginx config with proxy headers #121

Merged
merged 4 commits into from
Jul 26, 2019
Merged

Update nginx config with proxy headers #121

merged 4 commits into from
Jul 26, 2019

Conversation

Pyrrah
Copy link
Contributor

@Pyrrah Pyrrah commented Jul 23, 2019 •
edited

Link w/ #120: Add comments for users using argument USE_FORWARDED_HEADERS=1

@Pyrrah
Update nginx config with proxy headers
4c9375f 
Add comments for users using argument USE_FORWARDED_HEADERS=1
dunglas
dunglas requested changes Jul 23, 2019
View reviewed changes
README.md Outdated
@@ -322,6 +322,10 @@ server {
proxy_read_timeout 24h;
proxy_http_version 1.1;
proxy_set_header Connection "";
## Remove comments only if you use argument USE_FORWARDED_HEADERS=1 ##
Copy link
Owner

Choose a reason for hiding this comment

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

It’s safe to always provide these headers, no need to comment them. So, the comment should be “Be sure to set USE_FORWRDED_HEADERS to 1 to allow the hub to use those headers”.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok to change the comment with "Be sure to set USE_FORWARDED_HEADERS=1 to allow the hub to use those headers"

README.md Outdated
@@ -322,6 +322,10 @@ server {
proxy_read_timeout 24h;
proxy_http_version 1.1;
proxy_set_header Connection "";
## Remove comments only if you use argument USE_FORWARDED_HEADERS=1 ##
#proxy_set_header Host $host;
Copy link
Owner

@dunglas dunglas Jul 23, 2019 •
edited

Choose a reason for hiding this comment

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

X-Forwarded-Host must be set to prevent a security vulnerability. There are no needs to override Host however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I withdraw that

README.md Outdated
@@ -322,6 +322,10 @@ server {
proxy_read_timeout 24h;
proxy_http_version 1.1;
proxy_set_header Connection "";
## Remove comments only if you use argument USE_FORWARDED_HEADERS=1 ##
#proxy_set_header Host $host;
#proxy_set_header X-Real-IP $remote_addr;
Copy link
Owner

@dunglas dunglas Jul 23, 2019 •
edited

Choose a reason for hiding this comment

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

Setting this one is useless if X-Forwarded-For is set.
However X-Forwarded-Scheme must be set to prevent an attacker to hijack it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I withdraw that. Thanks for the feedback

@Pyrrah
Update nginx config with header X-Forwarded-For
475e32b 
To get the IP address of client connecting to the hub
@dunglas
Copy link
Owner

dunglas commented Jul 24, 2019 •
edited

It's not possible to remove the other headers from the NGINX's config. If the proxy_headers handler is registered, both X-Forwarded-For, X-Forwarded-Host and X-Forwarded-Proto must be set (or removed) by the reverse proxy. Otherwise, it introduces a security issue.

@Pyrrah
Copy link
Contributor Author

Pyrrah commented Jul 24, 2019

Thanks, I added the missing headers.
Also, I corrected a mistake in the configuration with port 443 (not 80).

dunglas
dunglas reviewed Jul 25, 2019
View reviewed changes
README.md Outdated

## Be sure to set USE_FORWARDED_HEADERS=1 to allow the hub to use those headers ##
proxy_set_header X-Forwarded-Host $host:$server_port;
proxy_set_header X-Forwarded-Server $host;
Copy link
Owner

Choose a reason for hiding this comment

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

This one is not supported, but X-Forwarded-Proto is missing.

Copy link
Owner

Choose a reason for hiding this comment

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

        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header X-Forwarded-Host $host;
        proxy_set_header X-Forwarded-Proto $scheme;

@Pyrrah
Update nginx config with proxy headers
5494f4a 
* Fixed mistake on port to give SSL connection (443, not 80)
* Added proxy-headers X-Forwarded-For, X-Forwarded-Host and X-Forwarded-Proto
@Pyrrah
Copy link
Contributor Author

Pyrrah commented Jul 25, 2019

ok, I'm tired -_-
I didn't take the right parameter you gave me. It's good now.

@dunglas dunglas merged commit 75db68f into dunglas:master Jul 26, 2019
@dunglas
Copy link
Owner

dunglas commented Jul 26, 2019

Thanks @Pyrrah!

@Pyrrah Pyrrah deleted the patch-1 branch August 22, 2019 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dunglas dunglas dunglas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Pyrrah @dunglas