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

Implemented DES #2

Merged
merged 15 commits into from
Nov 17, 2017
Merged

Implemented DES #2

merged 15 commits into from
Nov 17, 2017

Conversation

gsingh93
Copy link
Contributor

This is an implementation of DES. The included benchmark gives 2 MB/s performance, I'm not sure if that's good or bad. Most of the trickiness with the implementation is the fact that the most significant bit is considered bit zero instead of the least significant bit. It's possible I did some unnecessary shifting/arithmetic to account for this.

Note that the crate name des is taken on crates.io, so we might have to change that name.

I'm planning on adding more test cases. I have a file of (input, key, output) tuples that I need to parse and turn into the correct format for tests used in this repo.

@gsingh93
Copy link
Contributor Author

Hmm, now that I think about it, it would probably be cleaner to just use big endian integers and work from the LSB instead of using little endian and working from the MSB. I'll experiment with that.

@newpavlov
Copy link
Member

Thank you!
I will review your PR on this weekend. Regarding performance, this paper gives DES 21 MB/s, so there is room for improvement. You can use perf and flamegraphs for finding hot spots, additionally try to play with #[inline(always)] and loop unrolling, they can significantly improve performance.

Also you can implement 3DES, not sure if it's better to use the separate crate for it or include in this one.

@newpavlov
Copy link
Member

newpavlov commented Jan 26, 2017

Also check this implementation (it uses rayon for parallelism, so it dependent on std), I will try to contact owner of this crate later to ask if he would like to cooperate with us.

@newpavlov
Copy link
Member

newpavlov commented Jan 29, 2017

I've updated your code with optimizations I borrowed from antoyo's des-rs, it makes code less readable, but significantly faster. It could be nice to add tests which will check optimized transformations against straightforward ones written by you. I did similar test in magma. Also I've moved subkeys generation into new(), so if state will be reused subkeys will not be generated again. I'll add benchmarks of cipher initialization later.

I haven't modified apply_sboxes, probably it can be optimized too.

Currently I am getting 17 MB/s without initialization. Meanwhile OpenSSL implementation gives approximately 50 MB/s, so there is still room for improvement.

EDIT: Forgot about functions inlining, using it gives 19 MB/s respectively.


[dependencies]
generic-array = "0.5"
block-cipher-trait = "0.2"
Copy link
Member

@newpavlov newpavlov Jan 29, 2017

Choose a reason for hiding this comment

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

You use old version of crates:

generic-array = "0.6"
block-cipher-trait = "0.3"

(Forgot to update blowfish)

}

impl BlockCipherFixKey for Des {
type KeySize = U8;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be U7?

fn encrypt_block(&self, input: &Block<U8>, output: &mut Block<U8>) {
let block = read_u64_be(input);
let res = self.encrypt(block);
write_u64_be(output, res);
Copy link
Member

Choose a reason for hiding this comment

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

I've added read_u64_be to byte-tools. Don't forget to run cargo update to get new version.

@antoyo
Copy link

antoyo commented Feb 18, 2017

I've been invited to participate to this discussion, so here I am :p .
I won't have much time to work on this, unfortunately, but I can give you access to the des crate on crates.io.
I don't mind adding the Apache license.

How can I give you a write access to the des crate?
Is it throught the crates.io website?
I cannot access it anymore: the login doesn't work for me in Firefox and the other browsers that I use.
I already had this issue in the past, but I don't remember what was the fix.
Is it related to third-party cookies (I tried accepting them)?

@newpavlov
Copy link
Member

@antoyo
Thank you for your participation!

How can I give you a write access to the des crate?

You can do it using cargo with this command: cargo owner --add newpavlov. More information about it available here.

I cannot access it anymore: the login doesn't work for me in Firefox and the other browsers that I use.

You don't need to be authenticated on crates.io if your cargo token is intact. Your problems with cargo.io could be because of privacy settings, as it uses github for authentication. As a backup plan you can contact Alex Crichton.

@antoyo
Copy link

antoyo commented Feb 18, 2017

I added you as an owner.
Thanks for your interest in my work.
I can answer any question about the implementation.

@newpavlov
Copy link
Member

newpavlov commented Feb 21, 2017

@antoyo
Thank you! I will publish new version after merge.

Meanwhile can you compare your S-box implementation with gsingh93's implementation? As I understand in the later transformed S-boxes are used to improve performance. Simple tests show that this implementation is a bit faster than yours, but maybe you know about additional optimizations which can be applied here?

@newpavlov newpavlov mentioned this pull request Apr 24, 2017
19 tasks
@newpavlov
Copy link
Member

Sorry it took so long!

I will merge this PR, although before publishing I'll need to make some changes.

@newpavlov newpavlov merged commit bafc264 into RustCrypto:master Nov 17, 2017
@newpavlov
Copy link
Member

BTW it looks like you had a bug in decryption part.

@newpavlov
Copy link
Member

@gsingh93
If you'll have time please take a look at #12.

@gsingh93
Copy link
Contributor Author

Ack. Fairly busy for a while, so if you or anyone can fix that would be great. Otherwise, I'll take a look when I have time.

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.

3 participants