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

Update Dependencies #10

Closed
wants to merge 1 commit into from
Closed

Conversation

bhgomes
Copy link

@bhgomes bhgomes commented Sep 18, 2021

  • Updates dependencies to latest version
  • Fixes clippy issues
  • Fixes cargo build --no-default-features --features="alloc" issue (Vec was not imported)

@bhgomes
Copy link
Author

bhgomes commented Sep 18, 2021

Unfortunately, this does seem to break the encryption tests, although not the decryption ones, not sure what's up with that.

@fadeevab
Copy link
Owner

@bhgomes Hi! Thank you for the PR, I'll check it out.

@fadeevab
Copy link
Owner

@bhgomes This #11 should fix your issue. Alas, coverall.io is down the whole day, thus the build fails on the coverage check.

Is there any particular reason to upgrade cryptographic dependencies? I think it might cause the encryption test to fail (I suspect a difference in random seed generation).

@bhgomes
Copy link
Author

bhgomes commented Sep 18, 2021

Thanks for reviewing! The reason I wanted to upgrade was to try and get crates like zeroize and rand up to the latest versions to make cocoon compatible with other crates that use those two crates. rand is particularly necessary since cocoon exposes RngCore, CryptoRng, StdRng, and ThreadRng in its public API. Using the very old versions of the crypto crates didn't allow me to upgrade those dependencies.

Also, I think it's generally unwise to use super old versions of cryptography crates, especially ones that have deprecated features. If there's no way to fix the seeding changes (not sure if it's possible since StdRng was changed from ChaCha20 to ChaCha12 for example) then a semver bump is necessary to keep old cocoon users compatible without breaking their encryption.

@fadeevab
Copy link
Owner

since cocoon exposes RngCore, CryptoRng, StdRng, and ThreadRng in its public API.

Oh, right, what a mistake from my side, I should've hidden RNG inside. Hm... Let me publish a fix of the build first. Then I'm going to create a version 0.3.0 with incompatible API (without RNG templated parameter). In fact, most users wouldn't see any problem unless they specified templated Cocoon explicitly in their code (e.g. Cocoon<'a, StdRng, Creation>).

What do you think?

@bhgomes
Copy link
Author

bhgomes commented Sep 18, 2021

I think that's definitely a good idea. Not sure if that will fix packages that use cocoon and still want to use the latest version of rand and zeroize, I would have to run some tests. But that at least is a good issue to fix.

Any reason other than the breaking tests why you would not want to upgrade to the latest versions for crypto packages?

BTW: The cargo-outdated tool is pretty invaluable in discovering issues like compatibility with newer versions of packages.

@fadeevab
Copy link
Owner

@bhgomes Once it becomes compatible for you (with the latest rand) it becomes incompatible for others (in a case when somebody uses the current rand). So I would make cocoon incompatible for many things altogether in a new version. Thanks for cargo-outdated.

@bhgomes
Copy link
Author

bhgomes commented Sep 18, 2021

I think to fix the dependency issues, since you are going to remove the rand dependencies from the API, we should transfer everything to rand_chacha::ChaCha20Rng that way we can update and still pass the encryption tests. Also, it's guaranteed to be the same into the future. After reviewing the code it seems to me that that's the only reason for the breaking changes.

@fadeevab
Copy link
Owner

Done: PR #12

@fadeevab fadeevab closed this Sep 19, 2021
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.

2 participants