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

New crate versions #43

Closed
13 tasks done
newpavlov opened this issue Aug 13, 2019 · 51 comments
Closed
13 tasks done

New crate versions #43

newpavlov opened this issue Aug 13, 2019 · 51 comments

Comments

@newpavlov
Copy link
Member

newpavlov commented Aug 13, 2019

Migrating to 2018 edition is a long overdue and since it's a good chance to introduce breaking changes, I propose to use this issue to discuss them. Right now I have the following list of changes:

I may have forgotten some changes I had in mind. Feel free to suggest other public API changes.

@burdges
Copy link

burdges commented Aug 13, 2019

I think renaming input to update runs contrary to common terminology.

@tarcieri
Copy link
Member

To me the common terminology is Initialize-Update-Finalize (IUF) and update would make sense IMO

@newpavlov
Copy link
Member Author

@burdges
Can you provide examples of other libraries which use input? I would also prefer to keep the name to limit breakage, but it looks like it's fairly uncommon name for the method across hashing libraries.

@burdges
Copy link

burdges commented Aug 15, 2019

After a quick look, it appears terms like input and add are common only for hashmaps, etc., and update wins with it being the term from OpenSSL, libsodium, and Python, while Go and libgcrypt use write, and operators overloading being common too.

We'd use core::io::Write if it existed, but it does not so whatever. :)

@newpavlov
Copy link
Member Author

BTW maybe we should bump MSRV to 1.36? It will allow us to use alloc without compatibility workarounds.

@akhilles
Copy link
Contributor

akhilles commented Oct 3, 2019

Can block-cipher traits support IV non-reuse? With the current API, IV has to be introduced in the cipher’s constructor. We should be able to specify the IV in the encryption call instead.

@newpavlov
Copy link
Member Author

Maybe you mean stream-cipher or block-modes? IIRC the main reason for moving IV to new was interaction with seeking. Also it was more consistent with other traits.

What is motivation for such change in your opinion, assuming we will get the FromBlockCipher trait?

@akhilles
Copy link
Contributor

akhilles commented Oct 4, 2019

@newpavlov I was referring specifically to the BlockMode trait. Currently, in order to use a different IV for each encryption, you have to reconstruct the cipher object which results in poor performance. This encourages the reuse of IVs.

Instead, I think it's reasonable to have an API that allows you to specify IV for each encryption request. I believe mbedtls offers a similar API.

@tarcieri
Copy link
Member

tarcieri commented Oct 4, 2019

@akhilles I'd be all about finding misuse resistant nonce management strategies at the abstraction level of AEAD crates, but at the level of block ciphers, concerns like this ring hollow to me:

Currently, in order to use a different IV for each encryption, you have to reconstruct the cipher object which results in poor performance. This encourages the reuse of IVs.

Perhaps we haven't found a way to position block mode crates correctly, but here's how I would advertise them:

  • WARNING: Dangerous "hazmat" materials with "subtle" properties
  • Prone to explode in your face
  • Really you should use AEADs if you want safety

@akhilles
Copy link
Contributor

akhilles commented Oct 4, 2019

@tarcieri AES-CBC w/o auth tag and w/o reusing IVs is a perfectly legitimate (and common) use case. There is no API that supports it in this project.

What are the downsides to allowing IV to be provided in the encryption call?

@newpavlov
Copy link
Member Author

@akhilles
I am not sure if you proposal will work well with the stateful approach which we use for block modes API (see encrypt_blocks and decrypt_blocks methods). I think FromBlockCipher trait should work fine here. For example, instead of:

type AesCbc = Cbc<Aes128>;
let cipher = AesCbc::new(key);
let res1 = cipher.encrypt(buf1, pos1, iv1)?;
let res2 = cipher.encrypt(buf2, pos2, iv2)?;

You will write:

let cipher = Aes128::new(key);
let res1 = Cbc::from_cipher(cipher, iv1).encrypt(buf1, pos1)?;
let res2 = Cbc::from_cipher(cipher, iv2).encrypt(buf2, pos2)?;

It's a bit more verbose, but efficiency-wise it will be equivalent. Also it will be more flexible, since encrypt_blocks and decrypt_blocks will be left intact.

@akhilles
Copy link
Contributor

akhilles commented Oct 5, 2019

@newpavlov, I'd be perfectly happy with FromBlockCipher. I just wanted a way to change IV without going through round key generation.

Is there a PR open for the FromBlockCipher trait or is it still in the design phase?

@shepmaster
Copy link

  • Update generic-array to v0.13.

Since it's a breaking change comment

I don't know enough about the changes in generic-array 0.12 -> 0.13, but please remember that you can use version ranges if this crate doesn't use any changed APIs. For example, xxhash can work with basically any version of rand because it only uses rand::{thread_rng, Rng}.

@AxelNennker
Copy link

Can I bribe one of RustCrypto Developers with a beer next time somebody is in Berlin, Germany to do the cargo publish? Sorry, if this offer is not appropriate.

@tarcieri
Copy link
Member

Unfortunately I don't have access to release new crate versions, but in the meantime we can do the work enumerated in this issue

@frol
Copy link

frol commented Apr 23, 2020

@newpavlov Is there a chance to have some progress here? 👀

@tarcieri
Copy link
Member

I reached out to @newpavlov a few times now but haven't been able to get ahold of him.

@tarcieri
Copy link
Member

For people interested in new crate releases, I've made a contingency plan for publishing new versions provided @newpavlov doesn't return to do so: #102

It involves picking new names for each of the crates, which is less than ideal. I'm curious what people have to think about this approach.

@kpcyrd
Copy link

kpcyrd commented May 10, 2020

This would cause significant load on the debian-rust team because we need to get each package re-approved. Preferably crates that are this central in the rust eco system would have multiple maintainers with publish privileges.

Maybe this is something we can raise awareness about, collect instructions on how to find backup maintainers that are trusted in the community and encourage people to have multiple people with publish permissions (and/or permissions to assign more uploaders).

@tarcieri
Copy link
Member

@kpcyrd we were in the progress of migrating all of these crates to teams-based permissions rather than individual publishers, but that work was never completed, and at present @newpavlov has sole access to many of the crates.

I'm not in a hurry to do any of this so I'll wait until at least Q3 2020 before actually thinking about moving forward with it.

@AxelNennker
Copy link

would you please consider doing
"Release crypto-mac v0.8 which uses subtle v2. (see #35)"?

@tarcieri
Copy link
Member

tarcieri commented Jun 2, 2020

@AxelNennker we're getting ready to do a release of all of these crates. Hopefully later this week.

One remaining item I think it's worth considering is moving all the crates closer to Initialize-Update-Finalize (IUF) nomenclature (including crypto-mac) by renaming the result() method to finalize() or finish().

Several crates (including MAC implementations) use the finalize() name internally.

I'll go ahead and add a checkbox for this at the top.

@tarcieri
Copy link
Member

tarcieri commented Jun 2, 2020

@newpavlov are there any additional items you think are showstoppers for another release?

@tarcieri
Copy link
Member

tarcieri commented Jun 2, 2020

I took a look at renaming result => finalize and it seems there are many more cases that would need to be updated for consistency, namely:

https://docs.rs/digest/0.8.1/digest/trait.FixedOutput.html
https://docs.rs/digest/0.8.1/digest/trait.ExtendableOutput.html
https://docs.rs/digest/0.8.1/digest/trait.VariableOutput.html

tarcieri added a commit that referenced this issue Jun 2, 2020
As discussed in #43, this adopts `finalize` naming, following the
"Initialize-Update-Finalize" (IUF) interface naming scheme pervasive
throughout cryptography and cryptographic API design.
@tarcieri
Copy link
Member

tarcieri commented Jun 2, 2020

I opened #161 which renames the result methods to finalize.

If that looks good to everyone, I'll do another pass over all of the implementations to update them.

After that there are a few small remaining things I'd like to clean up, but that said, I think we're good for a release of many of the crates later this week.

@newpavlov
Copy link
Member Author

It would be nice to do the digest::Reset rework and solve the buffer-to-buffer issue. Otherwise at the moment I don't have any API changes in mind not listed in the OP.

@tarcieri
Copy link
Member

tarcieri commented Jun 2, 2020

#31 (buffer-to-buffer cipher operations) seems a bit involved and like it could use some more discussion and exploration of possible API designs. I'd rather punt on it for this release so it doesn't hold the releases of other crates back.

I can take a look at implementing the FixedOutput changes proposed in RustCrypto/hashes#86

@tarcieri
Copy link
Member

tarcieri commented Jun 3, 2020

I took a look at RustCrypto/hashes#86 and I'm a little bit confused by it (particularly _fixed_result_core).

I wonder if perhaps another trait and a blanket impl are in order?

@newpavlov
Copy link
Member Author

The idea is that we use a quasi-private method, which could leave hash instance in an incorrect state (well, strictly speaking it's still correct, but should not be used in practice) by passing false to reset. I will try to prepare PRs with those changes. I don't think we need an additional trait here, since it can be applied to all current implementors of FixedOutput directly and I think should be applicable for potential hardware-based solutions as well.

@tarcieri
Copy link
Member

tarcieri commented Jun 3, 2020

Sounds good.

In the meantime, I think the following crates can be published:

  • aead v0.3.0
  • block-cipher v0.7.0 (I kept the versioning consistent with block-cipher-trait)
  • crypto-mac v0.8.0
  • stream-cipher v0.4.0
  • universal-hash v0.4.0

(signature can also get a bump, but it depends on digest)

I can go ahead and put together CHANGELOG.mds for these.

@newpavlov
Copy link
Member Author

Sounds good!

@tarcieri
Copy link
Member

tarcieri commented Jun 4, 2020

I've released new versions of the aead, block-cipher, crypto-mac, stream-cipher, and universal-hash crates as outlined above.

@tarcieri
Copy link
Member

tarcieri commented Jun 9, 2020

I took a stab at addressing RustCrypto/hashes#86 in this PR:

https://github.com/RustCrypto/traits/pull/180/files

It's slightly different from what @newpavlov originally proposed, but eliminates the need to have an "internal" method on the trait.

@burdges
Copy link

burdges commented Jun 9, 2020

Also rust-lang/rfcs#2884

@tarcieri
Copy link
Member

tarcieri commented Jun 9, 2020

@burdges cool, nice to see progress is being made on RVO

@tarcieri
Copy link
Member

tarcieri commented Jun 9, 2020

Alright, with #183 and #184 landed I think we're ready to cut a new release of digest.

I'll go ahead and update https://github.com/RustCrypto/hashes first to make sure there's nothing we overlooked.

@shepmaster
Copy link

For future reference, switching from a generic to impl trait is generally a step back, as there's no way to use a turbofish with impl trait.

@tarcieri
Copy link
Member

tarcieri commented Jun 11, 2020

@shepmaster did you actually take a look at the relevant changes? They're all AsRef<[u8]> coercions. For these particular use cases, impl Trait is much more readable, and I don't think there's any potential turbofish use cases, so I don't think your comment is applicable.

@shepmaster
Copy link

I am upgrading a crate to support 0.9, so, yes, I am looking at the changes. My comment is applicable because you cannot know all the call sites. Maybe you made the decision on purpose and with full knowledge of the effects, I don't know. All I can do is transfer knowledge in case you didn't already know.

@tarcieri
Copy link
Member

For these particular cases, impl Trait in the argument position should be syntactic sugar for the generic parameter. If you have a use case where that isn’t so, I’d be curious to see it, however the only case I can think of is one where you were turbofishing a generic parameter which is now no-longer necessary. That said, I’d be curious to know specific examples where this is actually problematic from the caller’s side.

pkgw added a commit to tectonic-typesetting/tectonic that referenced this issue Jun 16, 2020
Cf. RustCrypto/traits#43 . I believe that we end up
re-exporting these APIs, so this is could be a breaking change for API
consumers, although it's not an API that we expect many people to be using
themselves.
smacfarlane added a commit to habitat-sh/builder that referenced this issue Jul 2, 2020
As part of the update to 0.9.0, the RustCrypto suite of crates has changed some function names in their public api:
RustCrypto/traits#43
RustCrypto/hashes#157

This commit updates to the 0.9.x version of the sha2 crate from that suite and changes function calls as appropriate.

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>
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

No branches or pull requests

9 participants