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

Validate signatures of builds #241

Closed
brson opened this issue Apr 1, 2016 · 56 comments
Closed

Validate signatures of builds #241

brson opened this issue Apr 1, 2016 · 56 comments

Comments

@brson
Copy link
Contributor

brson commented Apr 1, 2016

Right now the rust binaries are accompanies by gpg signatures, but rustup does not check them.

I don't want to bring in native dependencies unless they are dead simple to build. We should probably try the gpgme bindings first, since those would be compatible with our existing gpg keys. If that doesn't work out then we can explore more exotic options.

Previous thread.

@DemiMarie
Copy link

What about calling out to an external GPG process, like rustup.sh did?

@vks
Copy link

vks commented Jul 26, 2016

What about calling out to an external GPG process, like rustup.sh did?

This is what the gpgme bindings do effectively.

@brson
Copy link
Contributor Author

brson commented Jul 27, 2016

cc #242

@brson
Copy link
Contributor Author

brson commented Jul 27, 2016

@DemiMarie I'd rather not do that because we can't guarantee the presence of GPG, and I want everybody to get the advantage of signature validation.

@vks
Copy link

vks commented Jul 27, 2016

I want everybody to get the advantage of signature validation.

👍 This should not be optional, preventing downgrade attacks.

@brson
Copy link
Contributor Author

brson commented Jul 27, 2016

https://pijul.org/openpgp/ may be part of the solution.

@Diggsey
Copy link
Contributor

Diggsey commented Jul 27, 2016

Anything involving GPG will have to be optional.

@ghost
Copy link

ghost commented Jul 28, 2016

Ok, after writing both Thrussh and the OpenPGP crates, here are some thoughts:

Anything involving GPG will have to be optional.

This is a GnuPG issue. The binary called GPG changes name with different versions (which leads to really unstable situations in systems like NixOS, where it's normally called gpg2, but also sometimes gpg inside a nix-shell, depending on the channel).

First of all, I am really grateful to the the authors of OpenPGP and GnuPG for getting the world to use encryption and signatures in an almost easy way.

That said, I also realize that RFC 4880 is really unclear and ambiguous, even on really important things. GnuPG has the merit of being the main available implementation. It's also well tested and robust (despite being 200000 lines of C, not counting crypto).

However, according to my tests, GnuPG does not always include cryptographically important parts of OpenPGP, such as "modification codes", an OpenPGP feature described in the RFC, which are more commonly called MAC in other cryptography libraries. This might allow an attacker to modify an encrypted message without decrypting it.

If we try to implement OpenPGP in Rust, there are really big issues with the format, which IMHO do not make it suitable to use in applications such as rustup:

  • The scope of a signature is never clearly specified. In RFC4880, hashes of "data" are signed, but "data" is never clearly defined. GnuPG apparently chose to sign the hash of the contents of files, opened in binary mode (i.e. without converting \n to CRLF). This lack of clarity in the RFC is really problematic, as it might allow an attacker to change file names in the signature of multiple files. To achieve compatibility with GnuPG, Rustup packagers would have to make sure only tar archives get signed, and would also have to describe this very clearly to downstream packagers.
  • The RFC is very complicated for simple things, such as length encoding. For instance, the length of a 512 bytes RSA key can be encoded in four different format, which might allow implementations to save up to 3 bytes, compared to a simple big endian u32. And as Rust supporters probably know, it's quite easy to get array lengths wrong. If you want to achieve interoperability with the world outside Rust, you'll have to rely on C code decoding array lengths from a tricky format.

Btw, I realize there are lots of awesome C programmers out there, and the authors of the above mentioned software are clearly among them.

@ghost
Copy link

ghost commented Jul 28, 2016

Btw, I'm currently thinking of a format with the same features as PGP (a web of trust using the existing infrastructure, encryption and signatures).
I'm seriously thinking of reusing the SSH RFCs, because they are really well defined, precise, well tested, and able to encrypt/sign large amount of data efficiently.

@vks
Copy link

vks commented Jul 28, 2016

I don't think a web of trust makes sense for rustup. You have to trust on first use anyway, so it seems much simpler and more straight forward to just use TOFU.

@ghost
Copy link

ghost commented Jul 28, 2016

@vks: Before writing the OpenPGP crate, I used to believe this wasn't the case (maybe I've attended a Rust conference and signed the rustup key), and used to think that the web of trust was really cool. I'm not so sure anymore. Here is some arguments:

@Diggsey
Copy link
Contributor

Diggsey commented Jul 28, 2016

This is a GnuPG issue.

It's an issue for any method which attempts to use a user's existing web of trust to validate downloads: it's unrealistic to have that as a prerequisite to install toolchains via rustup.

As far as I'm concerned, the plan was always to just encode the public key inside the rustup binary, and have a pure-rust solution to verify the signatures of downloads against that public key. The "web of trust" only comes into play for someone installing rustup for the first time, and if they're doing that via their package manager, then even that's unnecessary.

@ghost
Copy link

ghost commented Jul 28, 2016

I see. Then the best method seems to be:

  1. Don't use PGP.
  2. Sign with Ed25519.
  3. Make it so that people organizing Rust events have a copy of the key (Ed25519 keys are really short), and show it on the first slide of the event. Key hashes are not sufficient, as demonstrated by GnuPG.

This doesn't solve everything, though: when the key or the cryptosystem is compromised, you'll have to "revoke" the key.

@vks
Copy link

vks commented Jul 28, 2016

Make it so that people organizing Rust events have a copy of the key

While that certainly does not hurt, I don't think it is necessary. The first time you install rustup, you have to trust the binary anyway (unless we get reproducible builds). So you have to trust that the rustup server and your https connection to it are not compromised, and that you get the right rustup binary with the correct hardcoded key.

I don't think revocation is a problem with TOFU (see ssh). You would have to just get the new key via https.

@zmanian
Copy link

zmanian commented Jul 29, 2016

I'd advocate for adopting The Update Framework for verifying the security of rust distributions.

There are numerous advantages of the The Update Framework especially semantics for key rotation, the use of only strong cryptography in the spec and json format data files etc. The Update Framework was designed specifically for the relevant use case of securing software distributed through a repository.

https://theupdateframework.github.io/

@steveklabnik
Copy link
Member

@zmanian rust-lang/crates.io#75

@brson
Copy link
Contributor Author

brson commented Jul 29, 2016

As far as I'm concerned, the plan was always to just encode the public key inside the rustup binary, and have a pure-rust solution to verify the signatures of downloads against that public key

Yeah that is my expectation. I don't know any other solution that can work in practice. We can't expect users to do any manual steps beyond clicking something on our website. rustup.sh did this while still using GPG though - it just told it which keys to use.

This doesn't solve everything, though: when the key or the cryptosystem is compromised, you'll have to "revoke" the key.

Yes, thanks for mentioning that. Key revocation is important.

@zmanian Thanks for bringing that to my attention. Looks very promising. We should consider it stongly.

Thanks for all the great feedback, all.

@djc
Copy link
Contributor

djc commented Sep 10, 2016

I've gathered up some code for this in https://github.com/djc/rust-sign/blob/master/src/main.rs; I think this provides pretty much all the pieces required. Integrating this into rustup.rs will require more context/design, I guess?

@vks
Copy link

vks commented Sep 11, 2016

@djc We might want to have something more complex like The Update Framework mentioned above.

@djc
Copy link
Contributor

djc commented Sep 12, 2016

@vks that's not really a helpful response, in that (a) you're suggesting the work I did has little to no value, and (b), you're not providing a clear way forward.

I came here via @brson's someday list thread and implemented the thing he mentioned there (using ring to sign/verify with Ed25519). Clearly, to be useful, this needs to be integrated into rustup.rs. Clearly, that's not what I've done in the rust-sign repo. Still, the code in the rust-sign repo provides most of the crypto-related pieces of Ed25519 signing and verification, so I believe it provides some value. I might be able to contribute more of my expertise and/or time here, but, as I said, I don't think I can do so unless more context/design/direction is provided.

@djc
Copy link
Contributor

djc commented Sep 12, 2016

Oh, and it seems that The Update Framework predates the JOSE (JSON crypto) family of IETF RFC's, so at least some of the ways the spec does crypto feels a bit antiquated. It might be better to adopt the roles and process from TUF without going by the letter of the spec.

@vks
Copy link

vks commented Sep 12, 2016

@djc You were asking for more context/design, and I think TUF provides this. In no way did I intend to suggest your work had little value or to provide a clear way forward.

@brson
Copy link
Contributor Author

brson commented Sep 14, 2016

@djc Thanks for putting that together! This looks like a great basis to work off of. Offhand I don't have a full design to suggest, and I need to read the TUF documentation still, but unless it seems strongly unadvisable I'm inclined to keep a pure-Rust solution, so maybe we can do a design influenced by TUF, even a standalone TUF-like library that all Rust programs can take advantage of.

There are a lot of pieces at play in Rust distribution. This is just a brain-dump of how things work today and the binaries that need to be verified:

  • The rustup binaries themselves are downloaded from addresses like https://static.rust-lang.org/rustup/dist/$triple/rustup-init, along with accompanying .sha256 files. Presumably the binaries need to be verified.
  • The rustup binaries are generated today on travis and appveyor, which is a fairly questionable place do be doing that, but given that that's where they are being built, it would be nice to have seperately-revokable subkeys for these builds.
  • There are several steps to acquiring the rust binaries: first rustup downloads the .toml manifests from addresses like https://static.rust-lang.org/dist/channel-rust-beta.toml. Since these contain URLs that rustup follows I guess they need to be verified.
  • From there rustup downloads several tarballs from the archives in directories like https://static.rust-lang.org/dist/2016-09-12/index.html. Each of these needs to be verified.
  • These binaries are generated by the rust build slaves, and the manifests on the build master. We can probably generate the signatures for all in one batch job on the build master. Again it would be nice to have a seperately-revokable key for this machine.
  • Another thing to take into account is that I plan to fix the notorious checksum failures by adding yet another layer of indirection to both the rustup and rust download process (basically, have one single file that acts as a "symlink" to an archive directory). Since this contains data that rustup will interpret it probably also needs to be verified.

I'm strongly interested in rewriting the entire release build process, but it seems unlikely to happen soon. So lots of complex legacy stuff to accommodate.

To make this more concrete, what I might expect the data used by rustup to end up looking like on static.rust-lang.org, based on how things are organized today, is:

  • static.rust-lang.org/
    • rustup/current.toml - this is the single "symlink" file that indicates: the location in the archives to find the current release; the checksum of the name of that location; the signature of the name of that location. This solves the previously-linked checksum problem by having a single stable URL that rustup looks for (the problem is caused by having two such URLs today that must be in sync, and sometimes aren't). It doesn't exist today.
    • rustup/archive/$version/$target/rustup-init - the rustup binary in the archives. These are uploaded to unique locations for each release and exist today. The 'current.txt' file contains the necessary info to derive this path.
    • rustup/archive/$version/$target/rustup-init.sha256 - the (existing) checksums
    • rustup/archive/$version/$target/rustup-init.sig - the (new) signatures
    • dist/rust-manifest-nightly-current.toml - as before, this is a new file that indicates where in the archives to find the rust-manifest-nightly.toml file (which today is not downloaded from the archives). It contains the metadata, that data's checksum, and its sig all in one file.
    • dist/$date/rust-manifest-nightly.toml - the archived manifest
    • dist/$date/rust-manifest-nightly.toml.sha256 - the (existing) checksums
    • dist/$date/rust-manifest-nightly.toml.sig - the (new) signatures
    • dist/$date/$package.tar.gz - the various tarballs installed by rustup
    • dist/$date/$package.tar.gz.sha256 - the (existing) checksums
    • dist/$date/$package.tar.gz.sig - the (new) signatures

This is all of course open for debate. An interesting thing to note here is that the sigs are stored in two different ways: in the "current.toml" files they are in toml keys in the file and are signatures of other toml keys; in other cases they exist next to the complete files they are signing.

Though I think putting signatures next to the bins is a generally good forward-compatible strategy with any changes to the way we might treat the manifests in the future. We could also consider embedding signatures of the tarballs directly in the manifests, though that doesn't work in a straightforward way for the manifests themselves.

I'm sure there's lots more to consider.

@naturallymitchell
Copy link

How do you guys like rsaltpack? It implements saltpack's encoding of MessagePack format using sodiumoxide.

@zmanian
Copy link

zmanian commented Sep 27, 2016

@brson What do you think are the next atomic steps to moving this forward?

Should we formalize what information the build system should publish?

@djc
Copy link
Contributor

djc commented Sep 28, 2016

I think we should figure out how/how much the update framework applies to the rustup ecosystem, determining what abstract elements we should implement. Then, the next step could be figure out a design for making those abstract elements concrete.

@brson
Copy link
Contributor Author

brson commented Oct 13, 2016

I made an irlo thread describing how I imagine solving several problems in the rustup distribution format: https://internals.rust-lang.org/t/future-updates-to-the-rustup-distribution-format/4196. It includes a scheme for integrating TUF.

@DemiMarie
Copy link

I LOVE this!!!!

The biggest advantage of having a portable TUF implementation with a C API
is that ex. OCaml and Perl (whose package managers are totally insecure)
could use it. I know that the insecurity of OPAM (the OCaml package
manager) has killed off my interest in OCaml development.

I would also like to see TUF applied to Cargo.

On Oct 13, 2016 1:34 PM, "Brian Anderson" [email protected] wrote:

The ideal way to implement TUF might be to create a Rust API, create a C
API on top, create Python bindings on top of that that conform to the
reference implementation, then use the reference implementation's test
suite to test conformance. As a nice side effect we'd then provide the
world a TUF that can be used via C.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#241 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGGWB5cjExDadtVZH-prOFvWQ27ChGy3ks5qzmufgaJpZM4H9Zcu
.

@nbraud
Copy link

nbraud commented Oct 15, 2016

@DemiMarie Unrelated to this specific issue, but for your information migrating OPAM to use TUF is a long-standing goal of the OPAM devs, has had received active development effort and is on the roadmap for v2.0.

@ghost
Copy link

ghost commented Oct 16, 2016

The ideal way to implement TUF might be to create a Rust API, create a C API on top, create Python bindings on top of that that conform to the reference implementation, then use the reference implementation's test suite to test conformance. As a nice side effect we'd then provide the world a TUF that can be used via C.

This sounds great. If there's any tension between a library usable from other languages, and a generic crate easy to use in Rust, I guess I'd vote for the second one.

My specific use case is Pijul: many projects are distributed as git repositories today (like NixOS), and being able to sign these using TUF would be really cool.

@DemiMarie
Copy link

@pijul be sure to sign SHA-2/Blake2b hashes of a Merkle tree, not the SHA-1 hash that Git does on its own (so you will need to do the hashing yourself).

@kamalmarhubi
Copy link
Contributor

The ideal way to implement TUF might be to create a Rust API, create a C API on top [...] As a nice side effect we'd then provide the world a TUF that can be used via C.

Argh a TUF implementation was something I was interested in doing before I ever wrote a line of Rust. Even met with the TUF authors, and this benefit of a Rust implementation was definitely interesting to them. But then I went nowhere with it.

Perhaps it's less intimidating to start with Rust updates as a much narrower use case than Crates.io...

@brson
Copy link
Contributor Author

brson commented Oct 21, 2016

It looks like I can't personally devote any time to implementing TUF in the near future, but I'd encourage anyone else to start on it.

@zmanian
Copy link

zmanian commented Apr 21, 2017

Work on a Rust TUF is going on here.
https://github.com/heartsucker/rust-tuf

@brson
Copy link
Contributor Author

brson commented May 11, 2017

@zmanian oh wow thank you! Excited to see where it goes.

@heartsucker
Copy link

Oh hai @brson. Checking in here to say that I'm almost done with delegations on rust-tufwhich means you could get a large chunk of the signing / verification benefits of TUF right very soon. Some of the other things (slow data, complete disaster recovery) haven't been implemented yet, so it's not TUF Compliant, but that shouldn't stop getting this in. There's some basic docs already, and plugging this in shouldn't (I hope) be too difficult.

Anyway. Is there a roadmap or plan to get this implemented? Because it requires some work on the maintainers' side like setting up keys and whatnot.

@brson
Copy link
Contributor Author

brson commented Jul 18, 2017

@heartsucker, @alexcrichton and I had a chat about tuf integration. Here are the scattered notes for future reference: https://public.etherpad-mozilla.org/p/rustup-tuf

@heartsucker
Copy link

@brson Would it be possible for you to make a milestone / project here for collecting issues related to this? There are some tasks that I might be able to delegate for this. I think one of them that I could offload to someone else would be update to hyper 0.10 since that's the version rust-tuf is built on.

@heartsucker
Copy link

There is additional discussion of the particulars of the TUF implementation on issue #1217.

@benaryorg
Copy link
Contributor

Has anyone yet mentioned or thought of using using signify (originating from OpenBSD IIRC)?
There seems to even be a Rust implementation (docs contain various links).

@DemiMarie
Copy link

DemiMarie commented Feb 23, 2018 via email

@heartsucker
Copy link

The point of using TUF is that it provides strong security properties from the metadata outside of just "we used a good signing scheme / hash algorithm." Signify and libsodium do not provide that.

@dbrgn
Copy link

dbrgn commented Dec 8, 2018

I haven't read through the entire thread, but just since it wasn't mentioned yet: If you want to verify PGP signatures, there's now Sequoia, written in Rust! Not sure how that is useful in context of TUF though, hope this comment isn't just useless spam.

@tarcieri
Copy link

tarcieri commented Dec 10, 2018

@dbrgn Sequoia looks interesting (I've been following it for awhile), but it seems like rustup would probably want to use the openpgp crate that Sequoia is based on, which I'm a bit confused about, because the one on crates.io hasn't been updated for 2 years, though the one in the repo has been updated quite recently (i.e. past few days)

Regarding OpenPGP + TUF, I have been in contact with the TUF developers about this particular problem. I would like to work with them to develop a solution that can allow PGP v4 key IDs to be used in place of TUF's own native key ID format, and OpenPGP signatures as the signature format.

I wrote up an initial proposal for that as part of this crates.io signing + TUF design:

withoutboats/rfcs#7

@kinnison
Copy link
Contributor

kinnison commented Dec 3, 2020

Rustup has been quietly verifying GPG signatures on channel downloads for some time now. I'm going to close this, although there's still the trust chain consideration I think other issues cover that quite well.

@kinnison kinnison closed this as completed Dec 3, 2020
@DemiMarie
Copy link

@kinnison does it fail if the signatures cannot be obtained or verified?

@kinnison
Copy link
Contributor

kinnison commented Dec 3, 2020

@DemiMarie Currently it warns, though nobody has reported the warning showing up. We intend to shift it to an error soon.

@DemiMarie
Copy link

Is there a way to force it to error?

@briansmith
Copy link

@DemiMarie Currently it warns, though nobody has reported the warning showing up. We intend to shift it to an error soon.

What is the issue that tracks that? I thought it was this one. There should be an open issue until it is fixed.

@kinnison
Copy link
Contributor

kinnison commented Dec 4, 2020

The high level tracking issue is here - #2028 and I've updated it with the current state of play AIUI. If you feel I should reopen this issue as well for tracking that then I'm okay to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests