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

rc6: RC6 Implementation #371

Closed
wants to merge 16 commits into from

Conversation

ashWhiteHat
Copy link

Part of #1

Hi there.
I implemented rc6.

Implementation and documentation are aligned with rc5 implementation.

I referred following document for key schedule, encryption and decryption algorithms.
https://www.grc.com/r&d/rc6.pdf

I checked whether it works by following test vectors.
https://datatracker.ietf.org/doc/html/draft-krovetz-rc6-rc5-vectors-00#section-3

I would appreciate it if you could confirm.
Thank you!

@ghost
Copy link

ghost commented Aug 27, 2023

+1

RustCrypto apparently already owns the crate "rc6" and just isn't doing anything with it, so not merging this seems somewhat silly.

@tarcieri
Copy link
Member

tarcieri commented Aug 27, 2023

The reason this hasn't been merged yet is because it hasn't yet been code reviewed, which is especially important for cryptographic code.

Please give us time to review it.

If you'd like to be helpful, it would be good to get some feedback on if it worked in a particular application you're interested in.

@ghost
Copy link

ghost commented Aug 27, 2023

The reason this hasn't been merged yet is because it hasn't yet been code reviewed, which is especially important for cryptographic code.

Please give us time to review it.

Oh, yeah, 100%. I apologize if I came off as impatient. My intent was to express interest in the feature, not to rush you guys.

If you'd like to be helpful, it would be good to get some feedback on if it worked in a particular application you're interested in.

I am planning on using it in an application soon, hence my original comment. I'll report back with my results.

@ghost
Copy link

ghost commented Aug 27, 2023

The RC6 standard doesn't mandate any key size, round count, or word length, but this implementation does. RustCrypto's RC5 implementation has a similar problem which I've opened an issue for (#381). RC5 and RC6 are very similar so most of what I wrote there should be applicable here.

@tarcieri
Copy link
Member

The test failures look unrelated.

We'd still be interested in this if you could rebase!

@ashWhiteHat
Copy link
Author

Hi @tarcieri
Thank you for the notification.

I rebased and fixed conflict.
I would appreciate it if you could confirm.
Thank you!

categories = ["cryptography"]

[dependencies]
cipher = { version = "0.4.3", features = ["zeroize"] }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cipher = { version = "0.4.3", features = ["zeroize"] }
cipher = { version = "0.5.0-pre.6", features = ["zeroize"] }

cipher = { version = "0.4.3", features = ["zeroize"] }

[dev-dependencies]
cipher = { version = "0.4.3", features = ["dev"] }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cipher = { version = "0.4.3", features = ["dev"] }
cipher = { version = "0.5.0-pre.6", features = ["dev"] }

@ashWhiteHat
Copy link
Author

This includes rc5 refactoring and it's hard to rebase.
I close this PR.

@tarcieri
Copy link
Member

@ashWhiteHat can you open a new PR with just the rc6 implementation?

@ashWhiteHat
Copy link
Author

Copy that.
Probably in weekend.

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