Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(540)

Issue 130950043: code review 130950043: go.crypto/sha3: update to sync with draft FIPS-202

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 7 months ago by coruus
Modified:
10 years, 6 months ago
Reviewers:
CC:
golang-codereviews, agl2, agl1
Visibility:
Public.

Description

go.crypto/sha3: update to sync with draft FIPS-202 1. API: This exposes a minimal API: the SHA-3 functions implement hash.Hash. The SHAKE functions implement a new "ShakeHash" interface that implements io.Reader, io.Writer, Reset(), and a Clone() function. (The previous Barrier() function has been removed.) In addition, go.crypto/sha3 exposes a KeccakF1600 function, to allow users building other primitives on top of KeccakF-1600 to benefit from (future) optimized implementations of the permutation. 2. Tests Added the complete set of ShortMsgKATs from https://github.com/gvanas/KeccakCodePackage 3. Correctness In sync with draft FIPS-202. 4. Documentation A summary of the security properties of the SHA-3 and SHAKE functions is provided in doc.go; some concrete recommendations as well. Fixes 8563.

Patch Set 1 #

Patch Set 2 : diff -r f3cebac2bd11c2bf03c15bfa4c66688ad02a0a40 https://code.google.com/p/go.crypto #

Total comments: 10

Patch Set 3 : diff -r f3cebac2bd11c2bf03c15bfa4c66688ad02a0a40 https://code.google.com/p/go.crypto #

Total comments: 1

Patch Set 4 : diff -r f3cebac2bd11c2bf03c15bfa4c66688ad02a0a40 https://code.google.com/p/go.crypto #

Patch Set 5 : diff -r f3cebac2bd11c2bf03c15bfa4c66688ad02a0a40 https://code.google.com/p/go.crypto #

Patch Set 6 : diff -r f3cebac2bd11c2bf03c15bfa4c66688ad02a0a40 https://code.google.com/p/go.crypto #

Total comments: 43

Patch Set 7 : diff -r 00a7d3b31bbab5795b4a51933c04fc2768242970 https://code.google.com/p/go.crypto #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+515 lines, -336 lines) Patch
A sha3/doc.go View 1 2 3 4 5 6 1 chunk +72 lines, -0 lines 0 comments Download
A sha3/keccakKats.json.deflate View 1 2 Binary file 0 comments Download
M sha3/keccakf.go View 1 2 3 4 5 6 2 chunks +5 lines, -11 lines 0 comments Download
M sha3/sha3.go View 1 2 3 4 5 6 2 chunks +164 lines, -167 lines 1 comment Download
M sha3/sha3_test.go View 1 2 3 4 5 6 5 chunks +135 lines, -158 lines 0 comments Download
A sha3/sha3f202.go View 1 2 3 4 5 6 1 chunk +75 lines, -0 lines 0 comments Download
A sha3/shake.go View 1 2 3 4 5 6 1 chunk +64 lines, -0 lines 1 comment Download

Messages

Total messages: 30
coruus
Hello golang-codereviews@googlegroups.com (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.crypto
10 years, 7 months ago (2014-08-20 04:24:06 UTC) #1
coruus
As a note, the JSON file with the processed Keccak Team KATs is available at: ...
10 years, 7 months ago (2014-08-20 04:28:29 UTC) #2
agl2
R=agl
10 years, 6 months ago (2014-08-21 17:08:25 UTC) #3
agl1
I can't download the JSON file at the moment because of a poor connection. How ...
10 years, 6 months ago (2014-08-21 17:23:14 UTC) #4
agl1
I can't download the JSON file at the moment because of a poor connection. How ...
10 years, 6 months ago (2014-08-21 17:23:15 UTC) #5
agl1
p.s. add "Fixes 8563." to the end of the description.
10 years, 6 months ago (2014-08-21 17:24:53 UTC) #6
agl1
p.s. add "Fixes 8563." to the end of the description.
10 years, 6 months ago (2014-08-21 17:24:56 UTC) #7
coruus
On 2014/08/21 17:23:15, agl1 wrote: > I can't download the JSON file at the moment ...
10 years, 6 months ago (2014-08-21 17:29:52 UTC) #8
agl2
On Thu, Aug 21, 2014 at 10:29 AM, <coruus@gmail.com> wrote: > 1.1 MiB. Will do; ...
10 years, 6 months ago (2014-08-21 18:09:07 UTC) #9
coruus
On 2014/08/21 17:23:14, agl1 wrote: > I can't download the JSON file at the moment ...
10 years, 6 months ago (2014-08-21 18:38:36 UTC) #10
coruus
https://codereview.appspot.com/130950043/diff/40001/sha3/sha3.go File sha3/sha3.go (left): https://codereview.appspot.com/130950043/diff/40001/sha3/sha3.go#oldcode1 sha3/sha3.go:1: // Copyright 2013 The Go Authors. All rights reserved. ...
10 years, 6 months ago (2014-08-21 18:42:19 UTC) #11
coruus
On 2014/08/21 18:09:07, agl2 wrote: > On Thu, Aug 21, 2014 at 10:29 AM, <mailto:coruus@gmail.com> ...
10 years, 6 months ago (2014-08-21 20:05:29 UTC) #12
agl1
Higher level comments: The interface is very broad and non-standard. Interface is something that we ...
10 years, 6 months ago (2014-08-22 18:49:28 UTC) #13
coruus
> Higher level comments: > Does it make sense to fit the Shake functions into ...
10 years, 6 months ago (2014-08-22 21:36:03 UTC) #14
agl2
(p.s. please ping when there's a code update pending to review.) On the the problems ...
10 years, 6 months ago (2014-08-27 18:50:18 UTC) #15
coruus
On 2014/08/27 18:50:18, agl2 wrote: > (p.s. please ping when there's a code update pending ...
10 years, 6 months ago (2014-08-27 19:38:18 UTC) #16
coruus
Hello golang-codereviews@googlegroups.com, agl@google.com, agl@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
10 years, 6 months ago (2014-08-28 18:56:46 UTC) #17
coruus
On 2014/08/28 18:56:46, coruus wrote: > Hello mailto:golang-codereviews@googlegroups.com, mailto:agl@google.com, mailto:agl@golang.org (cc: > mailto:golang-codereviews@googlegroups.com), > > ...
10 years, 6 months ago (2014-08-28 20:45:24 UTC) #18
agl1
https://codereview.appspot.com/130950043/diff/250001/sha3/doc.go File sha3/doc.go (right): https://codereview.appspot.com/130950043/diff/250001/sha3/doc.go#newcode15 sha3/doc.go:15: // If you aren't sure what function you need, ...
10 years, 6 months ago (2014-09-02 23:40:28 UTC) #19
coruus
https://codereview.appspot.com/130950043/diff/250001/sha3/doc.go File sha3/doc.go (right): https://codereview.appspot.com/130950043/diff/250001/sha3/doc.go#newcode15 sha3/doc.go:15: // If you aren't sure what function you need, ...
10 years, 6 months ago (2014-09-03 01:17:28 UTC) #20
coruus
10 years, 6 months ago (2014-09-03 01:17:29 UTC) #21
agl1
I made some more changes prior to landing but discovered that I didn't understand the ...
10 years, 6 months ago (2014-09-03 02:50:09 UTC) #22
coruus
On 2014/09/03 02:50:09, agl1 wrote: > I made some more changes prior to landing but ...
10 years, 6 months ago (2014-09-03 03:28:58 UTC) #23
coruus
In fact, I just remembered where I had a C function to print a little-endian ...
10 years, 6 months ago (2014-09-03 03:55:37 UTC) #24
coruus
https://codereview.appspot.com/130950043/diff/270001/sha3/shake.go File sha3/shake.go (right): https://codereview.appspot.com/130950043/diff/270001/sha3/shake.go#newcode19 sha3/shake.go:19: // error is input is written to it after ...
10 years, 6 months ago (2014-09-03 03:58:09 UTC) #25
agl1
https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go File sha3/sha3.go (right): https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode148 sha3/sha3.go:148: // QQQQ: Should this be an error? (This is ...
10 years, 6 months ago (2014-09-03 18:58:29 UTC) #26
agl1
Thanks for the explanation, that makes sense once I realise that the "| 01" notation ...
10 years, 6 months ago (2014-09-03 19:01:42 UTC) #27
agl1
*** Submitted as https://code.google.com/p/go/source/detail?r=2ce75e338fde&repo=crypto *** go.crypto/sha3: update to sync with draft FIPS-202 1. API: This ...
10 years, 6 months ago (2014-09-03 19:04:13 UTC) #28
coruus
LGTM
10 years, 6 months ago (2014-09-09 21:36:06 UTC) #29
coruus
10 years, 6 months ago (2014-09-15 15:38:27 UTC) #30
On 2014/09/09 21:36:06, coruus wrote:
> LGTM

Just a quick note: The committed version has about a 5% worst-case performance
regression on the set of benchmarks in sha3_test.go. (I'll open a new issue with
some optimizations when I have time.)

The included benchmarks aren't particularly good; in particular, they don't test
either repeated updates, or the ShakeHash-specific interface. Using the SHAKE
instances is much faster, because it avoids the allocation/state-copy mandated
by the Sum interface.

(I'm writing this because it increases the probability that I'll actually
follow-up on this quickly.)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b