-
-
Notifications
You must be signed in to change notification settings - Fork 750
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
new AEAD crypto with session keys #6463
Merged
Merged
Changes from all commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
d3f069c
crypto: fix/update borg version comments
ThomasWaldmann aff6261
crypto: cleanup, remove references to AES-GCM
ThomasWaldmann e647360
crypto: better raise NotImplementedError if we have no id_hash
ThomasWaldmann 57479fb
crypto: put the IV into the header, at the end of it
ThomasWaldmann 3473b17
crypto: improve attr naming
ThomasWaldmann 9633273
crypto: simplify api for new crypto, AEAD only needs 1 key
ThomasWaldmann 0f6f278
crypto: AEAD key classes
ThomasWaldmann 5c66fa4
crypto: layout updates, low-level does not deal with IV
ThomasWaldmann c010800
header_len=0 fits header=b'' default
ThomasWaldmann bb949b2
EVP_DecryptFinal_ex: fix check for return value
ThomasWaldmann 6c7b499
set aead auth tag directly before EVP_DecryptFinal_ev
ThomasWaldmann 6f2c587
tests: consistently give iv_int to ciphersuite
ThomasWaldmann 41082f5
crypto: add some tests for new key types
ThomasWaldmann 6d6d3ca
avoid losing the key
ThomasWaldmann 0b5a212
avoid losing the key (old crypto)
ThomasWaldmann 74ecb63
fix new crypto benchmarks for api change
ThomasWaldmann 41b8a04
use faster hmac.digest api
ThomasWaldmann d3b78a6
minor key.encrypt api change/cleanup
ThomasWaldmann 8bd9477
add aad parameter to borg.crypto.low_level api
ThomasWaldmann c50e112
also authenticate the chunkid when using the AEAD ciphers (AES-OCB/CH…
ThomasWaldmann f4a6ad0
docs: add new AEAD modes to security docs
ThomasWaldmann 948d67e
crypto.low_level: simplify return code checks (AEAD)
ThomasWaldmann e1313cc
crypto.low_level: simplify return code checks (legacy)
ThomasWaldmann ccf0875
EVP_DecryptFinal_ex: fix check for return value
ThomasWaldmann b3383a4
update borg init docs
ThomasWaldmann 298c5ee
docs: security infos only applying to legacy encryption
ThomasWaldmann ce24752
docs: update borg init examples
ThomasWaldmann 900a812
crypto: bump API_VERSION to 1.3_01
ThomasWaldmann e4b65de
crypto: add IV overflow check
ThomasWaldmann 3a0e1a1
crypto: low_level: reduce class inheritance depth
ThomasWaldmann dd2a054
crypto: key: reduce class inheritance depth
ThomasWaldmann af26835
delete pointless assert
ThomasWaldmann 10cbdcc
add encryption-aead diagram
ThomasWaldmann c668265
init olen to avoid some (false positive) compiler warnings
ThomasWaldmann File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Curious user here who happens to be a cryptographer.)
What gains do you expect from this?
So if I read this correctly, then the chunk id is an additional MAC over the uncompressed and unencrypted data. But the AEAD already authenticates the unencrypted data (no matter whether it's encrypted or not), so I fail to see the purpose of adding the id to the AAD.
Does the id have a purpose in other parts of borg or is this just for the encryption?
I may be able to provide a more detailed review but I realize I'm quite late to the party... Do you think this would still be helpful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Tim,
thanks for looking at our crypto, review there is very welcome (and not too late, borg2 is still in alpha, soon beta, so we could still change things)!
If we do not consider time needed for computing the MAC (chunkid) from plaintext, there is no advantage. But we would have to compute the MAC to verify if it is the same as the chunkid we wanted when we asked the repo for that chunk. borg 1.x does it like that. Without that, the self-made AEAD construction we used in 1.x (AES-CTR + HMAC-SHA256 and not including the chunkid) would just be able to tell that the content was written by us (authentic), but we could not be sure if we wrote that for that chunkid (could have been also some other chunkid).
If we feed the chunkid into the AAD computation when storing the chunk and also when requesting the chunk, the authentication will fail if:
Thus, we do not need to compute the MAC to verify if the chunk is really for the chunkid we wanted - saves some CPU time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe useful for crypto review:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this makes a lot of sense now! Adding the chunkid as AAD binds the key (as in "key-value store") to the value.
Indeed yes, I see that this would save CPU time. But then I'm confused whether it's really implemented like this? AFAIU the code still recomputes the MAC:
borg/src/borg/crypto/key.py
Line 897 in b52781a
Thanks for the warm welcome. I'll keep looking at the PR and docs here. I'll probably continue to ask some very basic questions, so please bear with me. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, fixed by #6880 (and added a test to make sure it works as expected).