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

Restrict interface bindings and test connections #16292

Merged
merged 1 commit into from
May 12, 2016
Merged

Conversation

amitmurthy
Copy link
Contributor

@amitmurthy amitmurthy commented May 10, 2016

This PR does the following:

  • addprocs restricts socket bindings to 127.0.0.1, the default ip or a specified bind-to address. Currently we listen on all interfaces.
  • addprocs(N) by default binds only to 127.0.0.1.
  • addprocs(N); addprocs(["some_host"]) will no longer work.
  • addprocs(N; restrict=false); addprocs(["some_host"]) is the alternative for adding local workers and then adding workers from a remote host.
  • SSHManager now binds only to the specified ip or to the first interface ip returned by getipaddr.
  • A cluster now has a "cookie" which is shared by all processes. The master generates a random string, which is then passed to the workers via startup parameter --worker <cookie>.
  • All incoming socket connections are validated by ensuring that the first message received has the cluster cookie.

This PR addresses basic hygiene by avoiding unnecessary socket bindings and testing for connections. Full fledged security requirements are best addressed through a custom ClusterManager.

Doc updates are pending.

@ViralBShah ViralBShah added backport pending 0.4 parallelism Parallel or distributed computation labels May 10, 2016
@ViralBShah ViralBShah added this to the 0.5.0 milestone May 10, 2016
@amitmurthy amitmurthy changed the title Restrict interface bindings and test connections RFC/WIP Restrict interface bindings and test connections May 10, 2016
@tkelman
Copy link
Contributor

tkelman commented May 10, 2016

If this gets backported, does this mean every worker will have to be using the very latest version of julia to be able to communicate with the main node? What about workers that get updated but node 1 still uses an older point version of julia? Would both of those situations break?

@amitmurthy
Copy link
Contributor Author

Yes.

ClusterManagers.jl will also have to be updated to support this. Any other custom ClusterManagers would break too.

We could port only the first commit, i.e., binding only to one interface to 0.4. That would only break the specific use case of a local addprocs followed by a remote one. Which can be fixed by specifying restrict=false.

@amitmurthy amitmurthy force-pushed the amitm/secure branch 2 times, most recently from e925935 to 46f8cad Compare May 11, 2016 06:28
@amitmurthy amitmurthy changed the title RFC/WIP Restrict interface bindings and test connections Restrict interface bindings and test connections May 11, 2016
@amitmurthy
Copy link
Contributor Author

Ready for review.

@@ -1204,6 +1204,7 @@ export
# multiprocessing
addprocs,
asyncmap,
cluster_cookie,
Copy link
Contributor

Choose a reason for hiding this comment

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

should this really be exported? it's kind of an internal detail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only required by folks writing their own ClusterManagers. But I guess we could document its use as Base. cluster_cookie()


Returns the cluster cookie. If a cookie is passed, also sets it as the cluster cookie.
"""
Base.cluster_cookie
Copy link
Contributor

Choose a reason for hiding this comment

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

better to do this inline rather than adding to this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had it inline but moved it after removing the export since we cannot have the function definition as Base.cluster_cookie.

@amitmurthy
Copy link
Contributor Author

Merging this in a short while if there are no major concerns.

@amitmurthy amitmurthy merged commit 8f4e33f into master May 12, 2016
@amitmurthy amitmurthy deleted the amitm/secure branch May 12, 2016 09:09
@tkelman
Copy link
Contributor

tkelman commented May 13, 2016

I wish you hadn't squashed this since the cookie part is not backwards compatible. Please open a separate PR against release-0.4 with the less breaking portion.

@amitmurthy
Copy link
Contributor Author

Will keep this in mind in the future. For this patch though the first commit wouldn't have merged cleanly and we also need to update the manual.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parallelism Parallel or distributed computation security System security concerns and vulnerabilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants