Skip to content

Specify a local address when exposing ports with Docker#20891

Merged
t3chguy merged 4 commits intoelement-hq:developfrom
gibson042:2022-02-docker-run-ports
Nov 1, 2024
Merged

Specify a local address when exposing ports with Docker#20891
t3chguy merged 4 commits intoelement-hq:developfrom
gibson042:2022-02-docker-run-ports

Conversation

@gibson042
Copy link
Copy Markdown
Contributor

@gibson042 gibson042 commented Feb 3, 2022

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

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


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
Comment thread README.md Outdated
Copy link
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
Contributor Author

@novocaine Done.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Sep 6, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

1 similar comment
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@gibson042 gibson042 requested a review from germain-gg September 7, 2024 17:30
@t3chguy
Copy link
Copy Markdown
Member

t3chguy commented Oct 31, 2024

@gibson042 sorry this slipped through the cracks but if you'd be willing to resolve the conflicts & sign the CLA we can get this merged

@t3chguy t3chguy marked this pull request as draft October 31, 2024 14:05
gibson042 and others added 3 commits October 31, 2024 14:23
…to localhost

Signed-off-by: Richard Gibson <richard.gibson@gmail.com>
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
@gibson042 gibson042 force-pushed the 2022-02-docker-run-ports branch from c3d8e30 to cb1fe99 Compare October 31, 2024 18:28
@gibson042
Copy link
Copy Markdown
Contributor Author

@t3chguy Done.

@t3chguy t3chguy added the T-Task Tasks for the team like planning label Nov 1, 2024
@t3chguy t3chguy marked this pull request as ready for review November 1, 2024 10:57
@t3chguy t3chguy enabled auto-merge November 1, 2024 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-Task Tasks for the team like planning

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants