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

[WIP] subtle: breaking changes for a hypothetical v3.0 #136

Closed

Conversation

tarcieri
Copy link
Contributor

@tarcieri tarcieri commented Aug 3, 2024

This commit contains a minimal set of proposed breaking changes, as an alternative to trying to make additive changes which may be "subtly" breaking:

This commit contains a minimal set of proposed breaking changes, as an
alternative to trying to make additive changes which may be "subtly"
breaking:

- Add `ConstantTimeClone` trait and use it as
  `ConditionallySelectable`'s supertrait bound (alternative to dalek-cryptography#118):
  this makes it possible to implement `ConditionallySelectable` on
  heap-allocated types (fixes dalek-cryptography#94)
- Remove `Default` bound on `CtOption::map` and `CtOption::and_then`:
  allows using these methods with types that don't impl `Default` and
  avoids potential timing sidechannels if there might be timing
  variability in `Default` types (fixes dalek-cryptography#63)
@tarcieri
Copy link
Contributor Author

tarcieri commented Aug 3, 2024

I think #118 might still be worth considering, in that since it relaxes bounds it isn't technically "breaking" (I believe), but would potentially result in code ceasing to compile if it was relying on the supertrait bound of ConditionallySelectable being Copy.

This PR is a "safer" option in terms of directly calling these changes out as breaking, but would split the subtle ecosystem between v2 and v3, which is rough given the trait-based API and how it's composed across crates.

One workaround, as I noted in #118, would be to use the "SemVer trick" and have subtle v2 optionally depend on subtle v3, which would make it possible for subtle v2 to add a blanket impl of its ConditionallySelectable for all types which impl subtle v3's ConditionallySelectable, as well as re-exporting the unchanged traits/types wholesale rather than defining them, which would allow a degree of interop.

@@ -575,7 +581,7 @@ impl ConditionallySelectable for Choice {
#[cfg(feature = "const-generics")]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do make breaking changes, we can completely remove this feature since it's now MSRV-compatible

@@ -575,7 +581,7 @@ impl ConditionallySelectable for Choice {
#[cfg(feature = "const-generics")]
impl<T, const N: usize> ConditionallySelectable for [T; N]
where
T: ConditionallySelectable,
T: ConditionallySelectable + Copy,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately we can't support anything other than Copy types here, since [T; N] receives a Copy impl for T: Copy, and that means it will receive ConstantTimeClone when T: Copy.

This means we can't add a blanket impl of ConstantTimeClone to [T; N] where T: Copy because it would overlap with the other blanket impls outlined above.

We could potentially add bounds like: Self: ConstantTimeClone, T: ConditionallySelectable + ConstantTimeClone, but that wouldn't actually be usable since [T; N] only receives a ConstantTimeClone impl for Copy types as described above.

What would be really nice here is some way of expressing disjoint bounds using hypothetical new type system features, e.g. being able to impl<T> ConstantTimeClone for [T; N] where T: ConstantTimeClone + !Copy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice not to explicitly use Copy here just to leave the door open for the blanket impl outlined above though

impl<T: ConditionallySelectable> ConditionallySelectable for CtOption<T> {
impl<T> ConditionallySelectable for CtOption<T>
where
T: ConditionallySelectable + Copy
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a similar problem where where we can't add a conditional ConstantTimeClone impl to CtOption<T> because it derives Copy and so already can receive the blanket impl.

@tarcieri
Copy link
Contributor Author

tarcieri commented Aug 3, 2024

Hmm, all of the complexity of these bounds has me wondering if we can just remove the Copy bound on ConditionallySelectable without any replacement, and eliminate any dependencies on Copy in the default methods.

@tarcieri
Copy link
Contributor Author

tarcieri commented Aug 3, 2024

Closing in #137 which avoids adding ConstantTimeClone by removing usages of Copy, and is significantly simpler

@tarcieri tarcieri closed this Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant