-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
doc: Add OpenSSL errors to API docs #34213
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
doc/api/errors.md
Outdated
The certificate has expired: that is the notAfter date is before the current | ||
time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
doc/api/errors.md
Outdated
<a id="CRL_NOT_YET_VALID"></a> | ||
#### `CRL_NOT_YET_VALID` | ||
|
||
The revocation data have a future issue date. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The revocation data have a future issue date. | |
The revocation data has a future issue date. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically "have" is more corerct. I'll reword it to avoid the awkwardness of "data".
doc/api/errors.md
Outdated
<a id="CERT_CHAIN_TOO_LONG"></a> | ||
#### `CERT_CHAIN_TOO_LONG` | ||
|
||
The certificate chain length is greater than the supplied maximum depth. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
.)
doc/api/errors.md
Outdated
|
||
The certificate’s signer was not a CA. This may happen if this was a version 1 | ||
certificate, which is common with some CAs, or a version 3 certificate without | ||
the basic constrains extension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
4001550
to
ff97f02
Compare
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. |
@j3lamp - need a rebase to resolve git conflicts |
ff97f02
to
eedf9bf
Compare
Would be nice to get this in. Seems good to go. /cc @Trott |
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, 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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Fixes: nodejs#33705 PR-URL: nodejs#34213 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>
eedf9bf
to
e22bc1e
Compare
Fixes: nodejs#33705 PR-URL: nodejs#34213 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>
Fixes: #33705 PR-URL: #34213 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>
Fixes: #33705 PR-URL: #34213 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>
Fixes: nodejs#33705 PR-URL: nodejs#34213 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>
Fixes: nodejs#33705 PR-URL: nodejs#34213 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>
* 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 <[email protected]>
* 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 <[email protected]>
* 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 <[email protected]> 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 <[email protected]> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Fixes: #33705
Checklist