Skip to content

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

Closed
gvollant wants to merge 1 commit into
curl:masterfrom
gvollant:pkcs12mem/pkcs12mem
Closed

Support of pkcs12 certificate in memory with libcurl setopt v2#5357
gvollant wants to merge 1 commit into
curl:masterfrom
gvollant:pkcs12mem/pkcs12mem

Conversation

@gvollant

@gvollant gvollant commented May 8, 2020

Copy link
Copy Markdown
Contributor

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

@gvollant
gvollant force-pushed the pkcs12mem/pkcs12mem branch from 7a454db to 39c2e75 Compare May 8, 2020 14:46
bagder
bagder reviewed May 8, 2020
View reviewed changes
Comment thread docs/libcurl/opts/CURLOPT_SSLCERT_BLOB.3 Outdated
Comment thread docs/libcurl/opts/CURLOPT_SSLCERT_BLOB.3 Outdated
Comment thread docs/libcurl/opts/CURLOPT_SSLCERT_BLOB.3 Outdated
Comment thread include/curl/easy.h Outdated
Comment thread docs/libcurl/opts/CURLOPT_SSLCERT_BLOB.3 Outdated
Comment thread docs/libcurl/symbols-in-versions Outdated
@gvollant
gvollant force-pushed the pkcs12mem/pkcs12mem branch from 303b59d to b091dbf Compare May 8, 2020 16:16
bagder
bagder reviewed May 8, 2020
View reviewed changes
Comment thread include/curl/easy.h Outdated
Comment thread include/curl/easy.h Outdated
Comment thread include/curl/easy.h Outdated
Comment thread include/curl/easy.h Outdated
Comment thread include/curl/easy.h Outdated
Comment thread lib/easy.c Outdated
Comment thread lib/easy.c Outdated
Comment thread lib/setopt.c Outdated
Comment thread lib/setopt.c Outdated
Comment thread lib/setopt.h Outdated
@gvollant
gvollant force-pushed the pkcs12mem/pkcs12mem branch 4 times, most recently from 694c6e5 to a33dd95 Compare May 9, 2020 01:36
cvengler
cvengler reviewed May 9, 2020
View reviewed changes
Comment thread lib/easy.c Outdated
@gvollant
gvollant force-pushed the pkcs12mem/pkcs12mem branch 4 times, most recently from b034ef6 to 5ae8d06 Compare May 9, 2020 22:15
@bagder bagder mentioned this pull request May 9, 2020
Closed
@gvollant
gvollant force-pushed the pkcs12mem/pkcs12mem branch from 5ae8d06 to e815049 Compare May 9, 2020 22:49
@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

gvollant commented May 10, 2020

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

bagder
bagder reviewed May 10, 2020
View reviewed changes
Comment thread include/curl/easy.h Outdated

@bagder bagder May 10, 2020

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

Comment thread include/curl/easy.h Outdated
Comment thread include/curl/easy.h Outdated
Comment thread docs/libcurl/opts/CURLOPT_SSLKEY_BLOB.3 Outdated
Comment thread docs/libcurl/opts/CURLOPT_SSLCERT_BLOB.3 Outdated
Comment thread lib/urldata.h Outdated
Comment thread lib/vtls/openssl.c Outdated
Comment thread lib/vtls/openssl.c Outdated
Comment thread lib/setopt.c Outdated
Comment thread lib/vtls/openssl.c Outdated
@bagder

bagder commented May 10, 2020

Copy link
Copy Markdown
Member

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

1501 is a known flaky test on windows.

@dfandrich

dfandrich commented May 11, 2020 via email

Copy link
Copy Markdown
Contributor

@bagder

bagder commented May 11, 2020

Copy link
Copy Markdown
Member

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

mback2k commented May 11, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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

gvollant commented May 11, 2020

Copy link
Copy Markdown
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

gvollant commented May 12, 2020

Copy link
Copy Markdown
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
bagder
bagder reviewed May 13, 2020
View reviewed changes

@bagder bagder left a comment

Copy link
Copy Markdown
Member

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

Comment thread docs/libcurl/opts/CURLOPT_ISSUERCERT_BLOB.3 Outdated
Comment thread docs/libcurl/opts/CURLOPT_PROXY_SSLCERT_BLOB.3 Outdated
Comment thread docs/libcurl/opts/CURLOPT_PROXY_SSLKEY_BLOB.3 Outdated
Comment thread docs/libcurl/opts/CURLOPT_PROXY_SSLKEY_BLOB.3 Outdated
Comment thread docs/libcurl/opts/CURLOPT_SSLCERT_BLOB.3 Outdated
Comment thread docs/libcurl/opts/CURLOPT_SSLKEY_BLOB.3 Outdated
Comment thread include/curl/easy.h Outdated
Comment thread include/curl/easy.h Outdated
Comment thread lib/setopt.c Outdated
Comment thread lib/vtls/openssl.c Outdated
@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
Closed
@bagder

bagder commented May 14, 2020

Copy link
Copy Markdown
Member

Conflicting files: lib/vtls/schannel.c

Looks simple enough to fix though!

@gvollant
gvollant force-pushed the pkcs12mem/pkcs12mem branch from 03b7a0c to c92a862 Compare May 15, 2020 08:48
@gvollant

gvollant commented May 15, 2020

Copy link
Copy Markdown
Contributor Author

Conflicting files: lib/vtls/schannel.c

Looks simple enough to fix though!

done.
Do you think I must done more workl?

@bagder

bagder commented May 15, 2020

Copy link
Copy Markdown
Member

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

bagder commented May 15, 2020

Copy link
Copy Markdown
Member

Thanks @gvollant for your hard work on this!

@gvollant

gvollant commented May 15, 2020

Copy link
Copy Markdown
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 :-)

@gvollant gvollant mentioned this pull request May 16, 2020
Closed
@JCMais

JCMais commented May 29, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

@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

gvollant commented May 29, 2020

Copy link
Copy Markdown
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 gvollant mentioned this pull request Feb 25, 2021
Closed
@gvollant gvollant mentioned this pull request Apr 23, 2021
Closed
@gvollant

gvollant commented May 5, 2021

Copy link
Copy Markdown
Contributor Author

#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

Reviewers

@bagder bagder bagder left review comments
+1 more reviewer
@cvengler cvengler cvengler left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@gvollant @bagder @dfandrich @mback2k @JCMais @cvengler