Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add deb and RPM repository config and documentation #1676

Merged
merged 14 commits into from
Sep 10, 2020
Merged

Conversation

s3krit
Copy link
Contributor

@s3krit s3krit commented Sep 2, 2020

This change adds some documentation to the README.md on how to install the latest version of Polkadot using our package repositories, and some additional config to Cargo.toml for building .deb and .rpm packages with cargo-deb and cargo-rpm
Internal documentation on how the repos are maintained can be found here.

@s3krit s3krit added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Sep 2, 2020
@s3krit s3krit requested a review from kirushik September 2, 2020 12:49
@s3krit s3krit self-assigned this Sep 2, 2020
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Sep 2, 2020
Copy link
Contributor

@kirushik kirushik left a comment

Choose a reason for hiding this comment

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

By the way, do we package systemd units for polkadot?
I think we probably should (at least eventually), and bonus points for setting the necessary security mechanisms up: running polkadot as a separate unprivileged user, setting up limited capabilities for the daemon, properly handling logs...

README.md Outdated
# Import the security@parity.io GPG key
curl -fsSL 'https://keys.mailvelope.com/pks/lookup?op=get&search=security%40parity.io&options=mr&exact=on' | apt-key add -
# Add the Parity repository and update the package index
echo 'deb https://releases.parity.io/deb buster main' >> /etc/apt/sources.list.d/parity.list
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please suggest putting the key into /usr/share/keyrings/ and using the deb [signed-by=<path to key>] URL… format instead?
There's no real reason to add a system-level apt key for a non-system repo.
(Warning: the key should be dearmored for this to work)

@s3krit
Copy link
Contributor Author

s3krit commented Sep 3, 2020

By the way, do we package systemd units for polkadot?
I think we probably should (at least eventually), and bonus points for setting the necessary security mechanisms up: running polkadot as a separate unprivileged user, setting up limited capabilities for the daemon, properly handling logs...

We don't yet no, but there's no reason not to. It would be nicer if we could configure the node with a config file rather than having to edit the systemd service file each time. There are two outstanding issues for this in the Substrate repo (below).

paritytech/substrate#6856
paritytech/substrate#5887

@kirushik
Copy link
Contributor

kirushik commented Sep 3, 2020

@s3krit the common way to handle this with systemd is to put your options into a env variable in /etc/default/polkadot, something like this:

POLKADOT_CLI_ARGS="--database paritydb --wasm-execution Compiled"

Then load those ENV vars into your Systemd unit with

[Service]
EnvironmentFile=/etc/default/polkadot

and then maybe using variable expansion when composing a string for the Exec= entry in the systemd unit.

I've seen quite a lot of daemons doing so, including docker for example.

@s3krit s3krit requested a review from kirushik September 4, 2020 18:11
Copy link
Contributor

@kirushik kirushik left a comment

Choose a reason for hiding this comment

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

This is awesome!
Like, I'm really-really happy we're finally doing this.

It's up to you if you're going to fix my two remaining comments, those are really tiny nits.

Side note: maybe let's see if there some additional features of systemd we can use to make this setup more secure by default?
I would suspect @ddorgan would have a lot of first-hand experience in running polkadot services in production, and @DemiMarie to know as much about capability filters and other Linux isolation tools as humanely possible.

README.md Outdated Show resolved Hide resolved
README.md Outdated
gpg --recv-keys --keyserver hkps://keys.mailvelope.com FF0812D491B96798
gpg --export security@parity.io > /usr/share/keyrings/parity.gpg
# Add the Parity repository and update the package index
echo 'deb [signed-by=/usr/share/keyrings/parity.gpg] https://releases.parity.io/deb buster main' > /etc/apt/sources.list.d/parity.list
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a really tiny nit, but since we're using the same repo for both buster and focal, maybe let's use some more generic name here in the path? I can easily imagine myself changing the distro name here, and ending up with an incorrect repository URL.
It's not a big issue (trivial to both check and figure out, and is probably only affects a small fraction of potential users anyway) — but I thought I'd better leave it as a note.

Copy link
Contributor

@DemiMarie DemiMarie left a comment

Choose a reason for hiding this comment

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

The Debian and Fedora package install directions have a security vulnerability, and there are places where additional documentation could be added. The systemd unit files should be locked down more as well; systemd-analyze security will help with that.

README.md Outdated

```
# Import the security@parity.io GPG key
gpg --recv-keys --keyserver hkps://keys.mailvelope.com FF0812D491B96798
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be a full (40 hex character) fingerprint, to prevent preimage attacks. If keys.mailvelope.com is a pool, we should also point to an individual server, as GPG can’t receive keys from a pool if it cannot resolve DNS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

40-hex-character fingerprint = 👍

Could you clarify on your second point? Do you mean specifying the keyserver with a single IP address in case of DNS failure? If so, is that a better solution than not being able to receive keys in the event of DNS issues?

Copy link
Contributor

Choose a reason for hiding this comment

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

GPG does support using an HTTP proxy, but only for standalone keyservers. It cannot receive keys from a pool without DNS. This has caused problems for me in the past when installing software in QubesOS, since QubesOS TemplateVMs can only access the Internet via a proxy and have no ability to resolve DNS.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Comment on lines +14 to +16
if [ ! -e "$config_file" ]; then
echo 'POLKADOT_CLI_ARGS=""' > /etc/default/polkadot
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if [ ! -e "$config_file" ]; then
echo 'POLKADOT_CLI_ARGS=""' > /etc/default/polkadot
fi

This isn’t actually needed, as systemd will treat an unset environment variable as an empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intent was to populate the blank config file so that users can edit it as necessary without having to consult the README again to see what the name of the CLI arg environment variable was named.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. That said, /etc/default/polkadot should be included in the package as a configuration file.

README.md Outdated Show resolved Hide resolved
Comment on lines 1 to 2
%define __spec_install_post %{nil}
%define __os_install_post %{_dbpath}/brp-compress
Copy link
Contributor

Choose a reason for hiding this comment

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

I have written my own RPM spec files and have never seen either of these lines, so I recommend adding a comment explaining what they do.

.gitignore Show resolved Hide resolved
scripts/packaging/polkadot.service Show resolved Hide resolved
Copy link
Contributor

@DemiMarie DemiMarie left a comment

Choose a reason for hiding this comment

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

Looks good!

ProtectKernelTunables=true
ProtectSystem=strict
RemoveIPC=true
RestrictAddressFamilies=AF_INET AF_INET6 AF_NETLINK AF_PACKET
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not need AF_PACKET. In this case, it is harmless, as it requires CAP_NET_RAW to use, but it is still good practice to avoid it.

README.md Outdated Show resolved Hide resolved
@s3krit
Copy link
Contributor Author

s3krit commented Sep 10, 2020

Final nags fixed :) Should be good to merge once green. I'll also manually add some info to the next release notes regarding our shiny new package repos

Copy link
Contributor

@DemiMarie DemiMarie left a comment

Choose a reason for hiding this comment

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

LGTM!

@s3krit s3krit merged commit d6ce4c0 into master Sep 10, 2020
@s3krit s3krit deleted the mp-package-config branch September 10, 2020 15:22
ordian added a commit that referenced this pull request Sep 10, 2020
* master:
  Allow the watermark to always land on the relay parent (#1689)
  Limit the maximum size of a downward message (#1690)
  Add deb and RPM repository config and documentation (#1676)
  Collator protocol subsystem (#1659)
s3krit added a commit to s3krit/polkadot-secure-validator that referenced this pull request Sep 16, 2020
This additional config is based on the output of `systemd-analyze security polkadot.service`. We recently added [our own](https://github.com/paritytech/polkadot/blob/master/scripts/packaging/polkadot.service) polkadot.service file in the polkadot repo and spent a little time researching which sandboxing and namespacing options we could take advantage of. Discussion for that PR is [here](paritytech/polkadot#1676 (comment)).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants