-
Notifications
You must be signed in to change notification settings - Fork 36
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
Use cosmosDB in the gateway and encrypt data with encryption key generated inside the enclave #2104
Conversation
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.
looks good in principle
few comments
// EncryptedDocument struct is used to store encrypted user data in CosmosDB | ||
// We use this structure to add an extra layer of security by encrypting the actual user data | ||
// The 'ID' field is used as the document ID and partition key in CosmosDB | ||
// The 'Data' field contains the base64-encoded encrypted user data |
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.
Can't the Data and ID be just bytes?
To avoid the conversion to and from strings
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.
Changed data to bytes, with ID it's more tricky because it's also a partition key and it would add more complexity.
By avoiding converting data from & to strings we avoided the majority of conversions as IDs are much shorter.
Signature []byte | ||
SignatureType int | ||
type GWUserDB struct { | ||
ID string `json:"id"` // Required by CosmosDB |
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.
there is an ID field in the EncryptedDocument as well.
Is this one here necessary?
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.
No necessary, removed now.
@@ -32,8 +32,15 @@ func NewContainerFromConfig(config wecommon.Config, logger gethlog.Logger) *Cont | |||
// create the account manager with a single unauthenticated connection | |||
hostRPCBindAddrWS := wecommon.WSProtocol + config.NodeRPCWebsocketAddress | |||
hostRPCBindAddrHTTP := wecommon.HTTPProtocol + config.NodeRPCHTTPAddress | |||
|
|||
// TODO fix passing this random key to next enclave and not generating it here every time we start the wallet extension | |||
randomKey, err := wecommon.GenerateRandomKey() |
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.
the randomkey needs to be persisted sealed by the current enclave, and read on restarts.
Have a look at: edglessdb.go:288
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.
this will be part of the next PR
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.
left a few more comments
} | ||
|
||
func (e *Encryptor) HashWithHMAC(data []byte) []byte { | ||
h := hmac.New(sha256.New, e.key) |
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.
can this be reused?
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.
this should not be reused (it has it's internal state and by reusing we might get different outputs)
"io" | ||
) | ||
|
||
type Encryptor struct { |
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.
please add a good comment describing the encryption approach. AES-GCM with key size, nonce size. How we generate a nonce for each.
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.
added a comment
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.
thanks.
"github.com/ten-protocol/go-ten/tools/walletextension/common" | ||
"github.com/ten-protocol/go-ten/tools/walletextension/storage/database/cosmosdb" | ||
"github.com/ten-protocol/go-ten/tools/walletextension/storage/database/sqlite" | ||
) | ||
|
||
type Storage interface { |
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.
let's create another implementation of Storage: StorageWithCache
- that takes an implementation of Storage
(either sqlite or cosmos) in the constructor, and takes care of the caching logic.
This way we won't risk hitting the database like we currently do when we call methods on this
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.
Implemented. Should we use this storage with cache by default?
e4d100c
to
611bc99
Compare
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.
lgtm.
Why this change is needed
We want to use a key value database in the gateway. We want to store encrypted data and our database schema in the gateway is very simple and easy to convert to key-value database.
We want to store encrypted data in the database with the key generated inside the gateway (which runs inside an enclave).
By encrypting data ourselves we can use "normal" database and avoid EdgelessDB.
What changes were made as part of this PR
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks