WIP Add file authentication #1688

enkore · 2016-10-06T11:29:27Z

First the implementation only; I'll have another idea or two here to make it safer in the ways we'll be using it, especially in the hashindex (C) code. Then I'll integrate it in the various spots that need it:

Gist:

Create an accompanying .bsi (_b_org _si_gnature, didn't want to use .sig due to PGP use). Contains signature algo + value in JSON.
When reading, signature state is continuously updated. When done, signatures are compared and appropiate errors are raised.

This has the advantage that the file doesn't have to be read twice and has no races that would be associated with reading twice.

~~The disadvantage is that invalid data may be processed (but I have an idea about that).~~ [1]
When writing, the same thing happens
Both the file name (but not the path) and the file length are encoded into the signature

Implementation is HMAC-SHA512 for now. Since most of these places we'll use it (indices, caches) will be rather large objects I didn't bother to add low-level OpenSSL wrappers for now.

[1] d46f3be solves this. By allowing us to sign parts of a file independently as we go (header and data in hashindex for example) this allows us to sanitize-by-signing parts of the file we need to work with later parts of the file.

If eg. the header were corrupted and says we got a gazillion entries, we can detect this now immediately, because the header can be signed independently and thus verified immediately.

codecov-io · 2016-10-06T12:15:06Z

Current coverage is 84.58% (diff: 95.45%)

Merging #1688 into master will decrease coverage by 0.08%

@@             master      #1688   diff @@
==========================================
  Files            20         21     +1   
  Lines          6599       6706   +107   
  Methods           0          0          
  Messages          0          0          
  Branches       1121       1140    +19   
==========================================
+ Hits           5587       5672    +85   
- Misses          740        759    +19   
- Partials        272        275     +3

Powered by Codecov. Last update 8567fcf...e61d8c1

ThomasWaldmann · 2016-10-06T12:10:02Z

src/borg/crypto.pyx

+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        # Sign length of file as well to avoid problems if only a prefix is read.
+        self.seek(0, io.SEEK_END)


check for exception here? guess you only want to sign if there was no exception that triggered leaving the scope.

ThomasWaldmann · 2016-10-06T12:13:25Z

src/borg/crypto.pyx

+    def signature(self):
+        """
+        Return final signature bytestring; further read()s or write()s are illegal.
+        """


raise NotImplemented ?

ThomasWaldmann · 2016-10-06T12:25:29Z

src/borg/key.py

+        # Changing the name however imbues a change of context that is not permissible.
+        filename = os.path.basename(self.path)
+        self.signer.update(b'%d' % len(filename))
+        self.signer.update(filename.encode())


well, we don't need it for borg filenames, but it does no harm either: .encode('utf-8')

"utf-8" is the default value for encode.

oh, rly? I thought it was ascii.

IIRC the default was adjustable on Python 2 (derived by locale, even?), but in 3 it's always utf-8. Sane defaults are nice :)

(btw we can clean those encode(utf8), decode(utf8) up some time)

ThomasWaldmann · 2016-10-06T12:29:19Z

src/borg/key.py

+            with open(signature_path, 'r') as fd:
+                signature = json.load(fd)
+                # Provisions for agility now, implementation later, but make sure the on-disk joint is oiled.
+                algorithm = signature['algorithm']


if the json does not have this key, you'll get a KeyError here.

These are caught below (OSError, ValueError, TypeError, LookupError); I can change the latter into KeyError (instead of LookupError - I thought indexing a list with a string would be IndexError, but it's actually TypeError), so this is more visible.

ThomasWaldmann · 2016-10-06T13:55:01Z

src/borg/testsuite/crypto.py

+        with ss:
+            assert ss.read() == data
+        assert ss.signature() == unhexlify('9f22d4ccc869fc0e697bcd8780ec7e24efa8d5c862367be393204bca6f2f0e0b'
+                                           '8992ff67d54e789f9e658bef65e0bcdf10afeaaec86a5c96a54f3a6cde083532')


use same key and data for read/write tests to show roundtrip?

ThomasWaldmann · 2016-10-06T14:09:05Z

src/borg/key.py

+
+
+class SignedFile(FileLikeWrapper):
+    # generic enough that it can be used without a KeyBase


put it into helpers.py? I'ld expect some trouble with circular dependencies when put here.

ThomasWaldmann · 2016-10-06T14:19:50Z

about *.bsi: sounds like one can not trust it. :-D

other ideas: .check, .sign(ature), .mac

enkore · 2016-10-06T15:08:43Z

.cyber? (Or would that be .cyberprotect?) Edit: Oh God, there's actually something with that name around!

Go safely into the digital corporate future with Allianz Cyber Protect Premium
...
You are vulnerable like never before to Cyber attacks, identity theft, Internet sabotage or data loss
...
"Hacker attacks become more directed, lasting and fatal", says Jens Krickhahn, AGCS expert for Cyberinsurance

Cybering intensifies

Anyhow.

.signature should work ok - this stuff is nowhere near 8.3 compatible anyway.

ThomasWaldmann · 2016-10-06T16:55:59Z

src/borg/crypto.pyx

+
+    WARNING: Seeks should only be used to query the size of the file, not
+    to skip data, because skipped data isn't read and not signed.
+


Maybe one should note that reading and writing should be in same order (usually writing the whole file in a linear manner) - otherwise the signature won't match. Also, seeks to create holes are not allowed.

ThomasWaldmann · 2016-10-06T17:27:26Z

src/borg/testsuite/signature.py

+class TestSignedFile:
+    @pytest.fixture
+    def key(self):
+        return bytes(1234)


any special reason to use 1234 zero bytes? Usually a hmac-sha512 key is 512bits (64 bytes).

ThomasWaldmann · 2016-10-06T17:28:06Z

src/borg/testsuite/signature.py

+class TestSignedFileParts:
+    @pytest.fixture
+    def key(self):
+        return bytes(1234)


ThomasWaldmann · 2016-10-06T18:00:49Z

src/borg/signature.py

@@ -0,0 +1,84 @@
+


remove empty line

ThomasWaldmann · 2016-10-06T18:03:11Z

src/borg/signature.py

+                # Provisions for agility now, implementation later, but make sure the on-disk joint is oiled.
+                algorithm = signature['algorithm']
+                if algorithm != signer.NAME:
+                    logger.info('Cannot verify signature for %s: Unknown algorithm %r', path, algorithm)


ThomasWaldmann · 2016-10-06T18:04:40Z

src/borg/testsuite/crypto.py

+        data = unhexlify('12345678')
+        fd = io.BytesIO(data)
+        key = b'\x0b' * 20
+        ss = StreamSigner_HMAC_SHA512(key, fd, write=True)


write=True?

enkore · 2016-10-06T19:08:40Z

I'm going to remove the keying part for now completely, because that requires changes to borg.key I don't want to do at all right here / right now.

ThomasWaldmann · 2016-10-06T19:37:46Z

src/borg/repository.py

@@ -13,6 +13,8 @@
 import msgpack

 import logging
+
+


remove empty lines?

ThomasWaldmann · 2016-10-06T19:42:04Z

src/borg/_hashindex.c

@@ -1,3 +1,4 @@
+#include <Python.h>


somehow all that Py* stuff isn't pretty. can we avoid by moving this to cython?

Moving it to Cython means exposing most C structures to Cython and staring at tons of generated C code to see that it actually does the right thing (like in #891). I wouldn't do it bundled together with a functional change.

RonnyPfannschmidt · 2016-10-07T07:54:13Z

isn't it sufficient to collect an hash and sign the results, that way the hash the user decided on would also be obtainable at zero cost

enkore · 2016-10-07T09:06:40Z

I don't understand what you're trying to say... can you explain? What do you mean by the user deciding on a hash? How is it zero cost?

enkore · 2016-10-07T09:15:25Z

Current state: Some minor issues to work out in the tests, seems to work well in real life. In some spots (esp. Repository.write_index) this made some duplication more visible, I'm considering to replace these with SaveFile maybe. (On the other hand the file emplacement done in write_index doesn't do much either). Same goes for Cache.

Needs some additional tests to actually check correct handling of signatures from the other code (cache, repo).

The HashIndex tests could now avoid writing to temporary files, which would be nice, because they're used in the self test.

RonnyPfannschmidt · 2016-10-07T10:08:54Z

@enkore instead of making a signature for files, just make a supported crypto hash of the content,

then sign the hash/collection of all hashes, that way obtaining that particular crypto hash is zero cost

enkore · 2016-10-07T10:44:33Z

@enkore instead of making a signature for files, just make a supported crypto hash of the content,

The latter part has always to happen, and takes most of the time: eg. hash a 200 MB index file @ 350 MB/s = 0.6 s CPU time. Making a public key signature (which we don't right now and probably won't for some time?) is comparatively little work; the same CPU does ~160 RSA 4096 bit signatures/s and around 10000 verifications/s. RSA is pretty much the slowest system anyone would consider (DSA is slower, but it's DSA), and others are an order of magnitude faster or so (Ed25519).

Besides having to make fewer signatures I don't really see any advantage, but another increase in complexity.

-> not locked -> index broken, replay needed -> tries to check self.lock -> but it's None - Repository wasn't locked in the first place

enkore · 2016-11-07T19:40:25Z

src/borg/platform/base.py

@@ -89,7 +90,8 @@ def __enter__(self):
        return self

    def __exit__(self, exc_type, exc_val, exc_tb):
-        self.close()
+        self.fd.__exit__(exc_type, exc_val, exc_tb)
+        #self.close()


XXX I really have to dive into this and see what really is correct

enkore · 2016-11-07T19:40:35Z

src/borg/platform/base.py


    def __exit__(self, exc_type, exc_val, exc_tb):
        from .. import platform
-        self.fd.close()
+        self.fd.__exit__(exc_type, exc_val, exc_tb)
+        #self.fd.close()


ditto

XXX I really have to dive into this and see what really is correct

- Preparation for borgbackup#1688 / borgbackup#1101 - Support hash indices >2 GB - Better error reporting

- Preparation for #1688 / #1101 - Support hash indices >2 GB - Better error reporting

This is based on much earlier work from October 2016 by me, but is overall simplified and changed terminology (from "signing" to hashing and integrity checking). See borgbackup#1688 for the full history.

enkore · 2017-05-11T10:58:18Z

It's time to close this one.

Implement IntegrityCheckedFile This is based on much earlier work from October 2016 by me, but is overall simplified and changed terminology (from "signing" to hashing and integrity checking). See #1688 for the full history.

ThomasWaldmann reviewed Oct 6, 2016

View reviewed changes

enkore added 2 commits October 7, 2016 10:13

Add crypto.StreamSigner +impl and key.SignedFile

e645e76

HashIndex: Use SignedFile

3540773

enkore force-pushed the issue/1101 branch from 6d195bd to 3540773 Compare October 7, 2016 08:45

enkore added 2 commits October 7, 2016 11:22

Avoid printing "Verified signature" if there actually was none.

610892a

cache -> wrong naming of archive caches

2beaa61

enkore changed the title ~~(WIP) Add file signing~~ ⚠️ WIP Add file signing Oct 9, 2016

enkore changed the title ~~⚠️ WIP Add file signing~~ WIP Add file signing Oct 9, 2016

enkore self-assigned this Oct 10, 2016

enkore mentioned this pull request Nov 5, 2016

hashindex size limitations #1809

Closed

enkore added 5 commits November 7, 2016 20:18

fixup test failure

63f8ac6

key import: acquire exclusive lock

afd8ba4

Fix locking logic bug in Repository recovery code if lock=False

9aa0ada

-> not locked -> index broken, replay needed -> tries to check self.lock -> but it's None - Repository wasn't locked in the first place

Cache: don't leave lock around if initial _do_open raises

2da5da5

SignedFile: short-circuit if reading and no signature exists

e61d8c1

enkore commented Nov 7, 2016

View reviewed changes

ThomasWaldmann added this to the 1.1.0rc1 milestone Nov 24, 2016

textshell changed the title ~~WIP Add file signing~~ WIP Add file authentication Nov 28, 2016

ThomasWaldmann modified the milestones: 1.1.0b4, 1.1.0rc1 Jan 12, 2017

ThomasWaldmann modified the milestones: 1.1.0b5, 1.1.0b4 Mar 4, 2017

ThomasWaldmann modified the milestones: 1.1.0b6, 1.1.0b5 Apr 24, 2017

enkore added a commit to enkore/borg that referenced this pull request May 9, 2017

hashindex: Use Python I/O

e7972b6

- Preparation for borgbackup#1688 / borgbackup#1101 - Support hash indices >2 GB - Better error reporting

enkore mentioned this pull request May 9, 2017

hashindex: Use Python I/O #2496

Merged

enkore added a commit to enkore/borg that referenced this pull request May 9, 2017

hashindex: Use Python I/O

6e56a3b

- Preparation for borgbackup#1688 / borgbackup#1101 - Support hash indices >2 GB - Better error reporting

enkore added a commit to enkore/borg that referenced this pull request May 9, 2017

hashindex: Use Python I/O

dc15cfe

- Preparation for borgbackup#1688 / borgbackup#1101 - Support hash indices >2 GB - Better error reporting

enkore added a commit to enkore/borg that referenced this pull request May 9, 2017

hashindex: Use Python I/O

f125bbe

- Preparation for borgbackup#1688 / borgbackup#1101 - Support hash indices >2 GB - Better error reporting

enkore added a commit that referenced this pull request May 9, 2017

hashindex: Use Python I/O (#2496)

6cd7d41

- Preparation for #1688 / #1101 - Support hash indices >2 GB - Better error reporting

enkore mentioned this pull request May 11, 2017

Implement IntegrityCheckedFile #2502

Merged

enkore closed this May 11, 2017

enkore mentioned this pull request May 25, 2017

1101.integration.cache #2568

Merged

This was referenced Jun 1, 2017

1101.integration.xxh #2580

Merged

repository: checksum index and hints #2584

Merged

Protect indices and other auxiliary files with checksums #1101

Closed

docs: file integrity #2591

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Add file authentication #1688

WIP Add file authentication #1688

enkore commented Oct 6, 2016 •

edited

Loading

codecov-io commented Oct 6, 2016 •

edited

Loading

ThomasWaldmann Oct 6, 2016

ThomasWaldmann Oct 6, 2016

ThomasWaldmann Oct 6, 2016

enkore Oct 6, 2016 •

edited

Loading

ThomasWaldmann Oct 6, 2016

enkore Oct 6, 2016 •

edited

Loading

ThomasWaldmann Oct 6, 2016

enkore Oct 6, 2016 •

edited

Loading

ThomasWaldmann Oct 6, 2016

enkore Oct 6, 2016

ThomasWaldmann Oct 6, 2016

ThomasWaldmann commented Oct 6, 2016

enkore commented Oct 6, 2016 •

edited

Loading

ThomasWaldmann Oct 6, 2016 •

edited

Loading

ThomasWaldmann Oct 6, 2016

ThomasWaldmann Oct 6, 2016

ThomasWaldmann Oct 6, 2016

ThomasWaldmann Oct 6, 2016

ThomasWaldmann Oct 6, 2016

enkore commented Oct 6, 2016 •

edited

Loading

ThomasWaldmann Oct 6, 2016

ThomasWaldmann Oct 6, 2016

enkore Oct 7, 2016 •

edited

Loading

RonnyPfannschmidt commented Oct 7, 2016

enkore commented Oct 7, 2016

enkore commented Oct 7, 2016 •

edited

Loading

RonnyPfannschmidt commented Oct 7, 2016

enkore commented Oct 7, 2016 •

edited

Loading

enkore Nov 7, 2016

enkore Nov 7, 2016

enkore commented May 11, 2017



		class SignedFile(FileLikeWrapper):
		# generic enough that it can be used without a KeyBase


		WARNING: Seeks should only be used to query the size of the file, not
		to skip data, because skipped data isn't read and not signed.

WIP Add file authentication #1688

WIP Add file authentication #1688

Conversation

enkore commented Oct 6, 2016 • edited Loading

codecov-io commented Oct 6, 2016 • edited Loading

Current coverage is 84.58% (diff: 95.45%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enkore Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enkore Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enkore Oct 6, 2016 • edited Loading

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 commented Oct 6, 2016

enkore commented Oct 6, 2016 • edited Loading

ThomasWaldmann Oct 6, 2016 • edited Loading

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

enkore commented Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enkore Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

RonnyPfannschmidt commented Oct 7, 2016

enkore commented Oct 7, 2016

enkore commented Oct 7, 2016 • edited Loading

RonnyPfannschmidt commented Oct 7, 2016

enkore commented Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enkore commented May 11, 2017

enkore commented Oct 6, 2016 •

edited

Loading

codecov-io commented Oct 6, 2016 •

edited

Loading

enkore Oct 6, 2016 •

edited

Loading

enkore Oct 6, 2016 •

edited

Loading

enkore Oct 6, 2016 •

edited

Loading

enkore commented Oct 6, 2016 •

edited

Loading

ThomasWaldmann Oct 6, 2016 •

edited

Loading

enkore commented Oct 6, 2016 •

edited

Loading

enkore Oct 7, 2016 •

edited

Loading

enkore commented Oct 7, 2016 •

edited

Loading

enkore commented Oct 7, 2016 •

edited

Loading