Per-DB signer tests #863
Per-DB signer tests #863
Conversation
cyli
force-pushed
the
signer-db-tests
branch
2 times, most recently
from
aabd5f6
to
fafad37
Compare
| // Add the key to cache | ||
| s.cachedKeys[privKey.ID()] = &cachedKey{key: privKey, role: role} | ||
| } | ||
| return privKey, role, err |
There was a problem hiding this comment.
non-blocking nit: while this shouldn't make any difference in actual values returned, it could be clearer if this returned data.PrivateKey{}, "", err
| @@ -77,7 +79,7 @@ func rdbPrivateKeyFromJSON(data []byte) (interface{}, error) { | |||
| // PrivateKeysRethinkTable is the table definition for notary signer's key information | |||
| var PrivateKeysRethinkTable = rethinkdb.Table{ | |||
| Name: RDBPrivateKey{}.TableName(), | |||
| PrimaryKey: RDBPrivateKey{}.KeyID, | |||
| PrimaryKey: "key_id", | |||
There was a problem hiding this comment.
do we need the hardcoded string instead of the constant here?
There was a problem hiding this comment.
RDBPrivateKey{}.KeyID was just the empty string, because it initialized the empty struct and so that value is just the zero value (so it wasn't correctly setting the primary key, so the unique key ID constraint wasn't being enforced).
If we wanted to declare a constant and use it to set the tag value (so we can use the constant here) or if we wanted to pull out the tag value, I think we're going to need some reflection to do so, or better yet: https://github.com/oleiade/reflections.
If we wanted to add that though, maybe it can be a different PR?
There was a problem hiding this comment.
Ah yes, sorry I missed that. I'm fine with a hardcoded string for now and following up with reflection in a separate PR
|
this is great, thank you for adding these fixes and tests! LGTM pending CI |
cyli
force-pushed
the
signer-db-tests
branch
2 times, most recently
from
df3e66f
to
f7319e0
Compare
| } | ||
|
|
||
| // NewCachedKeyStore returns a new trustmanager.KeyStore that includes caching | ||
| func NewCachedKeyStore(baseStore trustmanager.KeyStore) trustmanager.KeyStore { |
There was a problem hiding this comment.
I'm thinking this could also replace a chunk of trustmanager.GenericKeyStore down the road, particularly if we get around to separating the password stuff out. We'd end up with something like NewCachedKeyStore( NewGenericKeyStore( NewPasswdStorage( NewFileStorage(keyDirectory) ))) :-D
…round a KeyStore. This can be used in the future for filesystem KeyStores too. Note that existing sql_keydbstore tests will now fail in this commit, to be fixed in the next commit. Signed-off-by: Ying Li <[email protected]>
…test file which has logic that can also be used to test rethink, and which expects a sqlite or mysql initializer. Also rename the KeyDBStore to SQLKeyDBStore, and fix delete key to always succeed. Configure dbtests.sh script to run both the server and signer db tests now. Signed-off-by: Ying Li <[email protected]>
…ion tests. Fix the following in the RethinkDB implementation: - health check needs to ensure the user has access to the db and table - store the public and private bits as byte slices, because the otherwise rethink can't deal with the binary data - fix the rethink queries for rotating a key passphrase - use the same code as GetKey Signed-off-by: Ying Li <[email protected]>
…implementations. Also wait to lock the keydb cache until after a key was retrieved successfully. Signed-off-by: Ying Li <[email protected]>
|
LGTM! Thank you so much for adding the rethink tests! |
This adds specific mysql DB and rethink DB tests for the signer.
It also does the following:
KeyStoreimplementations into a wrapperKeyStoreKeyDBStoretoSQLKeyDBStoreand add SQL types to the Gorm model (otherwise the schema is not created correctly in MySQL tests)SQLKeyDBStore'sRemoveKeyto always succeed, even if the key doesn't existRethinkKeyDBStore's public and private bytes as[]byte, otherwise rethink won't know that it's binary data and will mangle the result - this doesn't matter for the private key bytes, since we encrypt it in such a way that it's base64 encoded, but I changed it anyway in case we want to change encryption method in the futureRethinkKeyDBStore's key rotation function - it now uses the same get query asGetKey