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.

Sign up for GitHub

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

Jump to bottom

Add manpages for SSL_get_certificate, SSL_get_privatekey #17815

Closed
wants to merge 7 commits into from
Closed

Add manpages for SSL_get_certificate, SSL_get_privatekey #17815

wants to merge 7 commits into from

Conversation

hlandau
Copy link
Member

@hlandau hlandau commented Mar 4, 2022

This is as I understand these functions from reading the code.

@hlandau
Add manpages for SSL_get_certificate, SSL_get_private_key
933833f 
This is as I understand these functions from reading the code.
@hlandau hlandau added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: documentation The issue/pr deals with documentation (errors) branch: 3.0 Merge to openssl-3.0 branch labels Mar 4, 2022
@hlandau hlandau self-assigned this Mar 4, 2022
@hlandau hlandau changed the title Add manpages for SSL_get_certificate, SSL_get_private_key Add manpages for SSL_get_certificate, SSL_get_privatekey Mar 4, 2022
mattcaswell
mattcaswell requested changes Mar 4, 2022
View reviewed changes
doc/man3/SSL_get_certificate.pod Outdated
certificate which represents the local peer's identity. For servers, it returns
the server's certificate. If a server has multiple certificates (for different
algorithms, for example RSA and ECDSA), the certificate in actual use is
returned. For clients, it returns the client certificate being used, if any.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this isn't quite correct. Before the handshake it returns the last certificate that was added to the SSL/SSL_CTX or NULL if none were added yet. During the handshake it selects a certificate to use for the connection and from that point on it returns the selected certificate.

Copy link
Member

Choose a reason for hiding this comment

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

I still don't think this is quite right. I would merge the text you added under RETURN VALUES into this text. The text about "multiple certificates" at the moment is quite misleading in the case where it is called before the handshake. Perhaps adding something like "or the most recently added certificate if called before a certificate has been selected". Or something like that.

@hlandau
Copy link
Member Author

hlandau commented Mar 4, 2022

How's this?

mattcaswell
mattcaswell requested changes Mar 4, 2022
View reviewed changes
doc/man3/SSL_get_certificate.pod Outdated
certificate which represents the local peer's identity. For servers, it returns
the server's certificate. If a server has multiple certificates (for different
algorithms, for example RSA and ECDSA), the certificate in actual use is
returned. For clients, it returns the client certificate being used, if any.
Copy link
Member

Choose a reason for hiding this comment

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

I still don't think this is quite right. I would merge the text you added under RETURN VALUES into this text. The text about "multiple certificates" at the moment is quite misleading in the case where it is called before the handshake. Perhaps adding something like "or the most recently added certificate if called before a certificate has been selected". Or something like that.

doc/man3/SSL_get_certificate.pod Outdated
object is available. Before the handshake has completed, the certificate
returned is the one most recently added to the SSL object, or NULL if no
certificate has yet been added. After the handshake has completed, the
certificate returned is the one which was selected during the handshake.
Copy link
Member

Choose a reason for hiding this comment

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

I'd keep this more about about error conditions etc, e.g. "SSL_get_certifciate() returns a pointer to the certificate or a NULL if there is no certificate"

@hlandau
Copy link
Member Author

hlandau commented Mar 4, 2022

Updated again.

mattcaswell
mattcaswell reviewed Mar 4, 2022
View reviewed changes
doc/man3/SSL_get_certificate.pod Outdated
=item

If it is called before the handshake has completed, it returns the most recently
added certificate, or NULL if no certificate has been added.
Copy link
Member

Choose a reason for hiding this comment

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

Strictly speaking the certificate selection happens during the handshake. This distinction is important in the case of callbacks that occur during the handshake (such as the tlsext_status_cb). So, the tlsext_statuc_cb will get the selected certificate (which is not necessarily the most recently added one) even though the handshake has not been completed yet.

@hlandau
Copy link
Member Author

hlandau commented Mar 4, 2022

Updated.

paulidale
paulidale approved these changes Mar 10, 2022
View reviewed changes
doc/man3/SSL_get_certificate.pod Outdated Show resolved Hide resolved
@paulidale paulidale removed the approval: otc review pending This pull request needs review by an OTC member label Mar 10, 2022
@hlandau
Copy link
Member Author

hlandau commented Mar 10, 2022

Updated.

@hlandau
Copy link
Member Author

hlandau commented Mar 10, 2022

Updated.

paulidale
paulidale approved these changes Mar 11, 2022
View reviewed changes
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

Still approved (of course).

t8m
t8m previously approved these changes Mar 11, 2022
View reviewed changes
@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Mar 11, 2022
@t8m t8m requested a review from mattcaswell March 11, 2022 10:02
@t8m t8m dismissed their stale review March 11, 2022 10:02

CI fails

@t8m
Copy link
Member

t8m commented Mar 11, 2022

The CI failure is relevant. Please fix.

@t8m t8m removed the approval: done This pull request has the required number of approvals label Mar 11, 2022
mattcaswell
mattcaswell requested changes Mar 11, 2022
View reviewed changes
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

Looks good apart from the nit (which is causing the CI failure)

doc/man3/SSL_get_certificate.pod Show resolved Hide resolved
@hlandau
Copy link
Member Author

hlandau commented Mar 11, 2022

Updated.

@t8m t8m added the approval: review pending This pull request needs review by a committer label Mar 11, 2022
@t8m
Copy link
Member

t8m commented Mar 11, 2022

@paulidale still OK?

@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Mar 11, 2022
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Mar 12, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Mar 14, 2022
@hlandau @t8m
Add manpages for SSL_get_certificate, SSL_get_private_key
2a92195 
This is as I understand these functions from reading the code.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #17815)
openssl-machine pushed a commit that referenced this pull request Mar 14, 2022
@hlandau @t8m
Add manpages for SSL_get_certificate, SSL_get_private_key
0fcbfa8 
This is as I understand these functions from reading the code.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #17815)

(cherry picked from commit 2a92195)
@t8m
Copy link
Member

t8m commented Mar 14, 2022

Merged to 3.0 and master branches. Thank you.

@t8m t8m closed this Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

@paulidale paulidale paulidale approved these changes

@levitte levitte Awaiting requested review from levitte

Assignees

@hlandau hlandau

Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch triaged: documentation The issue/pr deals with documentation (errors)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@hlandau @t8m @openssl-machine @mattcaswell @paulidale