add support for openssl backend in llcrypto

fix #442 (closed)

Someone who knows how to test performances should probably make sure this is actually faster. When testing, aes was 3 time faster, which probably means I tested it wrong.

I made it a feature since there aren't that many ways to specify optional dependencies.

requested review from @nickm

added 19 commits

• 7c31dc7f...58249924 - 18 commits from branch tpo/core:main
• 0f290da0 - add support for openssl backend in llcrypto

Compare with previous version

This looks sound to me, but I'd like us to figure out the feature structure here, for a couple of reasons:

• This isn't a strictly additive feature; it's an alternative implementation that replaces the default implementation. I agree that doing it the "right way" might not be worth the complexity, but we should discuss it.
• We should think about testing: our approach so far has been to run most tests with --all-features, but that means that we usually won't test the default implementations. Maybe it's time to introduce a "full" feature at the top-level, and have it include only the stable feature-like features?

@Diziet what do you think?

---

Two specific comments:

• It looks like the code is looking at feature = "openssl" but the Cargo.toml file has the feature called with-openssl. Does that work? Does each optional dependency count as a feature on its own?
• There should be crate-level documentation about what this feature does.

This isn't a strictly additive feature; it's an alternative implementation that replaces the default implementation

I think "additive" here means that enabling this feature doesn't break anything, and in particular doesn't make a breaking change to any APIs.

I haven't looked at this in detail (in particular, I have not looked at the exposed API of tor-llcrypto) but it seems to me to be possible (and desirable) for this particular change to be made in a way that is additive in that sense.

So I think (if indeed enabling this feature doesn't change the API, only the implementation) then doing this as a plain feature is fine. If that's not the case, then tor-llcrypto would seem to be exposing details it ought not to.

Maybe it's time to introduce a "full" feature at the top-level, and have it include only the stable feature-like features?

I think it is past time to do this, but mainly for other reasons than this one.

Whether or not we make that change soon, I think we definitely do need to test both the openssl-based implementation and the one based on the aes and ctr crates, at the tor-llcrypto API, with a comprehensive set of test vectors. Again, I haven't looked to see if those test cases are there already, but even if they are, I think this MR needs to contain something to run them in both configurations.

HTH.

There are indeed reasonably good test vectors. I think that running in multiple configurations isn't included yet.

The CI will currently run in both configurations, since rust-latest runs with default features, and rust-nightly runs with --all-features. But I do think that we'll need a bigger matrix of tests as well: #303 (closed).

What about having RustSha1 and possibly OpenSslSha1 (if compiled with the feature), and Sha1 be a type alias to one of those? That way a test with --all-features could test both. (and having the same with AES types)

What about having RustSha1 and possibly OpenSslSha ...

That seems like a good approach.

Can we make it a type alias (or import of) a private type name for an opaque type? That would mean that a caller couldn't distinguish the two at the API level and therefore changing the feature wouldn't be an API break.

I'm not thrilled by the idea of exposing both of those as part of the crate API, which I think would be necessary for testing them the way that our test vectors are run now. If we decide to go this route, I think we'd have to make the implementation-specific tests run as part of the crate-internal unit tests, which would take some refactoring to get everything nice. :/

Since we already need a matrix of features for other stuff, I would also be fine with not doing this, and leaving things alone for now.

After discussion with @Diziet, it appears we agree that we can merge this branch as it is, and do a matrix-of-options thing in #303 (closed). So, merging.

For the record, the rationale was:

• We already test both openssl and rust crypto backends in our CI "by accident", since one of our runners uses default features and another does --all-features.
• We already know that we need a matrix of features for #303 (closed).
• Therefore, there's not much point taking a half-measure just for this branch.

It looks like the code is looking at feature = "openssl" but the Cargo.toml file has the feature called with-openssl. Does that work? Does each optional dependency count as a feature on its own?

I doubt it does. I changed the name of the feature to fix an msrv issue, but forgot to update the cfg(feature)

added 1 commit

• 1d41ae92 - use the right feature name for llcrypto openssl

Compare with previous version

merged

mentioned in commit cfad648e

mentioned in issue #499 (closed)

mentioned in issue tpo/team#186 (closed)