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

Add back (off-by-default) "soft-aes" feature #10

Merged
merged 1 commit into from Jan 12, 2019

Conversation

Projects
None yet
2 participants
@tarcieri
Copy link
Contributor

tarcieri commented Jan 12, 2019

The aes crate v0.3 eliminated feature-gated fallback to soft AES.

This adds back a soft-aes feature to opt-in to a software implementation of AES on platforms where a hardware implementation is not available.

Right now the only supported hardware implementation is AES-NI. This approach mirrors the target gating used by the aes crate, and therefore unless the soft-aes feature is enabled this crate
will not compile on any other architecture besides x86(-64) with AES-NI target features enabled.

@tarcieri

This comment has been minimized.

Copy link
Contributor Author

tarcieri commented Jan 12, 2019

@newpavlov I think the aes crate would still benefit from a feature like this, even if it were enabled-by-default.

My personal inclination is to leave it off-by-default so users need to opt-in, but I'd be fine with the aes crate doing the opposite.

@tarcieri tarcieri force-pushed the soft-aes branch from 7bbf254 to ad367a5 Jan 12, 2019

Add back (off-by-default) "soft-aes" feature
The `aes` crate v0.3 eliminated feature-gated fallback to soft AES.

This adds back a `soft-aes` feature to opt-in to a software
implementation of AES on platforms where a hardware implementation is
not available.

Right now the only supported hardware implementation is AES-NI.
This approach mirrors the target gating used by the `aes` crate,
and therefore unless the `soft-aes` feature is enabled this crate
will not compile on any other architecture besides x86(-64) with
AES-NI target features enabled.

@tarcieri tarcieri force-pushed the soft-aes branch from ad367a5 to 8f8c7be Jan 12, 2019

@tarcieri tarcieri merged commit ea72462 into master Jan 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tarcieri tarcieri deleted the soft-aes branch Jan 12, 2019

@tarcieri tarcieri referenced this pull request Jan 12, 2019

Merged

v0.4.0 #12

@newpavlov

This comment has been minimized.

Copy link

newpavlov commented Jan 13, 2019

I am still not sure about use-cases for this feature flag. Can you elaborate why you may need it?

@tarcieri

This comment has been minimized.

Copy link
Contributor Author

tarcieri commented Jan 13, 2019

(See also discussion on #12)

@newpavlov right now it's difficult to tell if you are configuring target-feature correctly in order to receive the AES-NI implementation (which I needed for testing if that was the culprit for a performance regression I'm investigating and will open an issue about separately)

Per #12, it seems like without a more draconian mechanism, most people will not configure target-feature whatsoever, receive the software implementation instead, and go about their day.

I think in an ideal case, at least for me:

  • On x86/x86_64, target-feature=+aes,+ssse3 would be required for the crate to compile. This may be easier to do with a per-target architecture workspace for these features.
  • To support older x86/x86_64 CPUs without AES-NI, runtime CPU feature detection could be used, however this does not work inside Intel SGX, so really some way to allow the crate user to control this would also be ideal.
  • Other CPUs would always get the software implementation, at least until additional hardware implementations become available.

I can open an issue about this on on https://github.com/RustCrypto/block-ciphers if you'd like.

@newpavlov

This comment has been minimized.

Copy link

newpavlov commented Jan 16, 2019

I think if you want to ensure that miscreant is built with the required target-features, you can add cfg'd compile_error! with a warning and use aes crate as usual.

@tarcieri

This comment has been minimized.

Copy link
Contributor Author

tarcieri commented Jan 16, 2019

@newpavlov aah, indeed! I'll go ahead and make that change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.