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

impl standard IO traits for hashing #228

Merged
merged 4 commits into from
Jan 19, 2022
Merged

Conversation

vlmutolo
Copy link
Contributor

@vlmutolo vlmutolo commented Oct 17, 2021

See #227.

TODOs

  • Hashing
    • implement from_reader for BLAKE2b
    • implement from_reader for sha2 (if we feel like we need to)
      • sha256
      • sha384
      • sha512
  • AEAD
    • implement seal_copy for XChaCha20Poly1305
    • implement open_copy for XChaCha20Poly1305
    • Other, non-high-level-interface methods (?)
  • General
    • decide whether to keep or change all these function names, like from_reader, seal_copy, open_copy

EDIT: The scope of this PR has changed. We were going to try to fit the Read/Write impls for all of Orion's types into this PR, but then decided to split it up. #227 will keep track of all the impls.

Now this PR implements std::io::Write for Blake2b and adds a convenience function orion::hash::digest_from_reader.

@vlmutolo
Copy link
Contributor Author

vlmutolo commented Oct 17, 2021

In deciding what should be implemented, maybe we should just restrict it for now to the types that actually show up in the high-level interface. If people are reaching into hazardous anyway, maybe it's not a big deal for them to implement these interfaces on their own.

@vlmutolo vlmutolo self-assigned this Oct 17, 2021
@vlmutolo
Copy link
Contributor Author

@brycx Do you think there should be a new io feature for this? It currently doesn't compile without default features. Should I just feature-gate it behind safe_api?

@brycx
Copy link
Member

brycx commented Oct 18, 2021

@vlmutolo I think it'd be best to keep it behind safe_api for now. We can look into if it makes sense to provide Read/Write for hazardous-only things later.

ATM, I think safe_api makes most sense. Also, with the serde feature, I'd like to get the new wiki section written first, to see how much the new serde feature complicates the feature-history of Orion.

@brycx
Copy link
Member

brycx commented Dec 26, 2021

Is there something that blocks progression on this @vlmutolo? Should we discuss it more, maybe play around with more examples?

@vlmutolo
Copy link
Contributor Author

So I don't think anything is "blocking" this exactly, but there are some open design questions I have that would be good to discuss. I'll write a bit about that on the relevant issue.

@brycx
Copy link
Member

brycx commented Dec 27, 2021

I answered in the wrong place regarding some things. Apologies. Re-posting here:

decide whether to keep or change all these function names, like from_reader, seal_copy, open_copy

form_reader seems good to me. Perhaps the naming of the functions for non-streaming AEAD could be made a bit less generic. How about sealing_read()/opening_write() or seal_read()/seal_write()?

Do you think there should be a new io feature for this? It currently doesn't compile without default features.

I don't think another io feature is necessary. If we just gate it behind safe-api that should be fine, just like we do with the std:error:Error trait.

@brycx
Copy link
Member

brycx commented Dec 27, 2021

Edited your issue to having completed from_reader for BLAKE2b instead of Argon2i.

@brycx
Copy link
Member

brycx commented Dec 28, 2021

For when the implementation is more stable, I think it would be beneficial to try and integrate the *_from_reader() function into the streaming API consistency testing framework, to ensure equivalence in behavior. Possibly also Write impl if applicable. AEAD testing framework is an obvious candidate. But these could also be a different PR.

@vlmutolo
Copy link
Contributor Author

Maybe it would be a good idea to split this PR into two: one for hashing, one for AEAD.

@brycx
Copy link
Member

brycx commented Dec 29, 2021

Maybe it would be a good idea to split this PR into two: one for hashing, one for AEAD.

Good idea. I'd probably do so, especially to work on the tests and not grow the PRs too large with that alone.

@vlmutolo vlmutolo marked this pull request as ready for review December 30, 2021 16:05
@vlmutolo vlmutolo requested a review from brycx December 30, 2021 17:20
@vlmutolo vlmutolo changed the title impl standard IO traits for hashing and encryption/decryption impl standard IO traits for hashing Dec 30, 2021
@vlmutolo
Copy link
Contributor Author

I tried to use something like #[cfg_attr(docsrs, doc(cfg(feature = "safe_api")))] on the Blake2b Write impl, but couldn't figure get the little docs warning to show up. I also looked through how Orion already uses it and found that it was added for the serde impls here. However, when I render the docs with cargo doc --features serde, I still don't see the little bubble pop up locally. Am I just rendering the docs wrong?

@brycx
Copy link
Member

brycx commented Dec 30, 2021

Did you build the docs with the parameters listed in the README? The command you just posted in the comment won't enable the rendering IIRC.

@vlmutolo
Copy link
Contributor Author

Yep, that worked. Everything shows up as it should, including the feature bubble for Blake2b's Write impl.

Copy link
Member

@brycx brycx left a comment

Choose a reason for hiding this comment

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

Great start on getting io integration for the library! I have two general things in addition to code comments:

[1] Please rebase upon master so that we're sure the doctests run with --no-default-features (#263). I believe this should be the case already though, since the function itself is gated behind safe-api.

[2] Since the title of the PR is "IO traits for hashing" do you plan to add Write impls for SHA2 primitives in other PRs?

src/hazardous/hash/blake2/blake2b.rs Show resolved Hide resolved
src/high_level/hash.rs Outdated Show resolved Hide resolved
src/high_level/hash.rs Outdated Show resolved Hide resolved
src/high_level/hash.rs Show resolved Hide resolved
@vlmutolo
Copy link
Contributor Author

Please rebase upon master

👍

do you plan to add Write impls for SHA2 primitives

That's a good question. I can definitely do the same write impl for SHA as I did for Blake. Wasn't thinking about that, but I should probably add it to this PR.

@brycx brycx added this to the v0.17.1 milestone Dec 31, 2021
@brycx brycx added the new feature New feature or request label Dec 31, 2021
Vince Mutolo added 2 commits January 16, 2022 14:51
This commit adds support for Rust's standard Write trait to the
Blake2b type. It also adds a convenience function digest_from_reader
that will consume all data from a reader and output the digest.
@vlmutolo vlmutolo requested a review from brycx January 17, 2022 01:03
Copy link
Member

@brycx brycx left a comment

Choose a reason for hiding this comment

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

Sorry about the delay - LGTM, great work!

@vlmutolo vlmutolo merged commit ec24ade into orion-rs:master Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants