[CDRIVER-4454] Automatic Azure KMS Credentials #1097

vector-of-bool · 2022-08-26T01:07:38Z

This implements automatic KMS credentials for Azure, in the same manner as are available for AWS.

To optimize for test-ability in the absence of an Azure environment, the following changes are made:

The private HTTP API was refactored to allow inspection of the HTTP request string that would be sent. This way we can build a function that constructs an HTTP request, and then check it against a known-good request message, no HTTP server needed. This alleviates the necessity to have a "mock server" for the API, as we can assume that the proper server, when presented with such a known-good request, will respond as we expect it to. This assumes that the internal HTTP code sends/receives HTTP correctly, but that proposition can be validated and tested without implementing a fake Azure IMDS API server.
The Azure code is implemented as 1) a request-generator that fills in an HTTP request object, and 2) A JSON-response parser that inspects the HTTP JSON body and decomposes it according to the Azure schema. Because (2) does not perform actual I/O, we can simply feed it known-good and known-bad JSON strings and validate that it behaves appropriately, and therefore assume that it will behave correctly later on when fed the actual HTTP responses from an IMDS server.

vector-of-bool · 2022-08-26T01:09:37Z

src/libmongoc/src/mongoc/mcd-azure.c

+      bson_set_error (
+         error,
+         MONGOC_ERROR_PROTOCOL_ERROR,
+         64,


Using 64 is arbitrary. The error domain/code system is getting pretty messy. What should be done to make this more useful?

I think the error codes in each domain can have a different meaning. Two domains can have the same error code.

Consider adding an error code to mongoc_error_code_t, like MONGOC_ERROR_PROTOCOL_HTTP to signal an error in an HTTP response.

vector-of-bool · 2022-08-26T01:16:05Z

src/libmongoc/tests/test-libmongoc.c

+#define TEST_INSTALL(FuncName)                 \
+   if (1) {                                    \
+      extern void FuncName (TestSuite *suite); \
+      FuncName (&suite);                       \
+   } else                                      \
+      ((void) 0)


This is just a drive-by change to reduce some tedium in adding tests. Can be reverted/moved.

Nice, I like it.

This allows easier code review, informal verification, and testing

vector-of-bool · 2022-08-29T17:57:36Z

The HTTP retry-with-backoff logic has been moved to a separate function that doesn't concern itself with the request content. This allows easier informal verification of both the retry logic as well as code that invokes that that function.

src/libmongoc/tests/test-mcd-azure-imds.c

src/libmongoc/tests/test-libmongoc.c

src/libmongoc/src/mongoc/CMakeLists.txt

src/libmongoc/src/mongoc/mongoc-http.c

src/libmongoc/src/mongoc/mongoc-http-private.h

src/libmongoc/src/mongoc/mcd-azure.h

src/libmongoc/src/mongoc/mcd-azure.c

kevinAlbs · 2022-08-29T22:33:30Z

src/libmongoc/tests/test-libmongoc.c

+#define TEST_INSTALL(FuncName)                 \
+   if (1) {                                    \
+      extern void FuncName (TestSuite *suite); \
+      FuncName (&suite);                       \
+   } else                                      \
+      ((void) 0)


Nice, I like it.

src/libmongoc/src/mongoc/CMakeLists.txt

src/libmongoc/src/mongoc/mcd-azure.c

src/libmongoc/src/mongoc/mcd-azure.h

src/libmongoc/src/mongoc/mongoc-crypt.c

kevinAlbs

Nice, LGTM!

eramongodb

One last question regarding header file naming; otherwise, LGTM.

src/libmongoc/src/mongoc/CMakeLists.txt

src/libmongoc/tests/test-libmongoc.c

vector-of-bool · 2022-09-01T19:38:37Z

The three added commits can be viewed in isolation in commit order to build off of each other.

kevinAlbs · 2022-09-01T20:05:02Z

src/libmongoc/src/mongoc/mcd-azure.c

@@ -71,8 +71,12 @@ mcd_azure_access_token_try_init_from_json_str (mcd_azure_access_token *out,
   // token_type
   found = bson_iter_init_find (&iter, &bson, "token_type");
   const char *const token_type = !found ? NULL : bson_iter_utf8 (&iter, NULL);
+   // expires_on


"expires_in"

kevinAlbs · 2022-09-01T20:09:41Z

src/libmongoc/src/mongoc/mongoc-crypt.c

 */
 static bool
-_try_add_azure_from_env (bson_t *out, bson_error_t *error)
+_request_new_azure_mid_token (mcd_azure_access_token *out, bson_error_t *error)


What does "mid" refer to?

"managed identity" in this case. I can change that if it is unclear.

Since this is the only reference to "mid", I suggest renaming to _request_new_azure_access_token or defining "mid" in a comment.

kevinAlbs · 2022-09-01T20:11:15Z

src/libmongoc/src/mongoc/mongoc-crypt.c

+         // The token is still valid for another minute
+      } else {
+         // The token is about to expire. Destroy it. We will then ask IMDS for
+         // a new oen.


src/libmongoc/src/mongoc/CMakeLists.txt

src/libmongoc/src/mongoc/mcd-time.h

src/libmongoc/src/mongoc/mongoc-crypt.c

src/libmongoc/src/mongoc/mcd-azure.c

src/libmongoc/src/mongoc/mcd-time.h

src/libmongoc/src/mongoc/mongoc-crypt.c

src/libmongoc/src/mongoc/mcd-time.h

src/libmongoc/src/mongoc/mongoc-crypt.c

src/libmongoc/src/mongoc/mcd-time.h

src/libmongoc/tests/test-mcd-azure-imds.c

eramongodb · 2022-09-08T15:07:05Z

src/libmongoc/tests/test-mcd-azure-imds.c

+
+#define RAW_STRING(...) #__VA_ARGS__
+
+void


Suggested change

void

static void

eramongodb · 2022-09-08T15:07:13Z

src/libmongoc/tests/test-mcd-azure-imds.c

+   mcd_azure_access_token_destroy (&token);
+}
+
+void


Suggested change

void

static void

src/libmongoc/src/mongoc/mcd-integer.h

eramongodb

New files should probably include copyright notices.
One minor question; otherwise LGTM.

eramongodb · 2022-09-08T20:41:16Z

src/libmongoc/src/mongoc/mcd-integer.h

+#ifdef __cplusplus
+extern "C" {
+#endif


This does not seem particularly necessary. Remove?

Practically unecessary because everything is static and therefore bounded to the TU. This just becomes a habit when creating C headers.

vector-of-bool added 6 commits August 18, 2022 19:25

Initial automatic Azure KMS based on DRIVERS-2411

b9fface

Split request_send into testable components

5b20fc8

Add private API for querying Azure IMDS

e758329

Test cases for Azure IMDS

3d055b7

Convenience macro for test installation

2ac4fad

Use separate IMSD request API

c5a8f57

vector-of-bool requested a review from kevinAlbs August 26, 2022 01:07

vector-of-bool commented Aug 26, 2022

View reviewed changes

vector-of-bool added 3 commits August 26, 2022 01:12

No managedIdentity parameters for now

d14b796

Spelling

b0b31c0

Unused func, doc comment

3a5cbe3

vector-of-bool commented Aug 26, 2022

View reviewed changes

Speling

73c207b

vector-of-bool marked this pull request as ready for review August 26, 2022 01:18

kevinAlbs requested a review from eramongodb August 27, 2022 14:12

Refactor HTTP retry logic into a dedicated func

b81d47e

This allows easier code review, informal verification, and testing

Break out if the wait period becomes too large

ac3b6ab

eramongodb requested changes Aug 29, 2022

View reviewed changes

kevinAlbs reviewed Aug 29, 2022

View reviewed changes

vector-of-bool added 4 commits August 30, 2022 00:08

Fix: Clear error when checking for Azure requests

494adb5

PR feedback

a1abbf7

Drop HTTP retry logic

1e245c9

Weast const

dce0a4e

vector-of-bool requested review from eramongodb and kevinAlbs August 30, 2022 22:25

kevinAlbs approved these changes Aug 31, 2022

View reviewed changes

eramongodb approved these changes Sep 1, 2022

View reviewed changes

src/libmongoc/src/mongoc/CMakeLists.txt Show resolved Hide resolved

src/libmongoc/tests/test-libmongoc.c Show resolved Hide resolved

vector-of-bool added 2 commits September 1, 2022 19:31

Simple point-in-time and duration abstraction

57c37fc

Parse the "expires_in" from azure tokens

b89decd

Cache the Azure access token

9b4f52c

vector-of-bool requested review from kevinAlbs and eramongodb September 1, 2022 19:38

kevinAlbs approved these changes Sep 1, 2022

View reviewed changes

eramongodb requested changes Sep 1, 2022

View reviewed changes

vector-of-bool added 7 commits September 1, 2022 21:36

Handle strtoll parse errors

ba2807d

Normal form

cad3810

Overflow guards in time utils

c5ecf83

Naming tweak

bfcc84a

Simpler error clearing

c502346

Tweak comment wording

93a125b

Speling

afe21a5

vector-of-bool requested a review from eramongodb September 1, 2022 22:20

eramongodb requested changes Sep 2, 2022

View reviewed changes

src/libmongoc/src/mongoc/mcd-time.h Outdated Show resolved Hide resolved

src/libmongoc/src/mongoc/mcd-time.h Outdated Show resolved Hide resolved

src/libmongoc/tests/test-mcd-azure-imds.c Outdated Show resolved Hide resolved

vector-of-bool added 2 commits September 8, 2022 02:35

Prove correctness of integer bounds checks.

9af22d5

Typed assert

fc2d5b5

vector-of-bool force-pushed the CDRIVER-4454-azure-auto-kms branch from 820555c to fc2d5b5 Compare September 8, 2022 02:35

vector-of-bool requested a review from eramongodb September 8, 2022 02:36

eramongodb reviewed Sep 8, 2022

View reviewed changes

vector-of-bool added 2 commits September 8, 2022 20:21

Fix mult overflow proof, some test cases

39ae7dc

'static' for test fns

39854a6

vector-of-bool requested a review from eramongodb September 8, 2022 20:23

eramongodb approved these changes Sep 8, 2022

View reviewed changes

Clean up headers, linkages, preludes

b93e72d

vector-of-bool merged commit 686bff8 into mongodb:master Sep 9, 2022

[CDRIVER-4454] Automatic Azure KMS Credentials #1097

[CDRIVER-4454] Automatic Azure KMS Credentials #1097

Uh oh!

Conversation

vector-of-bool commented Aug 26, 2022

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!

vector-of-bool commented Aug 29, 2022

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

eramongodb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vector-of-bool commented Sep 1, 2022

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!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

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!