Skip to content
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

crypto: use BoringSSL compatible errors #37297

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Feb 9, 2021

This PR migrates OpenSSL-specific error messages (BNerr & Dherr) to the underlying error calls, which BoringSSL is also capable of invoking. Electron is currently patching these out, and we'd prefer not to.

Another consideration we might perhaps make, however, is instead just throwing more prototypical Node.js errors - curious what y'all think.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Feb 9, 2021
@codebytere codebytere force-pushed the boringssl-compatible-crypto-errors branch from 60485ad to f345f5d Compare February 9, 2021 19:48
@tniessen
Copy link
Member

tniessen commented Feb 9, 2021

Electron is currently patching these out, and we'd prefer not to.

Is electron currently using this exact patch?

Another consideration we might perhaps make, however, is instead just throwing more prototypical Node.js errors - curious what y'all think.

I think that's been a goal for a long time, but it isn't always simple. For example, when we do explicitly throw OpenSSL errors, we often do so to align with other APIs that might throw the exact same errors, except that they originate within OpenSSL and not Node.js. Changing the error codes to something else would not only be semver-major, but might also introduce inconsistencies with errors originating within OpenSSL.

@codebytere
Copy link
Member Author

codebytere commented Feb 9, 2021

@tniessen we patch it to be e.g.

OPENSSL_PUT_ERROR(DH, DH_R_BAD_GENERATOR);

which is the BoringSSL macro that maps to what y'all have now - this change is the common ancestor of both

@tniessen
Copy link
Member

@codebytere Thanks! Are all of these constants available in BoringSSL, or would this patch still cause compilation issues when linking against BoringSSL? I remember that some error codes didn't exist in BoringSSL when we ported some other API.

@codebytere
Copy link
Member Author

@tniessen yes! this is cross-compatible in its current state :)

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @codebytere :)

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@codebytere
Copy link
Member Author

Landed in 8353854

@codebytere codebytere closed this Feb 17, 2021
codebytere added a commit that referenced this pull request Feb 17, 2021
PR-URL: #37297
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere deleted the boringssl-compatible-crypto-errors branch February 17, 2021 04:27
targos pushed a commit that referenced this pull request Feb 28, 2021
PR-URL: #37297
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants