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

Specify a local address when exposing ports with Docker #20891

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

gibson042
Copy link

@gibson042 gibson042 commented Feb 3, 2022

cf. https://docs.docker.com/engine/reference/commandline/run/#publish-or-expose-port--p---expose

Notes: none


This PR currently has no changelog labels, so will not be included in changelogs.

A reviewer can add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is, or add Type: [enhancement/defect/task] to the description and I'll add them for you.

@gibson042 gibson042 requested a review from a team as a code owner February 3, 2022 15:33
@@ -174,15 +174,15 @@ The Docker image can be used to serve element-web as a web server. The easiest w
it is to use the prebuilt image:

```bash
docker run -p 80:80 vectorim/element-web
docker run -p 127.0.0.1:80:80 vectorim/element-web
Copy link
Author

Choose a reason for hiding this comment

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

I'd also like to make a further change if you don't object:

Suggested change
docker run -p 127.0.0.1:80:80 vectorim/element-web
docker run --rm -p 127.0.0.1:80:80 vectorim/element-web

Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

Before we can move this forward would you be able to write a description of what you're trying to achieve and what use cases you're trying to cater for

@gibson042
Copy link
Author

This corrects what looks like accidental creation of potentially attackable network exposure. From the linked Docker documentation:

Note that ports which are not bound to the host (i.e., -p 80:80 instead of -p 127.0.0.1:80:80) will be accessible from the outside.

The local development server should instead be confined only to the local host unless there is specific reason to make it network available (which is what this PR addresses).

@novocaine
Copy link
Contributor

The command is suggested to serve element-web as a web server, with all the use cases that entails.

I think you might be assuming that only a development or local use-case exists, but there is also the use case of serving it to other clients on the network (e.g. run your own app.element.io with your own customisations, as many people do).

@turt2live
Copy link
Member

fwiw, the documentation in this area was written more as a point of interest rather than something to copy/paste. It's fairly rare that folks use bare docker commands these days, so the important aspect becomes the ports and container name.

@gibson042
Copy link
Author

I think you might be assuming that only a development or local use-case exists, but there is also the use case of serving it to other clients on the network (e.g. run your own app.element.io with your own customisations, as many people do).

That use case is certainly valid, but I believe running a server that supports it should be intentional rather than accidental—it's generally bad form to encourage creation of unnecessary attack surface area.

fwiw, the documentation in this area was written more as a point of interest rather than something to copy/paste. It's fairly rare that folks use bare docker commands these days, so the important aspect becomes the ports and container name.

Acknowledged. Unless you see harm in these changes, though, I still consider it valuable to default to restricted access.

@novocaine
Copy link
Contributor

I think you might be assuming that only a development or local use-case exists, but there is also the use case of serving it to other clients on the network (e.g. run your own app.element.io with your own customisations, as many people do).

That use case is certainly valid, but I believe running a server that supports it should be intentional rather than accidental—it's generally bad form to encourage creation of unnecessary attack surface area.

My suggestion is to list the different commands for the different use cases noting the implications.

@gibson042
Copy link
Author

@novocaine Done.

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.

None yet

4 participants