code review 3069041: crypto/cipher: add package

crypto/cipher: add package

cipher is intended to replace crypto/block over time. This
change only adds basic parts: CBC and CTR mode and doesn't add
the package to the top-level Makefile.

[agl1, 2010-11-12 18:36:22]

Mailing this out for an early review of the design.

As opposed to the old 'block' package, 'cipher' exposes the cipher modes without
wrapping them in an io.Reader/Writer. This serves the more common case of
wishing to deal with small blocks of memory (i.e. TLS or SSH packets).

The streaming interface can be built on top of a BlockMode (i.e. as has been
done with the StreamReader/StreamWriter). In the end, 'cipher' should have a
BlockWriter which wraps a BlockMode and handles padding etc.

[r, 2010-11-12 19:15:01]

i'm going to leave this for rsc, who's back in a couple of days, because i'm not
sure about the tradeoffs.

[agl1, 2010-11-19 15:55:07]

On Fri, Nov 12, 2010 at 2:15 PM,  <r@golang.org> wrote:
> i'm going to leave this for rsc, who's back in a couple of days, because
> i'm not sure about the tradeoffs.

[+cc: rsc]

[rsc1, 2010-11-19 16:26:47]

seems like a good start to me

http://codereview.appspot.com/3069041/diff/6001/src/pkg/crypto/cipher/cbc.go
File src/pkg/crypto/cipher/cbc.go (right):

http://codereview.appspot.com/3069041/diff/6001/src/pkg/crypto/cipher/cbc.go#...
src/pkg/crypto/cipher/cbc.go:30: type CBCEncrypter cbc
please add doc comments for NewCBCDecrypter, NewCBCEncrypter.

if CBCEncrypter and CBCDecrypter stay they need doc comments too,
but i wonder if you could instead use

func NewCBCEncrypter(b Block, iv []byte) BlockMode

instead.  if there are no CBC-specific methods needed,
then returning BlockMode avoids exposing any 
implementation details and also avoids needing to
document the type.

http://codereview.appspot.com/3069041/diff/6001/src/pkg/crypto/cipher/cipher.go
File src/pkg/crypto/cipher/cipher.go (right):

http://codereview.appspot.com/3069041/diff/6001/src/pkg/crypto/cipher/cipher....
src/pkg/crypto/cipher/cipher.go:20: // Src and dst may point at the same memory.
"Dst and src" throughout.  (to match parameter arguments)

http://codereview.appspot.com/3069041/diff/6001/src/pkg/crypto/cipher/ctr.go
File src/pkg/crypto/cipher/ctr.go (right):

http://codereview.appspot.com/3069041/diff/6001/src/pkg/crypto/cipher/ctr.go#...
src/pkg/crypto/cipher/ctr.go:15: type CTR struct {
doc comments

if there are no CTR-specific methods my 
inclination is to hide ctr and use

func NewCTR(block Block, iv []byte) Stream

http://codereview.appspot.com/3069041/diff/6001/src/pkg/crypto/cipher/io.go
File src/pkg/crypto/cipher/io.go (right):

http://codereview.appspot.com/3069041/diff/6001/src/pkg/crypto/cipher/io.go#n...
src/pkg/crypto/cipher/io.go:34: Desynced bool
could make this Err os.Error and record the first error.

[agl1, 2010-11-19 16:51:59]

http://codereview.appspot.com/3069041/diff/6001/src/pkg/crypto/cipher/cbc.go
File src/pkg/crypto/cipher/cbc.go (right):

http://codereview.appspot.com/3069041/diff/6001/src/pkg/crypto/cipher/cbc.go#...
src/pkg/crypto/cipher/cbc.go:30: type CBCEncrypter cbc
On 2010/11/19 16:26:47, rsc1 wrote:
> please add doc comments for NewCBCDecrypter, NewCBCEncrypter.

done


> func NewCBCEncrypter(b Block, iv []byte) BlockMode

Done. The reason that I originally exposed a type is that I assumed that, if
other code held a CBCEncrypter directly, then calls to it would be direct rather
than via the interface. But I'm probably sacrificing too much for small
speedups.

http://codereview.appspot.com/3069041/diff/6001/src/pkg/crypto/cipher/cipher.go
File src/pkg/crypto/cipher/cipher.go (right):

http://codereview.appspot.com/3069041/diff/6001/src/pkg/crypto/cipher/cipher....
src/pkg/crypto/cipher/cipher.go:20: // Src and dst may point at the same memory.
On 2010/11/19 16:26:47, rsc1 wrote:
> "Dst and src" throughout.  (to match parameter arguments)
> 

Done.

http://codereview.appspot.com/3069041/diff/6001/src/pkg/crypto/cipher/ctr.go
File src/pkg/crypto/cipher/ctr.go (right):

http://codereview.appspot.com/3069041/diff/6001/src/pkg/crypto/cipher/ctr.go#...
src/pkg/crypto/cipher/ctr.go:15: type CTR struct {
On 2010/11/19 16:26:47, rsc1 wrote:
> doc comments
> 
> if there are no CTR-specific methods my 
> inclination is to hide ctr and use
> 
> func NewCTR(block Block, iv []byte) Stream

Done. (Same reasoning as the CBC case.)

http://codereview.appspot.com/3069041/diff/6001/src/pkg/crypto/cipher/io.go
File src/pkg/crypto/cipher/io.go (right):

http://codereview.appspot.com/3069041/diff/6001/src/pkg/crypto/cipher/io.go#n...
src/pkg/crypto/cipher/io.go:34: Desynced bool
On 2010/11/19 16:26:47, rsc1 wrote:
> could make this Err os.Error and record the first error.

Done.

[rsc1, 2010-11-19 17:31:22]

http://codereview.appspot.com/3069041/diff/25001/src/pkg/crypto/cipher/io.go
File src/pkg/crypto/cipher/io.go (right):

http://codereview.appspot.com/3069041/diff/25001/src/pkg/crypto/cipher/io.go#...
src/pkg/crypto/cipher/io.go:34: Desynced os.Error
I was thinking this could expose the actual
error instead of the desynced error.

// If any Write call returns an error, then the
// StreamWriter is out of sync and must be discarded.

and then in Write

if w.Err != nil {
    return 0, w.Err
}
...
n, err = w.W.Write(c)
if err == nil && n != len(c) {  // should not happen
    err = io.ErrShortWrite
}
if err != nil {
    w.Err = err
}
return

[agl1, 2010-11-19 18:01:09]

http://codereview.appspot.com/3069041/diff/25001/src/pkg/crypto/cipher/io.go
File src/pkg/crypto/cipher/io.go (right):

http://codereview.appspot.com/3069041/diff/25001/src/pkg/crypto/cipher/io.go#...
src/pkg/crypto/cipher/io.go:34: Desynced os.Error
On 2010/11/19 17:31:22, rsc1 wrote:
> I was thinking this could expose the actual
> error instead of the desynced error.

Oh, sure. Done.

[rsc1, 2010-11-19 18:08:46]

LGTM

[agl1, 2010-11-19 19:11:42]

Hello r, rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[agl1, 2010-11-19 19:12:27]

*** Submitted as http://code.google.com/p/go/source/detail?r=6b04915a1cf6 ***

crypto/cipher: add package

cipher is intended to replace crypto/block over time. This
change only adds basic parts: CBC and CTR mode and doesn't add
the package to the top-level Makefile.

R=r, rsc
CC=golang-dev
http://codereview.appspot.com/3069041

[agl1, 2010-11-19 21:18:45]

Submitted.