[CDRIVER-3491] mongoc_client_reset should reset key vault client Created: 17/Jan/20  Updated: 10/Feb/20  Resolved: 10/Feb/20

Status: Closed
Project: C Driver
Component/s: libmongoc
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: Kevin Albertson Assignee: Kevin Albertson
Resolution: Won't Do Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
related to PHPC-1529 Reset libmongocrypt's key vault clien... Closed

 Description   

A mongoc_client_t with automatic encryption (set via mongoc_client_enable_auto_encryption()) may use a separate inner mongoc_client_t passed via mongoc_auto_encryption_opts_set_keyvault_client to communicate with the key vault. Calling mongoc_client_reset should also reset the inner mongoc_client_t used to communicate with the key vault. Otherwise, the inner key vault client could end up reusing sessions from its own session pool.



 Comments   
Comment by Jeremy Mikola [ 31/Jan/20 ]

andreas.braun: I'm thinking of ways that PHPC could reset its key vault client automatically when the application's main client is reset. Storing a reference to the key vault mongoc_client_t on a Manager wouldn't help, since our Session and Cursor classes have direct references to a mongoc_client_t instead of an indirect reference through the Manager object. That said, if our list of persisted clients also associates a PID with each client, perhaps we can tweak our reset logic to iterate on that list and reset anything that wasn't created by the current PID (would catch an otherwise unused key vault client) and hasn't already been reset by the current PID (to avoid double resets).

Comment by Kevin Albertson [ 30/Jan/20 ]

For libmongoc, we already require the user to keep a handle to the key vault client so they can mongoc_client_destroy it later. Perhaps it is reasonable to just document that a user must mongoc_client_reset that client upon forking instead of resetting twice?

But I'm less familiar with how this works in regards to PHP. If it would make the PHP implementation easier, that's reason enough to reset the key vault client. I don't think the risk of double resetting is an issue. If a user were to call mongoc_client_reset on the key vault client in direct succession, then no new resources would have been created in between the two resets, so there'd be no orphaned resources.

 

jmikola let me know if you think this is a change worthwhile to implement, or if it would help with PHP. Otherwise, I'd vote to make this a docs only change.

Comment by Jeremy Mikola [ 17/Jan/20 ]

To clarify, we definitely don't want to reset the key vault client if it's identical to the application client (where FLE has been enabled).

With respect to a separate key vault client, it is possible that the application would still have a reference to it and might call mongoc_client_reset() on both the application and key vault clients. Before implementing this ticket, I think we should consider possible side effects of resetting a key vault client twice. This is something I was careful not to do in PHPC-1274 to avoid orphaning resources in the child process (such that their generation would never match an active client and thus never get cleaned up); however, this might not be a concern if all of the resetting happens immediately and in succession. In that case, the only side effect would be that we increment the generation twice, but there would be no resources created between calls to mongoc_client_reset() to potentially cause the issue I worked around in PHP.

Generated at Wed Feb 07 21:18:12 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.