pkg: add a package for hashing with Keccak with unit test

[adrianghc]

This pull request adds a package to support hashing with the Keccak algorithm that is the basis of SHA-3. While SHA-3 per the NIST standard uses a state width of 1600 bits, the code supported here uses a state width of 800 bits in order to increase hashing speed with low-powered devices. The package source is downloaded from my fork of the KeccakCodePackage repository. It can be built for the following architectures:

• native (x86)
• Cortex-M0, Cortex-M0+ (ARMv6-M)
• Cortex-M3, Cortex-M4, Cortex-M4F (ARMv7-M)

In addition to the package alone, this pull request adds unit tests for hashing with Keccak, placed under tests/unittests/tests-hashes along with the existing ones.

[miri64]

There is also a keccak import in #7881.

Also any reason why you only provided a PR against the release branch, but not master? New features usually only go into master and are then provided with the next release.

[miri64]

Please rebase upon master and check with #7881 if your implementations can be merged somehow.

[miri64]

Happy first contribution btw :-) 🎉

[adrianghc]

Pull request #7881 adds support for SHA-3 by directly adding the implementation as a module in sys whereas my pull request adds it as a package so I'm not sure how they could be merged as the approaches are somewhat opposite. However, it should be noted that while #7881 uses SHA-3 as per the NIST standard (i.e. with 1600 bit state), my PR imports a package where 800 bit state width is used, so under that perspective there is a place for both implementations perhaps?

And thanks. :)

I leave the rest of the review to people more knowledgeable in hashes.

[emmanuelsearch]

We should probably aim at a single PR which offers both NIST-standard SHA-3 and Keccak-800, based on Keccak imported as a package.

[emmanuelsearch]

@adrianghc please rebase on current master.

[kaspar030]

initial round

[mtausig]

I am putting my opinion on this PR already stated in #7881 here, too:

A good direction what this package would be, to make a generic "sponge crypto" package out of it.
Having one common implementation for the sponge construction, the ability to use multiple permutations (Keccaks of different width or other lightweight sponges like photon or quark) and then several functions with cryptographic applications of the sponge (hash, mac, rng, aead, xof, streamcipher).

[emmanuelsearch]

@adrianghc you still have 125 commits in your PR, most of them having nothing to do with this PR. Please fix.

[adrianghc]

@emmanuelsearch Forgive me if this is a stupid question, but how would I do this? I'm afraid my experience with Git has its limits.

[miri64]

@emmanuelsearch Forgive me if this is a stupid question, but how would I do this? I'm afraid my experience with Git has its limits.

@adrianghc if you want to, you can come by our office (we moved to 159 however) in the next few days and I can see if I can help you with that.

[adrianghc]

@miri64 Thank you very much for the offer, that sounds great. However, this would probably be best after all comments and requested changes here have been addressed and resolved, wouldn't it? Or wouldn't additional commits for those after the cleanup be a problem?

[miri64]

On a side note, I feel like 80 is too low. People can be expected to have wide screens these days, so fitting more than 80 characters should be no issue, and such a low character limit actually lessens readability when it forces awkward line breaks, e.g. in if-clauses or in function signatures.

That's why 80 is our soft limit and 100 the hard ;-).

[miri64]

Only micropython failing on nrf52dk. This is unrelated. Let's give it another spin without tests. @kaspar030 are you ok with this PR?

[adrianghc]

On a side note, I feel like 80 is too low. People can be expected to have wide screens these days, so fitting more than 80 characters should be no issue, and such a low character limit actually lessens readability when it forces awkward line breaks, e.g. in if-clauses or in function signatures.

That's why 80 is our soft limit and 100 the hard ;-).

Well, there are still warnings in Murdock when exceeding just the 80 character limit.

[miri64]

Well, there are still warnings in Murdock when exceeding just the 80 character limit.

Yes but warnings block your PR. See also discussion in #13400 :-).

[adrianghc]

Well, there are still warnings in Murdock when exceeding just the 80 character limit.

Yes but warnings block your PR. See also discussion in #13400 :-).

Then how exactly is there a 100 character limit if passing 80 already generates a warning?

[miri64]

Well, there are still warnings in Murdock when exceeding just the 80 character limit.

Yes but warnings block your PR. See also discussion in #13400 :-).

Then how exactly is there a 100 character limit if passing 80 already generates a warning?

See our coding conventions 80 characters is preferred, but it should never be longer than 100 characters. Disregarding that all Vera++ messages currently are warnings, as it is still more or less in a test phase: How do you model such a decision? Make 80 chars a warning and 100 chars an error. Right?

[adrianghc]

Well, there are still warnings in Murdock when exceeding just the 80 character limit.

Yes but warnings block your PR. See also discussion in #13400 :-).

Then how exactly is there a 100 character limit if passing 80 already generates a warning?

See our coding conventions 80 characters is preferred, but it should never be longer than 100 characters. Disregarding that all Vera++ messages currently are warnings, as it is still more or less in a test phase: How do you model such a decision? Make 80 chars a warning and 100 chars an error. Right?

I know about the coding conventions, my point is that if 80 characters already generates a warning and warnings block the PR, then the limit is effectively at 80 characters. But perhaps I misunderstood something. Also it seemed like my files were the only ones causing warnings for the 80 character limit, and I didn't want them to stand out like that.

[aabadie]

warnings block the PR

warnings in the static test should not block a PR. vera++ generates too many warnings so even if there are warnings it doesn't return in an error.

[miri64]

Also it seemed like my files were the only ones causing warnings for the 80 character limit, and I didn't want them to stand out like that

Vera++ also checks changed files on the CI, so they won't stick out in the overall picture ;-).

[adrianghc]

I guess it was all a misunderstanding then. I kind of regret changing all those lines to forcibly fit within 80 characters now. 😅 (All but two were already well within 100.)

[aabadie]

I kind of regret changing all those lines to forcibly fit within 80 characters now

If you'd use fixup commits instead of force-pushing all the time, you would have kept the possibility to use git to remove
these changes (and also simplify our review work)

[adrianghc]

I kind of regret changing all those lines to forcibly fit within 80 characters now

If you'd use fixup commits instead of force-pushing all the time, you would have kept the possibility to use git to remove
these changes (and also simplify our review work)

Well, I still have a backup of the old files, so I could still use those, but at this point I feel like maybe we could just get this over with. And to be honest with you, I didn't know about fixup commits. Please accept my apologies.

[aabadie]

I didn't know about fixup commits

They are described in the contributing document.

[adrianghc]

I didn't know about fixup commits

They are described in the contributing document.

Well, I didn't say I was blaming anyone but myself for not knowing about them.

[aabadie]

I have another question regarding the version used for this package (sorry for asking so late and close from an ACK :) ): the commit targeted by the package is bc592d1780a6b3204c26c6f872552ace8fc6fd65, created on december, 2017. So it's quite old and the project has evolved quite a bit since then. The codebase is now completely different. Are we trying to merge support for completely outdated code ?

[adrianghc]

Well, the PR is that old, too... I guess it could be merged in and if someone actually uses the Keccak package, then the package could be updated to the new codebase? This PR is a byproduct of my bachelor thesis from 2017 but right now I'm working on my masters thesis and I'm honestly not very interested in investing the work in updating a package that nobody may even end up using. Not to mention that with SHA-3 support now built in, as I've just found out, the purpose of this package becomes somewhat questionable. Basically what it offers vs built-in SHA-3 is Keccak-800 support, whereas SHA-3 uses Keccak-1600. Which isn't to say Keccak-800 support isn't interesting enough for inclusion but I'm not sure people are really going to use it, assuming they even know about it.

[Teufelchen1]

Marked as a draft. There seems to be little interest in merging this soon®. But the author stated that it might be worth looking at the PR when somebody else picks this topic up.