PHPC-2093: Key Management API #1339
Conversation
| @@ -492,6 +493,78 @@ void php_phongo_cursor_init_ce(INIT_FUNC_ARGS) /* {{{ */ | |||
| php_phongo_handler_cursor.offset = XtOffsetOf(php_phongo_cursor_t, std); | |||
| } /* }}} */ | |||
|
|
|||
| static void phongo_cursor_init(zval* return_value, zval* manager, mongoc_cursor_t* cursor, zval* readPreference, zval* session) /* {{{ */ | |||
There was a problem hiding this comment.
Noted that this logic was moved from phongo_execute.c
jmikola
force-pushed
the
phpc-2093
branch
2 times, most recently
from
8158830 to
0999c83
Compare
| } | ||
|
|
||
| if (bson_empty(&reply)) { | ||
| phongo_throw_exception(PHONGO_ERROR_UNEXPECTED_VALUE, "mongoc_client_encryption_delete_key returned an empty document"); |
There was a problem hiding this comment.
@alcaeus: this was added per your earlier comment in #1339 (comment)
| * key vault collection. The collection has a majority read concern, but the | ||
| * query itself specifies no filter or options. Create an empty query object | ||
| * and attach it to the cursor for the benefit of debugging. */ | ||
| if (!phongo_query_init(&query, NULL, NULL)) { |
There was a problem hiding this comment.
The side effect here is that the dumped Cursor will not show a majority read concern as it might otherwise if we were to execute a query through PHPLIB with a collection-level read concern. I don't think this is worth addressing, though, and the current behavior isn't much different if PHPC were to execute a query and inherit a client-level read concern (enforced internally by libmongoc).
| * | ||
| * This function will fall back to a modifier in the absence of a top-level | ||
| * option (where applicable). */ | ||
| bool phongo_query_init(zval* return_value, zval* filter, zval* options) /* {{{ */ |
There was a problem hiding this comment.
This function previously initialized the php_phongo_query_t struct. Since we now call this from two places (Query constructor and ClientEncryption::getKeys()), I needed to change it to initialize the entire zval.
We still have some branch logic below in the event that the zval was already initialized, which covers the original Query constructor use case. And we also have an instanceof check after that as an added sanity check before we fetch the internal struct.
| goto cleanup; | ||
| } | ||
|
|
||
| if (!phongo_cursor_init_for_query(return_value, &intern->key_vault_client_manager, cursor, intern->key_vault_namespace, &query, NULL, NULL)) { |
There was a problem hiding this comment.
phongo_cursor_init_for_query was changed to handle this use case. See my later comment in Query.c.
| /* {{{ proto object MongoDB\Driver\ClientEncryption::deleteKey(MongoDB\BSON\Binary $id) | ||
| Removes the key document with the given UUID (BSON binary subtype 0x04) from | ||
| the key vault collection. Returns the result of the internal deleteOne() | ||
| operation on the key vault collection. */ |
There was a problem hiding this comment.
Since DeleteResult only exists in PHPLIB, returning the result object as a plain document made the most sense. Constructing a BulkWriteResult isn't feasible for the same reason as rewrapManyDataKeys(), as we don't have a server hint available and thus cannot satisfy BulkWriteResult::getServer().
| /* {{{ proto object MongoDB\Driver\ClientEncryption::rewrapManyDataKey(array|object $filter[, array $options = array()]) | ||
| Decrypts multiple data keys and (re-)encrypts them with a new masterKey, or | ||
| with their current masterKey if a new one is not given. Returns an object | ||
| corresponding to the internal libmongoc result. */ |
There was a problem hiding this comment.
As previously discussed in the C/C++ Slack channel, I opted not to model the return value here. As I mentioned earlier in deleteKey(), we don't have a server hint available and can't create a BulkWriteResult. I didn't see any reason to model RewrapManyDataKeyResult since it just has an optional bulkWriteResult field.
| bulk_write_result = mongoc_client_encryption_rewrap_many_datakey_result_get_bulk_write_result(result); | ||
|
|
||
| if (bson_empty0(bulk_write_result)) { | ||
| BSON_APPEND_NULL(&reply, "bulkWriteResult"); |
There was a problem hiding this comment.
I think always having the bulkWriteResult field exist is preferable to leaving it unset.
| @@ -305,6 +656,23 @@ void phongo_clientencryption_init(php_phongo_clientencryption_t* intern, zval* o | |||
| ZVAL_ZVAL(&intern->key_vault_client_manager, key_vault_client_manager, 1, 0); | |||
| } | |||
|
|
|||
| /* Copy the key vault namespace, since it may be referenced later by | |||
| * ClientEncryption::getKeys(). The namespace will already have been | |||
| * validated by phongo_clientencryption_opts_from_zval. */ | |||
There was a problem hiding this comment.
This exists purely for Query debug info.
| define('CSFLE_KEY_VAULT_NS', 'encryption.__keyVault'); | ||
| define('CSFLE_KEY_VAULT_DATABASE_NAME', 'encryption'); | ||
| define('CSFLE_KEY_VAULT_COLLECTION_NAME', '__keyVault'); | ||
| define('CSFLE_KEY_VAULT_NS', CSFLE_KEY_VAULT_DATABASE_NAME . '.' . CSFLE_KEY_VAULT_COLLECTION_NAME); |
There was a problem hiding this comment.
Splitting these was necessary to utilize skip_if_not_clean in tests. I originally created a skip_if_ns_not_clean method but this seemed more straightforward.
| @@ -0,0 +1,31 @@ | |||
| --TEST-- | |||
| MongoDB\Driver\ClientEncryption::rewrapManyDataKey() accepts null for $options | |||
That is correct. Luckily, the stub hash in the arginfo file will conflict if the file was changed in both head and base branch of a PR, requiring a rebase and re-generation of the arginfo file. |
Use ClientEncryption::getKey in createDataKey tests
Also updates doc blocks to better clarify return types
Since php_phongo_bson_state contains other structs, using {0} as an initializer may cause "missing braces around initializer [-Werror=missing-braces]" errors on some platforms.
https://jira.mongodb.org/browse/PHPC-2093