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

doc: Add OpenSSL errors to API docs #34213

Merged
merged 1 commit into from
May 5, 2024

Conversation

j3lamp
Copy link
Contributor

@j3lamp j3lamp commented Jul 6, 2020

Fixes: #33705

Checklist

@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 Jul 6, 2020
Copy link
Member

@bnoordhuis bnoordhuis left a 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.

Comment on lines 2723 to 2724
The certificate has expired: that is the notAfter date is before the current
time.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

<a id="CRL_NOT_YET_VALID"></a>
#### `CRL_NOT_YET_VALID`

The revocation data have a future issue date.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The revocation data have a future issue date.
The revocation data has a future issue date.

Copy link
Contributor Author

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".

<a id="CERT_CHAIN_TOO_LONG"></a>
#### `CERT_CHAIN_TOO_LONG`

The certificate chain length is greater than the supplied maximum depth.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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().)


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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor Author

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.

@j3lamp
Copy link
Contributor Author

j3lamp commented Jul 6, 2020

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.

@nodejs-github-bot
Copy link
Collaborator

@j3lamp j3lamp requested a review from a team August 10, 2020 16:06
@PoojaDurgad
Copy link
Contributor

@j3lamp - need a rebase to resolve git conflicts

@DerekNonGeneric
Copy link
Contributor

Would be nice to get this in. Seems good to go.

/cc @Trott

@Trott
Copy link
Member

Trott commented Feb 22, 2021

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).

Copy link

@Mifrill Mifrill left a 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 <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>
@aduh95 aduh95 merged commit e22bc1e into nodejs:main May 5, 2024
42 of 43 checks passed
Ch3nYuY pushed a commit to Ch3nYuY/node that referenced this pull request May 8, 2024
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>
targos pushed a commit that referenced this pull request May 8, 2024
Fixes: #33705
PR-URL: #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>
lukins-cz pushed a commit to lukins-cz/OS-Aplet-node that referenced this pull request Jun 1, 2024
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>
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.

doc: UNABLE_TO_VERIFY_LEAF_SIGNATURE/unable to verify the first certificate error not documented