Issue 2687042: code review 2687042: Additional crypto library: Schneier's Twofish
|
Created:
14 years, 5 months ago by B-Ranger Modified:
1 year, 4 months ago Reviewers:
CC:
golang-dev, schulze Visibility:
Public. |
DescriptionAdditional crypto library: Schneier's Twofish
This library rounds of the set of already available go crypto libraries.
It fits into the existing interfaces and can be used in different block modes without any additional hassle.
I hope it will be added to the set of provided algorithms two offer an alternative
to the Rijandel (AES) algorithm.
Twofish is already widely used, e.g. by a lot of HDD encryption tools and should therefore be supported.
The algorithm was (as annoted in the source code) adopted from the FOSS librar Tom's LibCrypt. It was neither
optimised for speed nor any security features were added.
Patch Set 1 #Patch Set 2 : code review 2687042: Additional crypto library: Schneier's Twofish #
Total comments: 50
Patch Set 3 : code review 2687042: Additional crypto library: Schneier's Twofish #
Total comments: 22
Patch Set 4 : code review 2687042: Additional crypto library: Schneier's Twofish #Patch Set 5 : code review 2687042: Additional crypto library: Schneier's Twofish #
Total comments: 50
Patch Set 6 : code review 2687042: Additional crypto library: Schneier's Twofish #
Total comments: 11
Patch Set 7 : code review 2687042: Additional crypto library: Schneier's Twofish #
Total comments: 8
Patch Set 8 : code review 2687042: Additional crypto library: Schneier's Twofish #Patch Set 9 : code review 2687042: Additional crypto library: Schneier's Twofish #
MessagesTotal messages: 27
Hello golang-dev@googlegroups.com, rsc (cc: golang-dev@googlegroups.com, schulze@math.uni-hannover.de), I'd like you to review this change.
Sign in to reply to this message.
I haven't heard of your for a while. Since there was a change in the crypto implementation I'm tempted to adapt the code. But as I'm absolutely not familiar with the usual work flow I don't know if "not answering" is to be understand as "your issue is not interesting for us" or more like "keep waiting, we are coming back to you". So I'd appreciate any reaction to decide how to go on with this project.
Sign in to reply to this message.
Hello Just sit tight. Russ said today: "We're pretty far behind on code reviews, mainly due to vacations and focus on mind-numbing detail work like the cgo changes. My goal is to get caught up in mid December." Regards Albert
Sign in to reply to this message.
Thanks for submitting this. The code is not quite idiomatic Go yet. If you haven't already, I suggest reading http://golang.org/doc/effective_go.html for general notes. I've identified a handful of specific things below. http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... File src/pkg/crypto/twofish/twofish.go (right): http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:10: // It was heavily inspired by the go blowfish package s/$/./ also insert line above this one: // "LibTomCrypt is free for all purposes under the public domain." http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:23: const BlockSize = 16 // The Twofish block size in bytes. http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:28: /* A Cipher is an instance of Twofish encryption using a particular key. */ Please use // comments for consistency with the rest of the source tree http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:41: // The key argument should be the Twfish key, 16, 24 or 32 bytes. Twofish http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:48: var result Cipher let's call the cipher c. the fact that it's a cipher is more important than the fact that it's a return value http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:49: var tmpx0, tmpx1 byte delete declaration and use := below http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:50: var S [4 * 4]byte postpone declaration until use (say, right after the "create the S[..] words" comment) http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:51: var tmp [4]byte postpone declaration until use http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:52: tmp2 := make([]byte, 4) load32l should change, so this may not be necessary at all anymore, but in Go, if you say tmp2 := make([]byte, 4) you are setting tmp2 to the slice returned by make([]byte, 4). if you then say tmp2 = h_func(tmp[:], key, 0) you are setting tmp2 to the slice returned by h_func. the assignment does not copy the contents, so the original slice returned by make is discarded without ever being used. var tmp2 []byte would have sufficed here, but even better, drop the declaration entirely and wait until you need it below and use := http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:54: /* k = keysize/64 [but since our keysize is in bytes...] */ // k = # of 64 bit words in key http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:136: /* from here one you are looking definetifly under the hood */ delete http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:140: out = make([]byte, 4) this is allocating for every word of the input! you'd be much better off passing in the slice. that is, instead of copy(out, store32l(tmp)) use store32l(out, tmp) http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:142: out[i] = byte(tmp % 0x100) it amounts to the same thing but it is far more common in bit packing to use shifts and masks out[0] = byte(tmp) out[1] = byte(tmp>>8) out[2] = byte(tmp>>16) out[3] = byte(tmp>>24) http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:150: for i := 0; i < len(tmp); i++ { same here return uint32(src[0]) | uint32(src[1])<<8 | uint32(src[2])<<16 | uint32(src[3])<<24 http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:158: func rolc(x, y uint32) uint32 { the c is redundant with rotate. rol and ror is fine http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:172: [8]byte{0XA4, 0X55, 0X87, 0X5A, 0X58, 0XDB, 0X9E, 0X03}} please add a , to this line and move the } to the next same for the other tables http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:212: func gf_mult(a, b byte, p uint32) byte { go style is to use camelCase not under_scores. so gfMult etc http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:235: return load32l([]byte{x01, x5B, xEF, xEF}) this is allocating a 4-byte array just to turn 4 values into a uint32. just write the expression return uint32(x01) | uint32(x5B)<<8 | uint32(xEF)<<16 | uint32(xEF)<<24 also "xFF" is a confusing name for a variable. the first few times i read this code i though those were hex constants. mul01, mul5B, mulEF would be clearer. http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:249: func mds_mult(in []byte) []byte { You should take a close look at all the functions that return []byte in this file. I think your code would be much simpler if you just returned uint32 everywhere. In particular mds_mult returns []byte so that h_func returns []byte and then the caller of h_func just calls load32l to get the uint32 back! http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:258: func rs_mult(in, out []byte) { go libraries typically use dst, src. the crypto libraries were inconsistent but recently changed. http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:266: /* computes h(x) */ what is x? http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:267: func h_func(in, key []byte, offset int) []byte { s/h_func/h/ http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:268: var y [4]byte what you have is fine but it seems like it would be notationally even easier to use y0, y1, y2, y3 := x[0], x[1], x[2], x[3] and drop all the [] below http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:300: func (skey *Cipher) Encrypt(pt, ct []byte) { arguments should be named dst, src for consistency with other packages http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:301: var t1, t2 uint32 use := below http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:302: k := make([]uint32, 4) unused use k := below http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:304: if (pt == nil) || (ct == nil) || (skey == nil) { all () unnecessary http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:305: panic("Fehler, Key & sKey darf nicht null sein") english please also there's no need to check. these are use errors and you can just let them panic http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:325: b_bytes := store32l(b) this seems wasteful. just use the expressions below t2 := S2[byte(b)] ^ S3[byte(b>>8)] ^ S4[byte(b>>16)] ^ S1[byte(b>>24)] etc http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish.go:356: func (skey *Cipher) Decrypt(ct, pt []byte) { same comments as in Encrypt http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... File src/pkg/crypto/twofish/twofish_test.go (right): http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:1: // Performs a self-test of the Twofish block cipher return CRYPT_OK if copyright notice; see http://golang.org/doc/contribute.html#copyright http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:6: import ( sort imports http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:14: func hexprint(slice []byte) { fmt.Printf("% 02X\n", slice) would work too (at that point it might be worth using directly instead of defining hexprint). http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:23: func (buf buffer) Write(p []byte) (n int, err os.Error) { it's not exactly the same but i think using bytes.Buffer would work fine instead of buffer in this test http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:38: pt [16]byte if you make this []byte then the code below doesn't need [:] also please use enc and dec for encrypted and decrypted. the pt/ct abbreviations are not in common use in this tree. http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:43: tests := []test{ please make this a global. if you do that, then you can get rid of the test struct entirely. also you can elide some of the type names var twofishTests = []struct{ key []byte dec []byte enc []byte }{ { []byte{0x9f, 0x58, ...}, []byte{0xd4, ...}, []byte{0x01, ...}, }, ... } http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:49: 0x86, 0xCB, 0x08, 0x6B, 0x78, 0x9F, 0x54, 0x19}, please put multiline closing braces on next line http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:105: var tmp [2][]byte delay until use http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:113: for i := 0; i < len(tests); i++ { for i, tt := range twofishTests { then use tt instead of tests[i] everywhere http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:114: print("Test ", i, ": ") no printing please can give more information in the errors instead. http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:117: t.Errorf("Fehler in twofish_setup") english please also give information about what failed. t.Errorf("#%d: NewCipher: %v", i, err) http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:124: if bytes.Compare(myBuffer[0], tests[i].ct[:]) != 0 || bytes.Compare(myBuffer[1], tests[i].pt[:]) != 0 { if !bytes.Equal seems to match the intent better http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:125: println() no printing http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:126: t.Errorf("Fehler bei einfachem de- & encrypt Test") english please, and say what failed: var enc bytes.Buffer block.NewCBCEncrypter(key, &enc).Write(tt.dec) if !bytes.Equal(enc.Bytes(), tt.enc) { t.Errorf("#%d: encrypt = %x want %x", enc.Bytes(), tt.enc) } var dec bytes.Buffer if !bytes.Equal(dec.Bytes(), tt.dec) { t.Errorf("#%d: decrypt = %x want %x", dec.Bytes(), tt.dec) } http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:135: return delete; no reason to give up after first failure http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:140: for y := 0; y < 16; y++ { i is a more common loop variable http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:149: for y := 0; y < 16; y++ { not sure why tmp[0] is used here, since tmp[1] never is. also, give information in the failure: buf := make([]byte, 16) zero := make([]byte, 16) for i := 0; i < 1000; i++ { key.Encrypt(buf, buf) } for i := 0; i < 1000; i++ { key.Decrypt(buf, buf) } if !bytes.Equal(buf, zero) { t.Errorf("encrypt/decrypt 1000: have %x want %x", buf, zero) } http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:155: println("OK") no printing http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:157: println("CRYPT_OK") no printing http://codereview.appspot.com/2687042/diff/2001/src/pkg/crypto/twofish/twofis... src/pkg/crypto/twofish/twofish_test.go:159: return delete; unnecessary
Sign in to reply to this message.
Hello rsc, berengarlehr@googlemail.com, albert.strasheim (cc: golang-dev@googlegroups.com, schulze@math.uni-hannover.de), Please take another look.
Sign in to reply to this message.
Next patch set for the crypto/twofish library. Included are fixes for all of Ross' comments. Unfortunately the two-side-diff looks pretty ugly doe to the switch to tab instead of four spaces for indenting. twofish.go: a) Most comments where pretty straightforward. b) I took the liberty to remove mds_mult and rs_mult that where only referenced once and included them in the calling function. c) In order to purify the code the signature of sbox was changed from sbox(int, uint32) uint32 into sbox(int, byte) byte which saves some uin32-byte back-and-fourth casting. d) I was tempted to remove store32l/load32l completely but could resist. I hope, that was the right decision. e) What remains as a bellyache is the "Unrolled branchless GF multiplier" or gfMult which I was not able to translate into some understandable code. At least the description of this function as stated in LibTom to be equivalent to "ab mod p" is incomplete or wrong. test.go: a) I changed the buffer to bytes.Buffer as suggested which indeed removes a lot of unnecessary code. b) Due to the co-existence of crypto/cipher and crypto/block and the non-existence of the ECB mode in ../cipher I still had to use the old ../block structure. I think I learn a lot more stuff about go during the review and want to thank you Ross for your comments. My take home message is: You can always re-read the documentation.
Sign in to reply to this message.
I haven't looked closely at the new code yet. Adding agl for crypto. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... File src/pkg/crypto/twofish/twofish.go (right): http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:195: func sbox(i int, x byte) byte { I don't understand why this is a function. The qbox tables take 2*4*16 = 128 bytes, but it looks like i can only be 0 and 1 here, so computing and hard-coding the sbox as a table here would only be 512 bytes. And presumably a single index lookup would be dramatically faster than all this math. My suggestion: var sbox = [2][256]byte { ... table you compute and drop in here ... } and move qbox over into the test file. A new test TestSbox can compute what the array should be, using qbox, and complain if it's wrong.
Sign in to reply to this message.
(I only had 10 minutes so this is a very partial review. Will return to this later.) http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/Makefile File src/pkg/crypto/twofish/Makefile (right): http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/Makef... src/pkg/crypto/twofish/Makefile:1: # Copyright 2010 The Go Authors. All rights reserved. 2011 http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... File src/pkg/crypto/twofish/twofish.go (right): http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:1: // Copyright 2010 The Go Authors. All rights reserved. 2011 http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:18: // As defined in twofish.c and tomcrypt_cipher.h This comment seems a little out of place. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:135: // It is necessary to satisfy the Cipher interface in the package "crypto/block". Delete this line of the comment. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:139: func store32l(src uint32, dst []byte) { // store32l stores src into dst in little-endian form. Also, dst always comes first in Go code. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:147: // Convert [4]byte to uint32 // load32l reads a little-endian uint32 from src. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:152: // Rotate x circular left for y // rol returns x after a left circular rotation of y bits. (It's a shame that we can't use the CPU's support for this.) http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:157: // Rotate x circular right for y ditto with the comment. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:164: [8]byte{0x01, 0xA4, 0x55, 0x87, 0x5A, 0x58, 0xDB, 0x9E}, I don't think that the [8]byte is needed any more. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:172: [5]byte{1, 1, 0, 0, 1}, ditto with [5]byte. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:180: [4][16]byte{ ditto
Sign in to reply to this message.
Hello rsc, berengarlehr@googlemail.com, albert.strasheim, agl1 (cc: golang-dev@googlegroups.com, schulze@math.uni-hannover.de), Please take another look.
Sign in to reply to this message.
Hello rsc, berengarlehr@googlemail.com, albert.strasheim, agl1 (cc: golang-dev@googlegroups.com, schulze@math.uni-hannover.de), Please take another look.
Sign in to reply to this message.
Additional to the review comments I removed the tmpx0 and tmpx1 tmp-variable from NewCipher (Ls: 91f, 100f, 109f). I still would like to rewrite gfMult into a more expressive form but can't figure out what actually happens during all that bitshifting. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... File src/pkg/crypto/twofish/twofish.go (right): http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:1: // Copyright 2010 The Go Authors. All rights reserved. On 2011/01/08 16:21:58, agl1 wrote: > 2011 Done. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:18: // As defined in twofish.c and tomcrypt_cipher.h On 2011/01/08 16:21:58, agl1 wrote: > This comment seems a little out of place. Done. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:135: // It is necessary to satisfy the Cipher interface in the package "crypto/block". On 2011/01/08 16:21:58, agl1 wrote: > Delete this line of the comment. Done. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:139: func store32l(src uint32, dst []byte) { On 2011/01/08 16:21:58, agl1 wrote: > // store32l stores src into dst in little-endian form. > > Also, dst always comes first in Go code. Done. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:147: // Convert [4]byte to uint32 On 2011/01/08 16:21:58, agl1 wrote: > // load32l reads a little-endian uint32 from src. Done. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:152: // Rotate x circular left for y On 2011/01/08 16:21:58, agl1 wrote: > // rol returns x after a left circular rotation of y bits. > > (It's a shame that we can't use the CPU's support for this.) Done. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:157: // Rotate x circular right for y On 2011/01/08 16:21:58, agl1 wrote: > ditto with the comment. Done. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:164: [8]byte{0x01, 0xA4, 0x55, 0x87, 0x5A, 0x58, 0xDB, 0x9E}, On 2011/01/08 16:21:58, agl1 wrote: > I don't think that the [8]byte is needed any more. Done. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:172: [5]byte{1, 1, 0, 0, 1}, On 2011/01/08 16:21:58, agl1 wrote: > ditto with [5]byte. Actually, this variable is not used at all. I thought this would lead to a compiler error but I searched and didn't find it. So I removed this variable. http://codereview.appspot.com/2687042/diff/12001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:180: [4][16]byte{ On 2011/01/08 16:21:58, agl1 wrote: > ditto Done.
Sign in to reply to this message.
rsc: any last comments? http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... File src/pkg/crypto/twofish/twofish.go (right): http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:8: // The code is a port of the LibTom C implementation. // Twofish is defined in http://www.schneier.com/paper-twofish-paper.pdf [TWOFISH] // http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:24: // Two polynomials delete this comment http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:25: const mds_POLY = 0x169 const mdsPolynomial = 0x169 // x^8 + x^6 + x^5 + x^3 + 1, see [TWOFISH] 4.2 http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:26: const rs_POLY = 0x14D const rsPolynomial = 0x14d // x^8 + x^6 + x^3 + x^2 + 1, see [TWOFISH] 4.3 http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:149: // ror returns x after a right circular rotation of y bits. double space in here. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:154: // 4x8 rs linear transform // The RS matrix. See [TWOFISH] 4.3 http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:202: // ab mod p // gfMult returns a·b in GF(2^8)/p http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:235: // Avoid warnings, we'd never get here normally but just to calm compiler warnings... replace these two lines with panic("unreachable") http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:239: func h(in, key []byte, offset int) uint32 { // h implements the S-box generation function. See [TWOFISH] 4.3.5 http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:271: // Encrypt encrypts the 16-byte buffer src (I know that this was taken from aes/ and that package should also be cleaned up.) // Encrypt encrypts a 16-byte block from src to dst, which may overlap. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:277: func (skey *Cipher) Encrypt(src, dst []byte) { dst comes first. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:318: // Decrypt decrypts the 16-byte buffer ct // Decrypt decrypts a 16-byte block from src to dst, which may overlap. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:321: func (skey *Cipher) Decrypt(src, dst []byte) { dst comes first. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:352: // Pre-white // Undo pre-whitening http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... File src/pkg/crypto/twofish/twofish_test.go (right): http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:88: // Test if the sbox saved as variable containes the values as defined by it's function comments should be wrapped to 80 chars. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:98: // Test if the known plaintext (dec) is encrypted into the known crypttext (enc) ditto http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:117: // Test if 16 zero bytes can be encrypt 1000 times, decrypted and come ditto
Sign in to reply to this message.
http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... File src/pkg/crypto/twofish/twofish.go (right): http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:22: const BlockSize = 16 // BlockSize is the constant block size of Twofish. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:48: var c Cipher this could be moved down a few lines also it's going to be heap allocated and returned; might as well be clear about it c := new(Cipher) http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:50: // k = # of 64 bit words in key s/= #/is the number/ http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:127: // BlockSize returns the Twofish block size, 16 bytes. given this comment and the fact that the method must be exported, is there any reason to make the constant public too? (i'm asking agl) http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:130: // store32l stores src int dst in little-endian form. s/int/in/ http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:208: // Unrolled branchless GF multiplier usually when i see 'unrolled branchless' i don't expect the next statement to be a for loop http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:276: // instead, use an encryption mode like CBC (see crypto/block/cbc.go). this is a short ragged comment. godoc will present it as is; would be nice to format it better http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... File src/pkg/crypto/twofish/twofish_test.go (right): http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:41: // All test are designed for ECB mode s/test/tests/ http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:47: // This tests are extracted from LibTom s/This/These/ http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:88: // Test if the sbox saved as variable containes the values as defined by it's function s/containes/contains/ s/it's/its/ http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:88: // Test if the sbox saved as variable containes the values as defined by it's function On 2011/01/09 15:20:23, agl1 wrote: > comments should be wrapped to 80 chars. i think they're fine. the code is long anyway.
Sign in to reply to this message.
http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... File src/pkg/crypto/twofish/twofish.go (right): http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:127: // BlockSize returns the Twofish block size, 16 bytes. On 2011/01/09 17:40:00, r wrote: > given this comment and the fact that the method must be exported, is there any > reason to make the constant public too? (i'm asking agl) The public constant is for code which knows that it's using Twofish (or any specific cipher) so that it can say twofish.BlockSize. The method is for code which is generic over any cipher.
Sign in to reply to this message.
Hello rsc, berengarlehr@googlemail.com, albert.strasheim, agl1, r (cc: golang-dev@googlegroups.com, schulze@math.uni-hannover.de), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... File src/pkg/crypto/twofish/twofish.go (right): http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:8: // The code is a port of the LibTom C implementation. On 2011/01/09 15:20:23, agl1 wrote: > // Twofish is defined in http://www.schneier.com/paper-twofish-paper.pdf > [TWOFISH] > // Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:22: const BlockSize = 16 On 2011/01/09 17:40:00, r wrote: > // BlockSize is the constant block size of Twofish. Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:24: // Two polynomials On 2011/01/09 15:20:23, agl1 wrote: > delete this comment Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:25: const mds_POLY = 0x169 On 2011/01/09 15:20:23, agl1 wrote: > const mdsPolynomial = 0x169 // x^8 + x^6 + x^5 + x^3 + 1, see [TWOFISH] 4.2 Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:26: const rs_POLY = 0x14D On 2011/01/09 15:20:23, agl1 wrote: > const rsPolynomial = 0x14d // x^8 + x^6 + x^3 + x^2 + 1, see [TWOFISH] 4.3 Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:48: var c Cipher On 2011/01/09 17:40:00, r wrote: > this could be moved down a few lines > also it's going to be heap allocated and returned; might as well be clear about > it > c := new(Cipher) Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:50: // k = # of 64 bit words in key On 2011/01/09 17:40:00, r wrote: > s/= #/is the number/ Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:130: // store32l stores src int dst in little-endian form. On 2011/01/09 17:40:00, r wrote: > s/int/in/ Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:149: // ror returns x after a right circular rotation of y bits. On 2011/01/09 15:20:23, agl1 wrote: > double space in here. Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:154: // 4x8 rs linear transform On 2011/01/09 15:20:23, agl1 wrote: > // The RS matrix. See [TWOFISH] 4.3 Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:202: // ab mod p On 2011/01/09 15:20:23, agl1 wrote: > // gfMult returns a·b in GF(2^8)/p Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:208: // Unrolled branchless GF multiplier On 2011/01/09 17:40:00, r wrote: > usually when i see 'unrolled branchless' i don't expect the next statement to be > a for loop I finally figured out what "unrolled", "branchless" and "G(alois)F(ield) multiplier" means. I would prefer to leave the "enrolled" version as I think optimization should be done by the compiler. I would keep the code branchless in contrast to using conditions as I think that branches would enable timing attacks. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:271: // Encrypt encrypts the 16-byte buffer src On 2011/01/09 15:20:23, agl1 wrote: > (I know that this was taken from aes/ and that package should also be cleaned > up.) > > // Encrypt encrypts a 16-byte block from src to dst, which may overlap. Actually Blowfish but anyway, Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:276: // instead, use an encryption mode like CBC (see crypto/block/cbc.go). On 2011/01/09 17:40:00, r wrote: > this is a short ragged comment. godoc will present it as is; would be nice to > format it better I'm sorry, but I don't know what you mean by that. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:277: func (skey *Cipher) Encrypt(src, dst []byte) { On 2011/01/09 15:20:23, agl1 wrote: > dst comes first. Sorry for repeating that error, Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:318: // Decrypt decrypts the 16-byte buffer ct On 2011/01/09 15:20:23, agl1 wrote: > // Decrypt decrypts a 16-byte block from src to dst, which may overlap. Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:321: func (skey *Cipher) Decrypt(src, dst []byte) { On 2011/01/09 15:20:23, agl1 wrote: > dst comes first. Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:352: // Pre-white On 2011/01/09 15:20:23, agl1 wrote: > // Undo pre-whitening Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... File src/pkg/crypto/twofish/twofish_test.go (right): http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:41: // All test are designed for ECB mode On 2011/01/09 17:40:00, r wrote: > s/test/tests/ Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:47: // This tests are extracted from LibTom On 2011/01/09 17:40:00, r wrote: > s/This/These/ Done. http://codereview.appspot.com/2687042/diff/28001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:88: // Test if the sbox saved as variable containes the values as defined by it's function On 2011/01/09 17:40:00, r wrote: > s/containes/contains/ > s/it's/its/ Done.
Sign in to reply to this message.
Looks good after these changes. Re: ror and rol, I think we will eventually do what gcc does and recognize that pattern and implement it using the machine instructions. http://codereview.appspot.com/2687042/diff/41001/src/pkg/crypto/twofish/twofi... File src/pkg/crypto/twofish/twofish.go (right): http://codereview.appspot.com/2687042/diff/41001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:204: B := []uint32{0, uint32(b)} Make this B := [2]uint32 and similarly P := [2]uint32. Then they have a better chance of being stack allocated. As it is this will malloc two slices every time you call gfMult. http://codereview.appspot.com/2687042/diff/41001/src/pkg/crypto/twofish/twofi... File src/pkg/crypto/twofish/twofish_test.go (right): http://codereview.appspot.com/2687042/diff/41001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:5: // Perform self-test of the Twofish block cipher delete; implied by file name. http://codereview.appspot.com/2687042/diff/41001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:87: func TestTwofish(t *testing.T) { probably TestEncrypt would be better. you're already in the twofish package. http://codereview.appspot.com/2687042/diff/41001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:88: // Test if the sbox saved as variable contains the values as defined by its function move this up next to the definition of genSbox as its own test func TestSbox(t *testing.T) { ... }
Sign in to reply to this message.
On Tue, Jan 11, 2011 at 10:25 AM, <rsc@golang.org> wrote: > Looks good after these changes. Will land after these have been addressed and the author agreement has been signed. AGL
Sign in to reply to this message.
Hello rsc, berengarlehr@googlemail.com, albert.strasheim, agl1 (cc: golang-dev@googlegroups.com, schulze@math.uni-hannover.de), Please take another look.
Sign in to reply to this message.
On Tue, Jan 11, 2011 at 6:21 PM, <Berengar.Lehr@gmx.de> wrote: > Hello rsc, berengarlehr@googlemail.com, albert.strasheim, agl1 (cc: > golang-dev@googlegroups.com, schulze@math.uni-hannover.de), Will land when I get home (in an hour or so). I saw a typo but, no worries, I'll fix it when I land it. AGL
Sign in to reply to this message.
http://codereview.appspot.com/2687042/diff/41001/src/pkg/crypto/twofish/twofi... File src/pkg/crypto/twofish/twofish.go (right): http://codereview.appspot.com/2687042/diff/41001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:204: B := []uint32{0, uint32(b)} On 2011/01/11 15:25:37, rsc wrote: > Make this B := [2]uint32 and similarly P := [2]uint32. > Then they have a better chance of being stack allocated. > As it is this will malloc two slices every time you call > gfMult. Done. http://codereview.appspot.com/2687042/diff/41001/src/pkg/crypto/twofish/twofi... File src/pkg/crypto/twofish/twofish_test.go (right): http://codereview.appspot.com/2687042/diff/41001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:5: // Perform self-test of the Twofish block cipher On 2011/01/11 15:25:37, rsc wrote: > delete; implied by file name. Done. http://codereview.appspot.com/2687042/diff/41001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:10: "crypto/block" not required any more, deprecated anyway http://codereview.appspot.com/2687042/diff/41001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:87: func TestTwofish(t *testing.T) { On 2011/01/11 15:25:37, rsc wrote: > probably TestEncrypt would be better. > you're already in the twofish package. Testing the Cipher not only Encryption, so TestCipher, Done. http://codereview.appspot.com/2687042/diff/41001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:88: // Test if the sbox saved as variable contains the values as defined by its function On 2011/01/11 15:25:37, rsc wrote: > move this up next to the definition of genSbox as > its own test > > func TestSbox(t *testing.T) { > ... > } Done. http://codereview.appspot.com/2687042/diff/41001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:100: key, err := NewCipher(tt.key) it's not a key, it's a cipher http://codereview.appspot.com/2687042/diff/41001/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish_test.go:106: enc := bytes.NewBuffer(make([]byte, 0)) this and the following lines where changed to comply with deprecation of crypto/block (btw. ECB is a pretty fancy name for plain blockwise encryption, thx for the hint agl&rsc)
Sign in to reply to this message.
looks pretty good; after making the changes below it looks good to me. leaving for agl. http://codereview.appspot.com/2687042/diff/29005/src/pkg/crypto/twofish/twofi... File src/pkg/crypto/twofish/twofish.go (right): http://codereview.appspot.com/2687042/diff/29005/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:89: for i := 0; i <= 255; i++ { weird way to write the loop. better for i := range c.s[0] { or at least i < 256. http://codereview.appspot.com/2687042/diff/29005/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:96: for i := 0; i < 256; i++ { for i := range c.s[0] { http://codereview.appspot.com/2687042/diff/29005/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:103: for i := 0; i < 256; i++ { for i := range c.s[0] { http://codereview.appspot.com/2687042/diff/29005/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:117: for i := 0; i < 40; i++ { for i := range c.k { http://codereview.appspot.com/2687042/diff/29005/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:120: for i := 0; i < 4; i++ { for i := range c.s { http://codereview.appspot.com/2687042/diff/29005/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:121: for j := 0; j < 265; j++ { for j := range c.s[i] { (avoids typo 265) http://codereview.appspot.com/2687042/diff/29005/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:275: func (skey *Cipher) Encrypt(dst, src []byte) { should be c *Cipher. skey is an implementation detail leaking out http://codereview.appspot.com/2687042/diff/29005/src/pkg/crypto/twofish/twofi... src/pkg/crypto/twofish/twofish.go:319: func (skey *Cipher) Decrypt(dst, src []byte) { c *Cipher
Sign in to reply to this message.
Hello rsc, berengarlehr@googlemail.com, albert.strasheim, agl1 (cc: golang-dev@googlegroups.com, schulze@math.uni-hannover.de), Please take another look.
Sign in to reply to this message.
On 2011/01/19 19:41:08, rsc wrote: > looks pretty good; after making the changes below it looks good to me. > leaving for agl. I changed all occurrences of for where they were iterations over an array. changed 'skey' to 'c' and consequently 'a'..'d' to 'ia'..'id'.
Sign in to reply to this message.
On Wed, Jan 19, 2011 at 6:44 PM, <Berengar.Lehr@gmx.de> wrote: > I changed all occurrences of for where they were iterations over an > array. > changed 'skey' to 'c' and consequently 'a'..'d' to 'ia'..'id'. > > http://codereview.appspot.com/2687042/ I'll apply to the version in tree. AGL
Sign in to reply to this message.
On Wed, Jan 19, 2011 at 7:06 PM, Adam Langley <agl@golang.org> wrote: > I'll apply to the version in tree. Done: d1d939ce68 AGL
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
