Skip to content

Update gixy nginx config checker #2093

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

Draft
aequitas wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Update gixy nginx config checker #2093

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7618b8e
Update gixy nginx config checker
aequitas Jun 16, 2026
6a74c6a
Skip some gixy checks
bwbroersma Jun 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion docker/webserver.Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
ARG CERTBOT_VERSION=5.6.0
ARG GIXY_NG_VERSION=0.2.48

FROM nginx:1.31.2-alpine3.23

Expand All @@ -15,7 +16,8 @@ RUN apk upgrade --no-cache \

# install nginx config static analysis tool
RUN python3 -m venv /opt/gixy
RUN /opt/gixy/bin/pip install gixy==0.1.21
ARG GIXY_NG_VERSION
RUN /opt/gixy/bin/pip install gixy-ng==${GIXY_NG_VERSION}

# install certbot
RUN python3 -m venv /opt/certbot
Expand Down
4 changes: 4 additions & 0 deletions docker/webserver/nginx_templates/default.conf.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ server {

# monitoring, requires authentication, override headers, since CSP is too strict
location /grafana {
add_header_inherit off;

@bwbroersma bwbroersma Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The include below both have add_headers, therefore the default behaviour seems okay:

There could be several add_header directives. These directives are inherited from the previous configuration level if and only if there are no add_header directives defined on the current level. Inheritance rules can be redefined with the add_header_inherit directive (1.29.3).

Is the additional add_header_inherit really needed?

@aequitas aequitas Jun 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The new gixy-ng (https://github.com/dvershinin/gixy) fails on the current config because of the double headers https://github.com/internetstandards/Internet.nl/actions/runs/27760088839/job/82132454086#step:14:107. Maybe we can mark them so they are ignored (https://github.com/dvershinin/gixy/blob/master/docs/en/configuration.md)?. The only other issue found was a HSTS age of 0 which we also want to ignore. So the newer gixy didn't find anything interesting, maybe in future changes it might?

@bwbroersma bwbroersma Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Problem: [add_header_redefinition] Nested "add_header" drops parent headers.

Which is all by design, and the first time you work with nginx will find this out. I don't really get the warning, but I think we could add change:

Internet.nl/Makefile

Lines 313 to 315 in a5f620d

check-gixy: env=test
check-gixy: ## run nginx config check
${DOCKER_COMPOSE_CMD} exec webserver /opt/gixy/bin/gixy /etc/nginx/nginx.conf

To add -skips:

 	${DOCKER_COMPOSE_CMD} exec webserver /opt/gixy/bin/gixy -skips hsts_header,add_header_redefinition /etc/nginx/nginx.conf

Another question, shouldn't it be better if this would move to lint/check? (although it's way easier to integrate here I guess).

@aequitas aequitas Jun 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't like disabling tests all out because then we might miss valid issues in the future. But then again, we don't have these checks at all with the current gixy.

I think I put it into integrationtest originally because it needs the container to render the config from the templates at runtime and lint/test is more for static tests.

@bwbroersma bwbroersma Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I pushed the code just a few minutes before your comment. I agree, but is it possible on line level?
Does it still complain if add_header_inherit on; is set on http level (the highest level)?
Then maybe only HSTS can be skipped, until #324 is fixed.

include http.headers;
include hsts_h3.headers;
auth_basic "Please enter your monitoring username and password";
Expand All @@ -303,6 +304,7 @@ server {
proxy_pass $grafana;
}
location /prometheus {
add_header_inherit off;
include http.headers;
include hsts_h3.headers;
auth_basic "Please enter your monitoring username and password";
Expand All @@ -312,6 +314,7 @@ server {
proxy_pass $prometheus;
}
location /alertmanager {
add_header_inherit off;
include http.headers;
include hsts_h3.headers;
auth_basic "Please enter your monitoring username and password";
Expand All @@ -323,6 +326,7 @@ server {

# routinator proxy for internal use on multi instance setups
location /routinator/ {
add_header_inherit off;
include http.headers;
include hsts_h3.headers;

Expand Down
Loading