-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
plausible, nixos/plausible: Default to listen on localhost. #130297
base: master
Are you sure you want to change the base?
Conversation
text = '' | ||
#! ${pkgs.elixir}/bin/elixir | ||
{:ok, addr} = :inet.parse_address('${ip}') | ||
IO.puts(inspect(addr)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remaining debug statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's the thing that prints the Erlang-formatted IP address to stdout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made that clearer with a comment.
name = "ip-to-erlang-address-tuple.exs"; | ||
text = '' | ||
#! ${pkgs.elixir}/bin/elixir | ||
{:ok, addr} = :inet.parse_address('${ip}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a case statement here? Just in case the parse ip fails.
probably something like
case :inet.parse_address('${ip}') do
{:ok, addr} -> addr
{:error, message} -> raise message
end
I am not sure the effect of the raise in elixir in a file, does it really return exit code 1? (just something that has to be tested)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a case statement here?
Great point. I've force-pushed to print a better error message in that case.
I am not sure the effect of the raise in elixir in a file, does it really return exit code 1?
raise
does exit with code 1, but it also prints a stack trace. I've implemented an explicit stderr print now to print exactly what the user should see:
Invalid IP address 'garbage'; error: :einval
builder for '/nix/store/05317l8jk4iv7z0zxnvbsv1dgqk6hh6a-plausible-vm-args.drv' failed with exit code 1
I have to say I'm a little bit afraid that after you get the values for where to listen, you then have to put them in a vm.args.eex (I don't remember if that is the exact name of the file). Once you have the file, you either have to override upstream, or merge with upstream. Upstream doesn't seem to use that file at the moment, but they could in the future, so overriding seems a little dangerous to me. Merging is also fine at the moment, but could get complicated if upstream decides to use the file. How about we stop removing the RELEASE_COOKIE in nixpkgs? I am happy to go with your current approach, just outlining a possible alternative. |
@happysalada That will be futile: Generating a password into a publicly available package makes it not-a-secret. Anybody can just read and use it. It's not just a value that's a password in your nix store, it's a password that's on cache.nixos.org for everybody to see. The only reasonable use for password-protected Erlang is when the user entirely generates the cookies for their cluster themselves, known to nobody else. Thus, NixOS cannot do it for them.
I don't think a warning would be good enough (as seen in the other PR, we didn't even catch a hard runtime "no such function" error there). If we used the COOKIE stuff for default security, it should be an evaluation error. But most people will just want to use Plausible single-machine, instead of on a multi-node Erlang cluster, so I think making it listen to localhost is likely the better default in any case.
I don't really understand what you're referring to. Are you referring to something that I'm doing here regarding " |
a348267
to
8eb78f3
Compare
I even made a mistake with shell scripting here: set -eu -o pipefail
echo "-kernel inet_dist_use_interface \"$(${erlangAddressTupleCommand})\"" > "$out" Does not work, swallows the exit code of the Elixir script. This works: set -eu -o pipefail
IP=$(${erlangAddressTupleCommand})
echo "-kernel inet_dist_use_interface \"$IP\"" > "$out" bash/shell is just a lost cause. Even the most trivial two-liners are wrong, and nobody spots it, after we've been debugging and reviewing exactly that type of issue for days. |
Example to show how NixOS#130297 (comment) could be addressed.
8eb78f3
to
dac1687
Compare
Tangential:
This is what it would like if we had a 30e746b#diff-77a3dd66631fe818ad92b50d7abcaa3812f98c6f0b738cdcd64b1c3520b32becR296 I personally would like to have something like that in the future. |
On the shell tangeant. Let me try to clarify what I meant around the vm.args.eex. |
I've looked at various shell replacements in the past, and I like some of them, but I think that for general scripting in nixpkgs, Python would be a better fit over those, because there are probaly 100x-1000x more users that already understand Python well vs. these new shell language variants, it is at least as safe, and it's the same language used as in NixOS VM tests (so contributors have to learn less languages).
@happysalada Ah, I see. For reference, that's this line: Yes, Elixir doesn't seem to provide an easy way to merge the package's I've a pushed a commit that adds and assertion that will fail if they start putting something into |
# grep to print all lines that aren't comments, empty, or whitespace. | ||
FOUND_VM_ARG=0 | ||
grep -v -E '^(#|$|[[:space:]]+)' "${pkgs.plausible}"/releases/*/vm.args || FOUND_VM_ARG=1 | ||
if [ "$FOUND_VM_ARG" -eq 0 ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
(( FOUND_VM_ARG = 0 ))
Which to my knowledge is the recommended way to do arithmetic.
I don't have any strong feelings on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right, however I am not doing arithmetic. Just normal exit code handling, for which it is common to use strings. For example $?
also works that way.
I just have one last comment before we merge. |
In general for most packages, I think that is a good model, as releases are often better tested and run by more people (e.g. because they are present in multiple Linux distributions). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to merge.
ac4f121
to
2e89c54
Compare
I'm also planning to update the NixOS test to check that no non-localhost ports are open with defualt settings. |
The IP address on which the server is listening. | ||
|
||
When changing listen IPs, also consider | ||
<option>services.plausible.erlang.vmListenAddress</option> and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd strongly prefer to use <xlink linkend="opt-services.plausible.etc"/>
here as this makes links clickable :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd strongly prefer to use
<xlink linkend="opt-services.plausible.etc"/>
here as this makes links clickable :)
You mean <xref
I think. Good idea, I forgot about it. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we want to factor this out to have it reusable for other erlang applications.
"cachex": {:hex, :cachex, "3.3.0", "6f2ebb8f27491fe39121bd207c78badc499214d76c695658b19d6079beeca5c2", [:mix], [{:eternal, "~> 1.2", [hex: :eternal, repo: "hexpm", optional: false]}, {:jumper, "~> 1.0", [hex: :jumper, repo: "hexpm", optional: false]}, {:sleeplocks, "~> 1.1", [hex: :sleeplocks, repo: "hexpm", optional: false]}, {:unsafe, "~> 1.0", [hex: :unsafe, repo: "hexpm", optional: false]}], "hexpm", "d90e5ee1dde14cef33f6b187af4335b88748b72b30c038969176cd4e6ccc31a1"}, | ||
"certifi": {:hex, :certifi, "2.6.1", "dbab8e5e155a0763eea978c913ca280a6b544bfa115633fa20249c3d396d9493", [:rebar3], [], "hexpm", "524c97b4991b3849dd5c17a631223896272c6b0af446778ba4675a1dff53bb7e"}, | ||
- "clickhouse_ecto": {:git, "https://github.com/plausible/clickhouse_ecto.git", "b30ccc93a4101a25ff0bba92113e18d8a9a8b28e", []}, | ||
+ "clickhouse_ecto": {:git, "https://github.com/plausible/clickhouse_ecto.git", "1969f14ecef7c357b2bd8bdc3e566234269de58c", []}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just use substituteInPlace for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like it to fail loudly when the upstream commit changes though.
This is a safer default configuration, changing: * the plausible HTTP web server * the Erlang Beam VM inter-node RPC port * the Erlang EPMD port to be listening on localhost only. This makes Plausible have a safe default configuration, like all other networked services in NixOS For background discussion, see: NixOS#130244
This is a safer default configuration, changing * the Erlang Beam VM inter-node RPC port * the Erlang EPMD port to not listen to any sockets at all.
2e89c54
to
1f99f69
Compare
@Ma27 Yes. But that's better tracked by issue #130244 with its task:
and it would make sense for somebody who understands one of the other Elixir services to do it (probably not me for the time being, as Plausible is the first Elixir / Erlang service I use on NixOS). |
in terms of elixir services on nixpkgs, there is basically this and pleroma. |
Thinking about it some more, I don't think each application should start and set it's epmd port. The epmd dependency is hidden and there is no way to control it. |
@happysalada Regarding In 9c71958#diff-77a3dd66631fe818ad92b50d7abcaa3812f98c6f0b738cdcd64b1c3520b32becR174 and #143345 (comment) you enabled services.epmd.enable = true; for Plausible. But don't you still have to do
so that there aren't 2 |
I forgot to update this thread when I tested.
let me know if that doesn't answer your comment. |
@happysalada The question is: You switched to Or is there some automatic detection in Plausible that checks whether an epmd is already running on the machine? |
The mechanism is inside the beam. When you run a release, it first checks for an already running epmd and if it exists, connects to that. If none can be found, then it will start its own. |
This is the default mode for how releases are run. There is another mode that doesn't require epmd, but it's not ad simple as switching the flag. The setup was a little more complex than that, that's why in the end, I didn't choose to change from the default running mode of using epmd (one reason is because upstream is running this way, but another one is that it would require testing to verify that the new mode doesn't break anything). |
@happysalada Ah, I see. I think that means that there's still the race condition that the NixOS epmd is off, crashed, or being restarted, or was restarted by a human, at the time that the Plausible one gets started, so I think for production use cases it would be nice to ensure that it won't fall back to starting its own. Upstream Plausible just merged my patch to be able to specify This means that with the next release, we can remove my patch from this PR. |
Agreed on checking for the race condition. During the time I was running elixir in production, I've never seen this, but it could happen. One way to add the check would be for the plausible service "require"ing the epmd service to start. I saw that plausible merged your fix. I thought it was lost in the limbs, I'm happy it finally got merged! |
Even a systemd dependency would still be racy, since the system epmd could be temporarily down, e.g. currently being restarted, or not bound to the socket yet, and so on. The only safe way is configuration: To tell Plausible that it should never try to start its own epmd. |
Updated PR description:
|
What is the status of this PR, @nh2 — I think it would be an amazing addition, can I help? :) |
@nh2 Are you still actively working on this PR? I wonder if the scope of this PR will change if we upgrade plausable to a new version? |
@RaitoBezarius @softinio @happysalada I'm now revisiting this PR. I'll rebase it to NixOS 23.05, if I can within the next couple hours. Let me know if you discovered any new info on this topic. My plan is this:
|
@azahi @Tom-Hubrecht tagging more people who have expressed interest in plausible in the past. Personally the approach looks good to me, its just a matter of getting enough people to test. |
Closing in favour of simplified PR #266702. I also add a NixOS test line there that checks that it's listening only on localhost. |
Motivation for this change
This is a safer default configuration, changing:
to be listening on localhost only.
This makes Plausible have a safe default configuration, like all other
networked services in NixOS
For background discussion, see: #130244
TODO
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)