CSHARP-2838: MONGODB-AWS Support #22

MikalaiMazurenka · 2020-03-13T19:53:23Z

Evergreen: https://evergreen.mongodb.com/version/5e728a2056234309d47a9510

Note: Mark's AWS .NET POC branch: https://github.com/markbenvenuto/mongo-csharp-driver/commits/iam_auth

MikalaiMazurenka · 2020-03-13T20:07:08Z

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

+        public static string GetRegion(string host)
+        {
+            if (host == "sts.amazonaws.com")
+            {
+                return "us-east-1";
+            }
+
+            var split = host.Split('.');
+            if (split.Count() > 1)
+            {
+                return split[1];
+            }
+
+            return "us-east-1";
+        }


From specification (link):

Region is not a header, but simply part of the authorization header. Region by default is ‘us-east-1’ since this is the implicit region for ‘sts.amazonaws.com’. Drivers will need to derive the region to use from the endpoint. The region is the second piece of a FQDN name. While all official AWS STS endpoints start with “sts.”, there are non-AWS hosted endpoints and test endpoints that will not follow this rule.

Host | Region sts.amazonaws.com | us-east-1 first.second | second first | us-east-1

MikalaiMazurenka · 2020-03-13T20:13:07Z

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

+        }
+
+        /// <summary>
+        /// Signs the request.


For composing signature details refer to specification: link
Specifically this image (it is included in specification): link

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

MikalaiMazurenka · 2020-03-13T20:18:51Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+    /// </summary>
+    public class MongoAWSAuthenticator : SaslAuthenticator
+    {
+        private static readonly Lazy<HttpClient> _httpClientInstance = new Lazy<HttpClient>(() =>


Here is the usage of single instance HttpClient we discussed in slack.

Let's move the whole httpclient details into a helper class. Consider how it can be:
DmitryLukyanov@1b03be9#diff-ffb35b98eecd05d3c7b6133327bf13b3R375-R479

Great job! Thanks for suggested code!
Done.

MikalaiMazurenka · 2020-03-13T20:20:13Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+        private const string awsContainerCredentialsRelativeUriKey = "AWS_CONTAINER_CREDENTIALS_RELATIVE_URI";
+        private const string awsSessionTokenMechanismPropertyKey = "AWS_SESSION_TOKEN";
+        private const string ecsBaseUri = "http://169.254.170.2";
+        private const string ec2BaseUri = "http://169.254.169.254";
+        private const int randomLength = 32;


Constant values taken from spec: link

Mostly we use the upper camelcase for constants names

Also, the constant name does tell for what reason this value will be used. I think this value may be moved into the method private byte[] GenerateRandomBytes() similar how it's done in https://github.com/mongodb/mongo-csharp-driver/blob/master/src/MongoDB.Driver.Core/Core/Authentication/ScramShaAuthenticator.cs#L160

It's also used in server nonce validation here: https://github.com/MikalaiMazurenka/mongo-csharp-driver/pull/22/files#diff-ffb35b98eecd05d3c7b6133327bf13b3R436
So to avoid using magic numbers in code, I leave it as is. Regarding case: I see different formats throughout the solution. E.g. pascal case: link, winapi style: link (not sure how it's called), camel case: link. Could you please link some guidelines on this?
For now I've made the name more meaningful.

The only guide is to use the most latest way, and as far as I know, in the latest code, we use the upper camelcase format. It's a good point to mention it somewhere, I think we have some document which can be used for that target. @vincentkam do you remember the document which we wanted to use for that target (I remember we discussed similar case around a year ago)?

MikalaiMazurenka · 2020-03-13T20:25:59Z

src/MongoDB.Driver/MongoCredential.cs

+                    if (username == null)
+                    {
+                        if (evidence != null && evidence is PasswordEvidence)
+                        {
+                            throw new ArgumentException("A MONGODB-AWS must have access key id.");
+                        }
+
+                        return new MongoCredential(
+                            mechanism,
+                            new MongoExternalAwsIdentity(),
+                            evidence);
+                    }
+                    if (evidence == null || evidence is ExternalEvidence)
+                    {
+                        throw new ArgumentException("A MONGODB-AWS must have secret access key.");
+                    }


Regarding validation please refer to spec (link):

regardless of how Drivers obtain credentials the only valid combination of credentials is an access key ID and a secret access key or an access key ID, a secret access key and a session token.

This message about needing secret access key appears often enough that I wonder if we should have a constant that defines it?

Those are different:

"A MONGODB-AWS must have access key id."
"A MONGODB-AWS must have secret access key."

MikalaiMazurenka · 2020-03-13T20:27:19Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+            public string SessionToken;
+        }
+
+        private class MongoAWSMechanism : ISaslMechanism


The conversation process is described in specification: link

MikalaiMazurenka · 2020-03-13T20:28:52Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+        private static readonly Lazy<HttpClient> _httpClientInstance = new Lazy<HttpClient>(() =>
+        {
+            var httpClient = new HttpClient();
+            httpClient.Timeout = TimeSpan.FromSeconds(10);


From specification (link):

Drivers SHOULD enforce a 10 second read timeout while waiting for incoming content from both the ECS and EC2 endpoints.

MikalaiMazurenka · 2020-03-13T20:32:44Z

src/MongoDB.Driver.Core/Core/Misc/DefaultRandomByteGenerator.cs

+
+namespace MongoDB.Driver.Core.Misc
+{
+    internal class DefaultRandomByteGenerator : IRandomByteGenerator


As per conversation with Mark Benvenuto, for AWS auth I switched to random byte generation (with 256 possible values) instead of existing random character generation (with around 100).

MikalaiMazurenka · 2020-03-13T20:33:54Z

src/MongoDB.Driver.Core/MongoDB.Driver.Core.csproj

+  <ItemGroup>
+    <Reference Include="System.Net.Http" />
+  </ItemGroup>
+


This is an inevitable requirement for using HttpClient (link).

Should we be using a PackageReference, targeting https://www.nuget.org/packages/System.Net.Http/ instead of a Reference? I feel like Reference only works if the user already has that dll/package somewhere in their path but I'm not 100% on that...

evergreen/evergreen.yml

bazile-clyde · 2020-03-16T17:16:42Z

evergreen/evergreen.yml

+            PASS=$(urlencode "${iam_auth_ecs_secret_access_key}")
+            MONGODB_URI="mongodb://$USER:$PASS@localhost"
+          EOF
+          PROJECT_DIRECTORY=${PROJECT_DIRECTORY} evergreen/run-mongodb-aws-test.sh


Does this need to be silent? This is silent in Java because the URI is echoed as part of the command that runs the test here. And the URI will be echoed again when the test run. Thus any test that passed secrets as the value to -Dorg.mongodb.test.uri= was silenced. This doesn't appear to be the case from looking at run-mongodb-aws-test.sh but I haven't looked at the output in evergreen. If evergreen doesn't echo the URI I'd consider putting this in a separate command. Looking at the POC for Node or Python might be helpful.

I see Python splitting this test into two parts: one which must be silenced and one not (link)
I will run a patch build to check this, but I'm mostly sure cat command should be silenced.

Indeed silencing can be removed there.
Done.

bazile-clyde · 2020-03-16T17:17:47Z

evergreen/evergreen.yml

+            SESSION_TOKEN=$(urlencode $SESSION_TOKEN)
+            MONGODB_URI="mongodb://$USER:$PASS@localhost"
+          EOF
+          PROJECT_DIRECTORY=${PROJECT_DIRECTORY} DRIVERS_TOOLS=${DRIVERS_TOOLS} evergreen/run-mongodb-aws-test.sh


Per my comment above, I don't think this needs to be silent.

evergreen/evergreen.yml

MikalaiMazurenka

Ready for review!

evergreen/evergreen.yml

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

tests/MongoDB.Driver.Core.Tests/Core/Authentication/AvsSignatureVersion4Tests.cs

bazile-clyde · 2020-03-16T18:23:18Z

tests/MongoDB.Driver.Core.Tests/Core/Authentication/AvsSignatureVersion4Tests.cs

+            actual.Should().Be(expected);
+        }
+    }
+}


For the unit test in Java, I used the values in AWS's documentation as opposed to the values returned from the mock servers. This was to ensure that my code conformed to AWS's signature v4 process rather than the mocks rendition of it, should they differ. It might be worthwhile to the same as a cautionary measure.

MikalaiMazurenka

Ready for review!

evergreen/evergreen.yml

MikalaiMazurenka · 2020-03-16T17:37:15Z

evergreen/evergreen.yml

+            PASS=$(urlencode "${iam_auth_ecs_secret_access_key}")
+            MONGODB_URI="mongodb://$USER:$PASS@localhost"
+          EOF
+          PROJECT_DIRECTORY=${PROJECT_DIRECTORY} evergreen/run-mongodb-aws-test.sh


I see Python splitting this test into two parts: one which must be silenced and one not (link)
I will run a patch build to check this, but I'm mostly sure cat command should be silenced.

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

tests/MongoDB.Driver.Core.Tests/Core/Authentication/AvsSignatureVersion4Tests.cs

DmitryLukyanov

Partial review, mostly for the MongoAWSAuthenticator. I will continue reviewing.
Commit with suggestions: DmitryLukyanov@1b03be9

build.cake

evergreen/evergreen.yml

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

DmitryLukyanov · 2020-03-16T21:09:05Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+            }
+        }
+
+        private static BsonDocument ToDocument(byte[] bytes)


I'm not sure that this is the best way to do this work. However maybe I missed something, I need to understand what data are presented in bytes to be sure. I think we need to have a test in Core with mocked data similar to: https://github.com/mongodb/mongo-csharp-driver/blob/master/tests/MongoDB.Driver.Core.Tests/Core/Authentication/ScramSha256AuthenticatorTests.cs#L33. This will allow us to be able to run these code paths without any additional configuration

I've added the high level mocked test.

DmitryLukyanov · 2020-03-16T21:10:13Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+        {
+            BsonBinaryWriterSettings settings = new BsonBinaryWriterSettings()
+            {
+#pragma warning disable 618


I guess you need to change this code on the following:

if (BsonDefaults.GuidRepresentationMode == GuidRepresentationMode.V2) { clientSettings.GuidRepresentation = GuidRepresentation.Unspecified; }

Done. Please recheck that it's what you expected.

It's better to leave the suggested code since the reason why it's left without covering via methods or some other possible refactoring, that it will be removed in the version 3.0

DmitryLukyanov · 2020-03-16T21:13:41Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                    throw new MongoAuthenticationException(conversation.ConnectionId, message: "Server sent an invalid nonce.");
+                }
+
+                var tuple = AwsSignatureVersion4.SignRequest(


I don't think that tuple is a good enough variable name here. Also, please note that previously we decided to avoid using tuples in the driver code

Refactored with out parameter.
Done.

DmitryLukyanov · 2020-03-16T21:16:13Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                    document.Add("t", _sessionToken);
+                }
+
+                var clientSecondMessageBytes = ToBytes(document);


NOTE: previously we used encoding.GetBytes for similar case https://github.com/mongodb/mongo-csharp-driver/blob/master/src/MongoDB.Driver.Core/Core/Authentication/ScramShaAuthenticator.cs#L253 Not sure that document is better to use here. I will come back to it in my next round review.

My point that previously we didn't use BsonDocument for that target. Instead we used Utf8Encodings.Strict. See here: https://github.com/mongodb/mongo-csharp-driver/blob/master/src/MongoDB.Driver.Core/Core/Authentication/ScramShaAuthenticator.cs#L253
But I don't think that there is anything wrong in the current implementation. I will rely on other reviewers' opinion here

My apologies if I'm misunderstanding your comment Dima, but are you saying that we shouldn't use a BsonDocument here?

If so, I think the key difference between SCRAM-SHA and MONGODB-AWS is that the payload field in SCRAM-SHA uses the "SASL map" format (https://github.com/mongodb/mongo-csharp-driver/blob/master/src/MongoDB.Driver.Core/Core/Authentication/ScramShaAuthenticator.cs#L210), which is essentially a fancy comma separated value string, and the payload field in MONGODB-AWS uses BSON (https://github.com/mongodb/specifications/blob/master/source/auth/auth.rst#mongodb-aws). Therefore, I believe the approach of using a BsonDocument here is correct.

Why can't we just use:

var clientSecondMessageBytes = document.ToBson();

@vincentkam , I agree with you. Originally I thought that I saw comma separated data here, but probably i was wrong

DmitryLukyanov · 2020-03-16T21:17:09Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+            }
+        }
+
+        private class ClientLast : ISaslStep


it looks like this class does nothing (unless I didn't miss something)? Is it expected?

It completes the conversation.

as far as I can see there is no real logic here, instead of this, we can just return CompletedStep. But maybe it makes sense to leave it to be consistent with the other implementations

I don't mind either way: I defer to Mikalai here :)

I'm ok with the current version

I leave it as is unless there are other concerns.

MikalaiMazurenka · 2020-03-16T22:13:14Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                var serverNonce = serverFirstMessageDoc["s"].AsByteArray;
+                var host = serverFirstMessageDoc["h"].AsString;
+
+                if (serverNonce.Length != randomLength * 2 || !serverNonce.Take(randomLength).SequenceEqual(_nonce))


From spec (link):

Drivers MUST validate that the server nonce is exactly 64 bytes and the first 32 bytes are the same as the client nonce.

DmitryLukyanov

So, currently, my main suggestion is adding a new mocked test in Core to be able to debug this functionality without additional configuration

DmitryLukyanov · 2020-03-16T22:06:24Z

evergreen/run-mongodb-aws-test.sh

+shopt -s expand_aliases # needed for `urlencode` alias
+[ -s "${PROJECT_DIRECTORY}/prepare_mongodb_aws.sh" ] && source "${PROJECT_DIRECTORY}/prepare_mongodb_aws.sh"
+
+MONGODB_URI=${MONGODB_URI:-"mongodb://localhost"}


Can we have empty MONGODB_URI at all? And even if so, should we update the up level script to fill it? As I understand, we don't fill connection string on this level usually

if we don't want to miss a bug in the our configuration when we missed filling of MONGODB_URI, maybe it's better to trigger exception in this case?

Good point! With addition of code here: line MONGODB_URI should always be set.
Done.

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

DmitryLukyanov · 2020-03-16T22:20:17Z

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

+        /// <returns>The region.</returns>
+        public static string GetRegion(string host)
+        {
+            if (host == "sts.amazonaws.com")


Consider:

public static string GetRegion(string host) { string[] splitted; if (host == "sts.amazonaws.com" || (splitted = host.Split('.')).Length <= 1) { return "us-east-1"; } return splitted[1]; }

I'd say that current code is a bit more readable (for me at least), but if you think suggested way is better, I'll update it.

Maybe,
I read this code as: if a host is sts.amazonaws.com or host has one or fewer parts => use us-east-1, otherwise use the first. This is exactly what I wrote. But I will rely on your and other reviewers opinion here

DmitryLukyanov · 2020-03-16T22:20:58Z

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

+            byte[] salt,
+            string host)
+        {
+            var body = "Action=GetCallerIdentity&Version=2011-06-15";


can you clarify this line?

From specification (see the name/value table, closest anchor: link):

Body | Action=GetCallerIdentity&Version=2011-06-15\n

I saw this line in the doc. This is weird, but at the same time this is what the doc requires

tests/MongoDB.Driver.Tests/Communication/Security/AwsAuthenticationTests.cs

DmitryLukyanov · 2020-03-16T22:39:36Z

tests/MongoDB.Driver.Tests/Communication/Security/AwsAuthenticationTests.cs

+            var connectionString = Environment.GetEnvironmentVariable(__environmentVariableName);
+            connectionString.Should().NotBeNull();
+
+            using (var client = CreateDisposableClient(connectionString))


Consider simplifying:

[Fact] public void Aws_authentication_should_behave_as_expected() { using (var client = DriverTestConfiguration.CreateDisposableClient()) { // test that a command that doesn't require auth completes normally var adminDatabase = client.GetDatabase("admin"); var isMasterCommand = new BsonDocument("ismaster", 1); var isMasterResult = adminDatabase.RunCommand<BsonDocument>(isMasterCommand); // test that a command that does require auth completes normally var database = client.GetDatabase(DriverTestConfiguration.DatabaseNamespace.DatabaseName); var collection = database.GetCollection<BsonDocument>(DriverTestConfiguration.CollectionNamespace.CollectionName); var count = collection.CountDocuments(FilterDefinition<BsonDocument>.Empty); } }

Maybe it's not important, but this test will be called not only for aws tests. I think it's not problem to run this small tests in any case, but it may confuse if we check the EG log and will see aws test for the environment when it wasn't configured.

NOTE: lately we use the following test naming notation: Aws_authentication_should_behave_as_expected

Good point.
Done.

@rstam regarding Dmitry's comment https://github.com/MikalaiMazurenka/mongo-csharp-driver/pull/22/files#r393350233
I think it's not crucial for this test to be run only on AWS task, but if yes, I'm thinking about checking an environment variable, e.g. "AWS_TESTS_ENABLED".
Could you please suggest which way it should be done here?

vincentkam

Very quick comments + weighing in on the SecureString discussion: more to come!

vincentkam · 2020-03-16T23:33:51Z

evergreen/run-mongodb-aws-test.sh

+# show test output
+set -x
+
+powershell.exe .\\build.ps1 -target TestAwsAuthentication


Nit: add missing newline at the end of the file?

Thanks, I didn't notice that!
Done.

vincentkam · 2020-03-17T00:19:36Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+            IRandomByteGenerator randomByteGenerator)
+        {
+            var relativeUri = Environment.GetEnvironmentVariable(awsContainerCredentialsRelativeUriKey);
+            var methodsList = new Func<AwsCredentials>[]


Hm, perhaps another name for methodsList? credentialsGetters?

This use of a list funcs is quite neat IMHO! :)

Thanks! I've taken name from Dmitry's suggestions (link)

vincentkam · 2020-03-17T00:27:44Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                () => AcquireMechanismDataFromEc2Response(ec2BaseUri)
+            };
+
+            foreach (var method in methodsList)


Side comment:
I was curious as to whether this could be expressed via LINQ and came up with this, which I think should work due to lazy linq evaluation:

var credentials = methodsList .Select(GetCredentials => GetCredentials()) .Where(creds => creds != null) .FirstOrDefault(creds => return new MongoAWSMechanism(creds.Credentials, creds.SessionToken, randomByteGenerator)); if (credentials == null) { throw new ArgumentException("A MONGODB-AWS must have access key ID."); } return credentials;

But the foreach loop is totally fine, one line shorter, and probably easier to read :) Just thought I'd share a tiny bit of fun I had.

Thanks! I feel the foreach would be a bit more readable.

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

DmitryLukyanov · 2020-03-16T22:53:14Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                            sessionToken = (string)pair.Value;
+                            break;
+                        default:
+                            throw new ArgumentException($"Unknown AWS property '{pair.Key}'.", "properties");


nameof(properties)?

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

DmitryLukyanov · 2020-03-17T18:46:54Z

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

+    /// <summary>
+    /// The AWS signature version 4.
+    /// </summary>
+    public class AwsSignatureVersion4


Should this class really be public and even a separate non nested class?

it looks like the only logic which happens here is creating signature request

Perhaps we could make this class internal + static?

Good point!
Done.

DmitryLukyanov · 2020-03-17T18:51:21Z

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

+        /// </summary>
+        /// <param name="requestHeaders">The request headers.</param>
+        /// <returns>The canonical headers.</returns>
+        public static string GetCanonicalHeaders(SortedDictionary<string, string> requestHeaders)


Idk, but this name makes me feel that some real logic happens inside, but it can be called just ToString or so

I don't mind this name as a getter can have minimal logic. Also I feel like it might be a bit odd for a static class to have a ToString() method?

From my point of view this method improves readability, so I would like to leave it as is, unless there are other concerns.

a static class to have a ToString() method?

My point was that I don't see how changing a form from dictionary to a string can be named getting canonical headers? It sounds like just technical operation. In other words what was done in the scope of this method to call it getting canonical headers? Maybe I'm missing something. It's a minor point and I can rely on other reviewers here

I checked this link https://raw.githubusercontent.com/mongodb/specifications/master/source/auth/includes/calculating_a_signature.png and I see where this came from.
However, it's still confusing me a bit, since the dictionary before this method also was in canonical form, the only change here that we change a format of this data

DmitryLukyanov · 2020-03-17T18:52:22Z

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

+        /// </summary>
+        /// <param name="requestHeaders">The request headers.</param>
+        /// <returns>The signed headers.</returns>
+        public static string GetSignedHeaders(SortedDictionary<string, string> requestHeaders)


similar to the above, does this name tell what happens inside this method?

Yep, please refer to specification illustration: link
It's SignedHeaders on the picture and the method which acquires them should be called GetSignedHeaders.

As I wrote above, I will rely on the other reviewers' opinion here

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

DmitryLukyanov · 2020-03-17T18:54:35Z

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

+        /// <param name="salt">The salt.</param>
+        /// <param name="host">The host.</param>
+        /// <returns>The signed request.</returns>
+        public static Tuple<string, string> SignRequest(


CreateAuthenticateRequest?

I would like to leave it as is. Because this method does a lot of signing and returns signed request.

Based on this name, I would say that this method signs some request which was passed to him which is not true.
CreateSignedRequest?
I will rely on your and other reviewers' opinion here

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

DmitryLukyanov · 2020-03-17T19:12:31Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+        private const string awsContainerCredentialsRelativeUriKey = "AWS_CONTAINER_CREDENTIALS_RELATIVE_URI";
+        private const string awsSessionTokenMechanismPropertyKey = "AWS_SESSION_TOKEN";
+        private const string ecsBaseUri = "http://169.254.170.2";
+        private const string ec2BaseUri = "http://169.254.169.254";
+        private const int randomLength = 32;


Mostly we use the upper camelcase for constants names

DmitryLukyanov · 2020-03-17T19:14:57Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+        private const string awsContainerCredentialsRelativeUriKey = "AWS_CONTAINER_CREDENTIALS_RELATIVE_URI";
+        private const string awsSessionTokenMechanismPropertyKey = "AWS_SESSION_TOKEN";
+        private const string ecsBaseUri = "http://169.254.170.2";
+        private const string ec2BaseUri = "http://169.254.169.254";
+        private const int randomLength = 32;


Also, the constant name does tell for what reason this value will be used. I think this value may be moved into the method private byte[] GenerateRandomBytes() similar how it's done in https://github.com/mongodb/mongo-csharp-driver/blob/master/src/MongoDB.Driver.Core/Core/Authentication/ScramShaAuthenticator.cs#L160

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

vincentkam · 2020-03-17T21:54:33Z

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

+        /// </summary>
+        /// <param name="requestHeaders">The request headers.</param>
+        /// <returns>The canonical headers.</returns>
+        public static string GetCanonicalHeaders(SortedDictionary<string, string> requestHeaders)


I don't mind this name as a getter can have minimal logic. Also I feel like it might be a bit odd for a static class to have a ToString() method?

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

MikalaiMazurenka

Ready for review!

build.cake

evergreen/evergreen.yml

MikalaiMazurenka · 2020-03-17T19:04:54Z

evergreen/run-mongodb-aws-test.sh

+# show test output
+set -x
+
+powershell.exe .\\build.ps1 -target TestAwsAuthentication


Thanks, I didn't notice that!
Done.

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

MikalaiMazurenka · 2020-03-18T16:20:41Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+        private const string awsContainerCredentialsRelativeUriKey = "AWS_CONTAINER_CREDENTIALS_RELATIVE_URI";
+        private const string awsSessionTokenMechanismPropertyKey = "AWS_SESSION_TOKEN";
+        private const string ecsBaseUri = "http://169.254.170.2";
+        private const string ec2BaseUri = "http://169.254.169.254";
+        private const int randomLength = 32;


It's also used in server nonce validation here: https://github.com/MikalaiMazurenka/mongo-csharp-driver/pull/22/files#diff-ffb35b98eecd05d3c7b6133327bf13b3R436
So to avoid using magic numbers in code, I leave it as is. Regarding case: I see different formats throughout the solution. E.g. pascal case: link, winapi style: link (not sure how it's called), camel case: link. Could you please link some guidelines on this?
For now I've made the name more meaningful.

MikalaiMazurenka · 2020-03-18T16:24:18Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                            sessionToken = (string)pair.Value;
+                            break;
+                        default:
+                            throw new ArgumentException($"Unknown AWS property '{pair.Key}'.", "properties");


MikalaiMazurenka · 2020-03-18T16:29:52Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+        {
+            BsonBinaryWriterSettings settings = new BsonBinaryWriterSettings()
+            {
+#pragma warning disable 618


Done. Please recheck that it's what you expected.

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

tests/MongoDB.Driver.Tests/Communication/Security/AwsAuthenticationTests.cs

DmitryLukyanov · 2020-03-18T17:12:27Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                return roleRequest;
+            }
+
+            private static HttpRequestMessage CraeteTokenRequest(Uri baseUri)


CreateTokenRequest?

Good notice.
Done.

DmitryLukyanov · 2020-03-18T17:19:46Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+        private const string awsContainerCredentialsRelativeUriKey = "AWS_CONTAINER_CREDENTIALS_RELATIVE_URI";
+        private const string awsSessionTokenMechanismPropertyKey = "AWS_SESSION_TOKEN";
+        private const string ecsBaseUri = "http://169.254.170.2";
+        private const string ec2BaseUri = "http://169.254.169.254";
+        private const int randomLength = 32;


The only guide is to use the most latest way, and as far as I know, in the latest code, we use the upper camelcase format. It's a good point to mention it somewhere, I think we have some document which can be used for that target. @vincentkam do you remember the document which we wanted to use for that target (I remember we discussed similar case around a year ago)?

DmitryLukyanov · 2020-03-18T17:25:59Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+            var username = Environment.GetEnvironmentVariable("AWS_ACCESS_KEY_ID");
+            var password = Environment.GetEnvironmentVariable("AWS_SECRET_ACCESS_KEY");
+            var sessionToken = Environment.GetEnvironmentVariable("AWS_SESSION_TOKEN");
+            ValidateCredentials(username, password, sessionToken);


I think this method should be responsible only for the creation of Aws credentials. The whole validation can be moved in one place where we iterate the created credentials.
It will also allow us to avoid code duplication in the ValidateCredentials methods. The only reason why we have two methods now with fully the same idea is because we have passwords in two forms (string and SecurePassword), after this method, we will have only one variant of the password.

DmitryLukyanov · 2020-03-18T17:32:46Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+            return sessionToken;
+        }
+
+        private static SecureString ToSecureString(string str)


I'm not sure that we need to have this logic here. We already have the same method in UsernamePasswordCredential. I think we can create a helper for that target or move this method to BsonUtils. Thoughts?
I will be ok with any solution here.

As you can see UsernamePasswordCredential.ConvertPasswordToSecureString is private: link
And I wouldn't like to touch existing code, unless there's a crucial necessity. Moreover, my updated ToSecureString wouldn't throw on null input. So I would like to leave this method, unless you insist.

DmitryLukyanov · 2020-03-18T18:56:29Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+
+            var response = AwsHttpClientHelper.GetECSResponse(relativeUri).GetAwaiter().GetResult();
+            var parsedReponse = BsonDocument.Parse(response);
+            var username = parsedReponse.GetValue("AccessKeyId")?.AsString;


this code will trigger an exception in case if AccessKeyId is not presented in the document, you need to specify the default value:

var username = parsedReponse.GetValue("AccessKeyId", null)?.AsString;

DmitryLukyanov · 2020-03-18T19:01:16Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+
+        private static AwsCredentials CreateAwsCredentialsFromEcsResponse(string relativeUri)
+        {
+            if (relativeUri == null)


I'm not sure why you want to specify the relative path for that case in the top level, but the value latest/meta-data/iam/security-credentials/ for ec2 at the lowest level. I would say we need to specify both of them at the same level

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

DmitryLukyanov · 2020-03-18T19:04:13Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                HttpResponseMessage response;
+                try
+                {
+                    response = await __httpClientInstance.Value.SendAsync(request).ConfigureAwait(false);


copied from outdated: #22 (comment)

DmitryLukyanov · 2020-03-18T19:06:11Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+            }
+        }
+
+        private class ClientLast : ISaslStep


as far as I can see there is no real logic here, instead of this, we can just return CompletedStep. But maybe it makes sense to leave it to be consistent with the other implementations

DmitryLukyanov · 2020-03-18T20:00:51Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                    throw new MongoAuthenticationException(conversation.ConnectionId, message: "Server sent an invalid nonce.");
+                }
+
+                AwsSignatureVersion4.SignRequest(


I'm not sure that SignRequest method name really tells about what happens inside. As I understand, we just create an authenticateHeader

This name is perfectly fine.

DmitryLukyanov · 2020-03-18T20:01:33Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                    _sessionToken,
+                    serverNonce,
+                    host,
+                    out var authHeader,


as far as I know, mostly we don't use the short names in the driver code

DmitryLukyanov · 2020-03-18T20:04:21Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                    document.Add("t", _sessionToken);
+                }
+
+                var clientSecondMessageBytes = ToBytes(document);


My point that previously we didn't use BsonDocument for that target. Instead we used Utf8Encodings.Strict. See here: https://github.com/mongodb/mongo-csharp-driver/blob/master/src/MongoDB.Driver.Core/Core/Authentication/ScramShaAuthenticator.cs#L253
But I don't think that there is anything wrong in the current implementation. I will rely on other reviewers' opinion here

DmitryLukyanov · 2020-03-18T20:07:10Z

src/MongoDB.Driver/MongoCredential.cs

+                    EnsureNullOrExternalSource(mechanism, source);
+                    if (username == null)
+                    {
+                        if (evidence != null && evidence is PasswordEvidence)


Note: it's not really necessary to check evidence != null

It is done in other places: link and link
So I am doing this as in existing implementation.

DmitryLukyanov · 2020-03-18T20:07:50Z

src/MongoDB.Driver/MongoCredential.cs

+                    {
+                        if (evidence != null && evidence is PasswordEvidence)
+                        {
+                            throw new ArgumentException("A MONGODB-AWS must have access key id.");


I'm not sure about this exception. I will rely on @rstam opinion here

Ditto above: should this "A MONGODB-AWS credential must have an access key ID."? Perhaps we should use a constant?

I am implementing this according to existing code: here, here and here.

MikalaiMazurenka

Ready for review!

MikalaiMazurenka · 2020-03-18T20:03:47Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                return roleRequest;
+            }
+
+            private static HttpRequestMessage CraeteTokenRequest(Uri baseUri)


Good notice.
Done.

MikalaiMazurenka · 2020-03-18T20:12:57Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+            var username = Environment.GetEnvironmentVariable("AWS_ACCESS_KEY_ID");
+            var password = Environment.GetEnvironmentVariable("AWS_SECRET_ACCESS_KEY");
+            var sessionToken = Environment.GetEnvironmentVariable("AWS_SESSION_TOKEN");
+            ValidateCredentials(username, password, sessionToken);


MikalaiMazurenka · 2020-03-18T20:17:17Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+            return sessionToken;
+        }
+
+        private static SecureString ToSecureString(string str)


As you can see UsernamePasswordCredential.ConvertPasswordToSecureString is private: link
And I wouldn't like to touch existing code, unless there's a crucial necessity. Moreover, my updated ToSecureString wouldn't throw on null input. So I would like to leave this method, unless you insist.

vincentkam · 2020-03-18T20:39:36Z

src/MongoDB.Driver.Core/MongoDB.Driver.Core.csproj

@@ -99,6 +99,7 @@

  <ItemGroup Condition="'$(TargetFramework)' == 'net452'">
    <PackageReference Include="System.Runtime.InteropServices.RuntimeInformation" Version="4.3.0" />
+    <Reference Include="System.Net.Http" />


Hm, now I'm slightly confused: According to this, HttpClient is available for the following platforms

.NET Framework
4.8 4.7.2 4.7.1 4.7 4.6.2 4.6.1 4.6 4.5.2 4.5.1 4.5
.NET Standard
2.1 2.0 1.6 1.4 1.3 1.2 1.1

It seems weird for it to be available .Net Standard 1.4 and 1.6 but not 1.5, so this feels like a documentation error to me to be honest. I've confirmed via local testing that the current Driver.Core project builds when targeting .Net Standard 1.5 and 2.0, and fails to build when targeting .NET Framework 4.5.2 when I remove line 102 <Reference Include="System.Net.Http" />

My (other) question is: should we err on the side of safety and use a NuGet package reference instead of a regular reference? i.e.
<PackageReference Include="System.Net.Http" Version="4.3.4" /> (i.e. including https://www.nuget.org/packages/System.Net.Http/).

@DmitryLukyanov @rstam

I'll defer to you all on this one.

I think it may be safer to use the PackageReference. I believe by using Reference, the application will need to have the dll in its path somewhere: for our developer machines and the evergren machines, that wouldn't be a problem because we have the DLL from later versions of .NET. By using a PackageReference, I think that it'd allow a machine to not have the DLL on its path at all, as it'll simply download the package from Nuget as needed. What do you think Mikalai?

vincentkam · 2020-03-18T22:02:40Z

evergreen/run-mongodb-aws-test.sh

@@ -0,0 +1,35 @@
+#!/bin/bash


A random side nit for this and other bash scripts:
My loose understanding is that using
#!/usr/bin/env bash
increases the portability of the script (as bash may not always be in /bin/bash). Probably not an issue in this case since our scripts were tailored for Cygwin, where bash does indeed exist at /bin/bash all the time (AFAIK). So again, as this is merely a nit, feel free to ignore :)

Thanks for suggestion!
Done.

vincentkam · 2020-03-18T22:35:23Z

tests/MongoDB.Driver.Core.Tests/Core/Authentication/MongoAWSAuthenticatorTests.cs

+
+            var subject = new MongoAWSAuthenticator(credential, null, randomByteGeneratorMock.Object, dateTimeProviderMock.Object);
+
+            var connection = new MockConnection(serverId);


Ah interesting hm... perhaps we should mock ServerVersion as well.

vincentkam · 2020-03-18T22:41:36Z

tests/MongoDB.Driver.Core.Tests/Core/Authentication/MongoAWSAuthenticatorTests.cs

+
+            var actualRequestId0 = sentMessages[0]["requestId"].AsInt32;
+            var actualRequestId1 = sentMessages[1]["requestId"].AsInt32;
+            actualRequestId0.Should().BeInRange(expectedRequestId, expectedRequestId + 10);


Hm I wonder if this pattern was meant for "live" connections instead of "mocked" connections. We can keep it, or we can change it to use +1: I defer to Mikalai.

vincentkam · 2020-03-18T22:44:46Z

tests/MongoDB.Driver.Core.Tests/Core/Authentication/MongoAWSAuthenticatorTests.cs

+        private static string GetExpectedSaslContinueMessage(int requestId, BsonDocument clientMessage)
+        {
+            return  "{" +
+                        "opcode: \"query\", " +


Just an FYI: An alternative style to multiline bsondocuments, but feel free to simply resolve this after reading this, I just wanted to share another option :) :

mongo-csharp-driver/tests/MongoDB.Driver.Core.Tests/Core/Authentication/ScramSha256AuthenticatorTests.cs

Line 251 in 8925eae

@"{opcode: ""query""," +

rstam · 2020-03-19T00:05:11Z

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

+
+            timestamp = dateTime.ToString("yyyyMMddTHHmmssZ");
+
+            string datestamp = dateTime.ToString("yyyyMMdd");


nit: use var

rstam · 2020-03-19T00:05:54Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+using MongoDB.Bson.Serialization;
+using MongoDB.Bson.Serialization.Serializers;
+using MongoDB.Driver.Core.Connections;
+using MongoDB.Driver.Core.Misc;


nit: sort and remove unused namespaces

rstam · 2020-03-19T00:06:26Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+            UsernamePasswordCredential credential,
+            IEnumerable<KeyValuePair<string, string>> properties,
+            IRandomByteGenerator randomByteGenerator,
+            IClock dateTimeProvider)


nit: name parameter "clock"

rstam · 2020-03-19T00:06:32Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+            string username,
+            IEnumerable<KeyValuePair<string, string>> properties,
+            IRandomByteGenerator randomByteGenerator,
+            IClock dateTimeProvider)


nit: name parameter "clock"

rstam · 2020-03-19T00:06:46Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+            UsernamePasswordCredential credential,
+            IEnumerable<KeyValuePair<string, string>> properties,
+            IRandomByteGenerator randomByteGenerator,
+            IClock dateTimeProvider)


nit: name parameter "clock"

rstam · 2020-03-19T00:38:39Z

tests/MongoDB.Driver.Core.Tests/Core/Authentication/MongoAWSAuthenticatorTests.cs

+                            "\"mechanism\" : \"MONGODB-AWS\", " +
+                            $"\"payload\" : new BinData(0, \"{ToBase64(ToBytes(clientMessage))}\") " +
+                        "}" +
+                    "}";


Same as above regarding formatting.

rstam · 2020-03-19T00:39:42Z

tests/MongoDB.Driver.Core.Tests/Core/Authentication/MongoAWSAuthenticatorTests.cs

+            return Convert.ToBase64String(bytes);
+        }
+
+        private static byte[] ToBytes(BsonDocument doc)


I think this method is not needed. Callers can just use:

var bytes = document.ToBson();

rstam · 2020-03-19T00:42:35Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                }
+            }
+
+            throw new ArgumentException("A MONGODB-AWS must have access key ID.");


I agree that ArgumentException is questionable. ArgumentException is usually reserved for problems with the arguments of the method itself.

Not sure InvalidDataException is right either (there is no data...).

Normally I think we would throw an InvalidOperationException here.

rstam · 2020-03-19T00:44:29Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                }
+            }
+
+            throw new ArgumentException("A MONGODB-AWS must have access key ID.");


Regarding the error message, make sure every message is a grammatically correct sentence that clearly identifies the object that is involved. So "MONGODB-AWS" doesn't seem correct (i.e. what is a "MONGODB-AWS").

Seems like the correct object is either a AwsCredential or a MongoAwsAuthenticator depending on where the error check is.

rstam · 2020-03-19T00:47:57Z

src/MongoDB.Driver.Core/MongoDB.Driver.Core.csproj

@@ -99,6 +99,7 @@

  <ItemGroup Condition="'$(TargetFramework)' == 'net452'">
    <PackageReference Include="System.Runtime.InteropServices.RuntimeInformation" Version="4.3.0" />
+    <Reference Include="System.Net.Http" />


I'll defer to you all on this one.

DmitryLukyanov · 2020-03-19T13:22:20Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+
+        // constructors
+        /// <summary>
+        /// Initializes a new instance of the <see cref="GssapiAuthenticator"/> class.


GssapiAuthenticator

Good point.
Done.

DmitryLukyanov · 2020-03-19T13:22:29Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+        }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="GssapiAuthenticator"/> class.


The same as above

DmitryLukyanov · 2020-03-19T13:27:07Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                () => CreateAwsCredentialsFromEnvironmentVariables(),
+                () => CreateAwsCredentialsFromEcsResponse(relativeUri),
+                () => CreateAwsCredentialsFromEc2Response()
+            };


