Rewrite coreos-installer in Rust #65

bgilbert · 2019-10-25T10:37:39Z

Features:

Checks that target disk is not in use
Obtains image from local file, remote URL, or stream metadata
Streaming fetch and decompression
Compression format sniffing
Progress reporting
GPG signature verification, mandatory unless --insecure specified
Does not write partition table until signature is verified
Can copy Ignition config to disk
Can override Ignition platform ID
Can write first-boot kernel arguments
Doesn't depend on uniqueness of /boot UUID
Wipes destination partition table on error
Only executable dependencies: gpg, lsblk, udevadm

Command-line parameters are not compatible with the original, but the intent is that the coreos.inst.* kargs, to be implemented in a separate wrapper script, will maintain compatibility.

lucab · 2019-10-25T10:57:30Z

It would be useful to turn on travis on this repo, before merging this PR. I don't seem to have the powers to do that myself though.

src/blockdev.rs

src/source.rs

src/blockdev.rs

src/main.rs

bgilbert · 2019-10-26T01:50:03Z

Thanks for the review! Updated, and Travis is enabled. Also, as we discussed OOB, I've moved the functionality to an install subcommand for future-proofing.

lucab · 2019-10-28T13:42:05Z

An additional wishlist item that came to my mind: it would be great if we could run the installer from a container, just with the bindmounts for the target (rationale: that would allow us to use any linux-host as a provisioner).
Did you maybe try that, or see any clear blocker? Does it sound useful to you (perhaps coupled with a quay autobuilder task) or am I getting off track?

jlebon

Very nice overall! I like the 1M delayed write trick.

jlebon · 2019-10-28T14:09:50Z

src/signing-keys.asc

@@ -0,0 +1,91 @@
+-----BEGIN PGP PUBLIC KEY BLOCK-----


Hmm, why not just pick up the keys from fedora-gpg-keys/redhat-release-coreos (i.e. /etc/pki/rpm-gpg) ?

coreos-installer should run on arbitrary distros, which may not have those files.

OK so the idea here is that we'll publish binaries instead of/in addition to containers? What I'm trying to avoid is adding yet another spot that needs updating during each major release rollover. Can we pull it in as part of the build process at least?

It may make sense to overlay-merge (by filename?) two sets of keys:

these hardcoded ones (for fallback)

any on-disk ones (possibly fresher)

OK so the idea here is that we'll publish binaries instead of/in addition to containers?

It was, but if the container works and the UX for running it isn't too bad, I'd favor shipping that instead of a binary.

any on-disk ones (possibly fresher)

Where would we get these? If from /etc/pki/rpm-gpg, there are no symbolic references for the current key, so we'd have to either import everything we find there (boo) or have a heuristic for importing keys (boo).

If we do ship a bare binary, this would also mean that behavior would differ depending on the host OS, which doesn't seem great.

What I'm trying to avoid is adding yet another spot that needs updating during each major release rollover. Can we pull it in as part of the build process at least?

These are the roots of trust for install images, and IMO it's safer to manage them manually. We can always start that way and add dynamic roots or more automation later on.

In comparison, CL coreos-install intentionally hardcodes the GPG key and does not provide a command-line mechanism for overriding it.

So as an example right now we don't have any CI covering the coreos-installer path. So bumping from f30 to f31 without bumping the keys here would've possibly gone completely unnoticed until it shipped to users. (A gap we should fix of course!)

Where would we get these? If from /etc/pki/rpm-gpg, there are no symbolic references for the current key, so we'd have to either import everything we find there (boo) or have a heuristic for importing keys (boo).

These are the roots of trust for install images, and IMO it's safer to manage them manually. We can always start that way and add dynamic roots or more automation later on.

A big part of this discussion is essentially a direct consequence of coreos/fedora-coreos-tracker#187 (comment). WDYT about:

ship in a container with fedora-gpg-keys

parse the FCOS version to extract the major release, similar to what is proposed for RoboSignatory itself in that ticket

verify download using only that release's specific key from /etc/pki/rpm-gpg

?

This is similar to how Fedora verifies RPMs: https://src.fedoraproject.org/rpms/fedora-repos/blob/a12c809ab2dcb730cbeb4a03c50d20e3d25cc048/f/fedora.repo#_10.

For RHEL, the keys don't rotate nearly as much, so we can just bake it in.

Anyway, I'm cool with how things set up for now, and discussing this stuff in more details later on!

So as an example right now we don't have any CI covering the coreos-installer path. So bumping from f30 to f31 without bumping the keys here would've possibly gone completely unnoticed until it shipped to users. (A gap we should fix of course!)

If nothing else, that'll be addressed when we start running kola on Packet, where every machine will bootstrap with coreos-installer.

ship in a container with fedora-gpg-keys

I'd still prefer not to rely on external keys. It'd be great if cargo install coreos-installer worked properly on e.g. Debian.

parse the FCOS version to extract the major release, similar to what is proposed for RoboSignatory itself in that ticket
verify download using only that release's specific key from /etc/pki/rpm-gpg

I like the idea of binding to a specific key. Will file an issue.

Hmm, maybe stream metadata should list a signing key ID for each artifact?

Actually, I guess the key ID is already embedded in the signature, so listing it separately in metadata is probably redundant.

jlebon · 2019-10-28T14:22:59Z

src/main.rs

+extern crate progress_streams;
+extern crate reqwest;
+extern crate tempdir;
+extern crate xz2;


Shouldn't need these with Rust 2018.

jlebon · 2019-10-28T14:37:00Z

src/main.rs

+
+    // do the copy
+    config_dest.push("config.ign");
+    let mut config_in = OpenOptions::new()


Why not just std::fs::copy here?

We get a bit better control this way:

config_out.create_new(true) ensures that we'll fail if the file already exists for some reason. (Which it shouldn't, of course.)

std::fs::copy also copies permissions, which may not be desired.

jlebon · 2019-10-28T14:39:23Z

src/main.rs

+            // Rewrite the config.  Assume that we will only install
+            // from metal images and that their bootloader configs will
+            // always set ignition.platform.id.  Fail if those
+            // assumptions change.  This is deliberately simplistic.


This is fine, though should probably no-op return early on if platform is metal.

bgilbert · 2019-10-28T20:15:25Z

it would be great if we could run the installer from a container, just with the bindmounts for the target (rationale: that would allow us to use any linux-host as a provisioner).
Did you maybe try that, or see any clear blocker? Does it sound useful to you (perhaps coupled with a quay autobuilder task) or am I getting off track?

I haven't tried it, but I like the idea. I was originally thinking we'd offer a binary for download, but it does have dynamic library dependencies so that may not work well on other distros:

	liblzma.so.5 => /lib64/liblzma.so.5 (0x00007fb47a786000)
	libssl.so.1.1 => /lib64/libssl.so.1.1 (0x00007fb47a6f0000)
	libcrypto.so.1.1 => /lib64/libcrypto.so.1.1 (0x00007fb47a414000)
	libdl.so.2 => /lib64/libdl.so.2 (0x00007fb47a40e000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007fb47a3ed000)
	libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007fb47a3d3000)
	libc.so.6 => /lib64/libc.so.6 (0x00007fb47a20b000)
	libz.so.1 => /lib64/libz.so.1 (0x00007fb47a1f1000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fb47ac29000)

A container image would sidestep the whole problem. I probably won't pursue it immediately, but filed issue #67.

bgilbert · 2019-10-29T05:39:26Z

Updated!

lucab · 2019-10-29T12:38:17Z

We could (in theory) cross-compile to a static binary with musl, but in practice if it works I'd prefer pursuing the container route.

src/main.rs

crawford · 2019-10-25T11:59:38Z

src/main.rs

+    insecure: bool,
+}
+
+quick_main!(run);


Is this still needed? main can return a result now.

quick_main still does better pretty-printing than the default main.

crawford · 2019-10-29T19:06:02Z

src/verify.rs

+
+use error_chain::bail;
+use std::fs::{metadata, set_permissions, OpenOptions};
+use std::io;


This can be combined below:

use std::io{self, Read, Write};

crawford · 2019-10-29T19:08:22Z

src/main.rs

+    let uname = nix::sys::utsname::uname();
+    // Args are listed in --help in the order declared here.  Please keep
+    // the entire help text to 80 columns.
+    let app_matches = App::new("coreos-installer")


As an alternative to using Clap directly, StructOpt is pretty slick.

Nice! I played with it a bit and don't think it's worth a refactor right now, but I'll keep it in mind for the future.

Features: - Checks that target disk is not in use - Obtains image from local file, remote URL, or stream metadata - Streaming fetch and decompression - Compression format sniffing - Progress reporting - GPG signature verification, mandatory unless --insecure specified - Does not write partition table until signature is verified - Can copy Ignition config to disk - Can override Ignition platform ID - Can write first-boot kernel arguments - Doesn't depend on uniqueness of /boot UUID - Wipes destination partition table on error - Only executable dependencies: gpg, lsblk, udevadm Command-line parameters are not compatible with the original, but the intent is that the coreos.inst.* kargs, to be implemented in a separate wrapper script, will maintain compatibility.

Also relicense coreos-iso-embed-ignition.

lucab · 2019-10-30T08:24:01Z

I don't have any further high-level concerns, and the code itself looks good. I could just stamp the PR but I don't know what are the overarching integration plans wrt. the old one.

@bgilbert @jlebon @arithx how do you want to proceed here?

jlebon · 2019-10-30T14:59:36Z

Would be pretty cool to have the CI here actually do an install of FCOS. I think Travis CI can give you Ubuntu VMs, though we can also hook it up to CoreOS CI!

@bgilbert @jlebon @arithx how do you want to proceed here?

Now that we've branched, I'm thinking let's just merge this, cut a 0.0.1 release, and iterate on top as we uncover issues?

bgilbert · 2019-10-30T15:03:34Z

Now that we've branched, I'm thinking let's just merge this, cut a 0.0.1 release, and iterate on top as we uncover issues?

Yup, we have a legacy branch for existing users. I have a couple followup PRs before releasing, but 👍 to that plan generally.

lucab

LGTM from my side

lucab reviewed Oct 25, 2019

View reviewed changes

jlebon reviewed Oct 28, 2019

View reviewed changes

crawford reviewed Oct 29, 2019

View reviewed changes

bgilbert mentioned this pull request Oct 29, 2019

Revert "Use dd conv=sparse" #70

Merged

bgilbert added 3 commits October 29, 2019 17:02

Drop old coreos-installer

8c7148d

Switch license file to ASL 2.0

a6b8963

Also relicense coreos-iso-embed-ignition.

bgilbert mentioned this pull request Oct 30, 2019

Hardcoded list of signing keys is awkward #71

Open

lucab approved these changes Oct 30, 2019

View reviewed changes

bgilbert merged commit 6728bfa into coreos:master Oct 30, 2019

bgilbert deleted the rust branch October 30, 2019 15:35

This was referenced Oct 30, 2019

Allow e.g. -d /dev/vda #47

Closed

Don't require specifying image URL #53

Closed

Use bash strict mode #42

Closed

Add integrity checking for the downloaded image #30

Closed

Check for dependencies before installer execution #59

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite coreos-installer in Rust #65

Rewrite coreos-installer in Rust #65

bgilbert commented Oct 25, 2019 •

edited

Loading

lucab commented Oct 25, 2019

bgilbert commented Oct 26, 2019

lucab commented Oct 28, 2019

jlebon left a comment

jlebon Oct 28, 2019

bgilbert Oct 28, 2019

jlebon Oct 28, 2019

lucab Oct 28, 2019

bgilbert Oct 29, 2019

jlebon Oct 30, 2019

bgilbert Oct 30, 2019

bgilbert Oct 30, 2019

bgilbert Oct 30, 2019

jlebon Oct 28, 2019

jlebon Oct 28, 2019

bgilbert Oct 28, 2019

jlebon Oct 28, 2019

bgilbert commented Oct 28, 2019

bgilbert commented Oct 29, 2019

lucab commented Oct 29, 2019

crawford Oct 25, 2019

bgilbert Oct 29, 2019

crawford Oct 29, 2019

crawford Oct 29, 2019

bgilbert Oct 29, 2019

lucab commented Oct 30, 2019

jlebon commented Oct 30, 2019

bgilbert commented Oct 30, 2019

lucab left a comment

Rewrite coreos-installer in Rust #65

Rewrite coreos-installer in Rust #65

Conversation

bgilbert commented Oct 25, 2019 • edited Loading

lucab commented Oct 25, 2019

bgilbert commented Oct 26, 2019

lucab commented Oct 28, 2019

jlebon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgilbert commented Oct 28, 2019

bgilbert commented Oct 29, 2019

lucab commented Oct 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab commented Oct 30, 2019

jlebon commented Oct 30, 2019

bgilbert commented Oct 30, 2019

lucab left a comment

Choose a reason for hiding this comment

bgilbert commented Oct 25, 2019 •

edited

Loading