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

Reintroduce ExpandedSecretKey funcionality #298

Closed
poljar opened this issue Apr 6, 2023 · 11 comments · Fixed by #299
Closed

Reintroduce ExpandedSecretKey funcionality #298

poljar opened this issue Apr 6, 2023 · 11 comments · Fixed by #299

Comments

@poljar
Copy link
Contributor

poljar commented Apr 6, 2023

Hi there.

We're using the dalek crates in vodozemac as the core for our double ratchet implementation. This is a relatively new library intended to replace our old C based libolm library.

Now our problem is that libolm always expands Ed25519 secret keys, which are stored long-term.

As we have many users already using libolm, we would like to transfer this expanded Ed25519 key when we transition our users from libolm to vodozemac. We've looked into updating our ed25519-dalek dependency, but found that it is no longer possible to construct an ExpandedSecretKey as the type is no longer public, as per #205.

Is there any way to restore this functionality?

@tarcieri
Copy link
Contributor

tarcieri commented Apr 6, 2023

Per #205, ExpandedSecretKey has an unsafe API that was removed to fix a security vulnerability.

It also doesn't conform to any standard interface for Ed25519 keys, such as the one specified in RFC8032.

@poljar
Copy link
Contributor Author

poljar commented Apr 6, 2023

I can definitely see how this API is misuse-prone and aware it isn't a standard interface. We've long considered this behaviour of libolm to be problematic.

Still, it's perfectly possible to use it correctly and our crate packages it up safely, without re-exposing the insecure interface.

Given this is a backward-incompatible change, resulting in a relatively critical loss of functionality preventing us from ever seamlessly migrating, perhaps it could be considered for reintroduction under a feature flag?

Alternatively, many RustCrypto crates offer low level and easy to misuse interfaces under crates or submodules marked as hazmat. Maybe even marking the constructor as unsafe?

@tarcieri
Copy link
Contributor

tarcieri commented Apr 6, 2023

It's possible, although the @RustCrypto hazmat features generally exist as implementation building blocks of higher level constructions, whereas this would be supporting a nonstandard key format which is not allowed under RFC8032 or NIST SP 800-186.

I would suggest leaving it off the table for a 2.0 release and circling back afterward.

Alternatively, you can maintain a fork. I imagine you'll have to do this for many different libraries for different languages, since it's not a standard API.

@tarcieri
Copy link
Contributor

tarcieri commented Apr 6, 2023

An option that might be more in the spirit of a "hazmat" API would be a low-level signing API which accepts parameterized inputs (i.e. private scalar, derivation input, public key Edwards point, message hash), which could potentially also be used to implement constructions like the various flavors of Ed25519-BIP32

@poljar
Copy link
Contributor Author

poljar commented Apr 7, 2023

It's my understanding that ExpandedSecretKey was only insecure due to its sign_ interface accepting the public key as an argument, rather than the existence of ExpandedSecretKey itself being somehow problematic. Rather than outright removing it, couldn't we add ExpandedSecretKey back (as suggested by this comment) with a different sign interface that doesn't accept the public key but derives it in the implementation?

Alternatively, you can maintain a fork. I imagine you'll have to do this for many different libraries for different languages, since it's not a standard API.

Besides libolm itself, which we're trying to sunset, I don't have to maintain any forks. We are using a single library for all our target languages. It would be nice to keep it this way.

@tarcieri
Copy link
Contributor

tarcieri commented Apr 7, 2023

with a different sign interface that doesn't accept the public key but derives it in the implementation?

That would add an extra scalarmult to each signing operation, which would mean it couldn't be composed in terms of the existing implementation, where the corresponding public key is stored in SigningKey.

Effectively, we'd be adding an API just to support your nonstandard use case.

You can always vendor your own ExpandedSecretKey into your project?

@rozbb
Copy link
Contributor

rozbb commented Apr 7, 2023

@poljar Trying to determine how much burden this would put on you. Precisely what functionality do you need out of ExpandedSecretKey? If it's signing and that's it, then I think it'd be reasonable to keep the old def and vendor the sign() function from here. Are there other things you need?

@tarcieri
Copy link
Contributor

tarcieri commented Apr 7, 2023

@rozbb yeah, that's more or less what I was proposing here, although perhaps even going lower level with it: #298 (comment)

Doing that would be a universal solution for everyone who wants to do something nonstandard, so we're not adding one-off nonstandard features every time they come up.

@poljar
Copy link
Contributor Author

poljar commented Apr 11, 2023

If it's signing and that's it, then I think it'd be reasonable to keep the old def and vendor the sign() function from here. Are there other things you need?

The things we are using are as follows:

  1. ExpandedSecretKey::from_bytes()
  2. ExpandedSecretKey::sign()
  3. The Serialize and Deserialize implementations for ExpandedSecretKey.

#299 certainly helps, thanks for that. It still feels a bit weird to have to re-define ExpandedSecretKey to have the from_bytes() and serialization back, but it isn't too bad.

@tarcieri
Copy link
Contributor

Perhaps we could put ExpandedSecretKey under hazmat and add accessor methods for scalar and nonce (not wild about the name of the latter, but), so they can be used in conjunction with the low-level signing API.

@poljar
Copy link
Contributor Author

poljar commented Apr 11, 2023

Perhaps we could put ExpandedSecretKey under hazmat and add accessor methods for scalar and nonce (not wild about the name of the latter, but), so they can be used in conjunction with the low-level signing API.

That sounds nice, but helps me only if ExpandedSecretKey can be serialized and deserialized again. If that's available then I don't need to copy any code from ed25519-dalek into vodozemac, which would be the ideal scenario.

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 a pull request may close this issue.

3 participants