Skip to content

do not expose port 443 because it is not used by default #130

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

Closed
wants to merge 1 commit into from

Conversation

thaJeztah
Copy link

The default nginx configuration does not configure nginx to be
listening on port 443.

However, the Dockerfile exposes this port, which has some implications;

  • users get the expectation that nginx is listening on port 443, but
    when publishing the port discover that no connection can be
    established.
  • when using -P / --publish-all, docker will publish port 443 (using
    random port mapping)
  • some proxies use "expose" to detect which ports are provided by a
    container and make configuration decisions based on that information,
    again leading to port 443 being mapped, but not functional.

This change removes port 443 from the Dockerfile.

Removing this port can be a breaking change, so should be documented
(i.e., users should now add EXPOSE 443 in their Dockerfile if they
extend the nginx image, or use --expose 443 when running the image.

/cc @tianon to check if this matches the "best practices" for official images

The default nginx configuration does not configure nginx to be
listening on port 443.

However, the Dockerfile exposes this port, which has some implications;

- users get the expectation that nginx is listening on port 443, but
  when publishing the port discover that no connection can be
  established.
- when using `-P` / `--publish-all`, docker will publish port 443 (using
  random port mapping)
- some proxies use "expose" to detect which ports are provided by a
  container and make configuration decisions based on that information,
  again leading to port 443 being mapped, but not functional.

This change removes port 443 from the Dockerfile.

Removing this port can be a breaking change, so should be documented
(i.e., users should now add `EXPOSE 443` in their Dockerfile if they
extend the nginx image, or use `--expose 443` when running the image.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@alaz
Copy link

alaz commented Nov 17, 2016

It would be a breaking change for me indeed.

BTW, you probably know, there is a Docker issue moby/moby#3465 discussing a way to reset parent image properties, e.g. EXPOSE or VOLUME

@thaJeztah
Copy link
Author

I know about that issue. However, in this case, the EXPOSE is incorrect, because there is not port exposed by the image.

@thaJeztah
Copy link
Author

The image before nginx took over maintenance of this image did not contain port 443 (docker-library/nginx@070ac65), so it looks like this changed when it was transferred to nginx f985f4f

@tianon
Copy link
Contributor

tianon commented Nov 17, 2016

Can NGINX have port 443 enabled without certificates supplied? (and would it make sense to add that to the default config?)

Generally I do agree that removing this makes sense, but NGINX is a little bit of a special case. 😕

@thresheek
Copy link
Member

It does not make much sense just to enable nginx listening on port 443 by default IMO - it's expected that it will serve TLS connections, which is impossible without a proper TLS certificate and shipping a self-signed certificate will surely introduce more problems than it will solve.

It was an error introducing this one in the first place, indeed. I'm not sure that reverting it now is a good idea - I can imagine lots of images are depending on that already...

@thaJeztah
Copy link
Author

I see the problem of breaking backward compat, and decided to open this PR for discussion. otoh, ports opened that are not listened on (and incorrectly configured proxies), may also be an issue. Tough choice. 😞

@yosifkit
Copy link
Contributor

The "ports opened that are not listening" only actually opens a port when you -P on docker run (really, you should choose a port, like docker swarm mode services force).

As for automatic proxies that just route to a container based on its EXPOSED ports, they really should be doing a health check and would run into the same problem in reverse if nginx is configured to only listen on 443.

@thaJeztah
Copy link
Author

The "ports opened that are not listening" only actually opens a port when you -P on docker

Yes, and basically, that's the only use of EXPOSE - announce which ports should be published if -P / --publish-all is used, as it has no other functionality

@tantalic
Copy link

tantalic commented Feb 7, 2017

There are a number of tools which automatically use the EXPOSEd ports to perform health checks and the like. Having a port exposed but not active (or active with an SSL certificate that should not be trusted) in the default image results in a base image that cannot be used in such circumstances. Given that it (currently) is fairly simple to expose an additional port but not possible to unexposed a port it seems that only port 80 should be exposed.

@zyrill
Copy link

zyrill commented Feb 14, 2017

If I may weigh in: from an enterprise perspective, the "so we have this bugfix that breaks backwards compatibility" is really not a point that makes much sense. Quite the contrary: when designing my systems, I trust containers to be secure by default, so I can security test and monitor my infrastructure using those defaults plus any additional customisations we have in place. Example: if any traffic on virtual networks is discovered that flows on not explicitly defined routes or connects to ports that should be closed, you know something is dodgy.

Reading through the comments, everybody up until now concurs that the current behaviour is wrong.

It's a bug, fix it.

@thresheek
Copy link
Member

I'll introduce this as a breaking change on the next nginx mainline/stable rollover - somewhere in April/May'17.

@thresheek thresheek added this to the stable/mainline switch milestone Mar 17, 2017
@thresheek
Copy link
Member

Done starting with nginx:1.13.0 / mainline and nginx:1.12.0 / stable.

@thresheek thresheek closed this Apr 26, 2017
@thaJeztah
Copy link
Author

Thanks!

@thaJeztah thaJeztah deleted the dont-expose-unused-ports branch April 28, 2017 06:53
@AdrianRibao
Copy link

This broke my app and had to go back to a previous version.

What is the prefered method for opening port 443? Create a new Dockerfile based on this one and expose the port 443?

@alaz
Copy link

alaz commented Apr 28, 2017

@AdrianRibao the top post outlines two solutions.

@thaJeztah
Copy link
Author

@AdrianRibao if all you need is publish the port, just -p <host-port>:443; expose is not a requirement to actually use the port, only if you want to use random port-mapping

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.

8 participants