Add a Rust library #175
Add a Rust library #175
Conversation
|
Looks interesting, but my Rust is at a hello world level, so probably can't do a good review here :) |
There was a problem hiding this comment.
So I agree that the untyped variant is not great and neither is the typed variant. Is there a way where we could offer our own wrapper types? Or could we export these types as type aliases somehow? In general the underlying types are very simple, the traits are where it's at...
burgerdev
force-pushed
the
rust-squashed
branch
from
3c0a48c to
1de1469
Compare
|
This looks straightforward and simple now. Thank you! :D I had one question on chat if you can now implement these traits from another crate. And two things I stumbled upon was the dalek import from lib and lib being no_std. But I'd be happy to merge this as-is. The PR is still technically in draft mode, though. (And I understand that there are the TODOs in the code, so maybe we still want to extend TagArgs a little bit before merging? |
burgerdev
force-pushed
the
rust-squashed
branch
2 times, most recently
from
09cbe47 to
81a8d8d
Compare
|
This should be good to review now, I'll add a Github Action later on. |
|
@nicmue could you please take a look at this PR, with particular attention to module structure, Rust best practices and the test implementation? I wonder if there's a better way to execute all test vectors for a given |
There was a problem hiding this comment.
Since I am not familiar with the repo and the usage of this code I can't judge the functionality and therefore only the pure rust code:
Looks really good from my side. I just pointed out the warnings you will get when running cargo clippy (which I highly recommend) and a personal preference on the &dyn Fn... call in your run_vector functions. The model structure and implementation looks completely idiomatic to me and tbh I like how you handled the test execution for all your PublicKey/PrivateKey implementation.
|
@nicmue Thanks so much for the review, and sorry for taking some time to get back to it. This was really helpful, and now I know about A follow-up to this PR should add these checks to CI. |
|
@nicmue since you are now co-author here, do you want to sign Google's CLA? Alternatively, I can remove the |
Sure why not. What do I need to do? :D |
|
The CLA bot check failure has some info: https://github.com/google/glome/pull/175/checks?check_run_id=26263501184. tl;dr: sign in to https://cla.developers.google.com/clas and agree to the terms. |
Co-authored-by: Nico Mürdter <nicomuerdter@gmail.com>
Done you can try to retrigger. :) |
More or less the first thing I did when I started this branch was researching the appropriate X25519 library to use. Now, almost 3 years later, I still find it rather hard to decide. My top picks were
x25519_dalek,ed25519_compactandopensslbindings, in order of preference. But when I started with the Dalek crate, I encountered strange problems when using itsgetrandomfeature, so I rewrote the thing fored25519_compact. Quite a lot seems to have happened since then - I did not see any of the issues I had back then when I re-rewrote the thing just now.This train of rewrites is the main motivation behind me implementing two versions of the API. I really wanted to have an interface that does not leak the underlying X25519 library - this is how we ended up with the
untypedmodule. Happy to hear your thoughts, but I, for one, consider... not as an improvement over C, so I wrote the
typedmodule, too, which uses dalek types.There is probably a third option, with an API that accepts traits and next to it a few impls for x25519 choices, but I don't think my Rust is up to that yet.
EDIT - June 2024: I implemented tagging and verification in terms of traits and provided trait impls for openssl and dalek. This is probably the way to go.