fix(NODE-3118): keyAltNames option not serialized #176
Conversation
nbbeeken
force-pushed
the
NODE-3118/options
branch
from
9a7b738 to
cd4f48b
Compare
|
@addaleax Can't request your review directly but please feel free to take a look and advise here, thanks! 😃 |
There was a problem hiding this comment.
Dang, this is a really subtle bug sorry to see it slipped through. In libmongocrypt 1.x we introduced mongocrypt_ctx_setopt_key_encryption_key which was meant to supersede all of the other setopt methods. It's not clear to me why keyAltNames was omitted from that list, and I think an argument could be made that it should support it.
| @@ -202,12 +165,39 @@ module.exports = function(modules) { | |||
| * }); | |||
| */ | |||
| createDataKey(provider, options, callback) { | |||
| if (typeof options === 'function') (callback = options), (options = {}); | |||
| if (typeof options === 'function') { | |||
There was a problem hiding this comment.
I think the majority of the changes in this file are whitespace/style changes, and it made it really confusing to review. It looks like you could make a one-line change on the original L210 to:
const context = this._mongoCrypt.makeDataKeyContext(dataKeyBson, options);
and with your C++ changes the keyAltNames would be applied. Is that correct?
There was a problem hiding this comment.
Indeed the changes could've been less than what's here. I standby the decision to make things easier to read/debug for the next engineer. Is there something specific that you think is less optimal here?
There was a problem hiding this comment.
I won't comment on whether these changes subjectively improve readability/debugability for future developers, but I can say for sure that changing it this way makes it harder to review and easier to introduce new bugs.
I anchored this comment to splitting the one-liner into a few lines, but I was more concerned about sanitizeDataKeyOptions. It seems like you copied the contents into this function with some subtle changes (for-loop => map I think was one change). I read it a few times, but I couldn't tell you for sure whether there's a new bug there or not, or that the bug that I see in the old code is gone for sure.
If you're confident that it's a net improvement in code quality that's fine, but I'd ask that you consider the implications in future changes
There was a problem hiding this comment.
Thanks for the insight, I do agree that when addressing a bug fix it would be ideal to have just what is necessary to represent the fix in isolation from any refactor. Unfortunately this fix also required orchestrating new CI tasks/scripts to prove the code changes were, in fact, effective which required some debugging. Making the sanitization more functional was helpful as opposed to the key deletion and new array creating that then overwrote the old one logic.
We do have tests covering this usage here which tests for the proper errors as well.
makeDataKeyContextthat accepts one key,keyAltNamesAlso,
A number of the tests in this CI are skipped as they need to run against an actual mongodb. There is an accompanying PR I'll put up to the driver that will run these tests. (mongodb/node-mongodb-native#2833)