Argon2 the second part: implement key encryption / decryption #6469

hexagonrecursion · 2022-03-19T09:06:57Z

~~Note: this PR is not yet complete. I am submitting it to try running CI~~

This is the second part of #747

~~ - [x] Add code in decrypt_key_file necessary to be able to decrypt v2 keys ~~
~~ - [x] Implement encryption of keys in the new format ~~
See new TODO

~~This PR includes #6468~~

codecov-commenter · 2022-03-19T11:18:15Z

Codecov Report

Merging #6469 (1aa652e) into master (312cae5) will increase coverage by 0.32%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master    #6469      +/-   ##
==========================================
+ Coverage   82.76%   83.09%   +0.32%     
==========================================
  Files          39       39              
  Lines       10655    10573      -82     
  Branches     2089     2069      -20     
==========================================
- Hits         8819     8786      -33     
+ Misses       1327     1286      -41     
+ Partials      509      501       -8

Impacted Files	Coverage Δ
src/borg/helpers/passphrase.py	`77.87% <66.66%> (+2.17%)`	⬆️
src/borg/crypto/key.py	`92.08% <91.30%> (+0.96%)`	⬆️
src/borg/archiver.py	`78.43% <100.00%> (+0.29%)`	⬆️
src/borg/constants.py	`100.00% <100.00%> (ø)`
src/borg/upgrader.py	`36.58% <0.00%> (-26.64%)`	⬇️
src/borg/helpers/parseformat.py	`90.04% <0.00%> (+0.01%)`	⬆️
src/borg/archive.py	`81.70% <0.00%> (+0.09%)`	⬆️
src/borg/repository.py	`84.19% <0.00%> (+0.11%)`	⬆️
... and 1 more

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 28731c5...1aa652e. Read the comment docs.

hexagonrecursion · 2022-03-19T11:57:38Z

After refreshing my memory of Encrypt-then-MAC vs Encrypt-and-MAC vs MAC-then-Encrypt I feel like I would prefer to use a high level API that just does the right thing. Do you have one? If not I'll try to Encrypt-then-MAC. How hard can it be to remember to MAC all the bits in the message that affect the decryption?

hexagonrecursion · 2022-03-19T14:45:30Z

Done. I will mark this as ready for review after #6468 is merged.

hexagonrecursion · 2022-03-19T15:10:08Z

Note: a malicious key file might be able to perform denial of service e.g. by causing us to allocate too much RAM

src/borg/crypto/key.py

src/borg/testsuite/crypto.py

src/borg/item.pyx

ThomasWaldmann · 2022-03-19T20:25:30Z

About DoS via expensive argon2 parameters in key file: guess if someone wants to annoy you and has write access to your key files, they can do worse than that, like e.g. just delete the key files.

src/borg/crypto/key.py

src/borg/item.pyx

src/borg/testsuite/crypto.py

hexagonrecursion · 2022-03-22T19:17:09Z

This should be ready now.

ThomasWaldmann · 2022-03-22T20:19:05Z

maybe also do the "encrypt key file" here?

src/borg/testsuite/crypto.py

src/borg/crypto/key.py

hexagonrecursion · 2022-03-23T08:33:34Z

maybe also do the "encrypt key file" here?

back to draft then

1. Note: I have rebased this on top of 78f0414 and fixed the code to work with the new Passphrase.argon2 interface 2. Refactor: s/enc_key/encrypted_key/ - I intend to use the name enc_key for something else 3. Dispatch on algorithm instead of version borgbackup#747 (comment) New: rebased on 28731c5 (current master)

hexagonrecursion · 2022-03-27T10:14:11Z

Things left to do before this PR is ready:

Fix passphrase retry bug
borg init - defalt to argon2
borg key change-passphrase - keep key algorithm the same
borg key change-location - keep key algorithm the same

ThomasWaldmann · 2022-03-27T20:25:42Z

looks good so far. TBD yet whether we should rather use the recommended or 2nd recommended profile for argon2.

problem is that we have quite widespread use cases, some people use borg on their old raspi or on their mobile phone, other use it on server / workstation class hardware with lots of memory.

one way to solve that would be that memory limited users keep using pbkdf2 while we use the high-memory profile for argon2 for everybody else. another way would be to offer both argon2 profiles, so people with low memory do not need to use the old crap. :-)

hexagonrecursion · 2022-04-03T15:44:50Z

Should we run testsuite/benchmark.py at full KDF strength? I feel that weakening KDF may defeat the point of a benchmark.

ThomasWaldmann · 2022-04-03T15:49:18Z

Not sure about the benchmarks, but I guess:

we do not intend to performance optimise the kdf code, so having it at full strength would not add value to the benchmark results
it would increase resource usage and slow down the benchmarks
especially this could become an issue if we go for argon2 rfc profile 1 in the end, which has much higher resource usage

hexagonrecursion · 2022-04-04T03:33:55Z

I am starting one more experiment to see the difference between skipping KDF entirely and weakening its parameters

https://github.com/hexagonrecursion/borg/actions/runs/2087617735

ThomasWaldmann · 2022-04-04T12:14:15Z

Skip and weaken are often rather close. What are the differences between benchmark 0..3 configuration?

hexagonrecursion · 2022-04-04T12:24:41Z

My data tells me that TESTONLY_MOCK_KDF=weaken and TESTONLY_MOCK_KDF=skip performed essentially identically.

Which approach would you prefer?

Skip the KDF entirely
Weaken the KDF

A. Use the environment variable
B. Patch a python variable in contest.py

I will happily implement any combination

ThomasWaldmann · 2022-04-04T12:44:13Z

weaken (because it basically still executes kdf code normally, just with other params)
env var (because, as you discovered, only that works for ArchiverTestCaseBinary)

hexagonrecursion · 2022-04-05T04:46:41Z

What are the differences between benchmark 0..3 configuration?

None. I wanted to be confident in my methodology so I repeat the same test 4 times. I got consistent results as expected.

I have submitted one final benchmark - to verify that the performance improvement matches our expectation https://github.com/hexagonrecursion/borg/actions/runs/2094039179

My plan is to start working on borg key change-algorithm after this is merged. After that is merged too I'll work on adding an argon2 benchmark to borg benchmark cpu and after that I'll work on the changelog and usage docs.

ThomasWaldmann · 2022-04-05T13:54:00Z

The benchmark run you linked above did not work, rc == 1.

hexagonrecursion · 2022-04-05T14:12:54Z

Thanks. I have started a run with --show-output to see what failed https://github.com/hexagonrecursion/borg/actions/runs/2096410352

hexagonrecursion · 2022-04-05T18:57:00Z

Mission success https://github.com/hexagonrecursion/borg/actions/runs/2097120013

hexagonrecursion · 2022-04-05T18:59:23Z

Are we ready merge this now or are we going to expand the scope of this PR again?

ThomasWaldmann · 2022-04-05T19:39:32Z

src/borg/helpers/passphrase.py

+        if os.environ.get("BORG_TESTONLY_WEAKEN_KDF") is not None:
+            iterations = 1


could the code from yes module be used here?

as it is, there is some danger that people set this to 0 or 1.

if not:

os.environ.get("BORG_TESTONLY_WEAKEN_KDF", "0") == "1"

if not:

os.environ.get("BORG_TESTONLY_WEAKEN_KDF", "0") == "1"

Good idea! This have not occurred to me

ThomasWaldmann · 2022-04-05T19:40:13Z

src/borg/helpers/passphrase.py

+        if os.environ.get("BORG_TESTONLY_WEAKEN_KDF") is not None:
+            time_cost = 1
+            parallelism = 1
+            # 8 is the smallest value that aviods the "Memory cost is too small" exception


ThomasWaldmann

did a full review again and found some minor stuff.

ThomasWaldmann · 2022-04-06T04:14:12Z

src/borg/testsuite/archiver.py

+
+    def test_init_with_explicit_key_algorithm(self):
+        """https://github.com/borgbackup/borg/issues/747#issuecomment-1076160401"""
+        self.cmd('init', '--encryption=repokey', '--key-algorithm', 'pbkdf2', self.repository_location)


just for consistency: can you please give --key-algorithm in the same way as --encryption?

please fix all places with this.

ThomasWaldmann · 2022-04-06T04:17:51Z

src/borg/testsuite/archiver.py

+    def test_change_passphrase_does_not_change_algorithm(self):
+        self.cmd('init', '--encryption=repokey', '--key-algorithm', 'argon2', self.repository_location)
+        os.environ['BORG_NEW_PASSPHRASE'] = 'newpassphrase'
+
+        self.cmd('key', 'change-passphrase', self.repository_location)
+
+        with Repository(self.repository_path) as repository:
+            key = msgpack.unpackb(a2b_base64(repository.load_key()))
+            assert key[b'algorithm'] == b'argon2 aes256-ctr hmac-sha256'


well, guess this test should start from pbkdf2 key, because:

this is what people usually have now

we would change algorithm pbkdf2 -> argon2 (but not argon2 -> pbkdf2) if we would do automated upgrades on change passphrase. so if you want to make sure here that we do not upgrade, you need to use the constellation that would actually get upgraded.

ThomasWaldmann · 2022-04-06T04:18:55Z

src/borg/testsuite/archiver.py

+        self.cmd('init', '--encryption=keyfile', '--key-algorithm', 'argon2', self.repository_location)
+
+        self.cmd('key', 'change-location', self.repository_location, 'repokey')
+
+        with Repository(self.repository_path) as repository:
+            key = msgpack.unpackb(a2b_base64(repository.load_key()))
+            assert key[b'algorithm'] == b'argon2 aes256-ctr hmac-sha256'
+


same here, needs to start from pbkdf2.

ThomasWaldmann · 2022-04-06T04:22:58Z

src/borg/testsuite/crypto.py

+        'salt': b'salt'*4,
+        'argon2_time_cost': 3,
+        'argon2_memory_cost': 2**16,
+        'argon2_parallelism': 4,


this does not seem to match params seen in 265.

guess this can succeed because you are force-weakening the params in the tests even if the stored params disagree.

ThomasWaldmann · 2022-04-06T04:28:26Z

src/borg/testsuite/crypto.py

+    encrypted = msgpack.packb({
+        'version': 1,
+        'algorithm': 'sha256',
+        'iterations': 1,


but here the weakened params are stored in the key also, so this is not consistent procedure with the above.

Good catch!

ThomasWaldmann · 2022-04-06T04:38:15Z

src/borg/testsuite/crypto.py

+@unittest.mock.patch('getpass.getpass')
+def test_repo_key_detect_does_not_raise_integrity_error(getpass, monkeypatch):
+    """https://github.com/borgbackup/borg/pull/6469#discussion_r832670411"""
+    repository = MagicMock(id=b'repository_id')
+    getpass.return_value = "hello, pass phrase"
+    monkeypatch.setenv('BORG_DISPLAY_PASSPHRASE', 'no')
+    RepoKey.create(repository, args=MagicMock(key_algorithm='argon2'))
+    repository.load_key.return_value = repository.save_key.call_args.args[0]
+
+    # 1. detect() tries an empty passphrase first before prompting the user
+    # 2. load() was throwing integrity errors instead of returning None due to a bug


can you check this test again? it somehow unclear what it is doing / what it is checking. Also the comment is not very clear as it refers to functions you are not calling (directly).

hexagonrecursion · 2022-04-07T04:43:32Z

@ThomasWaldmann I have addressed your comments.

src/borg/testsuite/crypto.py

ThomasWaldmann · 2022-04-07T14:22:51Z

Thanks, merged!

hexagonrecursion force-pushed the argon2-decrypt branch from 280eb43 to b8a1369 Compare March 19, 2022 13:51

hexagonrecursion changed the title ~~WIP: Argon2 part 1: implement decryption of v2 keys~~ Argon2 part 1: implement decryption of v2 keys Mar 19, 2022

ThomasWaldmann reviewed Mar 19, 2022

View reviewed changes

ThomasWaldmann added this to the helium milestone Mar 20, 2022

hexagonrecursion changed the title ~~Argon2 part 1: implement decryption of v2 keys~~ Argon2 the second part: implement decryption of v2 keys (was: part 1) Mar 21, 2022

hexagonrecursion force-pushed the argon2-decrypt branch from c5f7181 to 3922314 Compare March 22, 2022 06:09

ThomasWaldmann reviewed Mar 22, 2022

View reviewed changes

src/borg/crypto/key.py Outdated Show resolved Hide resolved

src/borg/crypto/key.py Outdated Show resolved Hide resolved

src/borg/item.pyx Show resolved Hide resolved

src/borg/testsuite/crypto.py Outdated Show resolved Hide resolved

hexagonrecursion marked this pull request as ready for review March 22, 2022 18:52

hexagonrecursion requested a review from ThomasWaldmann March 22, 2022 18:52

ThomasWaldmann reviewed Mar 22, 2022

View reviewed changes

src/borg/testsuite/crypto.py Outdated Show resolved Hide resolved

ThomasWaldmann reviewed Mar 22, 2022

View reviewed changes

src/borg/crypto/key.py Outdated Show resolved Hide resolved

ThomasWaldmann reviewed Mar 22, 2022

View reviewed changes

src/borg/crypto/key.py Outdated Show resolved Hide resolved

hexagonrecursion marked this pull request as draft March 23, 2022 08:33

hexagonrecursion changed the title ~~Argon2 the second part: implement decryption of v2 keys (was: part 1)~~ Argon2 the second part: implement encryption and decryption of v2 keys (was: part 1) Mar 23, 2022

hexagonrecursion mentioned this pull request Mar 25, 2022

Fix use-after-free in borg benchmark cpu #6484

Closed

hexagonrecursion force-pushed the argon2-decrypt branch from 040e6be to 46243e1 Compare March 27, 2022 08:23

hexagonrecursion added 3 commits March 27, 2022 12:13

add a roundtrip test

5dd38c0

Make algorithm mandatory

c092bf2

Implement encryption

40dc9be

hexagonrecursion added 2 commits April 4, 2022 17:07

Remove redundant PBKDF2_ITERATIONS monkeypatch

40c5f4d

Tests: weaken the KDF instead of skipping it

24010a6

hexagonrecursion force-pushed the argon2-decrypt branch from 0e4d072 to 24010a6 Compare April 5, 2022 04:39

ThomasWaldmann reviewed Apr 5, 2022

View reviewed changes

hexagonrecursion added 2 commits April 6, 2022 05:00

typo

b5b3d43

Only weaken KDF if env var == "1"

d6474e9

hexagonrecursion requested a review from ThomasWaldmann April 6, 2022 02:10

ThomasWaldmann reviewed Apr 6, 2022

View reviewed changes

hexagonrecursion added 4 commits April 7, 2022 07:09

Clarify the purpose of the test

3c5c197

Use consistent argon2 paramaters in a test

2e1de16

Test that pbkdf2 is not autoupgraded

721edc9

Pass arguments consistently in tests

1aa652e

ThomasWaldmann reviewed Apr 7, 2022

View reviewed changes

src/borg/testsuite/crypto.py Show resolved Hide resolved

ThomasWaldmann merged commit 56c27a9 into borgbackup:master Apr 7, 2022

hexagonrecursion deleted the argon2-decrypt branch April 7, 2022 17:31

hexagonrecursion mentioned this pull request Apr 8, 2022

Argon2 the third part: borg key change-algorithm #6549

Merged

ThomasWaldmann mentioned this pull request Apr 10, 2022

Using a more modern KDF than PBKDF2 #747

Closed

		if os.environ.get("BORG_TESTONLY_WEAKEN_KDF") is not None:
		iterations = 1

Argon2 the second part: implement key encryption / decryption #6469

Argon2 the second part: implement key encryption / decryption #6469

Conversation

hexagonrecursion commented Mar 19, 2022 • edited Loading

codecov-commenter commented Mar 19, 2022 • edited Loading

Codecov Report

hexagonrecursion commented Mar 19, 2022

hexagonrecursion commented Mar 19, 2022

hexagonrecursion commented Mar 19, 2022

ThomasWaldmann commented Mar 19, 2022

hexagonrecursion commented Mar 22, 2022

ThomasWaldmann commented Mar 22, 2022

hexagonrecursion commented Mar 23, 2022

hexagonrecursion commented Mar 27, 2022 • edited Loading

ThomasWaldmann commented Mar 27, 2022

hexagonrecursion commented Apr 3, 2022

ThomasWaldmann commented Apr 3, 2022 • edited Loading

hexagonrecursion commented Apr 4, 2022 • edited Loading

ThomasWaldmann commented Apr 4, 2022 • edited Loading

hexagonrecursion commented Apr 4, 2022 • edited Loading

ThomasWaldmann commented Apr 4, 2022

hexagonrecursion commented Apr 5, 2022 • edited Loading

ThomasWaldmann commented Apr 5, 2022

hexagonrecursion commented Apr 5, 2022

hexagonrecursion commented Apr 5, 2022

hexagonrecursion commented Apr 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasWaldmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hexagonrecursion commented Apr 7, 2022

ThomasWaldmann commented Apr 7, 2022

hexagonrecursion commented Mar 19, 2022 •

edited

Loading

codecov-commenter commented Mar 19, 2022 •

edited

Loading

hexagonrecursion commented Mar 27, 2022 •

edited

Loading

ThomasWaldmann commented Apr 3, 2022 •

edited

Loading

hexagonrecursion commented Apr 4, 2022 •

edited

Loading

ThomasWaldmann commented Apr 4, 2022 •

edited

Loading

hexagonrecursion commented Apr 4, 2022 •

edited

Loading

hexagonrecursion commented Apr 5, 2022 •

edited

Loading