search

awslabs / tough

Rust libraries and tools for using and generating TUF repositories
195 stars 54 forks source link

Migrate crypto backend from `ring` to `aws-lc-rs` #824

Open ginglis13 opened 6 days ago

ginglis13 commented 6 days ago

The maintainers of tough wish to support the aws-lc-rs crypto library, which wishes to be a "drop-in replacement for ring that provides FIPS support and is compatible with the ring API".

We have some rough ideas for an approach and would like to get community input before such a change:

  1. Completely replace ring with aws-lc-rs as a drop-in replacement
  2. Use a cargo feature to gate usage of aws-lc-rs as the backend behind a feature (see rustls, which uses aws-lc-rs by default and offers ring as a feature on the crate: https://github.com/rustls/rustls?tab=readme-ov-file#platform-support)
  3. Expose a CryptoProvider interface for consumers of the library, with implementations for ring and aws-lc-rs (a la rustls: https://docs.rs/rustls/latest/rustls/crypto/struct.CryptoProvider.html)

We will keep this issue updated with our plans.

cc: @flavio @iliana @fghanmi and others in the community for your thoughts, questions, concerns, etc.

iliana commented 6 days ago

Thanks for the ping!

In a vacuum I think Option 1 makes perfect sense, except that aws-lc-rs doesn't currently build on illumos according to a coworker of mine. I think before I commit to saying this option works fine for us I want to chase that issue a bit and see if I can contribute a fix there (and maybe run a nightly illumos CI to catch issues early).

With my ex-maintainer hat on, for whatever that's still worth: I think the best option long-term is Option 3. I think it could be tricky to design a CryptoProvider interface if you're only building it for ring and aws-lc-rs, since one is a drop-in replacement for the other; I would recommend also trying to build a non-C-based implementation (e.g. RustCrypto and friends) of that interface to make sure the design of that traits are sensible, perhaps in a separate crate.

iliana commented 6 days ago

Update: I tested aws-lc-rs v1.9.0 (and also the current git HEAD) on my Helios system and it built fine, so either it was an environment error or something's changed in the past few months. Either way, I would consider that concern dealt with for the time being.

ginglis13 commented 6 days ago

@iliana thanks for such a quick response, and for chasing 1/ down a bit further on your end. Another option I'm chewing on is first refactoring tough to use aws-lc-rs (1.) and then leaving the door open for a CryptoProvider-esque interface if we find a need for it (3.). I'm still working on that refactor and testing and leaving this discussion open since this plan would fall through if the straight drop-in refactor doesn't work for consumers of tough

iliana commented 6 days ago

Once there's a branch with the refactor let me know, and I'll be happy to slot it into where we're using tough and make sure it still works as expected in our environment.

ginglis13 commented 5 days ago

Hey @iliana, I opened #825 taking the simple approach in (1.); please take a look and let me know your thoughts 😄

iliana commented 2 days ago

I'm running tests now. It turns out my coworker was actually right about build issues with AWS-LC (I forgot to test linking!), but another coworker chased down the changes that need to be made to get it to build fine. I'm running tests with your branch now.

https://github.com/aws/aws-lc/pull/1854 is our PR to AWS-LC.

iliana commented 2 days ago

Our unit/integration tests pass on #825, with some modifications for an API difference between ring and aws-lc-rs. As a heads up, we're currently only using Ed25519 for signing/verification, so we don't test all the ring/aws-lc-rs related codepaths.

ginglis13 commented 2 days ago

Thanks @iliana !

with some modifications for an API difference between ring and aws-lc-rs.

Any chance that was changes required around tough's usage of Ed25519KeyPair::from_pkcs8 ? ring-compatibility Or was this a change required in your consumption of tough?

As a heads up, we're currently only using Ed25519 for signing/verification, so we don't test all the ring/aws-lc-rs related codepaths.

Ack, we'll be testing against RSA, ecdsa, and ED25519

iliana commented 2 days ago

It was related to Ed25519KeyPair but not because of any changes in tough. (We are presently using Ed25519KeyPair::from_seed_unchecked which isn't supported at all in aws-lc-rs; all its functions require providing both the private key (seed) and public key to prevent misuse. So I had to rewrite a few things around key generation and parsing. I don't suspect it'll be blocking for us.)

flavio commented 1 day ago

For sigstore, I think we could cope with option 1, ideally speaking option 3 would be the best.

Thanks for having reached out!