GODRIVER-2352 Add clientEncryption entity to unified test format #920
Conversation
kevinAlbs
changed the title
ClientEncryption.Close() calls Disconnect on the keyVaultClient. Track IDs of clients used as keyVaultClient.
skip 1.7
mongo/integration/unified/entity.go
Outdated
| return newEntityNotFoundError("client", ceo.KeyVaultClient) | ||
| } | ||
|
|
||
| ce, err := mongo.NewClientEncryption(keyVaultClient.Client, options.ClientEncryption().SetKeyVaultNamespace(ceo.KeyVaultNamespace).SetKmsProviders(kmsProviders)) |
There was a problem hiding this comment.
Optional: Consider wrapping this line to make it easier to read.
E.g.
ce, err := mongo.NewClientEncryption(
keyVaultClient.Client,
options.ClientEncryption().
SetKeyVaultNamespace(ceo.KeyVaultNamespace).
SetKmsProviders(kmsProviders))
mongo/integration/unified/entity.go
Outdated
| // getKmsCredential processes a value of an input KMS provider credential. | ||
| // An empty document returns from the environment. | ||
| // A string is returned as-is. | ||
| func getKmsCredential(kmsDocument bson.Raw, credentialName string, defaultValue string) (*string, error) { |
There was a problem hiding this comment.
Unless there's a specific reason to return a *string, it's typically more idiomatic to return a string and use the string zero-value ("") as the return value in case of an error.
E.g.
func getKmsCredential(kmsDocument bson.Raw, credentialName string, defaultValue string) (string, error) {
// ...
return "", err
// ...
}There was a problem hiding this comment.
Done. That is good to know. My instinct is to use pointer for optional.
mongo/integration/unified/entity.go
Outdated
| // getKmsCredential processes a value of an input KMS provider credential. | ||
| // An empty document returns from the environment. | ||
| // A string is returned as-is. | ||
| func getKmsCredential(kmsDocument bson.Raw, credentialName string, defaultValue string) (*string, error) { |
There was a problem hiding this comment.
Optional: If all defaultValues come from environment variables, consider replacing the defaultValue parameter with an environment variable name parameter and perform the env var lookup in getKmsCredential. That would allow more specific messaging about any missing environment variables rather than the less specific "Please set CSFLE environment variables" message.
There was a problem hiding this comment.
Great idea. Some credentials have default values and no environment variable. Updated to separate the defaultValue from the environment variable, and print the environment variable in the error message.
|
|
||
| // CSFLEEnabled returns true if driver is built with Client Side Field Level Encryption support. | ||
| // Client Side Field Level Encryption support is enabled with the cse build tag. | ||
| func CSFLEEnabled() bool { |
There was a problem hiding this comment.
Optional: Functions that don't modify anything and return only a bool conventionally have "is" or "has" in the name. Consider updating to the slightly more conventional IsCSFLEEnabled().
mongo/integration/mtest/mongotest.go
Outdated
| if rob.CSFLE != nil { | ||
| if *rob.CSFLE != IsCSFLEEnabled() { |
There was a problem hiding this comment.
| if rob.CSFLE != nil { | |
| if *rob.CSFLE != IsCSFLEEnabled() { | |
| if rob.CSFLE != nil && *rob.CSFLE != IsCSFLEEnabled() { |
Suggestion to combine the two if lines into one to lower cyclomatic complexity.
| if local, ok := ceo.KmsProviders["local"]; ok { | ||
| kmsProviders["local"] = make(map[string]interface{}) | ||
|
|
||
| defaultLocalKeyBase64 := "Mng0NCt4ZHVUYUJCa1kxNkVyNUR1QURhZ2h2UzR2d2RrZzh0cFBwM3R6NmdWMDFBMUN3YkQ5aXRRMkhGRGdQV09wOGVNYUMxT2k3NjZKelhaQmRCZGJkTXVyZG9uSjFk" |
There was a problem hiding this comment.
Should this be exposed in the repository?
There was a problem hiding this comment.
Yes, this local master key is used to encrypt test data. It is not sensitive, and is included in the specification test README.
| if kmip, ok := ceo.KmsProviders["kmip"]; ok { | ||
| kmsProviders["kmip"] = make(map[string]interface{}) | ||
|
|
||
| kmipEndpoint, err := getKmsCredential(kmip, "endpoint", "", "localhost:5698") |
There was a problem hiding this comment.
Is the port always 5698 or could this change?
There was a problem hiding this comment.
This is expected to be 5698 from the specification test README
Start a KMIP test server on port 5698 by running drivers-evergreen-tools/.evergreen/csfle/kms_kmip_server.py.
gosec reports a false positive "Hardcoded credentials". See: https://securego.io/docs/rules/g101.html
mongo/integration/unified/entity.go
Outdated
| // A string is returned as-is. | ||
| func getKmsCredential(kmsDocument bson.Raw, credentialName string, envVar string, defaultValue string) (string, error) { | ||
| credentialVal, err := kmsDocument.LookupErr(credentialName) | ||
| if err == nil { |
There was a problem hiding this comment.
Optional: Consider inverting this error check to return early so you can un-indent the rest of the function code.
E.g.
credentialVal, err := kmsDocument.LookupErr(credentialName)
if err == bsoncore.ErrElementNotFound {
return "", nil
}
if err != nil {
return "", err
}
// ...
mongo/integration/unified/entity.go
Outdated
| return str, nil | ||
| } | ||
|
|
||
| if doc, ok := credentialVal.DocumentOK(); ok { |
There was a problem hiding this comment.
Optional: Similar to above, consider inverting this check to return early.
E.g.
doc, ok := credentialVal.DocumentOK()
if !ok {
return "", fmt.Errorf("expected String or Document for %v, got: %v", credentialName, credentialVal)
}
// ...
GODRIVER-2352
Add Unified Test Format support for 1.8. This is a prerequisite for the Go implementation of the CSFLE Key Management API.
Background & Motivation
See mongodb/specifications@e91f0ec and mongodb/specifications@0f65908 for the Unified Test Format changes.
Notes
ClientEncryption.Close()callsDisconnecton the keyVaultClient. Client entities used as keyVaultClients are tracked to prevent the test runner from callingDisconnecttwice.