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

internal/crypto: small simplifications #1187

Merged
merged 1 commit into from Aug 29, 2017

Conversation

Projects
None yet
3 participants
@FiloSottile
Contributor

FiloSottile commented Aug 28, 2017

  • append operates on len, not cap (not a bug since len is set to cap above, but let's avoid the confusion)
  • no need to extend ciphertext again to cap after we made it big enough
  • make consistent use of ciphertext[:ivSize] vs iv[:]
  • make all input problems errors and impossible/catastrophic cases panics
@fd0

Hey, thanks a lot for looking at the code and suggesting simplifications! I agree with most of the changes except one.

(I expected you to look at the crypto code if you ever stumble over restic ;)

Would you mind running gofmt on the code? Thanks!

e := cipher.NewCTR(c, iv)
plaintext = plaintext[:len(ciphertext)]

This comment has been minimized.

@fd0

fd0 Aug 28, 2017

Member

What happens if len(plaintext) is greater than len(ciphertext)-Extension? I think this line needs to stay, what do you think?

@fd0

fd0 Aug 28, 2017

Member

What happens if len(plaintext) is greater than len(ciphertext)-Extension? I think this line needs to stay, what do you think?

This comment has been minimized.

@FiloSottile

FiloSottile Aug 28, 2017

Contributor

XORKeyStream is defined in that case to only operate on len(dst).

I am, to be honest, not a fan of this API because it does not return the plaintext slice, only the length, and I can see an user failing to slice the plaintext in such a case, leaving trailing unauthenticated data.

@FiloSottile

FiloSottile Aug 28, 2017

Contributor

XORKeyStream is defined in that case to only operate on len(dst).

I am, to be honest, not a fan of this API because it does not return the plaintext slice, only the length, and I can see an user failing to slice the plaintext in such a case, leaving trailing unauthenticated data.

@FiloSottile

This comment has been minimized.

Show comment
Hide comment
@FiloSottile

FiloSottile Aug 28, 2017

Contributor

Hey, dammit, I thought I did gofmt by eye, but I was editing from the GitHub editor. I'll clone it and gofmt it :)

BTW, I like the design, and indeed I'm reviewing the crypto before adopting it ;) I'm doing a quick writeup, let me know if you want to read a draft.

Thanks for the software!

Contributor

FiloSottile commented Aug 28, 2017

Hey, dammit, I thought I did gofmt by eye, but I was editing from the GitHub editor. I'll clone it and gofmt it :)

BTW, I like the design, and indeed I'm reviewing the crypto before adopting it ;) I'm doing a quick writeup, let me know if you want to read a draft.

Thanks for the software!

internal/crypto: small simplifications
* append operates on len, not cap (not a bug since len is set to cap above, but let's avoid the confusion)
* no need to extend ciphertext again to cap after we made it big enough
* make consistent use of ciphertext[:ivSize] vs iv[:]
* make all input problems errors and impossible/catastrophic cases panics
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 28, 2017

Codecov Report

Merging #1187 into master will decrease coverage by 5.78%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1187      +/-   ##
==========================================
- Coverage   52.93%   47.15%   -5.79%     
==========================================
  Files         126      126              
  Lines       12059    12058       -1     
==========================================
- Hits         6384     5686     -698     
- Misses       4930     5682     +752     
+ Partials      745      690      -55
Impacted Files Coverage Δ
internal/crypto/crypto.go 63.41% <75%> (-0.23%) ⬇️
internal/backend/b2/b2.go 0% <0%> (-79.32%) ⬇️
internal/backend/swift/swift.go 0% <0%> (-73.9%) ⬇️
internal/backend/azure/azure.go 0% <0%> (-61.93%) ⬇️
internal/backend/gs/gs.go 0% <0%> (-59.85%) ⬇️
internal/backend/swift/config.go 34.37% <0%> (-56.25%) ⬇️
internal/archiver/archiver.go 64.79% <0%> (-0.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22e96a3...9940e8d. Read the comment docs.

codecov-io commented Aug 28, 2017

Codecov Report

Merging #1187 into master will decrease coverage by 5.78%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1187      +/-   ##
==========================================
- Coverage   52.93%   47.15%   -5.79%     
==========================================
  Files         126      126              
  Lines       12059    12058       -1     
==========================================
- Hits         6384     5686     -698     
- Misses       4930     5682     +752     
+ Partials      745      690      -55
Impacted Files Coverage Δ
internal/crypto/crypto.go 63.41% <75%> (-0.23%) ⬇️
internal/backend/b2/b2.go 0% <0%> (-79.32%) ⬇️
internal/backend/swift/swift.go 0% <0%> (-73.9%) ⬇️
internal/backend/azure/azure.go 0% <0%> (-61.93%) ⬇️
internal/backend/gs/gs.go 0% <0%> (-59.85%) ⬇️
internal/backend/swift/config.go 34.37% <0%> (-56.25%) ⬇️
internal/archiver/archiver.go 64.79% <0%> (-0.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22e96a3...9940e8d. Read the comment docs.

@fd0

This comment has been minimized.

Show comment
Hide comment
@fd0

fd0 Aug 29, 2017

Member

Heh, thanks a lot for the draft, added a few comments, and thanks for looking at the code! I appreciate that!

Member

fd0 commented Aug 29, 2017

Heh, thanks a lot for the draft, added a few comments, and thanks for looking at the code! I appreciate that!

@fd0

fd0 approved these changes Aug 29, 2017

@fd0 fd0 merged commit 9940e8d into restic:master Aug 29, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!

fd0 added a commit that referenced this pull request Aug 29, 2017

Merge pull request #1187 from FiloSottile/patch-1
internal/crypto: small simplifications
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment