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

Optimise and simplify hash, encrypt and decrypt functions #13

Merged
merged 6 commits into from
Jan 12, 2018
Merged

Optimise and simplify hash, encrypt and decrypt functions #13

merged 6 commits into from
Jan 12, 2018

Conversation

Pharap
Copy link
Contributor

@Pharap Pharap commented Jan 12, 2018

I applied the changes to both the original C++ version of cape and the C version of cape.
The C++ version definitely works but the C version might need verifying/tweaking since I don't know as much about the rules of C.

I also made some spelling corrections, corrected the specified max character limit and rewrote the comments in encrypt and decrypt to match the new behaviour.

This removes the operations that were cancelling each other out and reduces the algorithm to a single pass over the source instead of three passes.
@colinta
Copy link
Contributor

colinta commented Jan 12, 2018

I took a quick look, the changes to the C++ and C code look the same! I'll check the output now...

@Pharap
Copy link
Contributor Author

Pharap commented Jan 12, 2018

@colinta I'm reasonably confident they'll behave the same because I used the same code for both.
I'm more worried there's some obscure C rule that I don't know about that does something unexpected.

@gioblu
Copy link
Owner

gioblu commented Jan 12, 2018

Ciao @Pharap @colinta I have tested using the @Pharap version of Cape.h, but keeping master Cape_c and it works fine running 2 instances encrypting with Cape_c on one side, decrypting with Cape on the other, this is a good indication that all should be fine. I will try the contrary and then start comparing computation time. Thank you again @Pharap

@gioblu
Copy link
Owner

gioblu commented Jan 12, 2018

Testing with 10 character plaintext and a 10 characters key @Pharap Cape.h:
280 micros for encryption 172 for decryption
Master Cape.h:
636 micros for encryption 524 for decryption
👍 👍 👍 👍 👍

That is almost 2.3 times faster for encryption and 3 times faster for decryption!!

@colinta
Copy link
Contributor

colinta commented Jan 12, 2018

Awesome! My tests all came out the same, too. Well done!

Btw I also have a python port of the C++ code, I used it to generate my encrypted keyboard macros. I updated it to use @Pharap's code. https://gist.github.com/colinta/5fd618b6dc8b5ef19a4c55064d16bc18

@gioblu
Copy link
Owner

gioblu commented Jan 12, 2018

@colinta WOW! I didnt have the time to bring forward Cape implementation in other languages because I am working really hard full time on PJON. That could be really useful for other users, thank you. Do you think could have sense to provide the user with also the python implementation in the src dir? do you think could that non c / c++ file bother any compiler?

A separated repo could have sense although being Cape still unreleased in experimental phase could be more handy to be able to centralize them in a single repo for fast maintainance and update reasons across all architectures / programming languages. What do you think?

I will accept this pr 👍 thank you again @Pharap

@gioblu gioblu merged commit 2cc36e6 into gioblu:master Jan 12, 2018
@Pharap
Copy link
Contributor Author

Pharap commented Jan 12, 2018

do you think could that non c / c++ file bother any compiler?

I'm not sure, but I like the idea of keeping ports in a separate directory or a separate repo.

What do you think?

Although it might be easier to keep them all in one repo, this repo is supposed to be aimed at Arduino and other microcontrollers (i.e. it has platformio and Arduino IDE files), so I think a separate repo might be better, even if it means a bit of extra work.

If you do decide to start gathering versions of cape in other languages, I might be able to offer one or two.

thank you again @Pharap

No problem, glad I could provide some insight.

@gioblu
Copy link
Owner

gioblu commented Jan 12, 2018

Obviously would be cool to have a wide group of implementations, to make Cape really an useful tool, an implementation solely dedicated to Arduino platforms is near useless, a new mostly unkown crypto library like this implemented in many different programming languages could be instead extremely useful.

For sure it needs still some study before the next release, I hope to be able not to break its compatibility, but having you saved a lot of computation time we could consider to work a little more on the encryption scheme. What do you think @Pharap @colinta ?

About the repo, absolutely lets keep them separate if colinta is oriented to follow this through

@colinta
Copy link
Contributor

colinta commented Jan 12, 2018 via email

@Pharap
Copy link
Contributor Author

Pharap commented Jan 12, 2018

For sure it needs still some study before the next release, I hope to be able not to break its compatibility, but having you saved a lot of computation time we could consider to work a little more on the encryption scheme.

I think it would be better to release the current changes as 2.1.
The code is currently backwards compatible and any people using the library ought to be able to benefit from the increased speed and reduced code size as soon as possible.

If there are any security improvements to be made, they'd be breaking changes so the version would go up to 3.0 (assuming you're following a system similar to semver). Aside from the fact those improvements would take time to implement and test, you'd also need to give people plenty of warning so they can be prepared to switch and won't accidentally scramble their existing encrypted data.
(They'd have to decrypt and then re-encrypt anything.)

I’m happy to make another Pr!

@colinta, your python implementation would need updating before it's added anyway since it's still got the old comments.

@Pharap Pharap deleted the code-overhaul branch January 12, 2018 20:02
@gioblu
Copy link
Owner

gioblu commented Jan 12, 2018

CIao @Pharap I dont think master is backward compatible with 2.0. It will be probably 3.0, yes follow a semver like versioning generally.

@colinta I can take care of format and comments if that is needed. It would be nice to add an example too, I think it will be just ignored by Arduino IDE and not listed.

@Pharap
Copy link
Contributor Author

Pharap commented Jan 12, 2018

I dont think master is backward compatible with 2.0.

Why not?
All the same functions are there and it seems to be functionally equivalent.

I think it will be just ignored by Arduino IDE and not listed.

What do you mean by this?


By the way, I'm not sure Cape is actually registered as an IDE library since I can't find it by searching.
I can think of one of two reasons, either A) you haven't requested that it be added to the library list or B) it can't be added because the version number isn't actually semver compliant (semver requires a 3-part Major.Minor.Build format rather than a 2-part Major.Minor).


Also, should the discussion of making another repo for different programming language versions be its own issue rather than being tacked on to this merged PR?

@gioblu
Copy link
Owner

gioblu commented Jan 12, 2018

Ciao @Pharap

I have worked to the scheme since 2.0 and master is not backward compatible, for example see decrypt here: https://github.com/gioblu/Cape/blob/2.0/src/Cape.h#L53 compared with master: https://github.com/gioblu/Cape/blob/master/src/Cape.h#L57

I mean that in other repos like PJON where I handle many different architectures accessing the same example directory, I have noticed that the Arduino IDE simply just ignores other text file extensions not .ino and the non compatible examples and their dirs are simply not listed, so I do not see as problematic to include python examples in the examples directory.

I still have not registered Cape as an IDE library to study more in particular its implementation and be sure to provide at least the coverage I expect.

Yes a dedicated issue for the different programming language versions is required I agree, I will open it

@gioblu
Copy link
Owner

gioblu commented Jan 13, 2018

@Pharap I will open an issue also related to the versioning semver minor omission.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants