Skip to content

Listen at all network interfaces. #1616

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
Closed

Conversation

4ntoine
Copy link
Contributor

@4ntoine 4ntoine commented Jan 8, 2022

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • What kind of change does this PR introduce?
    This is a minor change: daemon listening on all the network interfaces, not just 127.0.0.1
  • What is the current behavior?
    Listening to 127.0.0.1
  • What is the new behavior?
    Listening to all the network interfaces. This might be needed for virtualization/containerization/back-end use cases.
    This does not limit/reduce anything.
  • Does this PR introduce a breaking change, and is
    titled accordingly?

    No changes required. The arduino daemon output will be a bit different: "Listening ... 0.0.0.0" instead of ".. 127.0.0.1" if anyone relies on this.
  • Other information:

See how to contribute

@per1234 per1234 added topic: code Related to content of the project itself topic: gRPC Related to the gRPC interface type: enhancement Proposed improvement labels Jan 8, 2022
@silvanocerza
Copy link
Contributor

Hey @4ntoine, I'm in the process of refactoring how settings are handled by the Arduino CLI both in daemon and command line mode, I suggest you take a look at #1622 since I know you're using the daemon mode a lot.

I also added a way to customize all settings so I think it supersedes this one.

@4ntoine
Copy link
Contributor Author

4ntoine commented Jan 13, 2022

@silvanocerza Hi, Silvano. Thanks for letting me know! I've quickly looked into it and it looks good to me. The only thing i think is important is to have 0.0.0.0 as a default, no matter hardcoded or configurable (preferred) as in the MR you've linked. This does not limit in any way or cause any problems but will work for back-end/containers/vms out-of-box.

The other minor thing i'm thinking of - those settings separation can take some time. If it does not sound realistic to land it (and release) within a month for instance (le'ts say until February), i'd land this trivial MR first and then land those MR just to have it working oob earlier.

@silvanocerza
Copy link
Contributor

@silvanocerza Hi, Silvano. Thanks for letting me know! I've quickly looked into it and it looks good to me. The only thing i think is important is to have 0.0.0.0 as a default, no matter hardcoded or configurable (preferred) as in the MR you've linked. This does not limit in any way or cause any problems but will work for back-end/containers/vms out-of-box.

Won't do.
That would give the possibility to execute arbitrary code on a machine by anyone since we don't have an authentication step.

You're free to use 0.0.0.0 through configuration after #1622 is merged if you want to take the risks that come with it.

The other minor thing i'm thinking of - those settings separation can take some time. If it does not sound realistic to land it (and release) within a month for instance (le'ts say until February), i'd land this trivial MR first and then land those MR just to have it working oob earlier.

Am pretty confident that we'll have the rework ready in less than a month, we're not probably going to release a new major when that's finished.

In any case, am closing this PR because of the security issues.

@silvanocerza silvanocerza added conclusion: declined Will not be worked on topic: security Related to the protection of user data and removed type: enhancement Proposed improvement labels Jan 13, 2022
@4ntoine
Copy link
Contributor Author

4ntoine commented Jan 13, 2022

Frankly speaking i'm not sure i understand what you mean by "arbitrary code" - it can call only what is downloaded by "core install" with package_index.json signature checked. "127.0.0.1" is not much different from "0.0.0.0". in that regard. Let's imagine any local (same host) script/app can connect and do some weird stuff.

Imo the right way would give the arduino-cli daemon only the limited rights not to hurt anything which is out of the scope of this MR.

Having it configurable would also satisfy my request.

@rhpco
Copy link

rhpco commented Jan 13, 2022

Frankly speaking i'm not sure i understand what you mean by "arbitrary code" - it can call only what is downloaded by "core install" with package_index.json signature checked. "127.0.0.1" is not much different from "0.0.0.0". in that regard. Let's imagine any local (same host) script/app can connect and do some weird stuff.

Imo the right way would give the arduino-cli daemon only the limited rights not to hurt anything which is out of the scope of this MR.

Having it configurable would also satisfy my request.

@4ntoine @silvanocerza
In general, binding the daemon to 0.0.0.0, if not necessary, can expose to the application to CWE-1327 threats.
For this reason, in this circumstance, it is important to follow the Security-By-Design and Reduce Attack Surface security principles.
I think that the desired behaviour, alternatively, can also be obtained adopting some container networking rules or using tools like socat.

my two cents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conclusion: declined Will not be worked on topic: code Related to content of the project itself topic: gRPC Related to the gRPC interface topic: security Related to the protection of user data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants