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

Add CURLE_SSL_CLIENTCERT error #6721

Closed
wants to merge 1 commit into from
Closed

Conversation

@ebejan
Copy link
Contributor

@ebejan ebejan commented Mar 11, 2021

-When server requests client certificate request during handshake, it returns generic error CURLE_SSL_CONNECT_ERROR. This is very specific error, and would like to be handled specifically, so adding different error code CURLE_SSL_CLIENTCERT

@ebejan
Copy link
Contributor Author

@ebejan ebejan commented Mar 11, 2021

As beginging, adding only to OSX.

@deepcode-ci-bot
Copy link

@deepcode-ci-bot deepcode-ci-bot bot commented Mar 11, 2021

Congratulations 🎉. DeepCode analyzed your code in 0.724 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

Copy link
Member

@bagder bagder left a comment

I lack a documentation update in docs/libcurl/libcurl-errors.3 but perhaps I'm mostly concerned that this creates a new return code and only it is only used in a single TLS backend, and in one that is mostly deprecated . Did you try to provide it for perhaps at least OpenSSL as well?

docs/libcurl/symbols-in-versions Outdated Show resolved Hide resolved
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
lib/vtls/sectransp.c Outdated Show resolved Hide resolved
@jay
Copy link
Member

@jay jay commented Mar 12, 2021

What is your use case for this error code, how is it necessary?

@jay jay added the SSL/TLS label Mar 12, 2021
@ebejan
Copy link
Contributor Author

@ebejan ebejan commented Mar 12, 2021

@jay Our curl wrapper detects CURLE_SSL_CLIENTCERT error and negotiates with the other side, gets all the certificates, and let user to select the certain certificates.

@bagder
Copy link
Member

@bagder bagder commented Mar 17, 2021

I don't mind adding a new error code for this condition. But...

I think this error code needs to also be supported at least for OpenSSL to be acceptable (and preferably for even more). As a I said before: Secure Transport is more or less deprecated by Apple themselves now and adding a global new curl error code only supported in that niche backend I think isn't meeting the bar we should set for new error codes.

Copy link
Member

@bagder bagder left a comment

I think this error code needs support in more TLS backends before we can merge support it.

@ebeworld
Copy link

@ebeworld ebeworld commented Apr 13, 2021

I have ported the change to OpenSSL

@ebejan ebejan requested a review from bagder Apr 13, 2021
Copy link
Member

@bagder bagder left a comment

I would appreciate if you squashed this set of commits to the lowest number of commits you think think it should use - to ease reviewing.

@@ -3317,6 +3317,11 @@ static CURLcode ossl_connect_step2(struct Curl_easy *data,
error_buffer */
strcpy(error_buffer, "SSL certificate verification failed");
}
else if((lib == ERR_LIB_SSL) &&
(reason == SSL_R_TLSV13_ALERT_CERTIFICATE_REQUIRED)) {

This comment has been minimized.

@bagder

bagder Apr 14, 2021
Member

This sounds TLS 1.3 specific, isn't it?

This comment has been minimized.

@ebeworld

ebeworld Apr 29, 2021

Yes, that is true.

This comment has been minimized.

@bagder

bagder Apr 29, 2021
Member

So you don't think it's a problem that if you happen to negotiate another TLS version you won't get this error code for the otherwise very similar situation?

This comment has been minimized.

@ebeworld

ebeworld Apr 30, 2021

I would like to add the error code to other TLS versions, but don't know how yet. So, wanted to add concepts first TLS 1.3 and above then slowly add for other versions.

@@ -3317,7 +3317,7 @@ static CURLcode ossl_connect_step2(struct Curl_easy *data,
error_buffer */
strcpy(error_buffer, "SSL certificate verification failed");
}
#if OPENSSL_VERSION_NUMBER >= 0x10101000L
#if OPENSSL_VERSION_NUMBER >= 0x10101000L && !defined(LIBRESSL_VERSION_NUMBER)

This comment has been minimized.

@bagder

bagder Apr 14, 2021
Member

Why is this excluding libressl? I think a comment here explaining this could be a good idea.

This comment has been minimized.

@ebeworld

ebeworld Apr 29, 2021

SSL_R_TLSV13_ALERT_CERTIFICATE_REQUIRED is only available on OpenSSL v1.1.1 above and added comments for it.

@ebeworld ebeworld force-pushed the ebeworld:CURLE_SSL_CLIENTCERT branch from 3d6b7c0 to 0ccf862 Apr 29, 2021
@ebeworld
Copy link

@ebeworld ebeworld commented Apr 29, 2021

@bagder I have squashed the changes and added comments as requested.

@ebeworld ebeworld force-pushed the ebeworld:CURLE_SSL_CLIENTCERT branch 2 times, most recently from 4d28576 to f144b7c Apr 29, 2021
When server requests client certificate request during handshake, it returns generic error CURLE_SSL_CONNECT_ERROR.

This is very specific error, and would like to be handled specifically, so adding different error code CURLE_SSL_CLIENTCERT
@ebeworld ebeworld force-pushed the ebeworld:CURLE_SSL_CLIENTCERT branch from 12dd031 to 8f4d362 Apr 30, 2021
@gvollant
Copy link
Contributor

@gvollant gvollant commented Apr 30, 2021

Do you have an easy method to reproduce this error?

@ebeworld
Copy link

@ebeworld ebeworld commented Apr 30, 2021

Do you have an easy method to reproduce this error?

@gvollant If the mutual TLS authentication set on both client and server, client does not give its certificates, the error will be return. That is our use case and at that point client asks end user to select certificates for mutual handshake. (in case of client aware of multiple certificates, client does not know which one to provide, hence end users interaction is needed.)

@bagder bagder closed this in 94241a9 May 3, 2021
@bagder
Copy link
Member

@bagder bagder commented May 3, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants