CDRIVER-4439 add AWS credential cache #1207

kevinAlbs · 2023-02-15T18:19:35Z

Summary

Add global cache for AWS credentials.
Add a new target, test-awsauth, to test new auth in AWS environments.

Verified with this patch build of AWS tasks.

Background & Motivation

DRIVERS-2333 describes the required behavior.

The test-awsauth target replaces mongoc-ping in AWS tests. test-awsauth includes caching tests specific to AWS authentication. test-awsauth requires access to API in private headers to modify the cache.

Cache locking

The cache is locked when making requests for credentials that may be cached.
This is consistent with the AWS SDK behavior locking around fetching.
I assume this is intended to prevent duplicate requests.

test-awsauth is intended to replace mongoc-ping in AWS tests. test-awsauth will include caching tests specific to AWS.

There is no reason to condition on the presence of ENABLE_MONGODB_AWS_AUTH Removing reduces duplicate definitions.

add _mongoc_aws_credentials_copy_to add _mongoc_aws_credentials_cache_t

test-awsauth statically links the C driver

vector-of-bool · 2023-02-15T19:27:53Z

src/libmongoc/src/mongoc/mongoc-cluster-aws-private.h

+   // expiration is the time in milliseconds since the Epoch when these
+   // credentials expire. If expiration is 0, the credentials do not have a
+   // known expiration.
+   uint64_t expiration_ms;


Could this be done with mcd_duration from mcd-time.h?

It would require a conversion of the time points:

Calculate the duration $D$ as the difference between the current UTC time and Expiration.

Create an mcd_duration d from $D$

Store the computed expire-time: mcd_time_point expires_at = mcd_later(mcd_now(), d)

Not sure if it's worth the trouble, but may eek out later issues with a bare integer-as-time. Also has a "never-expires" of MCD_TIME_POINT_MAX and "always-expired" MCD_TIME_POINT_MIN.

I think so. Updated to use mcd_timer.

vector-of-bool · 2023-02-15T19:29:25Z

src/libmongoc/src/mongoc/mongoc-cluster-aws-private.h

 bool
 _mongoc_aws_credentials_obtain (mongoc_uri_t *uri,
                                _mongoc_aws_credentials_t *creds,
                                bson_error_t *error);

