Path Encryption #448
Path Encryption #448
Conversation
mobyvb
force-pushed
the
path-encryption
branch
2 times, most recently
from
4b53db6
to
5ac1d4c
Compare
pkg/paths/paths.go
Outdated
| key, err = deriveSecret(key, seg) | ||
| if err != nil { | ||
| return nil, err | ||
| // TODO(moby) this is a hacky way to deal with buckets |
There was a problem hiding this comment.
you're a hacky way to deal with buckets
dylanlott
dismissed
their stale review
Didn't mean to make an Approved yet while it's still WIP
mobyvb
force-pushed
the
path-encryption
branch
from
50a20cc
to
ad57767
Compare
mobyvb
force-pushed
the
path-encryption
branch
from
d85f322
to
cedaa99
Compare
pkg/paths/paths.go
Outdated
| @@ -61,6 +61,34 @@ func (p Path) HasPrefix(prefix Path) bool { | |||
| return true | |||
| } | |||
|
|
|||
| // EncryptWithBucket creates new Path by encrypting the current path with the given key | |||
| // while keeping the bucket portion of the path unencrypted | |||
| func (p Path) EncryptWithBucket(key []byte) (encrypted Path, err error) { | |||
There was a problem hiding this comment.
The logic in this method is specific to the Stream Store. The Path type is abstract and knows nothing about buckets. Thus, similar to the getSegmentPath method, it would be better if this method is a private helper function in the Stream Store.
Also encryptAfterBucket would be a better name. It makes it clear that encryption is applied after the bucket part of the path.
Same applies for DecryptWithBucket.
pkg/paths/paths.go
Outdated
| // EncryptWithBucket creates new Path by encrypting the current path with the given key | ||
| // while keeping the bucket portion of the path unencrypted | ||
| func (p Path) EncryptWithBucket(key []byte) (encrypted Path, err error) { | ||
| pathItems := []string(p) |
There was a problem hiding this comment.
We don't need this type cast. We can call directly p[0] and p[1:].
pkg/paths/paths.go
Outdated
| bucket := pathItems[0] | ||
| toEncrypt := Path(pathItems[1:]) | ||
|
|
||
| encPath, err := toEncrypt.Encrypt(key) |
There was a problem hiding this comment.
Although we don't encrypt the bucket part of the path, we still need to derive a key from it. We want a file with the same path to be encrypted differently under different buckets.
So we need something like:
bucketKey, err := DeriveKey(key, 1)Same applies for DecryptWithBucket.
pkg/paths/paths.go
Outdated
| func (p Path) EncryptWithBucket(key []byte) (encrypted Path, err error) { | ||
| pathItems := []string(p) | ||
| bucket := pathItems[0] | ||
| toEncrypt := Path(pathItems[1:]) |
There was a problem hiding this comment.
We don't need this type case too. toEncrypt := p[1:] should be enough.
There was a problem hiding this comment.
Also what happens if the path consists only of the bucket, i.e. the use case for adding, removing and listing buckets?
pkg/storage/streams/store.go
Outdated
| return nil, false, err | ||
| } | ||
|
|
||
| prefixNoBucket := paths.Path([]string(prefix)[1:]) |
There was a problem hiding this comment.
When deriving the key we should include the bucket part, as explained for the EncryptWithBucket method.
pkg/storage/streams/store.go
Outdated
| return nil, false, err | ||
| } | ||
|
|
||
| encStartAfter, err := startAfter.Encrypt(prefixKey) |
There was a problem hiding this comment.
What if the prefix is nil? This would mean that startAfter and endBefore include the bucket part too, doesn't it? In this case, I guess we should call EncryptWithBucket instead.
There was a problem hiding this comment.
The problem is they won't include the bucket part if the prefix is not nil, so I will make two separate cases for this
There was a problem hiding this comment.
The new listNoPrefix helper method duplicates quite some logic from the main List method. It would be cleaner and easier to maintain if we would just define a smaller helper methods like:
// encryptMarker is a helper method for encrypting startAfter and endBefore markers
func (s *streamStore) encryptMarker(marker paths.Path, prefixKey []byte) (paths.Path, error) {
if bytes.Equal(s.rootKey, prefixKey) { // empty prefix
return encryptAfterBucket(marker, s.rootKey)
}
return marker.Encrypt(prefixKey)
}and use it to encrypt startAfter and endBefore.
Similar for decrypting.
pkg/storage/streams/store.go
Outdated
| @@ -363,7 +408,11 @@ func (s *streamStore) List(ctx context.Context, prefix, startAfter, endBefore pa | |||
| if err != nil { | |||
| return nil, false, err | |||
| } | |||
| items[i] = ListItem{Path: item.Path, Meta: newMeta, IsPrefix: item.IsPrefix} | |||
| decPath, err := item.Path.Decrypt(prefixKey) | |||
There was a problem hiding this comment.
Here too: what if the prefix is nil?
mobyvb
force-pushed
the
path-encryption
branch
from
b15aa47
to
525ab2e
Compare
pkg/storage/streams/store.go
Outdated
| return nil, false, err | ||
| } | ||
|
|
||
| encStartAfter, err := startAfter.Encrypt(prefixKey) |
There was a problem hiding this comment.
The new listNoPrefix helper method duplicates quite some logic from the main List method. It would be cleaner and easier to maintain if we would just define a smaller helper methods like:
// encryptMarker is a helper method for encrypting startAfter and endBefore markers
func (s *streamStore) encryptMarker(marker paths.Path, prefixKey []byte) (paths.Path, error) {
if bytes.Equal(s.rootKey, prefixKey) { // empty prefix
return encryptAfterBucket(marker, s.rootKey)
}
return marker.Encrypt(prefixKey)
}and use it to encrypt startAfter and endBefore.
Similar for decrypting.
mobyvb
force-pushed
the
path-encryption
branch
from
525ab2e
to
dd152f6
Compare
* begin adding path encryption * do not encrypt/decrypt first element of path (bucket) * add path encryption for delete and list * use encrypted paths in streamstore.Meta * fix listing with encrypted paths * move encrypt/decryptAfterBucket to streamstore * fix listing with no prefix * remove duplicate logic for listing with no prefix
No description provided.