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

Create an end to end encryption library #645

Merged
merged 1 commit into from May 18, 2016

Conversation

Projects
None yet
2 participants
@sselph
Copy link

sselph commented May 16, 2016

No description provided.

@odeke-em

This comment has been minimized.

Copy link
Owner

odeke-em commented May 16, 2016

Nice! This is cool. I'll review this when I get back home in about 9 hours.

@sselph

This comment has been minimized.

Copy link
Author

sselph commented May 17, 2016

Noticed io.MultiReader and io.TeeReader which remove the need for a large portion of code so I fixed that.

@@ -1,7 +1,7 @@
sudo: false
language: go
go:
- 1.3
- 1.5

This comment has been minimized.

Copy link
@odeke-em
)

// PreferedVersion is the prefered version of encryption.
const PreferedVersion = V1

This comment has been minimized.

Copy link
@odeke-em
if err != nil {
return nil, err
}
er, err := encrypters[PreferedVersion](r, password)

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

Let's break this out and catch any missing versions

encrypterFn, ok := encrypters[PreferedVersion]
if !ok {
    return nil, fmt.Errorf("%v version could not be found", PreferedVersion)
}
encReader, err := encrypterFn(r, password)
if err != nil {
return nil, err
}
d, ok := decrypters[version]

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner
decryptFn, ok := decrypters[version]
return nil, err
}
d, ok := decrypters[version]
if !ok {

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

Yap, just like you did here to first look up the decrypter by version, do that too above in looking up the encrypter.


// writeVersion converts a Version to a []byte.
func writeVersion(i Version) ([]byte, error) {
buf := new(bytes.Buffer)

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner
if err := binary.Write(buf, binary.LittleEndian, i); err != nil {
   return nil, err
}

// readVersion reads and returns a Version from reader.
func readVersion(r io.Reader) (Version, error) {
var i Version

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner
func readVersion(r io.Reader) (v Version, err error) {
    err = binary.Read(r, binary.LittleEndian, &v)
    return v, err
}
// TestRoundTrip tests several size sets of data going through the encrypt/decrypt
// to make sure they come out the same.
func TestRoundTrip(t *testing.T) {
for _, size := range []int{24, 66560} {

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

Could we test with odd sizes too, and also with 0? Or does that not add anything useful to the tests?

func TestRoundTrip(t *testing.T) {
for _, size := range []int{24, 66560} {
t.Logf("Testing file of size: %db", size)
f, err := randBytes(size)

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

Could we use b instead since f usually implies usage of a file or reader?

t.Errorf("randBytes(%d) => %q; want nil", size, err)
continue
}
er, err := NewEncrypter(bytes.NewBuffer(f), password)

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

encReader, err :=
instead of er which is very similar to err.

t.Errorf("NewEncrypter() => %q; want nil", err)
continue
}
cipher, err := ioutil.ReadAll(er)

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

I was gonna suggest that we skip this and instead pass it directly to the *Decrypter but I realize that it is essential for the encrypter to be full Readable from. Nice.


// The number of iterations to use in scrypt.Key
// Must be a power of 2.
scryptIter = 262144 // 2^18

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner
scryptIter = 1<<18

Maybe even scryptIterations?

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

Could you please add a reference to https://godoc.org/golang.org/x/crypto/scrypt#Key as a comment before scryptIter?
This way we can refer to it. You can also disregard my comment 1<<18 since 262144 presents a better readability and comparison against the documented recommended iteration count of interactive logins as of 2009 are N=16384 from https://godoc.org/golang.org/x/crypto/scrypt#Key

h := hmac.New(hashFunc, hmacKey)
h.Write(salt)
h.Write(iv)
dst, err := ioutil.TempFile("", "drive-encrypted-")

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

We could write a wrapper for a TempFile and add a .Close() or .Done() method to it so that we can avoid having to deal with .Close() then os.Remove(...) everytime we need to use a temp file. Let me spin up for you a package.

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

@sselph I've made for you something here odeke-em/go-utils#1 or odeke-em/go-utils@21e7e7b

dst, err := tmpfile.New(&tmpfile.Context{
   CreateInIsolateDir: true,
   Suffix: "drive-encrypted-",
})

// We can then do this once, without the multiple os.RemoveAll's ;)
defer dst.Done()

This comment has been minimized.

Copy link
@sselph

sselph May 18, 2016

Author

I had a couple issues with the tmpfile package but worked around it by passing my own dir and not using the isolateddir. I added some notes to https://github.com/odeke-em/go-utils/pull/1/files

// Start Verifying the HMAC of the message.
h := hmac.New(hashFunc, hmacKey)
h.Write(salt)
h.Write(iv)

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

We can combine these two and add error checking as well

for _, b := range [][]byte{salt, iv} {
   if err := h.Write(b); err != nil {
      return nil, err
   }
}

This comment has been minimized.

Copy link
@sselph

sselph May 17, 2016

Author

I can but I skipped checking for an error since writing to a Hash never returns an error https://golang.org/pkg/hash/#Hash

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

Ah I see, thanks for the reference. All good then.

sReader: &cipher.StreamReader{R: dst, S: cipher.NewCTR(b, iv)},
}
w := io.MultiWriter(h, dst)
buf := bufio.NewReaderSize(r, 16*1024)

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

16KB reads.
We could make this a variable for easy reading

const _16KB = 16 * 1024
...
buf := bufio.NewReaderSize(r, _16KB)
for {
   b, err := buf.Peek(_16KB)
   ...
}
// TestRoundTrip tests several size sets of data going through the encrypt/decrypt
// to make sure they come out the same.
func TestRoundTrip(t *testing.T) {
for _, size := range []int{24, 1024, 15872, 16364, 16384, 16394, 16896, 66560} {

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

Could we test with multiple passwords e.g

spasswords := [...]string{
   0: "",
   1: "test",
   2: "a",
   3: "      ",
   4: fmt.Sprintf("%s", randBytes(13)),
   5: fmt.Sprintf("%s", randBytes(400)),
}
sizes := []int{....}
for _, spass := range spasswords {
   password := []byte(spass)
   for _, size := range sizes {
   ...
   }
}
// See the License for the specific language governing permissions and
// limitations under the License.

package dcrypto

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

Let's make this package dcrypto_test to avoid leaking helper functions to the main package code.

This comment has been minimized.

Copy link
@sselph

sselph May 17, 2016

Author

They shouldn't leak anything during a normal build.

From https://golang.org/pkg/testing/
create a file whose name ends _test.go that contains the TestXxx functions as described here. Put the file in the same package as the one being tested. The file will be excluded from regular package builds but will be included when the “go test” command is run.

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

What I mean by that is this

$ cat << ! > at.go
package at

func BlobAt() string {
  return hello()
}
$ cat << ! > at_test.go
package at

import (
  "testing"
)

func hello() string {
  return "hello"
}

func Test(t *testing.T) {
}
$ go test

will include hello from at_test

Where as if you made

package at_test

import (
  "testing"
)

func hello() string {
  return "hello"
}

func Test(t *testing.T) {
}

you wouldn't be able to use unexported function hello from at_test.go in at.

It isn't a big deal nor show stopper, so all good.

This comment has been minimized.

Copy link
@sselph

sselph May 18, 2016

Author

Ah I see what you were talking about. I don't think it is a bug concern but after second thought, this is testing the exported functions so might be good to make it separate.

t.Errorf("ioutil.ReadAll(*Encrypter) => %q; want nil", err)
continue
}
dr, err := NewDecrypter(bytes.NewBuffer(cipher), password)

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

Could we test out with different passwords as mentioned in https://github.com/odeke-em/drive/pull/645/files#r63466677.

This comment has been minimized.

Copy link
@sselph

sselph May 17, 2016

Author

I can do this but each test case takes ~2s to run since the key generation has to take place twice. Adding 0 and an odd test case has us at 8s so each password would add another 8s. I'm fine adding more just wanted you to be aware.

This comment has been minimized.

Copy link
@odeke-em

odeke-em May 17, 2016

Owner

For sure, thanks for letting me know. Later on, we can do -short and regular modes of testing.

@odeke-em

This comment has been minimized.

Copy link
Owner

odeke-em commented May 17, 2016

Hello @sselph thanks for the PR, I've gone through the first round of review and left some comments. Looks good!

@sselph

This comment has been minimized.

Copy link
Author

sselph commented May 17, 2016

I had a reply to a few of the comments. The ones without a reply I've fixed locally and will send the commit after hearing back on the comments.

@odeke-em

This comment has been minimized.

Copy link
Owner

odeke-em commented May 17, 2016

Dope, thank you!

@sselph

This comment has been minimized.

Copy link
Author

sselph commented May 18, 2016

Thanks. I think I addressed the current round of feedback.

@odeke-em

This comment has been minimized.

Copy link
Owner

odeke-em commented May 18, 2016

Solid plan, thank you for addressing those. I think we are good to merge. Could you squash those commits into one? This is great stuff I can't wait to hook it in.

@sselph

This comment has been minimized.

Copy link
Author

sselph commented May 18, 2016

hmm i thought I squashed it down but seems i made it even worse. I don't typically use git. I'll try again.

@odeke-em

This comment has been minimized.

Copy link
Owner

odeke-em commented May 18, 2016

No worries, here are some pitches I can provide, you can do something like this

$ git rebase -i head~8 # Since it seems 8 commits ahead
$ # When prompted by git in your editor
$ # Then pick which will be the top commit after picking between `pick` and `squash`.
$ # Once you have the commits all squashed, you can then commit -a --amend
$ git commit -a --amend
$ # Then create the final commit message, referring to the issue e.g
Fixes #<issue_number_here>
$ # Then git push -f on the branch you are on e.g master
$ git push origin master -f
$ # Then boom we are good to go!

@sselph sselph force-pushed the sselph:master branch 2 times, most recently from 0d5ced4 to 7195673 May 18, 2016

@sselph

This comment has been minimized.

Copy link
Author

sselph commented May 18, 2016

Wow I think it it may have worked.

@odeke-em

This comment has been minimized.

Copy link
Owner

odeke-em commented May 18, 2016

Boom, we gucci to go ;)

@odeke-em

This comment has been minimized.

Copy link
Owner

odeke-em commented May 18, 2016

Now let's update the commit message to be something like this

$ git commit -a --amend

Then for the commit message

end-to-end: add encryption library

... Mention some details about the library
....

Updates #543
end-to-end: add encryption library
Adds AES256-CTR+HMAC-SHA-512 en/decrpytion functions that wrap io.Reader.

Updates #543

@sselph sselph force-pushed the sselph:master branch from 7ec541d to b959fe1 May 18, 2016

@sselph

This comment has been minimized.

Copy link
Author

sselph commented May 18, 2016

Okay I think it is correct.

@odeke-em

This comment has been minimized.

Copy link
Owner

odeke-em commented May 18, 2016

Awesome. LGTM! Thank you very much @sselph. Your work puts us on the roadmap of just plugging and playing to have end to end encryption. I don't think such a library exists currently for Go. You could spin this off as a separate package and announce it. Lots of people could benefit from it.

Let me merge this in, and we can in later iterations look at you spinning up your package for this.

@odeke-em odeke-em merged commit 859a511 into odeke-em:master May 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@odeke-em

This comment has been minimized.

Copy link
Owner

odeke-em commented May 19, 2016

@sselph

This comment has been minimized.

Copy link
Author

sselph commented May 20, 2016

Thanks, I'm glad I could help. I'll look to see what it will take to spin this off as its own package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.