I think it was my suggestion to have debugging a bit simpler. But I like Robert's idea too.

DmitryLukyanov · 2020-03-19T13:28:56Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+        {
+            var username = Environment.GetEnvironmentVariable("AWS_ACCESS_KEY_ID");
+            var password = Environment.GetEnvironmentVariable("AWS_SECRET_ACCESS_KEY");
+            var sessionToken = Environment.GetEnvironmentVariable("AWS_SESSION_TOKEN");


Should we add an exception in this case? If only one or two required fields were added?

DmitryLukyanov · 2020-03-19T13:30:51Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+            var parsedReponse = BsonDocument.Parse(response);
+            var username = parsedReponse.GetValue("AccessKeyId")?.AsString;
+            var password = parsedReponse.GetValue("SecretAccessKey")?.AsString;
+            var sessionToken = parsedReponse.GetValue("Token")?.AsString;


Should we add an exception in case if only one or two required fields were received?

DmitryLukyanov · 2020-03-19T13:33:47Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                var context = BsonDeserializationContext.CreateRoot(jsonReader);
+                return BsonDocumentSerializer.Instance.Deserialize(context);
+            }
+        }


I like Robert's suggestion

DmitryLukyanov · 2020-03-19T13:51:46Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                    document.Add("t", _sessionToken);
+                }
+
+                var clientSecondMessageBytes = ToBytes(document);


