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

A few iterations on the end-to-end code #651

Merged
merged 1 commit into from May 23, 2016
Merged

Conversation

sselph
Copy link

@sselph sselph commented May 22, 2016

No description provided.

@@ -91,19 +99,23 @@ func keys(pass, salt []byte, iterations int) (aesKey, hmacKey []byte, err error)
return aesKey, hmacKey, nil
}

// hashReadWriter hashes on write and on read finalizes the hash and returns it.
// Writes after a Read will return an error.
type hashReadWriter struct {
hash hash.Hash
done bool
sum io.Reader
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above the comment type hashReadWriter, please add

var _ io.ReadWriter = &hashReadWriter{}

This way we can always be sure that (*hashReadWriter) implements io.Reader and io.Writer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

@odeke-em
Copy link
Owner

Thank you @sselph for the PR. I've added some comments. When I get back home in a few, I'll spin up a CLI app that uses the dcrypto package and then we can use that same logic to hook it up with push and pull for drive.

@sselph
Copy link
Author

sselph commented May 22, 2016

I think I addressed the comments in the review. I had a question about the need for getting the checksum. I wrote it before checking if it was even needed.

@odeke-em
Copy link
Owner

Thanks @sselph, just a few not yet addressed like bytes.NewBuffer to bytes.NewReader#651 (comment), #651 (comment), #651 (comment), #651 (diff), #651 (comment)

@odeke-em
Copy link
Owner

In other news, I created a CLI here https://gist.github.com/odeke-em/a21249c126d4e0a5033ed51011d67419, to test out the new decrypter in previous CL ie encrypting a file first then decrypting the encrypting file. However, I've gotten runtime slice out of bound panics

$ drive-crypto-samples --password "foolsGold" --decrypt mk.enc > tt.dec
panic: runtime error: slice bounds out of range

goroutine 1 [running]:
panic(0xec4a0, 0xc4200120c0)
    /Users/emmanuelodeke/go/src/go.googlesource.com/go/src/runtime/panic.go:500 +0x18c
github.com/odeke-em/drive/src/dcrypto/v1.newDecryptReader(0x185640, 0xc42002e028, 0xc420012900, 0x9, 0x10, 0x40000, 0xc430098040, 0x0, 0x0)
    /Users/emmanuelodeke/go/src/github.com/odeke-em/drive/src/dcrypto/v1/v1.go:217 +0xa10
github.com/odeke-em/drive/src/dcrypto/v1.NewDecryptReader(0x185640, 0xc42002e028, 0xc420012900, 0x9, 0x10, 0x6, 0xc42002e028, 0x3, 0x0)
    /Users/emmanuelodeke/go/src/github.com/odeke-em/drive/src/dcrypto/v1/v1.go:164 +0x52
github.com/odeke-em/drive/src/dcrypto.NewDecrypter(0x185640, 0xc42002e028, 0xc420012900, 0x9, 0x10, 0x6, 0x0, 0x0, 0xc42002e028)
    /Users/emmanuelodeke/go/src/github.com/odeke-em/drive/src/dcrypto/dcrypto.go:85 +0xb7
main.main.func1(0x185640, 0xc42002e028, 0xc420012900, 0x9, 0x10, 0x10, 0x2a, 0x19c7c0, 0xc4200128b0)
    /Users/emmanuelodeke/go/src/github.com/odeke-em/drive-crypto-samples/main.go:21 +0x5f
main.main()
    /Users/emmanuelodeke/go/src/github.com/odeke-em/drive-crypto-samples/main.go:43 +0x3a7

@sselph
Copy link
Author

sselph commented May 22, 2016

Weird I thought I pushed the changes but guess it didn't push. I NewBuffer is now NewReader everywhere. I accidentally pushed more test cases trying to trigger the bug you are seeing.

For the bug. Is it triggering all the time for this file and if so what is the file size? before and after encryption. I think I chose my tests wrong and will adjust them again to see if I can test what I was originally trying to test.

@sselph
Copy link
Author

sselph commented May 23, 2016

I updated the test locally to iterate over all sizes 0-64KB to see if some size caused the issue. The only way I can trigger this is by corrupting the encrypted file so it is too small. Basically it still has the version, salt, iv but isn't long enough to include the full hmac. I'll add in a check for this in the code to prevent that.

@odeke-em
Copy link
Owner

False alarm, my bad. I was rushing out of the house so improperly run the test. You were spot on with the prognosis of the crash, basically only the headers had been written to file. I've been able to re-run my tests encrypting and decrypting the original files that were tripping out.
Looks gucci to me otherwise, please squash the commits into one. Let's merge this in, and then spin up a separate package for dcrypto. I can start an organization that we'll put the repo in, if the paperwork hasn't yet come through. This way we can iterate on it in there and create utils outside of drive. Great stuff.

Add utility functions to allow hashing local files to match encrypted files.
Encode scrypt iterations in to file header.
Un-export a few things.

Updates odeke-em#543
@sselph
Copy link
Author

sselph commented May 23, 2016

Squashed.

@odeke-em
Copy link
Owner

Dope! Thank you @sselph. Merging it in.

@odeke-em odeke-em merged commit 87197b6 into odeke-em:master May 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants