Upgrade to ring 0.17

[thomaseizinger]

This PR prepares the library for the next ring release. Most prominently, ring now exposes its randomness source properly for ECDSA keypairs. In a bigger context, this now allows for deterministic certificates!

See melekes/rust-libp2p#12.

[melekes]

[melekes]

error doesn't contain any useful info?

[thomaseizinger]

It does! But generally, it is more useful if an error returns its source via source. If you use something like anyhow, that will concatenate the various error messages for you. If you also include them in the Display implementation, you end up duplicating the message.

See rust-lang/project-error-handling#23.

[thomaseizinger]

Happy to drop that commit in case people disagree!

[est31]

I share the concerns written by yaahc in that thread, I think we should not implement the source function (and print the error instead).

Thing's like logging or printing errors directly, or panicking with Result's containing E: Error types that haven't been converted to a Reporter will fail to introduce the party that is responsible for iterating over the sources and printing all of the errors.

[est31]

Thanks! I wonder a little when the ring 0.17.0 release will be, the last alpha for it was almost one year ago.

[melekes]

Thanks! I wonder a little when the ring 0.17.0 release will be, the last alpha for it was almost one year ago.

briansmith/ring#1504 (comment) looks like it might be soon? 🤞

[thomaseizinger]

Oh, CI is not green. Investigating.

[thomaseizinger]

Oh, CI is not green. Investigating.

Turns I didn't test with all features. Should be green now!

[est31]

looks like it might be soon?

Interesting. Let's see whether it turns out to happen. briansmith/ring#1416 (comment)

[iamjpotts]

If a goal of rcgen is ease of use, should there be a reasonable default SecureRandom for the current method signatures, and additional methods for specifying a SecureRandom implementation?

I.e. don't add an additional required argument to get basic functionality; have an additional method instead to customize it.

[thomaseizinger]

If a goal of rcgen is ease of use, should there be a reasonable default SecureRandom for the current method signatures, and additional methods for specifying a SecureRandom implementation?

The very basic top-level function for generating a certificate still defaults to SystemRandom and so does the one for generating one from CertificateParams.

This PR only exposes SystemRandom in APIs that users would only use if they want more control over the certificate. In that case, exposing the randomness source too is a small price to pay for something that could potentially be a showstopper for certain usecases.

I.e. don't add an additional required argument to get basic functionality; have an additional method instead to customize it.

I disagree with that. In an ideal world, APIs are orthogonal to each other. That will lead to more utility with a smaller number of APIs and fewer layers of indirection. If your usecase always requires a certain combination of parameters, write yourself a small wrapper that passes constant parameters in.

[est31]

Suggested change

	/// If you need to control the [`KeyPair`], set it ahead of time before calling this function.
	/// If you want to control the [`KeyPair`], set it ahead of time before calling this function.

[est31]

@iamjpotts makes some good points.

I've done some research why we need to add the secure random generator here as a parameter. It seems it was exposed about one year ago, and is part of a mechanism for nonce hardening, introduced in briansmith/ring#662 , to protect from bad PRNGs, as this comment lays out. One comment in the PR alludes to it having been added because BoringSSL has it.

IMO if you assume in your security model that the RNG is faulty, then you are into a lot of trouble, and I'm not sure software like ring is really hardened towards that goal anyways. E.g. does the RSA component suffer from a similar issue? I have to do more research, but maybe it's not worth the additional API complexity it adds, even for ring...

As for the API complexity, this is definitely a negative impact.

There are use cases to have custom random number generators, but they are quite rare. I generally want to make it customizeable, but I agree with @iamjpotts that this is making the API too complex. Also, we are not exposing the RNG in the sign function, so it's a bit inconsistent.

I think an RNG would pass the complexity threshold where a builder pattern would make sense. That is, ideally you'd have functions both with and without RNG params, but they would add 5 more loading functions to KeyPairs API, which is already a bit bloated due to the sign algo auto detection/specifying sign algo choice. I think the best would be to pass SystemRandom for now, and make RNG customization a future goal once we add a builder or struct based API.

[thomaseizinger]

I've done some research why we need to add the secure random generator here as a parameter. It seems it was exposed about one year ago, and is part of a mechanism for nonce hardening, introduced in briansmith/ring#662 , to protect from bad PRNGs, as this comment lays out. One comment in the PR alludes to it having been added because BoringSSL has it.

The underlying reason why ECDSA needs an RNG is because when ECDSA was devised, it needed to be different enough from Schnorr signatures to not infringe with the patent that was still valid back then. The unfortunate consequence of the different design is that it needs a random number for each signature. If the RNG used here is bad, you will end up leaking your private key, given the attacker has a way of getting you to make enough signatures.

There are use cases to have custom random number generators, but they are quite rare. I generally want to make it customizeable, but I agree with @iamjpotts that this is making the API too complex.

Our usecase is deterministic certificates to avoid having to write them to disk but derive them from some higher-level secret via HKDFs. We need to control the RNG so we can seed it accordingly. I don't think this is particularly uncommon. In fact, certificate pinning is quite common these days. That requires certificates to either be persistent or generate them deterministically from some other persistent secret. For a library whose whole job it is to generate self-signed certificates, allowing them to be deterministic seems quite useful to me. It makes dev-ops much easier because there are less secrets the user needs to backup / take care of.

Also, we are not exposing the RNG in the sign function, so it's a bit inconsistent.

That was an oversight in my patch then!

I think an RNG would pass the complexity threshold where a builder pattern would make sense. That is, ideally you'd have functions both with and without RNG params but they would add 5 more loading functions to KeyPairs API, which is already a bit bloated due to the sign algo auto detection/specifying sign algo choice. I think the best would be to pass SystemRandom for now, and make RNG customization a future goal once we add a builder or struct based API.

If exposing the RNG is a non-goal, then that is a show-stopper for us. Unrelated to this, a recent discussion revealed that depending on rcgen is probably to risky for us anyway so we are likely to go with a different solution of generating the certificate.

I can invest a bit more time into this PR if you don't expect many changes but I'd be grateful if someone would just take it over :)

[est31]

The unfortunate consequence of the different design is that it needs a random number for each signature.

We need randomness for each signature, yes. That's why we pass in a SecureRandom at the time point of signing. This is fine. What I complained about was passing in randomness at KeyPair loading. It's not fault of your PR though.

If exposing the RNG is a non-goal, then that is a show-stopper for us.

Exposing the RNG is definitely a goal. I just thought it was low-priority because I hadn't heard of users who actually want it, and the way this PR is doing it is sub-optimal. I now have heard of users who want to customize the RNG, so it's less low-priority now :).

Unrelated to this, a recent discussion revealed that depending on rcgen is probably to risky for us anyway so we are likely to go with a different solution of generating the certificate.

That's unfortunate to hear. Note that you are asking a lot from me when you ask for deterministic output not just between runs of the same program, but also deterministic output wrt cargo update. Former is definitely something I'm open supporting (rcgen already supports it but not in an easy to use way, you have to custom-implement the RemoteKeyPair trait, e.g. see the from_remote test in webpki.rs, costs around 20 lines of additional code). Latter is only partially under my control. E.g. ring could change its pattern how it accesses the RNG, etc. So tomaka's concerns are very accurate.

I also doubt that simple_x509 can provide a similar guarantee btw, and I think that tomaka's comment applies to that library in the same fashion. Your PR seems to be using ring for the signatures as well. There is also the concern of what happens if you want to change e.g. from ring to another crate.

I think if you want to improve determinism further, you might try ed25519 keys. Those are well-supported by rcgen and I think they should be 100% deterministic in a way that is portable across libraries, but I haven't tested it.

@thomaseizinger my criteria for merging this PR are:

• removal of the public API changes due to SecureRandom. They feel orthogonal to me. I offer to refactor the KeyPair APIs in a way that you can pass in random yourself after this PR has been merged. I want the changes removed so that if I decide to keep some of the functions instead of moving everything to the builder I don't have to change them back to not take a SecureRandom.
• ring 0.17.0 is released. I don't want to depend on pre-release versions of ring, not even in master as I want always to be able to publish to crates.io. The time that 0.17.0 is released is unclear. This is probably the factor which will delay this PR the most, but I'm open to pleasant surprises from the ring maintainers :).
• the smaller concerns I pointed out above are resolved.

