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

Flexible release cookie #809

Merged
merged 7 commits into from
Aug 22, 2020

Conversation

Olivier-Boudeville
Copy link
Contributor

More flexible specification of the cookie of a release, to avoid that rpc operations fail in case of runtime cookie change.

@Olivier-Boudeville
Copy link
Contributor Author

Apparently the release script can already reuse any cookie set in the COOKIE environment variable, so I guess this pull request can be dropped!

…already taken into account. Yet it was shadowed if any -setcookie was set on the command-line, which is not the case anymore.
@Olivier-Boudeville
Copy link
Contributor Author

New proposed fix: using COOKIE (instead of any new ERLX_OVERRIDE_COOKIE) and ensuring that, if defined, it prevails over any other setting (namely any -setcookie on the command-line).

…and synchronicity (is_alive) even in the absence of relx hooks.
@Olivier-Boudeville
Copy link
Contributor Author

Reaaaady to ship then!!!

@Olivier-Boudeville
Copy link
Contributor Author

Ah sorry should have been left open in order to be merged.

Olivier-Boudeville added a commit to Olivier-Boudeville/us-web that referenced this pull request Aug 11, 2020
@ferd ferd requested a review from tsloughter August 12, 2020 14:27
@tsloughter
Copy link
Member

So this is adding COOKIE which if set is used instead of any other cookie value?

There are some whitespace changes that need to be fixed in the PR but besides that I can probably merge this.

Note, not sure if this was part of your thinking with the PR or not but just in case it was, the cookie will still be set in the command and thus visible in ps.

@Olivier-Boudeville
Copy link
Contributor Author

So this is adding COOKIE which if set is used instead of any other cookie value?

Yes, typically there might be in vm.args a -setcookie foobar_initial_cookie that will be seen on the command line yet will not be the runtime one if COOKIE=... is used (because a cookie was specified in the application config file).

Indeed the start/stop scripts (ex: triggered by systemd) would run a command like:
/bin/sudo -u ${foobar_username} COOKIE="some real cookie" ${epmd_opt} ${authbind} --deep ${foobar_exec} daemon

(works great)

Note, not sure if this was part of your thinking with the PR or not but just in case it was, the cookie will still be set in the command and thus visible in ps.

With said sudo/authbind, I could check that the right cookie is used (the application one), and I do not think it can be found with a ps (tried to grep it with a ps -edf with no luck); we can only find:

some-srv 3558692 3558690 0 18:03 pts/17 00:00:01 /opt/foobar-server/foobar-0.0.10/bin/foobar -A 128 -- -root /opt/foobar-server/foobar-0.0.10 -progname opt/foobar-server/foobar-0.0.10/bin/foobar -- -home /home/some-srv -epmd_port 44000 -- -boot /opt/foobar-server/foobar-0.0.10/releases/0.0.10/start -mode embedded -boot_var SYSTEM_LIB_DIR /usr/local/lib/erlang/lib -config /opt/foobar-server/foobar-0.0.10/releases/0.0.10/sys.config -name foobar -setcookie foobar_initial_cookie -- -- console --relx-disable-hooks --

There are some whitespace changes that need to be fixed in the PR but besides that I can probably merge this.

OK, thanks! I thought I had respected the original whitespaces, I must have made a mistake (I rely on the Emacs whitespace.el clean-up a lot); I just looked at the resulting priv/templates/extended_bin yet I cannot see how whitespaces differ?

@tsloughter
Copy link
Member

I don't understand how this works if -setcookie isn't what is used for the cookie.

@Olivier-Boudeville
Copy link
Contributor Author

I don't understand how this works if -setcookie isn't what is used for the cookie.

The release will start with this default cookie (as set in vm.args) then will read its general, single, higher-level configuration file, in which (among many other settings) a cookie might be defined by the user, in which case erlang:set_cookie/2 is used by this application to change dynamically the cookie it relies on. So ps will still show the default cookie, whereas any program to interact with the VM (including the erl_call in the release script) will have to use the newer cookie (most probably by parsing by itself said application configuration file) - hence the need to be able to specify it to erl_call, thanks to the COOKIE environment variable.

An example thereof is https://github.com/Olivier-Boudeville/us-web/blob/65e2eea2fe92c0bd41f59d2fceddacdc505f4136/priv/bin/start-us-web.sh#L98

The interest of this procedure is to reduce the "attack surface" of a server, by ensuring (beyond strict firewalling policies) that it runs as an unprivileged user (despite opening TCP ports 80/443) , on a non-standard EPMD port, and (here) with a cookie that can be better protected in an (access-restricted) configuration file rather than being exposed to any local user, which can determine it (with a mere ps) and basically do everything he wants then with that VM.

@tsloughter
Copy link
Member

I see what I was missing, I thought the run commands like foreground and start hardcoded in -setcookie $COOKIE, but they don't, only the commands like rpc and remote console do that.

Ok, I think this makes sense then.

@tsloughter
Copy link
Member

@Olivier-Boudeville there are still white space changes that need to be cleaned up.

@tsloughter
Copy link
Member

Also, I'm hoping the VM will support an environment variable for the cookie in the next major release. Not knowing what that variable will be named I think it best to "namespace" this variable by naming it RELX_COOKIE.

@Olivier-Boudeville
Copy link
Contributor Author

@tsloughter Hello Tristan, sorry for the differences that remained, I had missed them in meld's window. Must be fixed now. In my setting I auto-patch my release script with this change and 'daemon' works like a charm. Thanks!

@tsloughter
Copy link
Member

@Olivier-Boudeville looks good now just need the change to using RELX_COOKIE so we don't conlfict with anything else at some point.

@Olivier-Boudeville
Copy link
Contributor Author

@tsloughter Hello Tristan, I applied this change (one should note thus that any COOKIE environment variable will be thus ignored), please tell me if it is OK for integration.

Olivier-Boudeville added a commit to Olivier-Boudeville/us-web that referenced this pull request Aug 21, 2020
@tsloughter
Copy link
Member

Thanks. Merging now. I created #814 to track that we need a test. I didn't want to block on it since it may not be so simple to run them for you, if it was just a test that runs with rebar3 ct I'd have wanted you to add it with this PR. I can add the test at some point if running the shelltests doesn't work for you.

@tsloughter tsloughter merged commit 2033aaa into erlware:master Aug 22, 2020
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.

2 participants