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

nginx: reject requests with unexpected Host header #809

Merged

Conversation

alxndrsn
Copy link
Contributor

@alxndrsn alxndrsn commented Dec 2, 2024

Supersedes #747

Blocked by #814

What has been done to verify that this works as intended?

Tests.

Why is this the best possible solution? Were any other approaches considered?

#747 was also considered - this is up for debate.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Shouldn't change normal usage.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Probably not.

Before submitting this PR, please make sure you have:

  • branched off and targeted the next branch OR only changed documentation/infrastructure (master is stable and used in production)
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

@alxndrsn alxndrsn marked this pull request as ready for review December 3, 2024 06:19
@alxndrsn alxndrsn requested a review from brontolosone December 3, 2024 06:19
alxndrsn pushed a commit to alxndrsn/odk-central that referenced this pull request Dec 3, 2024
Working on getodk#809 I noticed that the location of SSL certs is based either on the domain name, or on the method of supply of SSL certs.

Cert provision approach should probably not affect the nginx "server_name" setting.

Also, the old variable name `CNAME` (short for "certificate name?") is easily confused with the DNS concept of CNAME records ("canonical names").
@alxndrsn alxndrsn requested a review from yanokwa December 6, 2024 06:38
Copy link
Contributor

@brontolosone brontolosone left a comment

Choose a reason for hiding this comment

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

I have fears, please see comments

files/nginx/odk.conf.template Outdated Show resolved Hide resolved
files/nginx/redirector.conf Show resolved Hide resolved
test/test-nginx.js Show resolved Hide resolved
test/test-nginx.js Outdated Show resolved Hide resolved
alxndrsn added a commit that referenced this pull request Dec 7, 2024
Working on #809 I noticed that the location of SSL certs is based either on the domain name, or on the method of supply of SSL certs.

Cert provision approach should probably not affect the nginx "server_name" setting.

Also, the old variable name `CNAME` (short for "certificate name?") is easily confused with the DNS concept of CNAME records ("canonical names").
@matthew-white matthew-white merged commit 0f7bad6 into getodk:next Dec 10, 2024
2 checks passed
alxndrsn pushed a commit to alxndrsn/odk-central that referenced this pull request Dec 10, 2024
This comment was merged as part of getodk#809, but was not correct by the time the PR was merged.
@alxndrsn alxndrsn deleted the enforce-host-check-https-bronto-approach branch December 10, 2024 05:20
alxndrsn added a commit that referenced this pull request Dec 10, 2024
This comment was merged as part of #809, but was not correct by the time the PR was merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants