user: Add cockroachdb interface implementation #856

lukebp · 2019-05-07T12:15:33Z

Closes #820.

This commit adds a cockroachdb implementation of the user database
interface. Some details about the implementation:

Data at rest is encrypted. A few public fields (ID, username,
pubkeys) have been pulled out to allow them to be queryable.
politeawww_dbutil has been updated to support the cockroachdb
implementation. Commands have been added for creating encryption
keys and for migrating an existing user database from leveldb to
cockroachdb.
The ability to rotate encryption keys has been added to politeiawww.
The user lookup by email function has been removed from the user
database implementation. Email is part of the encrypted user data blob
and user lookups by email need to be completely phased out of
politeiawww.

Further work that will be required

Switch all user lookups by email in politeia to be user lookups by
username or ID. The politeiawww userEmails cache can be removed once
this is done.
Remove the politeiawww userPubkeys cache now that the pubkeys are
queryable.

I'll open issues for these once this PR gets merged.

thi4go · 2019-05-07T15:22:58Z

Tested with pi/cms modes and everything seems to be running smooth, no bugs testing all features from my end. Great PR!

This commit adds a cockroachdb implementation of the user database interface. Some details about the implementation: - Data at rest is encrypted. A few public fields (ID, username, pubkeys) have been pulled out to allow them to be queryable. - politeawww_dbutil has been updated to support the cockroachdb implementation. Commands have been added for creating encryption keys and for migrating an existing user database from leveldb to cockroachdb. - The ability to rotate encryption keys has been added to politeiawww. - The user lookup by email function has been removed from the user database interface. Email is part of the encrypted user data blob and user lookups by email need to be completely phased out of politeiawww. **Further work that will be required** - Switch all user lookups by email in politeia to be user lookups by username or ID. The politeiawww userEmails cache can be removed once this is done. - Remove the politeiawww userPubkeys cache now that the pubkeys are queryable. I'll open issues for these once this PR gets merged.

README.md

marcopeereboom · 2019-05-07T15:58:37Z

README.md

+setting.  To rotate keys, set `oldencryptionkey` to the existing key and set
+`encryptionkey` to the new key.  Starting politeiawww with both of these config
+params set will kick off a key rotation.
+


Neat but does this re-encrypt the entire db? I ask because what happens if you do this twice?

Yes, it will re-encrypt the entire db. If you do it twice, without changing the config params, it will throw a cannot decrypt error on the first user it tries to decrypt because the old key is no longer valid.

marcopeereboom · 2019-05-07T15:59:33Z

politeiad/cache/cockroachdb/cockroachdb.go

@@ -827,7 +827,7 @@ func New(user, host, net, rootCert, cert, key string) (*cockroachdb, error) {
 	// names manually.
 	c.recordsdb.SingularTable(true)

-	log.Infof("Cache host: %v", h)
+	log.Infof("Cache host  : %v", h)


eew. Don't put a space before the :

marcopeereboom · 2019-05-07T16:08:24Z

politeiawww/politeiawww.go

@@ -112,6 +112,7 @@ type politeiawww struct {
 	test bool

 	// Following entries require locks
+	// XXX userPubkeys can be removed now that the userdb is queryable


we do want to remain compat with leveldb so while it can go away we may have to move this functionality into the db (e.g. make it queriable and maybe add an index)

marcopeereboom · 2019-05-07T16:09:09Z

politeiawww/user.go

@@ -29,10 +29,15 @@ const (

 	// Route to reset password at GUI
 	ResetPasswordGuiRoute = "/password" // XXX what is this doing here?
+
+	emailRegex = "^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@" +


This needs to use single quotes.

util/encrypt.go

marcopeereboom · 2019-05-07T16:12:10Z

util/encrypt.go

+}
+
+// NewEncryptionKey creates and save a new encryption key at the provided
+// filepath.


marcopeereboom · 2019-05-07T16:15:26Z

util/encrypt.go

@@ -0,0 +1,92 @@
+// Copyright (c) 2017-2019 The Decred developers


Keys must always be pointers. You violate that rule in this file.

marcopeereboom · 2019-05-07T16:16:11Z

util/encrypt.go

+
+// EncryptionKey wraps a 32 byte encryption key and the time when it
+// was created.
+type EncryptionKey struct {


This needs a version field.

marcopeereboom · 2019-05-07T16:17:04Z

util/encrypt.go

+
+// Encrypt encrypts a byte slice with the provided version using the
+// provided key.
+func Encrypt(version uint32, key [32]byte, data []byte) ([]byte, error) {


Key must be a pointer. Alternatively, you could use a target instead of passing the key in.

Made all keys pointers.

crypto-rizzo · 2019-05-07T16:58:04Z

PR is running smooth on my end for Pi and CMS as well! Only part I'm a tad confused about is the usage of localDB vs. levelDB inside of politeiawww_dbutil. I left an inline comment below for this

politeiawww/cmd/politeiawww_dbutil/politeiawww_dbutil.go

victorgcramos

Tested and it worked here! I left one small comment regarding the doc example for dev environment.

politeiawww/cmd/politeiawww_dbutil/README.md

marcopeereboom · 2019-05-08T11:31:06Z

util/encrypt.go

+// EncryptionKey wraps a 32 byte encryption key.
+type EncryptionKey struct {
+	Version uint      `json:"version"` // Version of this struct
+	Key     *[32]byte `json:"key"`     // Key used for encryption


After rereading this, this file needs to go.

Hex encode the key on disk.
When you load it and convert it from hex into a [32]byte slice you return the address to the caller.
Zero out in between copies of the key.
Store key as a pointer in the calling context.

lukebp force-pushed the userdb branch from 36257fe to d5be704 Compare May 7, 2019 15:45

marcopeereboom reviewed May 7, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

marcopeereboom reviewed May 7, 2019

View reviewed changes

marcopeereboom suggested changes May 7, 2019

View reviewed changes

crypto-rizzo reviewed May 7, 2019

View reviewed changes

politeiawww/cmd/politeiawww_dbutil/politeiawww_dbutil.go Show resolved Hide resolved

victorgcramos reviewed May 7, 2019

View reviewed changes

politeiawww/cmd/politeiawww_dbutil/README.md Show resolved Hide resolved

lukebp added 4 commits May 7, 2019 18:34

fixes for review comments

8d53df9

more fixes

68b21b5

more fixes

712f619

update email regex

a3534b1

marcopeereboom suggested changes May 8, 2019

View reviewed changes

lukebp added 3 commits May 8, 2019 12:17

remove encrypt.go

fd2b045

cleanup

087fd82

zero out key

40753c6

marcopeereboom approved these changes May 8, 2019

View reviewed changes

lukebp merged commit c407f5c into decred:master May 8, 2019

This was referenced May 8, 2019

Scale www database #689

Closed

Invalidate all sessions of a user on password change #647

Closed

backup functionality while running needed. #605

Closed

lukebp deleted the userdb branch June 20, 2019 11:45

vibros68 pushed a commit to vibros68/politeia that referenced this pull request Aug 17, 2021

fix redirect on proposals/new (decred#856)

c786eb3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

user: Add cockroachdb interface implementation #856

user: Add cockroachdb interface implementation #856

lukebp commented May 7, 2019 •

edited

Loading

thi4go commented May 7, 2019

marcopeereboom May 7, 2019

lukebp May 7, 2019

marcopeereboom May 7, 2019

lukebp May 7, 2019 •

edited

Loading

marcopeereboom May 7, 2019

lukebp May 7, 2019

marcopeereboom May 7, 2019

lukebp May 8, 2019

marcopeereboom May 8, 2019

lukebp May 8, 2019

marcopeereboom May 7, 2019

lukebp May 7, 2019

marcopeereboom May 7, 2019

lukebp May 7, 2019

marcopeereboom May 7, 2019

lukebp May 7, 2019

marcopeereboom May 7, 2019

lukebp May 7, 2019

crypto-rizzo commented May 7, 2019 •

edited

Loading

victorgcramos left a comment

marcopeereboom May 8, 2019

lukebp May 8, 2019

		@@ -0,0 +1,92 @@
		// Copyright (c) 2017-2019 The Decred developers

user: Add cockroachdb interface implementation #856

user: Add cockroachdb interface implementation #856

Conversation

lukebp commented May 7, 2019 • edited Loading

thi4go commented May 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebp May 7, 2019 • 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

crypto-rizzo commented May 7, 2019 • edited Loading

victorgcramos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebp commented May 7, 2019 •

edited

Loading

lukebp May 7, 2019 •

edited

Loading

crypto-rizzo commented May 7, 2019 •

edited

Loading