refactor: get rid of native OpenSSL dependency #265
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
drubin
previously requested changes
Sep 8, 2018
| ]) | ||
| // Note: Can't use the certpem.info() method here because of multiple bugs. | ||
| // And yes, this API is insane. Crypto people are bonkers. Seriously. - JE | ||
| const certInfo = certpem.debug(crtData) |
There was a problem hiding this comment.
My js isn't that good but I think debug is an alias for getCertInfo maybe we can use that so it makes more logical sense?
https://git.coolaj86.com/coolaj86/cert-info.js/src/branch/master/index.js#L68
There was a problem hiding this comment.
What bugs does info create? Feels like we just copied their code excluding the common name field?
| @@ -28,6 +28,7 @@ | |||
| "async-lock": "^1.1.3", | |||
| "axios": "^0.18.0", | |||
| "bluebird": "^3.5.1", | |||
| "certpem": "^1.1.2", | |||
There was a problem hiding this comment.
I think considering how little we use the native implementation this is a good way for now, also native openssl is always a screw up with miss matched versioning so anything that avoids this feels good.
eysi09
force-pushed
the
no-native-openssl
branch
from
0063769 to
64e962a
Compare
eysi09
approved these changes
Sep 10, 2018
eysi09
dismissed
drubin’s stale review
I'd like to include this PR in our next release and the issues raised above aren't a stopper
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By removing the x509 library, in favor of the (admittedly odd) certpem/pkijs libs
Closes #261