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

Support of pkcs12 certificate in memory with libcurl setopt v2 #5357

Closed
wants to merge 1 commit into from

Conversation

gvollant
Copy link
Contributor

@gvollant gvollant commented May 8, 2020

This pull request is a reopen of previous pull request, with a lot of modification and confusion
#4371

We can use a PKCS12 certificate with curl, but only from a disk file. We add the feature of using a certificate from memory block without creating temporary file.

This version uses memory blob.
Pehaps a better solution can be use the same logic than POSTFIELD...

docs/libcurl/opts/CURLOPT_SSLCERT_BLOB.3 Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_SSLCERT_BLOB.3 Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_SSLCERT_BLOB.3 Outdated Show resolved Hide resolved
include/curl/easy.h Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_SSLCERT_BLOB.3 Outdated Show resolved Hide resolved
docs/libcurl/symbols-in-versions Outdated Show resolved Hide resolved
include/curl/easy.h Outdated Show resolved Hide resolved
include/curl/easy.h Outdated Show resolved Hide resolved
include/curl/easy.h Outdated Show resolved Hide resolved
include/curl/easy.h Outdated Show resolved Hide resolved
include/curl/easy.h Outdated Show resolved Hide resolved
lib/easy.c Outdated Show resolved Hide resolved
lib/easy.c Outdated Show resolved Hide resolved
lib/setopt.c Outdated Show resolved Hide resolved
lib/setopt.c Outdated Show resolved Hide resolved
lib/setopt.h Outdated Show resolved Hide resolved
@gvollant gvollant force-pushed the pkcs12mem/pkcs12mem branch 4 times, most recently from 694c6e5 to a33dd95 Compare May 9, 2020 01:36
lib/easy.c Outdated Show resolved Hide resolved
@gvollant gvollant force-pushed the pkcs12mem/pkcs12mem branch 4 times, most recently from b034ef6 to 5ae8d06 Compare May 9, 2020 22:15
@gvollant gvollant requested a review from bagder May 9, 2020 22:49
@gvollant gvollant force-pushed the pkcs12mem/pkcs12mem branch 5 times, most recently from 71ca058 to cf59aa3 Compare May 10, 2020 08:38
@gvollant
Copy link
Contributor Author

Near all test are successful, except on msys1 mingw64 openssl
"TESTFAIL: These test cases failed: (203) (1056) (1143) (1299) 1501"

I have no idea...

curl_init_blob_flag((structblob), (ptr), (len), CURL_BLOB_DUPFLAG_NOCOPY)

#define curl_init_blob_dup(structblob, ptr, len) \
curl_init_blob_flag((structblob), (ptr), (len), CURL_BLOB_DUPFLAG_COPY)
Copy link
Member

Choose a reason for hiding this comment

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

Width this very simple struct, do we really need an init macro at all, yet again three different ones? We don't really offer macros in our API before, and you have not provided documentation for these.

Also, why a size_t for flags? It seems like an non-ideal choice since it can vary in size and is often 64 bits when we have need for a single bit right now...

include/curl/easy.h Outdated Show resolved Hide resolved
include/curl/easy.h Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_SSLKEY_BLOB.3 Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_SSLCERT_BLOB.3 Outdated Show resolved Hide resolved
lib/urldata.h Outdated Show resolved Hide resolved
lib/vtls/openssl.c Outdated Show resolved Hide resolved
lib/vtls/openssl.c Outdated Show resolved Hide resolved
lib/setopt.c Show resolved Hide resolved
lib/vtls/openssl.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member

bagder commented May 10, 2020

These test cases failed: (203) (1056) (1143) (1299) 1501

1501 is a known flaky test on windows.

@dfandrich
Copy link
Contributor

dfandrich commented May 11, 2020 via email

@bagder
Copy link
Member

bagder commented May 11, 2020

1501 should really have the "flaky" keyword then, which it doesn't have now.

Probably. I've hesitated because it is only flaky on (appveyor running) Windows, and the flaky keyword disables it in all systems...

We should probably instead make appveyor's CI jobs ignore the results of it. @mback2k, do you think that would make your current Windows CI/tests efforts harder or won't it matter?

@mback2k
Copy link
Member

mback2k commented May 11, 2020

We should probably instead make appveyor's CI jobs ignore the results of it. @mback2k, do you think that would make your current Windows CI/tests efforts harder or won't it matter?

It won't matter, but actually it is not just 1501 that is flaky even though that one hits it more often. I still (again) see the log output missing failures, also on Azure nowadays which is kinda surprising.

Edit: actually I already completely disabled test 1501 on the relevant AppVeyor builds due to its extreme flaky state via 4fdb200.

@gvollant gvollant force-pushed the pkcs12mem/pkcs12mem branch 3 times, most recently from be81d4b to f82f1ae Compare May 11, 2020 20:01
@gvollant
Copy link
Contributor Author

It seel they are some error with test561 under Windows

I've no idea about this problem...

@gvollant gvollant requested a review from bagder May 11, 2020 21:29
@gvollant
Copy link
Contributor Author

yesterday, I added a simple commit with just spacing modification 1c0505a and I have less windows test error than f82f1ae

@gvollant gvollant force-pushed the pkcs12mem/pkcs12mem branch 3 times, most recently from dae633b to 6786759 Compare May 12, 2020 08:14
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

I have a set of minor remarks...

docs/libcurl/opts/CURLOPT_ISSUERCERT_BLOB.3 Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_PROXY_SSLCERT_BLOB.3 Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_PROXY_SSLKEY_BLOB.3 Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_PROXY_SSLKEY_BLOB.3 Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_SSLCERT_BLOB.3 Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_SSLKEY_BLOB.3 Outdated Show resolved Hide resolved
include/curl/easy.h Outdated Show resolved Hide resolved
include/curl/easy.h Outdated Show resolved Hide resolved
lib/setopt.c Show resolved Hide resolved
lib/vtls/openssl.c Outdated Show resolved Hide resolved
@gvollant gvollant force-pushed the pkcs12mem/pkcs12mem branch 2 times, most recently from 783b152 to 03b7a0c Compare May 13, 2020 20:08
@gvollant gvollant mentioned this pull request May 13, 2020
@bagder
Copy link
Member

bagder commented May 14, 2020

Conflicting files: lib/vtls/schannel.c

Looks simple enough to fix though!

@gvollant
Copy link
Contributor Author

Conflicting files: lib/vtls/schannel.c

Looks simple enough to fix though!

done.
Do you think I must done more workl?

@bagder
Copy link
Member

bagder commented May 15, 2020

Thanks @gvollant, I'll take it from here. I spotted some minor details I wanted to edit before I merge.

@bagder bagder closed this in cac5374 May 15, 2020
@bagder
Copy link
Member

bagder commented May 15, 2020

Thanks @gvollant for your hard work on this!

@gvollant
Copy link
Contributor Author

Thanks @gvollant, I'll take it from here. I spotted some minor details I wanted to edit before I merge.

Thank you.
I looked again, and I have a question :

We don't modify Curl_clone_primary_ssl_config , Curl_free_primary_ssl_config and Curl_ssl_config_matches at beginning of vtls.c

I just want be sure this was not needed

regards and thank you for working together :-)

@JCMais
Copy link
Contributor

JCMais commented May 29, 2020

@bagder @gvollant does that mean that in the next libcurl version I will be able to provide the CA cert directly instead of a file path using CURLOPT_CAINFO? If yes, what is the correct option from these, CURLOPT_ISSUERCERT_BLOB?

This looks awesome!

@gvollant
Copy link
Contributor Author

@bagder @gvollant does that mean that in the next libcurl version I will be able to provide the CA cert directly instead of a file path using CURLOPT_CAINFO? If yes, what is the correct option from these, CURLOPT_ISSUERCERT_BLOB?

This is the subject of #4679 which somes works (don't merge with current master, by example)

@gvollant
Copy link
Contributor Author

gvollant commented May 5, 2021

#6662 is now merged with CA as blob

@gvollant gvollant deleted the pkcs12mem/pkcs12mem branch May 5, 2021 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants