Integrate encryption into dpa and chunker #327

gbalint · 2018-03-14T12:00:53Z

This PR is the integration of the encrpytion package into DPA and Chunker.

The goal was to encapsulate the encryption/decryption into a Putter and Getter interface, which stores and retrieves chunks.
Putter and Getter works with byte slices and not Chunk objects. So it handles the hashing and the Chunk creation. So all this can be removed from chunker, which can just directly use Putter.Put and Getter.Get with byte slices to store and retrieve chunks.
There is a HasherStore implementation of Putter and Getter which decides on whether to decrypt or not base on key length. One HasherStore instance is used for just one chunk storage/retrieval, so it can be initialized whether to encrypt or not when putting.
Another change is that the chunker objects are not reused anymore for multiple parallel split/join/append requests. This made the code much simpler, because the object itself can store the data necessary for the chunking task, no need to pass down a lot of parameters to the function calls.

fixes #270 #280 #281 #289

zelig · 2018-03-24T12:54:28Z

swarm/storage/dpa.go

-	self.running = false
-	close(self.quitC)
-}
+// func (self *DPA) Start() {


Fixed in db1b51a

zelig · 2018-03-25T01:41:12Z

swarm/storage/pyramid.go

@@ -195,6 +189,9 @@ func (self *PyramidChunker) Split(data io.Reader, size int64, chunkC chan *Chunk
 	}()

 	defer close(quitC)
+	defer func() {


no need for func() {}() see line above

Fixed in f32d4cc

zelig · 2018-03-25T01:46:25Z

swarm/storage/pyramid.go

@@ -203,28 +200,25 @@ func (self *PyramidChunker) Split(data io.Reader, size int64, chunkC chan *Chunk
 		}
 	case <-time.NewTimer(splitTimeout).C:
 	}
-	return rootKey, storageWG.Wait, nil
+	return rootKey, func() { return }, nil


no need for return

This was before wait was implemented hasherstore. Proper wait implementation is added in f32d4cc

zelig · 2018-03-25T21:45:06Z

swarm/storage/chunker.go

-		key:       key,
-		chunkC:    chunkC,
+		key: key,
+		// chunkC:    chunkC,


remove commented line

Fixed in f32d4cc

zelig · 2018-03-25T21:45:47Z

swarm/storage/chunker.go

-		key:       key,
-		chunkC:    chunkC,
+		key: key,
+		// chunkC:    chunkC,
 		chunkSize: self.chunkSize,
 		branches:  self.branches,
 		hashSize:  self.hashSize,
 		depth:     depth,


comment on depth should be added (TODO)?

Fixed in 582c6e5

zelig · 2018-03-25T21:48:07Z

swarm/storage/hasherstore.go

+	}
+}
+
+func NewHasherStore(chunkStore ChunkStore, hashFunc SwarmHasher, toEncrypt bool) *hasherStore {


comment exported functions

Fixed in 8016f18

zelig · 2018-03-25T21:48:22Z

swarm/storage/hasherstore.go

+
+	return Reference(append(chunk.Key, encryptionKey...)), nil
+
+	//TODO: implement wait for storage


I removed this and added proper wait implementation in f32d4cc

zelig · 2018-03-25T21:50:30Z

swarm/storage/pyramid.go

@@ -494,8 +483,8 @@ func (self *PyramidChunker) prepareChunks(isAppend bool, chunkLevel [][]*TreeEnt
 		workers := self.getWorkerCount()
 		if int64(len(jobC)) > workers && workers < ChunkProcessors {
 			self.incrementWorkerCount()
-			storageWG.Add(1)
-			go self.processor(self.workerCount, jobC, chunkC, errC, quitC, storageWG)
+			// storageWG.Add(1)


Fixed in f32d4cc

This is not a fully functional change. Chunker api changed: it uses Putter and Getter interfaces to put and get chunks from the store and to do the encryption/decryption if needed. These putters and getters should also the hashing, so hashing related code could be removed from chunker. Some things are temporary, pyramid chunker doesn't work yet (only tree chunker), and hashing and putting in dpa is not parallelized.

PyramydChunker doesn't work yet with Putter/Getter

creation This fixes the tests with pyramid chunker

Chunker code is much simpler if all data of the job is stored on the chunker object itself.

nonsense · 2018-03-26T13:19:05Z

swarm/storage/chunker_test.go

@@ -263,7 +174,7 @@ func TestSha3ForCorrectness(t *testing.T) {

 }

-func TestDataAppend(t *testing.T) {


@gbalint why is this commented out?

@nonsense accident, I reenabled it in d8ee6a1

nonsense · 2018-03-26T13:20:04Z

swarm/storage/hasherstore.go

@@ -0,0 +1,223 @@
+// Copyright 2016 The go-ethereum Authors


Bump up copyright to 2018.

@nonsense Fixed in 46e9832

nonsense · 2018-03-26T13:28:08Z

swarm/storage/hasherstore.go

+	return Reference(append(chunk.Key, encryptionKey...)), nil
+}
+
+// Returns data of the chunk with the given reference (retrieved from the ChunkStore of hasherStore).


Nitpick, but godoc comments should start with the name of the of the symbol described. Applies to multiple places. https://blog.golang.org/godoc-documenting-go-code

@nonsense Sure, it is not a nitpick, I'm happy to receive these kind of review comments. It is fixed in 46e9832

nonsense · 2018-03-26T13:28:29Z

swarm/storage/hasherstore.go

+	return chunkData, nil
+}
+
+// The Close() indicates that no more chunks will be put with the hasherStore, so the Wait


Close indicates...

Fixed in 2329cfb

nonsense · 2018-03-26T13:38:25Z

swarm/storage/hasherstore.go

+// NewHasherStore creates a hasherStore object, which implements Putter and Getter interfaces.
+// With the HasherStore you can put and get chunk data (which is just []byte) into a ChunkStore
+// and the hasherStore will take core of encryption/decryption of data if necessary
+func NewHasherStore(chunkStore ChunkStore, hashFunc SwarmHasher, toEncrypt bool) *hasherStore {


I think HasherStore is a good candidate for a few unit tests. It is a new type, and having a bunch of simple unit tests would help documenting it and reduce the cognitive load on readers, since a few simple examples of actual use of the type speak much more than any comments.

I totally agree, I'm adding a few

Unit test added in cdc7bed

Very nice. I would however suggest we do this with table-driven tests in order to have idiomatic go code. Additional benefit is:

Less overhead of having to think about multiple methods.

Input params to tests are named.

https://gist.github.com/nonsense/87ff83e0a76f3b75372f5f3c52dbc439

What do you think?

There is only a slight difference in this test, as both are quite simple, but if we have more parameters, naming them makes it much easier to reason about the test.

https://github.com/golang/go/wiki/TableDrivenTests

@nonsense Nice, I committed it in ba258d7

gbalint · 2018-03-26T13:40:44Z

swarm/storage/hasherstore.go

+
+func newChunkEncryption() *chunkEncryption {
+	return &chunkEncryption{
+		spanEncryption: encryption.New(0, math.MaxUint32, sha3.NewKeccak256),


Instead of MaxUint32 let's use the branches value

Fixed in b737d49

nonsense · 2018-03-26T13:40:48Z

swarm/storage/hasherstore.go

+
+func (h *hasherStore) storeChunk(chunk *Chunk) {
+	// need to do this parallelly
+	if h.store != nil {


Why are we checking for h.store? Shouldn't we just assume it is there?

There is one place where we use it with nil, because we only need it for hashing: https://github.com/ethersphere/go-ethereum/blob/swarm-network-rewrite-encryption/cmd/swarm/hash.go#L41
This cmd line tool just gets the hash without storing data. So I think it makes sense to have a nil store if you only need the "hasher" functionality, but not the "store".

nolash · 2018-03-26T18:36:33Z

5 of my comments got hidden I see. I don't know what that means...

janos

Just a suggestion for dpa Start and Stop functions. If the intension is to reintroduce workers for the dpa, would it be easer to make this methods noop and not remove them from where they are called. Or maybe to even remove the Start func and start the processing loop in the constructor, and keep only the Stop.

janos · 2018-03-27T09:43:03Z

swarm/storage/chunker_test.go

-		err = fmt.Errorf("Append error: %v", err)
-	}
+func (t *testChunkStore) Close() {
+	return


Remove the return statement.

@janos Fixed in 49a4d9c

janos · 2018-03-27T12:32:13Z

swarm/storage/dpa_test.go

-	dpa.Start()
-	defer dpa.Stop()
+
+	dpa := NewDPA(localStore, NewDPAParams())
 	defer os.RemoveAll("/tmp/bzz")


Shouldn't this be defer os.RemoveAll(tdb.dir)? I know that this is the old code, but this test may be improved here as well.

Instead of using a nil ChunkStore in hasherStore when we don't want the hasherStore to actually store the chunk data, now a FakeChunkStore instance should be used. Because of this the nil checks of the hasherStore.store could be removed.

The branches what is changing, and it is dependent on chunk size and hash size

gbalint · 2018-03-28T11:18:30Z

@nolash false is hardcoded as toEncrypt only temporarily, because this PR does not yet contain any code to wire the new encryption feature of DPA to api.go or server.go. That will happen in a different API

gbalint · 2018-03-28T11:20:20Z

@janos There is no need to reintroduce workers into DPA, as DPA does not do storage anymore. There are workers in chunker which call hasherStore Put/Get parallelly, and the store itself is asynchronous. So even in hasherStore there is no need for workers in my opinion.

gbalint · 2018-03-28T11:21:06Z

@nolash A comment is hidden if the code which you commented on has been changed. But I'm checking the hidden comments too...

It was in pyramid.go, although TreeSplitter is in chunker.go

incrementWorkerCount can be moved into runWorker remove obsolete comment

Depth can only be 0 because of a bug: #344

There was a bug that data was not lengthened to padding

nonsense · 2018-03-29T11:31:28Z

swarm/storage/chunker_test.go

-
-	reader := chunker.Join(key, chunkC, 0)
-	return reader
+func newTestHasherStore(chunkStore ChunkStore, hash string) *hasherStore {


Nitpick, but I think this method is redundant - just use directly the NewHasherStore.

nonsense

provided tests are passing - LGTM. there are many small things we can amend or refactor in the future, but I think this PR is already an improvement.

janos

Minor comments, but the PR is 👍

janos · 2018-03-29T12:08:53Z

swarm/storage/hasherstore.go

+}
+
+func parseReference(ref Reference) (Key, encryption.Key, error) {
+	encryptedKeyLength := KeyLength + encryption.KeyLength


KeyLength and encryption.KeyLength are constants, encryptedKeyLength can go outside of the function.

@janos I decided to make it a parameter as KeyLength is not constant, but the hashSize, which depends on the hash function, see 0bfe320

janos · 2018-03-29T12:22:13Z

swarm/storage/types.go

@@ -206,7 +207,7 @@ func (c *Chunk) WaitToStore() {
 func FakeChunk(size int64, count int, chunks []*Chunk) int {
 	var i int
 	hasher := MakeHashFunc(SHA3Hash)()
-	chunksize := getDefaultChunkSize()
+	chunksize := DefaultChunkSize


chunksize is not needed, it can be replaced with just DefaultChunkSize in two lines bellow.

@janos Fixed in 0bfe320

…entation It is not just the test which needs this kind of implementation

…ption

gbalint added in progress encryption labels Mar 14, 2018

gbalint requested review from nonsense, janos and zelig March 21, 2018 14:48

zelig suggested changes Mar 25, 2018

View reviewed changes

gbalint added 5 commits March 26, 2018 10:34

swarm/storage: Use TreeChunker in DPA

c9e5f37

PyramydChunker doesn't work yet with Putter/Getter

swarm/storage: HasherStore needs a new hasher instance on every chunk

5e70f70

creation This fixes the tests with pyramid chunker

swarm/storage: Add back tree/pyramid comparison in chunker test

1827520

swarm: Refactor chunker, new chunker instance is needed for each job

f32d4cc

Chunker code is much simpler if all data of the job is stored on the chunker object itself.

gbalint force-pushed the swarm-network-rewrite-encryption branch from cc05f60 to f32d4cc Compare March 26, 2018 08:37

gbalint changed the title ~~[WIP] Integrate encryption into dpa and chunker~~ Integrate encryption into dpa and chunker Mar 26, 2018

swarm/storage: Remove commented code

db1b51a

gbalint added ready for review and removed in progress labels Mar 26, 2018

gbalint requested a review from nolash March 26, 2018 08:58

swarm/storage: Added comments

8016f18

gbalint removed the ready for review label Mar 26, 2018

cmd/swarm: Remove Branches from config

8a6cf6f

gbalint added the ready for review label Mar 26, 2018

swarm/storage: Remove commented code

132b3fc

gbalint requested a review from nagydani March 26, 2018 12:54

nonsense reviewed Mar 26, 2018

View reviewed changes

gbalint commented Mar 26, 2018

View reviewed changes

nonsense reviewed Mar 26, 2018

View reviewed changes

janos reviewed Mar 27, 2018

View reviewed changes

gbalint added 5 commits March 27, 2018 15:39

swarm/storage: Reenable chunker_test.TestDataAppend

d8ee6a1

swarm/storage: Fix comments

46e9832

swarm/storage, cmd/swarm: Add FakeChunkStore

c03ae51

Instead of using a nil ChunkStore in hasherStore when we don't want the hasherStore to actually store the chunk data, now a FakeChunkStore instance should be used. Because of this the nil checks of the hasherStore.store could be removed.

swarm/storage: Use DefaultChunkSize az constant, not DefaultBranches

68cf01e

The branches what is changing, and it is dependent on chunk size and hash size

swarm/storage: Remove unnecessary return statement

49a4d9c

gbalint added 9 commits March 28, 2018 14:01

swarm/storage: Rename unfinishedChunk to unfinishedChunkData

3358a59

swarm/storage: Move NewTreeSplitterParams to chunker.go

484e22c

It was in pyramid.go, although TreeSplitter is in chunker.go

swarm/storage: Minor cleanup

b0ab49f

incrementWorkerCount can be moved into runWorker remove obsolete comment

swarm/storage: Add TODO comment to chunker depth parameter

582c6e5

Depth can only be 0 because of a bug: #344

swarm/storage: Fix padding in hasherStore

534217f

There was a bug that data was not lengthened to padding

swarm/storage: Add unit test for hasherStore

cdc7bed

swarm/storage: Decrease initial counter in span encryption

b737d49

swarm/storage: Renames in benchmark tests

c59a6a4

swarm/storage: Fixed pyramid benchmark test

bc32751

gbalint added ready for another review and removed ready for review labels Mar 29, 2018

nonsense reviewed Mar 29, 2018

View reviewed changes

nonsense approved these changes Mar 29, 2018

View reviewed changes

swarm/storage: Convert TestHasherStore to table-driven test

ba258d7

janos approved these changes Mar 29, 2018

View reviewed changes

gbalint added 3 commits March 29, 2018 15:20

swarm/storage, cmd/swarm: New MapChunkStore, simple ChunkStore implem…

6c5a075

…entation It is not just the test which needs this kind of implementation

swarm/storage: Cleanup on hashSize/chunkSize handling

0bfe320

Merge branch 'swarm-network-rewrite' into swarm-network-rewrite-encry…

e187fd0

…ption

gbalint merged commit 167159b into swarm-network-rewrite Mar 30, 2018

gbalint deleted the swarm-network-rewrite-encryption branch March 30, 2018 10:40


		return Reference(append(chunk.Key, encryptionKey...)), nil

		//TODO: implement wait for storage

		@@ -263,7 +174,7 @@ func TestSha3ForCorrectness(t *testing.T) {

		}

		func TestDataAppend(t *testing.T) {

Integrate encryption into dpa and chunker #327

Integrate encryption into dpa and chunker #327

Conversation

gbalint commented Mar 14, 2018 • 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

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

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

nonsense Mar 26, 2018 • 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

nonsense Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nonsense Mar 29, 2018 • 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

nolash commented Mar 26, 2018

janos 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

gbalint commented Mar 28, 2018

gbalint commented Mar 28, 2018

gbalint commented Mar 28, 2018

Choose a reason for hiding this comment

nonsense left a comment

Choose a reason for hiding this comment

janos 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

gbalint commented Mar 14, 2018 •

edited

Loading

nonsense Mar 26, 2018 •

edited

Loading

nonsense Mar 26, 2018 •

edited

Loading

nonsense Mar 29, 2018 •

edited

Loading