If you want to use rcgen in your linked PR, I recommend you to implement RemoteKeyPair as described above, this already works with released rcgen and ring versions.

[thomaseizinger]

The unfortunate consequence of the different design is that it needs a random number for each signature.

We need randomness for each signature, yes.

Ah yeah, I mixed it up. For some reason I thought schnorr doesn't need randomess but of course there is a nonce there too.

That's why we pass in a SecureRandom at the time point of signing. This is fine. What I complained about was passing in randomness at KeyPair loading.

Yeah fair enough. Hopefully ring at some point implements RFC6979 as pointed out here. That would give us deterministic nonces which would remove the need for the randomness source at signature time.

Former is definitely something I'm open supporting (rcgen already supports it but not in an easy to use way, you have to custom-implement the RemoteKeyPair trait, e.g. see the from_remote test in webpki.rs, costs around 20 lines of additional code).

I tried using the RemoteKeyPair trait but it panicked with "cannot serialize a remote keypair". I was using the code through webrtc's certificate stuff so there might have been an error there.

I also doubt that simple_x509 can provide a similar guarantee btw, and I think that tomaka's comment applies to that library in the same fashion. Your PR seems to be using ring for the signatures as well. There is also the concern of what happens if you want to change e.g. from ring to another crate.

All valid points. The less moving parts the better though. I've now pinned the version of simple_x509 which should avoid the cargo update problem. Given that library aims for a much smaller task than rcgen, I think it is a better fit.

I think if you want to improve determinism further, you might try ed25519 keys. Those are well-supported by rcgen and I think they should be 100% deterministic in a way that is portable across libraries, but I haven't tested it.

Unfortunately, browsers don't necessarily need to support ed25519. See https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/generateCertificate#standard_configurations.

@thomaseizinger my criteria for merging this PR are:

* removal of the public API changes due to `SecureRandom`. They feel orthogonal to me. I offer to refactor the `KeyPair` APIs in a way that you can pass in random yourself after this PR has been merged. I want the changes removed so that if I decide to keep some of the functions instead of moving everything to the builder I don't have to change them back to not take a `SecureRandom`.

Okay, I can remove those API changes. I'll default to SystemRandom everywhere then.

* ring 0.17.0 is released. I don't want to depend on pre-release versions of ring, not even in master as I want always to be able to publish to crates.io. The time that 0.17.0 is released is unclear. This is probably the factor which will delay this PR the most, but I'm open to pleasant surprises from the ring maintainers :).

Yes that makes total sense. I opened it as a draft because of that, didn't expect it to get merged before it is actually released.

[est31]

I tried using the RemoteKeyPair trait but it panicked with "cannot serialize a remote keypair". I was using the code through webrtc's certificate stuff so there might have been an error there.

Yeah, webrtc tries to serialize the key pair. The panic message you refer to comes from us and it is issued when someone tries to do such serialization with remote key pairs. remote key pairs were designed with the use case of HSMs etc in mind where you don't have the private key, so trying to serialize it doesn't make much sense. This is I guess therefore more an issue in webrtc than in rcgen, as it doesn't seem to support remote key pairs. Admittedly, a KeyPair doesn't give you any sign primitive so if all you do is get a KeyPair, you have to serialize it. Maybe I should add an accessor function for the case that it is a remote key pair, that just exposes the internal dyn trait object.

[jiegec]

ring 0.17 is out: briansmith/ring@d34858a, it's time to revive this pr.

[est31]

@jiegec that's great news! I'd love to hear from @thomaseizinger if they still want to work on this PR or if we should look into updating ring on our own. There have been a bunch of refactorings since that made this PR get out of sync.

From my understanding @thomaseizinger might not need the 0.17.0 update themselves any more? Let's give them until wednesday to respond if they want to work on this.

[thomaseizinger]

Thanks for the ping! I was planning on booting this back up! My usecase is deterministic certificate generation which we still want. I'll check whether that is possible using the latest ring version now :)

[est31]

@thomaseizinger thanks for the confirmation ❤️ ! Looking forward to seeing ring 0.17 being supported :).

[thomaseizinger]

Closing in favor of #166.