MONGOCRYPT-432 Allow keyAltName in encryptedFieldsMap

[mdb-ad]

Background

Adds keyAltName to keyId client-side translation to libmongocrypt that allows users to specify human-readable keyAltName strings instead of binary key IDs.

Implementation

Whenever encryptionInformation is appended to at outgoing command, libmongocrypt looks through the encrypted fields for keyAltName fields and translates them to keyId.

Testing

Tested on the C driver with spec test: https://spruce.mongodb.com/version/697803dbc0c964000764d2a4/

[kevinAlbs]

Does "cleanupStructuredEncryptedData" also need to be checked?

[kevinAlbs]

Rather than downcasting to _mongocrypt_ctx_encrypt_t, consider overriding the vtable function for mongo_done_keys here. This behavior is specific to an auto-encryption context. This may permit moving need_keys_for_encryptedFields to _mongocrypt_ctx_encrypt_t.

[kevinAlbs]

The scope also notes:

A new error message will be added for duplicate keyAltNames.

Suggest adding a test for duplicate keyAltNames. I expect this validation still needs to be added.

[kevinAlbs]

Rather than call bson_iter_init_find again, consider replacing field_iter with separate keyid_iter and keyaltname_iter:

bson_iter_t keyid_iter;
bson_iter_t keyaltname_iter;
if (bson_iter_init_find(&keyid_iter, field, "keyId")) {
    has_keyid = true;
}
if (bson_iter_init_find(&keyaltname_iter, field, "keyAltName")) {
    has_keyaltname = true;
}

[kevinAlbs]

Suggested change

	char idx_str[32];
	const char *idx_str_ptr;
	const size_t ret = bson_uint32_to_string((uint32_t)idx, &idx_str_ptr, idx_str, sizeof idx_str);
	BSON_ASSERT(ret > 0 && ret <= (int)sizeof idx_str);
	const char *idx_str_ptr = bson_iter_key(&arr_it);

I expect the output indexes are equal to the input indexes. So the string keys "0", "1", "2", ... can be copied directly.

[kevinAlbs]

Minor: suggest combining to simplify:

if (bson_iter_init_find(&doc_it2, &elem_doc, "keyAltName") && BSON_ITER_HOLDS_UTF8(&doc_it2)) {

[kevinAlbs]

bson_append_binary can fail (returns a bool). libmongocrypt is inconsistent about error handling for BSON appending: sometimes asserts, sometimes ignores, sometimes reports.

Since mc-schema-broker.c consistently reports BSON errors, suggest changing this function to report BSON errors. A helper function to translate one field may help to implement the "goto fail" pattern. Here is a suggested refactor that including other suggestions:

// translate_field_keyAltName_to_keyId translates one field. Returns false on error.
static bool translate_field_keyAltName_to_keyId(const bson_t *old_doc,
                                                bson_t *new_doc,
                                                _mongocrypt_key_broker_t *kb,
                                                int *found_keyAltName,
                                                mongocrypt_status_t *status) {
    BSON_ASSERT_PARAM(old_doc);
    BSON_ASSERT_PARAM(new_doc);
    BSON_ASSERT_PARAM(found_keyAltName);
    BSON_ASSERT_PARAM(kb);

    _mongocrypt_buffer_t unused = {0}, key_id_out = {0};
    bson_value_t key_alt_name_v = {0};
    bool ret = false;

    // Copy excluding "keyAltName"
    bson_copy_to_excluding_noinit(old_doc, new_doc, "keyAltName", NULL);

    // Translate keyAltName (if present) to keyId
    bson_iter_t keyAltName_iter;
    if (bson_iter_init_find(&keyAltName_iter, old_doc, "keyAltName") && BSON_ITER_HOLDS_UTF8(&keyAltName_iter)) {
        _bson_value_from_string(bson_iter_utf8(&keyAltName_iter, NULL), &key_alt_name_v);
        if (!_mongocrypt_key_broker_decrypted_key_by_name(kb, &key_alt_name_v, &unused, &key_id_out)) {
            _mongocrypt_key_broker_status(kb, status);
            goto fail;
        }
        TRY_BSON_OR(bson_append_binary(new_doc, "keyId", -1, key_id_out.subtype, key_id_out.data, key_id_out.len)) {
            goto fail;
        }
    }

    ret = true;
fail:
    _mongocrypt_buffer_cleanup(&unused);
    _mongocrypt_buffer_cleanup(&key_id_out);
    bson_value_destroy(&key_alt_name_v);
    return ret;
}

int mc_translate_fields_keyAltName_to_keyId(const bson_t *fields_bson,
                                            _mongocrypt_key_broker_t *kb,
                                            bson_t *out,
                                            mongocrypt_status_t *status) {
    BSON_ASSERT_PARAM(fields_bson);
    BSON_ASSERT_PARAM(kb);
    BSON_ASSERT_PARAM(out);
    int found_keyAltName = 0;

    bson_iter_t arr_it;
    if (!bson_iter_init(&arr_it, fields_bson)) {
        CLIENT_ERR("failed to iterate 'fields' array");
        return -1;
    }

    while (bson_iter_next(&arr_it)) {
        const char *idx_str = bson_iter_key(&arr_it);

        if (BSON_ITER_HOLDS_DOCUMENT(&arr_it)) {
            bson_t elem_doc;
            if (!mc_iter_document_as_bson(&arr_it, &elem_doc, status)) {
                return -1;
            }
            bson_t new_doc = BSON_INITIALIZER;
            if (!translate_field_keyAltName_to_keyId(&elem_doc, &new_doc, kb, &found_keyAltName, status)) {
                return -1;
            }
            TRY_BSON_OR(bson_append_document(out, idx_str, -1, &new_doc)) {
                bson_destroy(&new_doc);
                return -1;
            }
        } else {
            /* Non-document elements: copy as-is. */
            if (!BSON_APPEND_VALUE(out, idx_str, bson_iter_value(&arr_it))) {
                CLIENT_ERR("failed to append field value");
                return -1;
            }
        }
    }

    return found_keyAltName;
}

[kevinAlbs]

I expect calling bson_strdup can be avoided. Call bson_copy_to_excluding_noinit first, then check if a "keyAltName" field exists.

[kevinAlbs]

Suggested change

	return -1;
	return false;

[kevinAlbs]

Can translating "create" be done without adding to encryptedFieldsMap? I expect this may have impact that poses other questions. E.g. should this overwrite an existing entry in encryptedFieldsMap? The empty check means only the first one is stored. I expect "create" could be run multiple times on the same collection (e.g. if the first fails).

You may be able to add a function similar to _try_schema_from_create_or_collMod_cmd to obtain encryptedFields from the "create" command.

[kevinAlbs]

See this suggested change: b5ff740

This appears to pass tests. It reads "encryptedFields" from the create command without modifying encryptedFieldsMaps.