@vincentkam , I agree with you. Originally I thought that I saw comma separated data here, but probably i was wrong

DmitryLukyanov · 2020-03-19T13:52:10Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+            }
+        }
+
+        private class ClientLast : ISaslStep


I'm ok with the current version

MikalaiMazurenka

Ready for review!

MikalaiMazurenka · 2020-03-19T13:23:40Z

evergreen/run-mongodb-aws-test.sh

@@ -0,0 +1,35 @@
+#!/bin/bash


Thanks for suggestion!
Done.

MikalaiMazurenka · 2020-03-19T13:24:42Z

evergreen/run-mongodb-aws-test.sh

+set -o errexit  # Exit the script with error if any of the commands fail
+
+############################################
+#            Main Program                  #


MikalaiMazurenka · 2020-03-19T13:26:47Z

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

+
+            timestamp = dateTime.ToString("yyyyMMddTHHmmssZ");
+
+            string datestamp = dateTime.ToString("yyyyMMdd");


MikalaiMazurenka · 2020-03-19T13:31:21Z

src/MongoDB.Driver.Core/Core/Authentication/AwsSignatureVersion4.cs

+        }
+
+        //private static methods
+        private static string GetCanonicalHeaders(SortedDictionary<string, string> requestHeaders)


In my opinion this name correlates to specification. Moreover Java uses the same name for method with same functionality and semantics: link
I prefer to leave it as is, unless there are other concerns besides naming.

MikalaiMazurenka · 2020-03-19T13:36:55Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+using MongoDB.Bson.Serialization;
+using MongoDB.Bson.Serialization.Serializers;
+using MongoDB.Driver.Core.Connections;
+using MongoDB.Driver.Core.Misc;


MikalaiMazurenka · 2020-03-19T17:52:29Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+
+        // constructors
+        /// <summary>
+        /// Initializes a new instance of the <see cref="GssapiAuthenticator"/> class.


Good point.
Done.

MikalaiMazurenka · 2020-03-19T17:52:37Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+        }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="GssapiAuthenticator"/> class.


MikalaiMazurenka · 2020-03-19T17:52:50Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+            {
+                () => CreateAwsCredentialsFromMongoCredentials(username, securePassword, properties),
+                () => CreateAwsCredentialsFromEnvironmentVariables(),
+                () => CreateAwsCredentialsFromEcsResponse(relativeUri),


MikalaiMazurenka · 2020-03-19T17:56:33Z

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs

+                if (_sessionToken != null)
+                {
+                    document.Add("t", _sessionToken);
+                }


MikalaiMazurenka · 2020-03-19T17:57:04Z

tests/MongoDB.Driver.Core.Tests/Core/Authentication/MongoAWSAuthenticatorTests.cs

+
+            var serverFirstMessage = new BsonDocument()
+                .Add("s", new BsonBinaryData(serverNonce))
+                .Add("h", new BsonString(host));


MikalaiMazurenka requested review from rstam, jyemin, vincentkam and DmitryLukyanov March 13, 2020 19:53

MikalaiMazurenka commented Mar 13, 2020

View reviewed changes

MikalaiMazurenka requested a review from bazile-clyde March 13, 2020 20:43

MikalaiMazurenka force-pushed the csharp2838 branch from 34aa986 to 9f08c4f Compare March 13, 2020 21:46

CSHARP-2838: MONGODB-AWS Support

2614e75

MikalaiMazurenka force-pushed the csharp2838 branch from 9f08c4f to 2614e75 Compare March 13, 2020 21:55

bazile-clyde reviewed Mar 16, 2020

View reviewed changes

evergreen/evergreen.yml Outdated Show resolved Hide resolved

bazile-clyde reviewed Mar 16, 2020

View reviewed changes

evergreen/evergreen.yml Show resolved Hide resolved

bazile-clyde reviewed Mar 16, 2020

View reviewed changes

evergreen/evergreen.yml Outdated Show resolved Hide resolved

bazile-clyde reviewed Mar 16, 2020

View reviewed changes

evergreen/evergreen.yml Show resolved Hide resolved

CSHARP-2838: Minor evergreen test fixes

da7b029

MikalaiMazurenka commented Mar 16, 2020

View reviewed changes

evergreen/evergreen.yml Show resolved Hide resolved

evergreen/evergreen.yml Outdated Show resolved Hide resolved

evergreen/evergreen.yml Outdated Show resolved Hide resolved

bazile-clyde reviewed Mar 16, 2020

View reviewed changes

src/MongoDB.Driver.Core/Core/Authentication/MongoAWSAuthenticator.cs Outdated Show resolved Hide resolved

bazile-clyde reviewed Mar 16, 2020

View reviewed changes

tests/MongoDB.Driver.Core.Tests/Core/Authentication/AvsSignatureVersion4Tests.cs Outdated Show resolved Hide resolved

bazile-clyde reviewed Mar 16, 2020

View reviewed changes

tests/MongoDB.Driver.Core.Tests/Core/Authentication/AvsSignatureVersion4Tests.cs Outdated Show resolved Hide resolved

bazile-clyde reviewed Mar 16, 2020

View reviewed changes

CSHARP-2838: Fix typo in file name

5704607

MikalaiMazurenka commented Mar 16, 2020

View reviewed changes

DmitryLukyanov requested changes Mar 16, 2020

View reviewed changes

MikalaiMazurenka commented Mar 16, 2020

View reviewed changes

DmitryLukyanov requested changes Mar 16, 2020

View reviewed changes

vincentkam reviewed Mar 17, 2020

View reviewed changes

jyemin removed their request for review March 17, 2020 18:58

DmitryLukyanov requested changes Mar 17, 2020

View reviewed changes

vincentkam requested changes Mar 18, 2020

View reviewed changes

CSHARP-2838: Update code according to review comments

dde784d

MikalaiMazurenka commented Mar 18, 2020

View reviewed changes

MikalaiMazurenka requested review from bazile-clyde, DmitryLukyanov and vincentkam March 18, 2020 16:37

DmitryLukyanov reviewed Mar 18, 2020

View reviewed changes

DmitryLukyanov requested changes Mar 18, 2020

View reviewed changes

CSHARP-2838: Minor refactoring

8925eae

MikalaiMazurenka commented Mar 18, 2020

View reviewed changes

MikalaiMazurenka requested a review from DmitryLukyanov March 18, 2020 20:23

vincentkam reviewed Mar 18, 2020

View reviewed changes

rstam requested changes Mar 19, 2020

View reviewed changes

DmitryLukyanov reviewed Mar 19, 2020

View reviewed changes

CSHARP-2838: Update code according to review comments

255c230

MikalaiMazurenka commented Mar 19, 2020

View reviewed changes

MikalaiMazurenka closed this Mar 19, 2020

DmitryLukyanov mentioned this pull request Mar 20, 2020

CSHARP-2838: MONGODB-AWS Support #23

Closed


		var subject = new MongoAWSAuthenticator(credential, null, randomByteGeneratorMock.Object, dateTimeProviderMock.Object);

		var connection = new MockConnection(serverId);


		timestamp = dateTime.ToString("yyyyMMddTHHmmssZ");

		string datestamp = dateTime.ToString("yyyyMMdd");

CSHARP-2838: MONGODB-AWS Support #22

CSHARP-2838: MONGODB-AWS Support #22

Conversation

MikalaiMazurenka commented Mar 13, 2020 • 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

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

MikalaiMazurenka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikalaiMazurenka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitryLukyanov left a comment • 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

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

DmitryLukyanov left a 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

Choose a reason for hiding this comment

vincentkam left a 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

MikalaiMazurenka commented Mar 13, 2020 •

edited

Loading

DmitryLukyanov left a comment •

edited

Loading