-
Notifications
You must be signed in to change notification settings - Fork 636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add checkServerIdentity callback #403
Conversation
Why did you make the new argument an object instead of just a function? |
@@ -58,6 +60,43 @@ ChannelCredentials::~ChannelCredentials() { | |||
grpc_channel_credentials_release(wrapped_credentials); | |||
} | |||
|
|||
struct callback_md { | |||
Nan::Callback *callback; | |||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you use a struct here, instead of using the callback
pointer directly?
@@ -148,9 +187,42 @@ NAN_METHOD(ChannelCredentials::CreateSsl) { | |||
"createSsl's second and third arguments must be" | |||
" provided or omitted together"); | |||
} | |||
|
|||
verify_peer_options verify_options = {NULL, NULL, NULL}; | |||
if (info.Length() >= 4 && info[3]->IsObject()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the length check is necessary here. As far as I can tell, indexing past the end returns undefined. In addition, I would like to also have an error thrown if the argument is provided but not the correct type, like with the other arguments.
Also, you need to regenerate the build files by running the |
The answer to both is because in several of my first iterations adding additional verification options, there were several other options (cert fingerprint assertions, skipping hostname verification, etc). All that's been removed now and there's just the one callback option, so it totally makes sense to remove the struct being passed as userdata and I've done that. As for whether the fourth argument should be an object or just the callback function, I can simplify that if you want (but if you think it may be useful to keep the API as it is to allow for additional options, it could be useful left as-is?). |
7b0bfbe
to
613295f
Compare
I'm fine with leaving the argument as an object. I wish the API overall was more consistent, but having the user pass a function directly might be more confusing. Also, I am pretty sure you can simplify the logic of handling the object. I think that if the key is not present, |
Yep you're right. Thanks for the pointer. I also added a few tests to check for the expected type assertions of that argument. |
OK, one final thing: can you update the documentation in |
Sure thing, |
The typescript type looks correct to me. In the jsdoc comment, you can actually do However, looking at that documentation made me realize that I am not really comfortable with a callback that is supposed to throw an error. Nothing else in our API does that, and the equivalent callback in the Node TLS library returns an error on failure, or |
Good catch! For some reason I read it as throwing an error on failure instead of returning it, so that's what I was designing against. I've corrected the implementation and documentation to expect the same behavior: a returned error on failure or otherwise undefined. |
I think that it is probably worth the trouble to try to make the API here identical to the Node TLS equivalent, which would require parsing the certificate. This would particularly help with porting this new API to the pure JavaScript library, which uses the Node TLS module under the hood. I think we could use the PKIjs library to do that, with the caveat that it only works on Node 6+, so we would want to load it dynamically and throw an error on lower Node versions. I'm not entirely sure the parsed format it outputs is identical to what the TLS module gives you, but I think it's very close. |
Yep, I think you're right that I should update the cert parameter to a parsed version. If we're doing the parsing in JS (e.g. with PKIjs) do you have a hint for the best way to call back into a specific JS function (instead of a passed-in callback) from C++? I feel like it would be cleanest if the C++ In lieu of that, my first pass working on this was to change |
After some further consideration and internal discussion, I suggest that you hold off on further changes to this PR. We are not sure what is the best way to present the certificates to the user and we are planning to create a gRFC (https://github.com/grpc/proposal) to figure out how to proceed here. |
Cool, I'll keep an eye out for that and jump in on the discussion when it pops up. Thanks for the update! |
…ment. Add some tests asserting type-checking behavior.
… options to typescript definition.
… if an error is returned. Clean up documentation and add a test assertion on returned Error.
…th raw DER buffer.
ddc2367
to
3493031
Compare
…eceived by the checkServerIdentity callback.
I'm not really sure why the tests are failing now. They seem to be failing to load |
Actually, I know what the problem is. @JackOfMostTrades please edit We really should add a test for that. |
Never mind, I made the change myself, and it looks like that did fix the problem. |
Core PR 15274 added an option to provide a callback during peer certificate verification in order to perform additional validation. This PR exposes that option in node as a "checkServerIdentity" function that has the same semantics as the option of the same name available in the node TLS library (except that it does not replace regular hostname; it is in addition to it).
Example usage: