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.

Sign up for GitHub

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

Jump to bottom

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

Open
ginglis13 opened this issue Sep 12, 2024 · 10 comments
Open

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

ginglis13 opened this issue Sep 12, 2024 · 10 comments

Comments

@ginglis13
Copy link
Contributor

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.
@jpculp jpculp pinned this issue Sep 12, 2024
@iliana
Copy link
Contributor

iliana commented Sep 12, 2024

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
Copy link
Contributor

iliana commented Sep 12, 2024

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
Copy link
Contributor Author

@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
Copy link
Contributor

iliana commented Sep 12, 2024

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 ginglis13 mentioned this issue Sep 13, 2024
Merged
@ginglis13
Copy link
Contributor Author

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

@iliana iliana mentioned this issue Sep 16, 2024
Merged
@iliana
Copy link
Contributor

iliana commented Sep 16, 2024

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.

aws/aws-lc#1854 is our PR to AWS-LC.

@iliana
Copy link
Contributor

iliana commented Sep 16, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

iliana commented Sep 17, 2024

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
Copy link
Contributor

flavio commented Sep 17, 2024

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

Thanks for having reached out!

@flavio flavio mentioned this issue Sep 17, 2024
Open
justsmth pushed a commit to aws/aws-lc that referenced this issue Sep 25, 2024
@iliana @jclulow
support building on illumos systems (#1854)
468ca3f 
### Issues:
n/a

### Description of changes: 
The team that works on [awslabs/tough](https://github.com/awslabs/tough)
wants to support using the aws-lc-rs Rust crate
(awslabs/tough#824). As one of several
downstream consumers of tough outside of AWS, we were asked for
feedback. Oxide uses tough as part of our product which runs on the
illumos operating system.

When linking Rust code using aws-lc-rs, the linker cannot find several
`aws_lc_*` symbols:

```
Undefined            first referenced
 symbol                  in file
aws_lc_0_21_1_bn_gather5            /home/iliana/git/omicron/target/debug/deps/libaws_lc_sys-ff0ab07b18206e1f.rlib(bcm.c.o)
aws_lc_0_21_1_rsaz_1024_red2norm_avx2 /home/iliana/git/omicron/target/debug/deps/libaws_lc_sys-ff0ab07b18206e1f.rlib(bcm.c.o)
aws_lc_0_21_1_aesni_gcm_encrypt     /home/iliana/git/omicron/target/debug/deps/libaws_lc_sys-ff0ab07b18206e1f.rlib(bcm.c.o)
aws_lc_0_21_1_chacha20_poly1305_seal /home/iliana/git/omicron/target/debug/deps/libaws_lc_sys-ff0ab07b18206e1f.rlib(e_chacha20poly1305.c.o)
aws_lc_0_21_1_chacha20_poly1305_open /home/iliana/git/omicron/target/debug/deps/libaws_lc_sys-ff0ab07b18206e1f.rlib(e_chacha20poly1305.c.o)
[ ... this goes on for some hundreds of lines ... ]
```

`uname -p` on illumos systems returns `i386` on 64-bit machines. This
resulted in assembly code not being linked into the AWS-LC library due
to architecture misdetection. To determine the native instruction set,
`isainfo -n` is used instead.

Once these cryptic errors were out of the way, we got a more normal
linking error indicating some missing libraries; symbols that are in the
libc on other platforms are in separate libraries on illumos.

I'm sending this PR on behalf of my coworker, @jclulow, but I can help
handle any requested changes.

### Call-outs:
n/a

### Testing:
Tested on an illumos system via aws-lc-rs by patching the crate in
Cargo.toml with:

```
[patch.crates-io.aws-lc-sys]
git = "https://github.com/oxidecomputer/aws-lc-rs"
branch = "illumos/aws-lc-sys/v0.21.1"
```

which updates the AWS-LC submodule to this commit.

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.

---------

Co-authored-by: Joshua M. Clulow <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@flavio @iliana @ginglis13