CSHARP-3936: Support KMIP in client side field level encryption. #675

DmitryLukyanov · 2021-11-09T20:35:27Z

No description provided.

DmitryLukyanov · 2021-11-09T20:35:40Z

.gitignore

@@ -47,7 +47,7 @@ Help
 src/packages

 # Nupkg artifacts
-*.nupkg
+#*.nupkg


will be reverted

DmitryLukyanov · 2021-11-09T20:35:43Z

NuGet.config

+	<packageSources>
+	  <add key="LocalPackages" value="libs" />
+	</packageSources>
+	<activePackageSource>


will be reverted

DmitryLukyanov · 2021-11-09T20:36:22Z

evergreen/evergreen.yml

@@ -303,6 +303,7 @@ functions:
          CLIENT_PEM=${DRIVERS_TOOLS}/.evergreen/x509gen/client.pem \
          REQUIRE_API_VERSION=${REQUIRE_API_VERSION} \
          FRAMEWORK=${FRAMEWORK} \
+          ALLOW_NOSSL=true \


for kmip we need to pass client certificate regardless ssl options

DmitryLukyanov · 2021-11-09T20:36:47Z

evergreen/evergreen.yml

@@ -922,44 +964,6 @@ tasks:
          vars:
            FRAMEWORK: netstandard21

-    - name: test-kms-tls-expired-certificate-netstandard21


new tests cover this logic

DmitryLukyanov · 2021-11-09T20:37:24Z

evergreen/get-python-path.sh

+#   PYTHON                                           The python path
+
+# Don't write anything to output
+if [ -z "$Venv" ]; then


a new script that provides python path

DmitryLukyanov · 2021-11-09T20:51:17Z

...MongoDB.Driver.Tests/Specifications/client-side-encryption/ClientSideEncryptionTestRunner.cs

+            if (testCase.Name.Contains("kmip"))
+            {
+                // kmip requires configuring kms mock server
+                RequireEnvironment.Check().EnvironmentVariable("KMS_MOCK_SERVERS_ENABLED");


skip because kmip tests require kms mock servers

DmitryLukyanov · 2021-11-09T20:52:01Z

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

@@ -40,6 +46,7 @@

 namespace MongoDB.Driver.Tests.Specifications.client_side_encryption.prose_tests
 {
+    [Trait("Category", "FLE")]


some of tests here still fail from time to time, investigating

DmitryLukyanov · 2021-11-09T20:52:53Z

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

+                AssertException(exception);
+            }
+
+            void AssertException(Exception exception)


asserts exception based on OS, TF, Certificate configuration, KMS provider

DmitryLukyanov · 2021-11-09T20:53:36Z

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

+
+                    return false;
+                });
+                clientEncryptionOptions._tlsOptions(tlsOptions); // avoid validation on serverCertificateValidationCallback


I use reflection, because settings serverCertificateValidationCallback is disabled via public API

DmitryLukyanov · 2021-11-09T20:54:12Z

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

