implement verifyFromStr #63
Conversation
* upstream/master: test: update cert & chain for verify (Southern#62)
|
@lolBig may I ask you to use |
|
Morning - has there been any further progress on this one? My understanding is this addresses #42 ? Thank you |
include/x509.h
Outdated
|
|
||
| Local<Value> try_parse(const std::string& dataString); | ||
| Local<Value> verify(const std::string& dataString); | ||
| Local<Value> verifyFromStr(const std::string& dataString); |
There was a problem hiding this comment.
Change to verify_from_str().
index.js
Outdated
| try { | ||
| x509.verifyFromStr(certStr, CABundleStr); | ||
| } | ||
| catch (verificationError) { |
There was a problem hiding this comment.
Move catch to previous line, please :)
index.js
Outdated
| catch (verificationError) { | ||
| caughtErr = verificationError; | ||
| } | ||
| finally { |
index.js
Outdated
| @@ -6,6 +6,19 @@ exports.getAltNames = x509.getAltNames; | |||
| exports.getSubject = x509.getSubject; | |||
| exports.getIssuer = x509.getIssuer; | |||
|
|
|||
| exports.verifyFromStr = function(certStr, CABundleStr, cb) { | |||
There was a problem hiding this comment.
Valid if certStr and CABundleStr is a Buffer or String.
There was a problem hiding this comment.
CABundleStr should be caBundleStr. It is not a constant, therefore it should not start with a capital.
| X509_free(ca_cert); | ||
| BIO_free_all(ca_bio); | ||
| X509_free(cert); | ||
| BIO_free_all(cert_bio); |
There was a problem hiding this comment.
Check store, verify_ctx and others is a NULL, free a NULL might cause segfault.
There was a problem hiding this comment.
these functions have checked NULL, the free function also does nothing when ptr is NULL , so we don't need to check again
README.md
Outdated
| @@ -137,6 +137,21 @@ x509.verify( | |||
|
|
|||
| ``` | |||
|
|
|||
| #### x509.verifyFromStr(`certStr`, `caStr`, function(err, result){ /*...*/}) | |||
There was a problem hiding this comment.
use CABundleStr to replace caStr to keep consistent with verify().
There was a problem hiding this comment.
CABundleStr should be caBundleStr. It is not a constant, therefore it should not start with a capital.
| caughtErr = verificationError; | ||
| } | ||
| finally { | ||
| cb(caughtErr); |
There was a problem hiding this comment.
Valid the callback is a function, too.
index.js
Outdated
| @@ -6,6 +6,32 @@ exports.getAltNames = x509.getAltNames; | |||
| exports.getSubject = x509.getSubject; | |||
| exports.getIssuer = x509.getIssuer; | |||
|
|
|||
| exports.verifyFromStr = function(certStr, CABundleStr, cb) { | |||
| if (typeof cb !== 'function') { | |||
| throw new Error('cb should be function') | |||
There was a problem hiding this comment.
Miss semi-colons in this file.
|
@lolBig The CI in node stable seems failed. |
|
@yorkie seems node-x509 can not make compatable with Node v10.7.0 |
|
@yorkie worked with Node v8.10.0 |
|
Ok, #71 fixes the problem, so I will merge this after it gets done :) |
README.md
Outdated
| @@ -137,6 +137,21 @@ x509.verify( | |||
|
|
|||
| ``` | |||
|
|
|||
| #### x509.verifyFromStr(`certStr`, `CABundleStr`, function(err, result){ /*...*/}) | |||
There was a problem hiding this comment.
As @Southern said at #63 (comment), please update the CABundleStr to caBundleStr.
No description provided.