Skip to content

Conversation

@moonchen
Copy link
Contributor

@moonchen moonchen commented Nov 4, 2022

Additional certificates in a chain should be loaded with the SSL_CTX_add_extra_chain_cert API.

Client certificate chain loading was using the wrong OpenSSL API,
causing the chain to be loaded incorrectly.
@moonchen moonchen changed the title Fix Loading Client Certificate Chain Fix the Loading of Client Certificate Chains Nov 4, 2022
@moonchen
Copy link
Contributor Author

moonchen commented Nov 4, 2022

[approve ci autest]

@masaori335 masaori335 added the TLS label Nov 7, 2022
@masaori335 masaori335 added this to the 10.0.0 milestone Nov 7, 2022
@bneradt bneradt requested a review from duke8253 November 8, 2022 00:12
Copy link
Contributor

@duke8253 duke8253 left a comment

Choose a reason for hiding this comment

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

I think this is a good find. But the OpenSSL docs isn't very specific on whether the first cert needs to be added using SSL_CTX_use_certificate. Since it says (https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_use_certificate.html) "The rest of the certificates needed to form the complete certificate chain can be specified using the SSL_CTX_add_extra_chain_cert function". Could you confirm that all certs can be added to the chain using SSL_CTX_add_extra_chain_cert, including the first one?

@moonchen
Copy link
Contributor Author

moonchen commented Nov 8, 2022

I think this is a good find. But the OpenSSL docs isn't very specific on whether the first cert needs to be added using SSL_CTX_use_certificate. Since it says (https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_use_certificate.html) "The rest of the certificates needed to form the complete certificate chain can be specified using the SSL_CTX_add_extra_chain_cert function". Could you confirm that all certs can be added to the chain using SSL_CTX_add_extra_chain_cert, including the first one?

The first cert is still added using SSL_CTX_use_certificate on line 828, so this only changes the loading of the second certificate and subsequent certs.

Before this change, loading a certificate chain and a private key will cause OpenSSL to give an error that the cert and key don't match. I can confirm that loading the chain this way gets OpenSSL to pass the SSL_CTX_check_private_key.

@vmamidi
Copy link
Contributor

vmamidi commented Nov 8, 2022

I think this is a good find. But the OpenSSL docs isn't very specific on whether the first cert needs to be added using SSL_CTX_use_certificate. Since it says (https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_use_certificate.html) "The rest of the certificates needed to form the complete certificate chain can be specified using the SSL_CTX_add_extra_chain_cert function". Could you confirm that all certs can be added to the chain using SSL_CTX_add_extra_chain_cert, including the first one?

I do not think SSL_CTX_add_extra_chain_cert can add the first one.

@zwoop
Copy link
Contributor

zwoop commented Nov 8, 2022

@bryancall Approves this, but he's lost his Github access because his mobile provider is terrible.

@vmamidi vmamidi merged commit 9a7b464 into apache:master Nov 8, 2022
zwoop pushed a commit that referenced this pull request Nov 8, 2022
Client certificate chain loading was using the wrong OpenSSL API,
causing the chain to be loaded incorrectly.

(cherry picked from commit 9a7b464)
@zwoop
Copy link
Contributor

zwoop commented Nov 8, 2022

Cherry-picked to v9.2.x

@zwoop zwoop modified the milestones: 10.0.0, 9.2.0 Nov 8, 2022
SolidWallOfCode pushed a commit to SolidWallOfCode/trafficserver that referenced this pull request Nov 15, 2022
Client certificate chain loading was using the wrong OpenSSL API,
causing the chain to be loaded incorrectly.
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Feb 21, 2023
* asf/9.2.x:
  Fix Loading Client Certificate Chain (apache#9177)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants