Skip to content

Commit

Permalink
Use ssl_reject_handshake to reject requests to default https site
Browse files Browse the repository at this point in the history 
Instead of creating a dummy certificate, we can return an SSL protocol error, which will generate a descriptive error message in the browser.
  • Loading branch information
@TheBeeZee
TheBeeZee committed Feb 3, 2023
1 parent fd30cfe commit a7f0c3b
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 18 deletions.
4 changes: 1 addition & 3 deletions docker/rootfs/etc/nginx/conf.d/default.conf
View file
Expand Up @@ -32,9 +32,7 @@ server {
server_name localhost;
access_log /data/logs/fallback_access.log standard;
error_log /dev/null crit;
ssl_certificate /data/nginx/dummycert.pem;
ssl_certificate_key /data/nginx/dummykey.pem;
include conf.d/include/ssl-ciphers.conf;

This comment has been minimized.

Sign in to view

Copy link
@nietzscheanic

nietzscheanic May 19, 2023

Contributor

that broked tlsv1.3 and the cipher suite definition

This comment has been minimized.

Sign in to view

Copy link
@TheBeeZee

TheBeeZee via email May 19, 2023

Author Contributor

This comment has been minimized.

Sign in to view

Copy link
@nietzscheanic

nietzscheanic May 20, 2023

Contributor

you removed the ssl_protocols and ssl_ciphers directive in the default config due to removing the include of conf.d/include/ssl-ciphers.conf. see my pull request for more information. i did a quick search in the issues and linked all related reports of the problem to the push request. #2932

This comment has been minimized.

Sign in to view

Copy link
@TheBeeZee

TheBeeZee via email May 22, 2023

Author Contributor

This comment has been minimized.

Sign in to view

Copy link
@nietzscheanic

nietzscheanic May 22, 2023 •
edited

Contributor

maybe it's better NOT to put the ssl directives outside the server context into the http context. i see a chance that this will break dedicated ssl configurations for different ip- or port-based server configurations at the same host. i would feel better with leaving the default ssl configuration in the the default server configuration. in this way someone can define different ssl ciphers and ssl protocols for different ip- or port-based server configurations. please have a look at this argumentation: mozilla/ssl-config-generator#141 (comment)

This comment has been minimized.

Sign in to view

Copy link
@TheBeeZee

TheBeeZee via email May 22, 2023

Author Contributor

This comment has been minimized.

Sign in to view

Copy link
@nietzscheanic

nietzscheanic May 22, 2023 •
edited

Contributor

hmm, i think you have missed, that this behaviour only applies to name-based virtual hosts. To quote makhomed:

You tries to set different ssl_protocols in name-based virtual
servers. This will not work, because OpenSSL fixes
protocol before the name is known. At the same time, in different
IP-based (or port-based) virtual servers are quite possible
use different ssl_protocols.

This comment has been minimized.

Sign in to view

Copy link
@TheBeeZee

TheBeeZee via email May 22, 2023

Author Contributor

This comment has been minimized.

Sign in to view

Copy link
@nietzscheanic

nietzscheanic May 23, 2023

Contributor

I just studied the NGINX documentation for this question. And you are completely right with your presumption.

I would think it should be possible to provide defaults in the http context and override them in server contexts no?

@ https://www.nginx.com/blog/avoiding-top-10-nginx-configuration-mistakes/#directive-inheritance

NGINX directives are inherited downwards, or “outside‑in”: a child context – one nested within another context (its parent) – inherits the settings of directives included at the parent level. For example, all server{} and location{} blocks in the http{} context inherit the value of directives included at the http level, and a directive in a server{} block is inherited by all the child location{} blocks in it. However, when the same directive is included in both a parent context and its child context, the values are not added together – instead, the value in the child context overrides the parent value.

So I will revise my pull request and move the include from the first server directive up to the host directive. ✌️
ssl_reject_handshake on;

return 444;
}
15 changes: 0 additions & 15 deletions docker/rootfs/etc/services.d/nginx/run
View file
Expand Up @@ -30,21 +30,6 @@ then
else
echo resolver "$(awk 'BEGIN{ORS=" "} $1=="nameserver" { sub(/%.*$/,"",$2); print ($2 ~ ":")? "["$2"]": $2}' /etc/resolv.conf) valid=10s;" > /etc/nginx/conf.d/include/resolvers.conf
fi
# Generate dummy self-signed certificate.
if [ ! -f /data/nginx/dummycert.pem ] || [ ! -f /data/nginx/dummykey.pem ]
then
echo "Generating dummy SSL certificate..."
openssl req \
-new \
-newkey rsa:2048 \
-days 3650 \
-nodes \
-x509 \
-subj '/O=localhost/OU=localhost/CN=localhost' \
-keyout /data/nginx/dummykey.pem \
-out /data/nginx/dummycert.pem
echo "Complete"
fi

# Handle IPV6 settings
/bin/handle-ipv6-setting /etc/nginx/conf.d
Expand Down

1 comment on commit a7f0c3b

@dinbtechit
Copy link

Choose a reason for hiding this comment

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

This whole thing broke the ability to access nginx proxy manage on https. Is there any other way to enable https?

Please sign in to comment.