@@ -1023,52 +1288,61 @@ public void ViewAreProhibitedTest([Values(false, true)] bool async)
        [SkippableTheory]
        [ParameterAttributeData]
        public void UnsupportedPlatformsTests(
-            [Values("local", "aws", "azure", "gcp")] string kmsProvider,
+            [Values("gcp")] string kmsProvider, // the rest kms providers are supported on all supported TFs


this test is not longer complicated because most of the cases were gone for now

DmitryLukyanov · 2021-11-09T20:58:55Z

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

@@ -690,13 +693,15 @@ void ValidKmsEndpointConfigurator(string kt, Dictionary<string, object> ko)
                    case "gcp":
                        ko.Add("endpoint", "oauth2.googleapis.com:443");
                        break;
+                    case "kmip":
+                        //ko.Add("endpoint", "localhost:5698");


missed this, will fix in the next commit

DmitryLukyanov · 2021-11-10T03:11:44Z

Ready for review, the only pending question is related to failing tests with unexpected exception:

Unable to read data from the transport connection: An existing connection was forcibly closed by the remote host.

like here. It looks like it's related to mock server behavior, I asked Kevin maybe he faced with this, so will reach our him tommorow

DmitryLukyanov · 2021-11-10T03:13:38Z

...Driver.Tests/Specifications/client-side-encryption/prose-tests/ClientEncryptionProseTests.cs

@@ -567,6 +564,10 @@ EncryptionAlgorithm ParseAlgorithm(string algorithm)
        // gcp
        [InlineData("gcp", "cloudkms.googleapis.com:443", null, "parse error")]
        [InlineData("gcp", "example.com:443", "Invalid KMS response", null)]
+        // kmip
+        [InlineData("kmip", null, null, "$HostNotFound$")]


I don't use enum itself, because it breaks a bit Test Explorer on .net framework, due some serialization issue

I use a code instead a message, because the message is platform dependent in some cases

JamesKovacs

See comments inline to discuss. Main point of discussion is around testing strategy. A few auxiliary questions about API and impl.

JamesKovacs · 2021-11-10T00:02:08Z

evergreen/add-certs-if-needed.sh


 SSL=${SSL:-nossl}
 OCSP_TLS_SHOULD_SUCCEED=${OCSP_TLS_SHOULD_SUCCEED:-nil}
 OCSP_ALGORITHM=${OCSP_ALGORITHM:-nil}
+ALLOW_NOSSL=${ALLOW_NOSSL:-nil}


ALLOW_NOSSL is making my brain hurt trying to figure out if SSL is allowed or not. What about calling this option REQUIRE_CERTS or something similar?

this option allows skipping the validation where we add the certificate to trusted storage only if ${SSL}="ssl", so this describes exactly what happens. I don't mind to change it, but REQUIRE_CERTS is not what this variable have control about. What about: IGNORE_SSL_CHECK?

Given that there is no harm in adding certs even if they're unused, why don't we simplify the script to always add the certs and remove the need for this option?

this file is used in other logic like in OCSP and other, I would not want to change anything there

evergreen/get-python-path.sh

evergreen/run-tests.sh

evergreen/evergreen.yml

src/MongoDB.Driver/Encryption/AutoEncryptionLibMongoController.cs

JamesKovacs · 2021-11-10T21:43:34Z

src/MongoDB.Driver/Encryption/LibMongoCryptControllerBase.cs

@@ -267,5 +283,28 @@ private async Task SendKmsRequestAsync(KmsRequest request, CancellationToken can
                }
            }
        }
+
+        // nested type
+        private class NetworkStreamFactory : IStreamFactory


Examining TcpStreamFactory, we handle differences between .NET Framework and .NET Core. NetworkStreamFactory is implemented identically to the .NET Framework CreateStream code. Are we sure we don't need the extra logic in TcpStreamFactory for proper operation on .NET Core?

src/MongoDB.Driver/SslSettings.cs

JamesKovacs · 2021-11-10T21:46:18Z

tests/MongoDB.Driver.Core.TestHelpers/Logging/LoggableTestClass.cs

@@ -23,6 +23,7 @@

 namespace MongoDB.Driver.Core.TestHelpers.Logging
 {
+    [DebuggerStepThrough]


We should make this change in the context of CSHARP-3904, not this PR.

...MongoDB.Driver.Tests/Specifications/client-side-encryption/ClientSideEncryptionTestRunner.cs

src/MongoDB.Driver/Encryption/AutoEncryptionOptions.cs

JamesKovacs

Some outstanding comments that need to be addressed. Please let me know if you have any questions or would like to discuss.

JamesKovacs · 2021-11-11T20:40:51Z

src/MongoDB.Driver/Encryption/AutoEncryptionOptions.cs

@@ -216,7 +216,7 @@ public override string ToString()
            }
            if (_tlsOptions != null)
            {
-                sb.AppendFormat("TlsOptions: {0}", _tlsOptions.Select(t => new BsonDocument(t.Key, "<hidden>")).ToJson(jsonWriterSettings));
+                sb.AppendFormat("TlsOptions: {0}, ", _tlsOptions.ToJson(jsonWriterSettings));


I think hiding the tlsOptions - which could contain sensitive data - other than the key name is the correct thing to do even if it makes debugging more difficult. Why did you remove the <hidden> replacement code?

I described it here: #675 (comment) so, to simplify logic, but I see that it gives some problem with serialization IntPtr like:

MongoDB.Bson.BsonSerializationException : No serializer found for type System.IntPtr.

EG: https://evergreen.mongodb.com/build/dot_net_driver_kms_tls_mocked_tests_windows__version~5.0_os~windows_64_topology~standalone_ssl~nossl_patch_60ad0c84b164c0648eabe7dea72aa6d94f8289aa_618d6e7de3c33142e63b40c5_21_11_11_19_27_33
I will probably revert it

SslStreamSettings has 5 properties:
CheckCertificateRevocation - might be useful but currently only true is allowed for KMIP/KMS
ClientCertificates - we don't want to expose these
ClientCertificateSelectionCallback - delegate; only useful thing to show would be whether it is set or not
EnabledSslProtocols - might be useful to show these but we could potentially leak security information about whether support for older (and less secure) protocols is enabled
ServerCertificateValidationCallback - delegate; only useful thing to show would be whether it is set or not

Of the 5 properties, they either have known values for KMIP/KMS or would leak security information. So I think <hidden> is a better choice rather than risking leaking sensitive information.

JamesKovacs

LGTM

JamesKovacs · 2021-11-17T01:00:39Z

evergreen/evergreen.yml

+          FRAMEWORK=${FRAMEWORK} \
+          TARGET="TestCsfleWithMockedKms" \
+            evergreen/run-tests.sh
+            evergreen/cleanup-test-resources.sh


cleanup-test-resources.sh requires the OS environment variable but you're not setting it. When you set environment variables like this ENV=VALUE cmd, the environment variable only applies to cmd and not to any following commands.

evergreen/evergreen.yml

src/MongoDB.Driver/Encryption/KmsProvidersHelper.cs

JamesKovacs · 2021-11-17T23:47:20Z

src/MongoDB.Driver/Encryption/LibMongoCryptControllerBase.cs

@@ -267,5 +283,28 @@ private async Task SendKmsRequestAsync(KmsRequest request, CancellationToken can
                }
            }
        }
+
+        // nested type
+        private class NetworkStreamFactory : IStreamFactory


Let's leave it as it is here and revisit later if necessary.

JamesKovacs · 2021-11-17T23:48:35Z

tests/MongoDB.Driver.Core.TestHelpers/Logging/LoggableTestClass.cs

@@ -23,6 +23,7 @@

 namespace MongoDB.Driver.Core.TestHelpers.Logging
 {
+    [DebuggerStepThrough]


I'm good with implementing CSHARP-3904 immediately. I just don't want to have [DebuggerStepThrough] changes mixed into the KMIP changes.

DmitryLukyanov · 2021-11-18T22:39:39Z

EG: https://evergreen.mongodb.com/version/6196db963627e004ec5ce600

JamesKovacs

LGTM

CSHARP-3936: Support KMIP in client side field level encryption.

1f636f2

DmitryLukyanov commented Nov 9, 2021

View reviewed changes

DmitryLukyanov requested a review from jyemin November 9, 2021 20:54

DmitryLukyanov commented Nov 9, 2021

View reviewed changes

JamesKovacs requested review from JamesKovacs and removed request for jyemin November 9, 2021 23:29

DmitryLukyanov commented Nov 10, 2021

View reviewed changes

CSHARP-3936: Some fixes.

cfde7b3

DmitryLukyanov force-pushed the csharp3936 branch from 6d38038 to cfde7b3 Compare November 10, 2021 12:30

CSHARP-3936: Some fixes.

1e01177

JamesKovacs requested changes Nov 10, 2021

View reviewed changes

CSHARP-3963: Code review.

fcc3702

DmitryLukyanov requested a review from JamesKovacs November 11, 2021 15:44

CSHARP-3936: Code review.

0029789

JamesKovacs requested changes Nov 11, 2021

View reviewed changes

CSHARP-3936: Code review.

f534ef2

DmitryLukyanov requested a review from JamesKovacs November 12, 2021 00:17

CSHARP-3936: Always configure ca certs.

953f366

JamesKovacs approved these changes Nov 17, 2021

View reviewed changes

DmitryLukyanov added 3 commits November 19, 2021 01:30

CSHARP-3936: Revert temp changes.

41ab988

CSHARP-3936: Code review.

22ab2cd

CSHARP-3936: Solve conflicts.

7c2d5c8

CSHARP-3936: Fix OS script.

58ccd79

DmitryLukyanov force-pushed the csharp3936 branch 2 times, most recently from 4929f15 to 58ccd79 Compare November 18, 2021 23:21

JamesKovacs approved these changes Nov 18, 2021

View reviewed changes

Merge conflict.

cd6b87e

DmitryLukyanov merged commit 5cdfe04 into mongodb:master Nov 18, 2021

CSHARP-3936: Support KMIP in client side field level encryption. #675

CSHARP-3936: Support KMIP in client side field level encryption. #675

Uh oh!

Conversation

DmitryLukyanov commented Nov 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DmitryLukyanov commented Nov 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesKovacs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JamesKovacs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesKovacs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DmitryLukyanov commented Nov 18, 2021 •

edited

Loading