+void
+_mongoc_aws_credentials_copy_to (const _mongoc_aws_credentials_t *src,


I believe it is more conventional to have the dest parameter first.

I plan to keep as is. The src, dst order is consistent with all other C driver functions named *_copy_to.

.evergreen/scripts/run-aws-tests.sh

.evergreen/scripts/run-mongodb-aws-ecs-test.sh

src/libmongoc/src/mongoc/mongoc-cluster-aws-private.h

src/libmongoc/src/mongoc/mongoc-cluster-aws.c

src/libmongoc/src/mongoc/mongoc-init.c

src/libmongoc/tests/test-awsauth.c

vector-of-bool · 2023-02-16T21:21:12Z

src/libmongoc/src/mongoc/mongoc-cluster-aws.c

+   mcd_timer expiration_timer;
+   bool expiration_set;


These can be dropped and the values written directly into creds->expiration, IIUC.

eramongodb

Some minor suggestions remaining; otherwise, LGTM.

eramongodb · 2023-02-21T20:43:11Z

src/libmongoc/src/mongoc/mongoc-cluster-aws.c

+      // Zero creds, so callers can safely call _mongoc_aws_credentials_cleanup.
+      *creds = MONGOC_AWS_CREDENTIALS_INIT;


Suggested change

// Zero creds, so callers can safely call _mongoc_aws_credentials_cleanup.

*creds = MONGOC_AWS_CREDENTIALS_INIT;

_mongoc_aws_credentials_cleanup (creds);

*creds = MONGOC_AWS_CREDENTIALS_INIT;

Suggest redefining behavior to be capable of handling an already-initialized "non-zero" creds argument, and suggest removing the line "The passed creds is expected to be uninitialized or zeroed." from documentation, to avoid encouraging the "require/support an uninitialized argument for a conditionally-initializing function" pattern.

to avoid encouraging the "require/support an uninitialized argument for a conditionally-initializing function" pattern.

Done. I agree with preferring not to support uninitialized arguments. That has been a source of confusion in C driver APIs that expect an uninitialized bson_t. CDRIVER-3368 is one example.

eramongodb · 2023-02-21T20:45:26Z

src/libmongoc/src/mongoc/mongoc-cluster-aws-private.h

+   (_mongoc_aws_credentials_t)                                                 \
+   {                                                                           \
+      .access_key_id = NULL, .secret_access_key = NULL, .session_token = NULL, \
+      .expiration = {{0}},                                                     \


Suggested change

.expiration = {{0}}, \

.expiration = {.value = {0}, .set = false}, \

Addresses the following warning:

warning: missing field 'set' initializer [-Wmissing-field-initializers] *creds = MONGOC_AWS_CREDENTIALS_INIT; ^ note: expanded from macro 'MONGOC_AWS_CREDENTIALS_INIT' .expiration = {{0}}, \ ^

Thank you for the suggested fix.

kevinAlbs

*-auth tasks with latest are failing with:

[2023/02/22 21:47:29.636] /data/mci/a00f187bc87fef01fb2eecba06c8c43c/mongoc/src/libmongoc/tests/test-mongoc-scram.c:295 _check_error(): [Unable to use SCRAM-SHA-256 based authentication for user without any SCRAM-SHA-256 credentials registered] does not contain [Authentication failed]

This is failing on the waterfall, and appears unrelated to this commit. The AWS tasks still pass, proceeding with merge.

kevinAlbs · 2023-02-22T16:05:58Z

src/libmongoc/src/mongoc/mongoc-cluster-aws-private.h

+   (_mongoc_aws_credentials_t)                                                 \
+   {                                                                           \
+      .access_key_id = NULL, .secret_access_key = NULL, .session_token = NULL, \
+      .expiration = {{0}},                                                     \


Thank you for the suggested fix.

kevinAlbs · 2023-02-22T16:06:52Z

src/libmongoc/src/mongoc/mongoc-cluster-aws.c

+      // Zero creds, so callers can safely call _mongoc_aws_credentials_cleanup.
+      *creds = MONGOC_AWS_CREDENTIALS_INIT;


to avoid encouraging the "require/support an uninitialized argument for a conditionally-initializing function" pattern.

Done. I agree with preferring not to support uninitialized arguments. That has been a source of confusion in C driver APIs that expect an uninitialized bson_t. CDRIVER-3368 is one example.

kevinAlbs added 8 commits February 15, 2023 13:15

add test-awsauth

065a1b0

test-awsauth is intended to replace mongoc-ping in AWS tests. test-awsauth will include caching tests specific to AWS.

unconditionally define _mongoc_aws_credentials_cleanup

c22bdf6

There is no reason to condition on the presence of ENABLE_MONGODB_AWS_AUTH Removing reduces duplicate definitions.

add AWS credential cache

90524db

add _mongoc_aws_credentials_copy_to add _mongoc_aws_credentials_cache_t

use AWS credential cache

3f2cdc9

use test-awsauth in Evergreen

2a185e2

remove outdated docs of optional variables

6776f34

fix docs to note TESTCASE is an env var

6ceae5c

remove unnecessary additions runtime paths

d1624d5

test-awsauth statically links the C driver

kevinAlbs marked this pull request as ready for review February 15, 2023 19:20

kevinAlbs requested review from vector-of-bool and eramongodb February 15, 2023 19:20

vector-of-bool reviewed Feb 15, 2023

View reviewed changes

use mcd_timer for expiration

1b93a95

kevinAlbs requested a review from vector-of-bool February 16, 2023 01:33

remove unused vars

d9b8d8e

eramongodb reviewed Feb 16, 2023

View reviewed changes

vector-of-bool reviewed Feb 16, 2023

View reviewed changes

kevinAlbs added 9 commits February 19, 2023 14:09

simplify bash scripts

21d8491

fix comment for expiration

c5a9dea

be more explicit about _mongoc_aws_credentials_cache_get behavior

6c8f40f

rename helper to expiration_to_mcd_timer

dad4cf5

compute in int64_t domain

922e2c3

remove unnecessary expiration vars

e2fa0ac

add MONGOC_AWS_CREDENTIALS_INIT macro

1c6c266

only initialize and cleanup AWS cache if AWS is enabled

0c9b872

remove unnecessary can_setenv

34a024d

kevinAlbs requested review from vector-of-bool and eramongodb February 19, 2023 15:33

kevinAlbs added 2 commits February 19, 2023 15:35

fix -Wmissing-braces warning

9664419

fix signature of test_aws_cache

59fa557

eramongodb approved these changes Feb 21, 2023

View reviewed changes

vector-of-bool approved these changes Feb 22, 2023

View reviewed changes

kevinAlbs added 3 commits February 22, 2023 16:03

do not support uninitialized creds in _mongoc_aws_credentials_cache_get

bc2f59b

do not reuse creds after cleanup

154d6c7

fix -Wmissing-field-initializers warning

a6a75c1

kevinAlbs commented Feb 23, 2023

View reviewed changes

kevinAlbs merged commit 8eaac64 into mongodb:master Feb 23, 2023

		// Zero creds, so callers can safely call _mongoc_aws_credentials_cleanup.
		*creds = MONGOC_AWS_CREDENTIALS_INIT;

	.expiration = {{0}}, \
	.expiration = {.value = {0}, .set = false}, \

CDRIVER-4439 add AWS credential cache #1207

CDRIVER-4439 add AWS credential cache #1207

Uh oh!

Conversation

kevinAlbs commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background & Motivation

Cache locking

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eramongodb 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!

kevinAlbs 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!

kevinAlbs commented Feb 15, 2023 •

edited

Loading