Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: expose client's peer certificate der data #1062

Conversation

cruz3rblade
Copy link
Contributor

This PR implements #1058 and adds access to the client's peer DER certificate data.
The function returns an object with a "raw" field in it. So if anyone else wants to expand the information returned about the certificate in the future, they can simply append a new key to the object.

Note: I'm not super familiar with C++, so I hope I got everything right. Please go easy on me with the review 😅

@uNetworkingAB
Copy link
Contributor

This looks good as an early variant, but ideally it would be handled entirely by the library without any Node.js features or copies of data. But for an early draft of the feature, this looks good.

@cruz3rblade
Copy link
Contributor Author

Thank you for your feedback. How would you like to proceed with this? Are we good to merge this one, or should I implement this logic at the library level?

@uNetworkingAB
Copy link
Contributor

You have the code formatting of a psychopath but that's fine. This is fine as a first step until more thought can be made. This API can break in future releases.

@cruz3rblade
Copy link
Contributor Author

I have made a few changes and removed the NodeJS copies. Also, If you prefer I can move extractX509PemCertificate method to the C++ library and expose it in HttpResponse so all the wrapper has to do is call res->getX509Certificate()

@uNetworkingAB uNetworkingAB merged commit 37e4402 into uNetworking:master Aug 7, 2024
1 check failed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants