doc: Add OpenSSL errors to API docs - #34213
Conversation
Sorry, something went wrong.
bnoordhuis
left a comment
There was a problem hiding this comment.
Generally LGTM, thanks.
I'm fairly sure UNABLE_TO_DECRYPT_CERT_SIGNATURE and UNABLE_TO_DECRYPT_CRL_SIGNATURE are never actually returned by openssl even though it lists them as error codes (and node handles them.)
I think HOSTNAME_MISMATCH is also one that's never going to show up because node handles host name verification itself, through tls.checkServerIdentity().
I'll leave it up to you if you want to leave them in or not.
Sorry, something went wrong.
There was a problem hiding this comment.
| The certificate has expired: that is the notAfter date is before the current | |
| time. | |
| The certificate has expired: the notAfter date is before the current time. |
For consistency with the preceding description.
Sorry, something went wrong.
There was a problem hiding this comment.
| The revocation data have a future issue date. | |
| The revocation data has a future issue date. |
Sorry, something went wrong.
There was a problem hiding this comment.
Technically "have" is more corerct. I'll reword it to avoid the awkwardness of "data".
Sorry, something went wrong.
There was a problem hiding this comment.
| The certificate chain length is greater than the supplied maximum depth. | |
| The certificate chain length is greater than the maximum depth. |
"supplied" suggests it's configurable but it's not (in node - no binding for X509_STORE_CTX_set_depth().)
Sorry, something went wrong.
There was a problem hiding this comment.
| the basic constrains extension. | |
| the basic constraints extension. |
I'd leave out the bit about v1 vs. v3 certificates. Openssl's docs say this, which IMO is generic yet complete enough:
a CA certificate is invalid. Either it is not a CA or its extensions are not consistent with the supplied purpose.
"The certificate’s signer" is a good clarification but I'm not 100% sure you can't also get it when you manipulate a CA certificate directly. In that case the error is about the certificate itself, not its signer.
Sorry, something went wrong.
There was a problem hiding this comment.
Okay, I thought that GnuTLS' description was clear, but I am no expert. I'll switch it to OpenSSL's description. (Which is mostly what I used for the rest anyway.)
I'd be more embarrassed about "constrains" if it wasn't wrong in what I copied.
Sorry, something went wrong.
|
I left the three errors that shouldn't be seen simply to keep the list of what errors that node handles and the list that are documented the same. I made the rest of the changes you suggested. Thank you for the thorough review. Oh, I also rebased to avoid conflicts. |
Sorry, something went wrong.
Sorry, something went wrong.
|
@j3lamp - need a rebase to resolve git conflicts |
Sorry, something went wrong.
|
Would be nice to get this in. Seems good to go. /cc @Trott |
Sorry, something went wrong.
|
Non-blocking nit: I'd prefer the errors in each subsection be in alphabetical order unless there is a hugely compelling reason like some other obvious logical order. So, for example, CERT_HAS_EXPIRED would come before CERT_NOT_YET_VALID rather than after it. Looks good to me. The C++ change is a comment only so running Jenkins may be superfluous. If anyone disagrees, we can run Jenkins once it comes out of lockdown (probably in a few hours). |
Sorry, something went wrong.
Mifrill
left a comment
There was a problem hiding this comment.
Is it worth adding a link to a source like https://www.openssl.org/docs/ or any other authoritative sources to ensure accuracy, since this documentation is always good to verify the content against the official OpenSSL documentation?
Additionally, if there have been any recent updates or changes to these error codes, it's essential to reflect those changes in the documentation, since this PR is open for a while.
Sorry, something went wrong.
Fixes: nodejs#33705 PR-URL: nodejs#34213 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Fixes: nodejs#33705 PR-URL: nodejs#34213 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
* chore: bump node in DEPS to v20.15.0 * doc: Add OpenSSL errors to API docs nodejs/node#34213 * test: crypto-rsa-dsa testing for dynamic openssl nodejs/node#52781 * src: allow preventing debug signal handler start nodejs/node#46681 * cli: allow running wasm in limited vmemory with --disable-wasm-trap-handler nodejs/node#52766 * chore: fixup indices --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
* chore: bump node in DEPS to v20.15.0 * doc: Add OpenSSL errors to API docs nodejs/node#34213 * test: crypto-rsa-dsa testing for dynamic openssl nodejs/node#52781 * src: allow preventing debug signal handler start nodejs/node#46681 * cli: allow running wasm in limited vmemory with --disable-wasm-trap-handler nodejs/node#52766 * chore: fixup indices --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
* chore: bump node in DEPS to v20.15.0 * doc: Add OpenSSL errors to API docs nodejs/node#34213 * test: crypto-rsa-dsa testing for dynamic openssl nodejs/node#52781 * src: allow preventing debug signal handler start nodejs/node#46681 * cli: allow running wasm in limited vmemory with --disable-wasm-trap-handler nodejs/node#52766 * chore: fixup patch indices * chore: update patches --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
* chore: bump node in DEPS to v20.15.0 * doc: Add OpenSSL errors to API docs nodejs/node#34213 * test: crypto-rsa-dsa testing for dynamic openssl nodejs/node#52781 * src: allow preventing debug signal handler start nodejs/node#46681 * cli: allow running wasm in limited vmemory with --disable-wasm-trap-handler nodejs/node#52766 * chore: fixup patch indices * chore: update patches --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Fixes: #33705
Checklist