hyperledger / anoncreds-clsignatures-rs

Apache License 2.0
7 stars 12 forks source link

Use default BigNumContext to be compatible with old openssl. #25

Closed jovfer closed 1 year ago

jovfer commented 1 year ago

As far as I noticed, this is the only place incompatible with some old OpenSSL versions. Is it OK to roll back this change to the Ursa code version?

I don't see another usage of the secure OpenSSL heap, so I doubt the value of this particular call of new_secure. Right next call (BN::new() ) uses the default heap, so I'm confused about this inconsistency.

Please let me know if I missed some critical details.

andrewwhitehead commented 1 year ago

This defines the openSSL context used in all the BigNum operations, and using new_secure means that it is zeroed out when freed. If there's a need to be compatible with older versions then that could be added as a feature flag, but I can't say that I see the benefit.

swcurran commented 1 year ago

To make sure I understand your comment @andrewwhitehead — we should definitely keep new_secure (for the zeroed out feature), and to add new as an option, we’d have to add more complexity, including a config parameter, so we probably should not do this.

I guess the question for @jovfer is if supporting old versions of OpenSSL is worth the added complexity it will take?

jovfer commented 1 year ago

@andrewwhitehead, now I'm confused even more. Moved my concern to a separate thread

https://github.com/hyperledger/anoncreds-clsignatures-rs/issues/26

PS: a potential fix for the #26 is incompatible with old OpenSSL as well, but let's sync on the usage of OpenSSL secure heap first. I have an option to just use vendored openssl, thanks to the authors for the option and you for the easy way to enable it. So, this PR is rather a contribution for compatibility and/or raising the concern on the inconsistency of secure heap usage, not the only solution for my scenario.

FYI @swcurran

andrewwhitehead commented 1 year ago

Closing for now, we can revisit a compatibility flag if needed.