Skip to content

nginx: reject requests with unexpected Host header#809

Merged
matthew-white merged 39 commits intogetodk:nextfrom
alxndrsn:enforce-host-check-https-bronto-approach
Dec 10, 2024
Merged

nginx: reject requests with unexpected Host header#809
matthew-white merged 39 commits intogetodk:nextfrom
alxndrsn:enforce-host-check-https-bronto-approach

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 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

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
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.
yanokwa pushed a commit to yanokwa/odk-central that referenced this pull request Jun 11, 2025
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").
yanokwa pushed a commit to yanokwa/odk-central that referenced this pull request Jun 11, 2025
yanokwa pushed a commit to yanokwa/odk-central that referenced this pull request Jun 11, 2025
This comment was merged as part of getodk#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