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

Making docker usage with localhost and external ip more clear #3836

Merged
merged 4 commits into from
Apr 30, 2024
Merged

Making docker usage with localhost and external ip more clear #3836

merged 4 commits into from
Apr 30, 2024

Conversation

rakovskij-stanislav
Copy link
Contributor

@rakovskij-stanislav rakovskij-stanislav commented Oct 2, 2023

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

This pull request makes a small change in README.md related to running in docker. Previously it said that using -p 3001:3001 will expose this port to localhost, but it actually exposes it to 0.0.0.0, and it can lead problems like a target for brute force (it's expecially dangerous for VPS or for machines that have external IP).

Type of change

Please delete any options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

image

It displays the situation met by me by using default commandline.

image

And what would be if we will use the second commandline.

@chakflying
Copy link
Collaborator

chakflying commented Oct 2, 2023

Previous related discussion: #3002

@rakovskij-stanislav
Copy link
Contributor Author

Previous related discussion: #3002

My PR does not change a code, but only extends docs with a clarification that using ordinary "3001:3001" could lead unexpected results like (e.g. security issues like continious brute force, leak of password via unencrypted http due MiTM attack). I know that is would be a problem of end user itself, but not everybody is friendly with container technologies and administration :)

README.md Outdated Show resolved Hide resolved
@chakflying
Copy link
Collaborator

I agree that this is a better solution. However, I think the language in the proposed change is not clear and maybe confusing.

If you want to run it at localhost...

In both cases, the container is still running at localhost.

(without exposing port for other users...

"Other users" can be on the same machine and they can still access the application.

I think the only difference here is whether the port binding is accessible for external IPs. Explaining that clearly would be enough.

@CommanderStorm

This comment was marked as resolved.

@louislam
Copy link
Owner

louislam commented Oct 4, 2023

I understand 3001:3001 is too open, but 127.0.0.1:3001:3001 really is too closed.

If people copy this command in the README.md without reading, I think there will be a lot of people asking why Uptime Kuma is not working.

So please move it to the wiki instead under Change Port and Volume section without a full command:
https://github.com/louislam/uptime-kuma/wiki/%F0%9F%94%A7-How-to-Install

in terms of documentation, I think this might be a good usecase for an alert.

It seems that the feature had been removed.

@CommanderStorm

This comment was marked as resolved.

@louislam

This comment was marked as resolved.

@CommanderStorm

This comment was marked as resolved.

@rakovskij-stanislav
Copy link
Contributor Author

I understand 3001:3001 is too open, but 127.0.0.1:3001:3001 really is too closed.

@louislam, uptime kuma allows intruder who can brute force admin password (first issue - password strength, uptime kuma allows me to use admin/admin credentials on setup stage, we need to forbid some dictionary (e.g. from rockyou dictionary) and easy-to-brute passwords) without rate-limiting and captcha after few failture attempts (it's the second security issue). User compromise leads to credentials leak (easy way - using Export Backup) and recon (via pinging other services and manipulating with payloads).

README.md Outdated Show resolved Hide resolved
@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Feb 4, 2024

I have updated the docs-change proposed in this PR.
I think this is now ready for merging @louislam

@CommanderStorm CommanderStorm added the area:documentation Improvements or additions to documentation label Apr 3, 2024
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Thanks for clarification in the docs ✨

All changes in this PR are small and uncontroversial
⇒ merging with junior maintainer approval

@CommanderStorm CommanderStorm merged commit 9f2cf28 into louislam:master Apr 30, 2024
5 checks passed
@@ -43,11 +43,18 @@ It is a temporary live demo, all data will be deleted after 10 minutes. Sponsore
docker run -d --restart=always -p 3001:3001 -v uptime-kuma:/app/data --name uptime-kuma louislam/uptime-kuma:1
```

Uptime Kuma is now running on http://localhost:3001
Uptime Kuma is now running on <http://0.0.0.0:3001>.
Copy link
Owner

Choose a reason for hiding this comment

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

I am still thinking, using localhost here could have more accessible for someone who would like to try this in their current PC. While 0.0.0.0 is not working for any cases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not quite sure what case you are refering to.
I think you are referring to cases where the default route is not local or completely borked. Thing is, that I am not quite sure if localhost wou work in this case.

I assume that in somebody goes around and configures his*her default network route, they are knowlegable to know how to deal with this
=> likely does not matter

If you think this is better, I can revert this change ^^

https://superuser.com/questions/949428/whats-the-difference-between-127-0-0-1-and-0-0-0-0
does claim

The all-zero value does have a special meaning. So it is "valid", but has a meaning that may not be appropriate (and thus treated as not valid) for particular circumstances. It is basically the "no particular address" placeholder. For things like address binding of network connections, the result can be to assign an appropriate interface address to the connection. If you are using it to configure an interface, it can remove an address from the interface, instead. It depends on the context of use to determine what "no particular address" really does.

In the context of a route entry, it usually means the default route. That happens as a result more of the address mask, which selects the bits to compare. A mask of "0.0.0.0" selects no bits, so the compare will always succeed. So when such a route is configured, there is always somewhere for packets to go (if configured with a valid destination).

README.md Show resolved Hide resolved
CommanderStorm added a commit that referenced this pull request May 1, 2024
louislam pushed a commit that referenced this pull request May 2, 2024
Co-authored-by: Nelson Chan <3271800+chakflying@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants