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

Inconsistency in cert/key params, OpenSSL ENGINE / PKCS#11 issues #974

Closed
dwmw2 opened this issue Aug 22, 2016 · 9 comments
Closed

Inconsistency in cert/key params, OpenSSL ENGINE / PKCS#11 issues #974

dwmw2 opened this issue Aug 22, 2016 · 9 comments

Comments

@dwmw2
Copy link
Contributor

dwmw2 commented Aug 22, 2016

Rather than continuing to pollute #964 I'll open a new ticket. This command line works when curl is built against GnuTLS or a suitably fixed NSS:

curl -E pkcs11:manufacturer=piv_II;id=%01;pin-value=1234 $URL

It doesn't work with OpenSSL, for a number of reasons.

One of the simple problems is that the curl command-line tool will automatically assume you want CURLOPT_SSLENGINE_DEFAULT if you pass --engine. This seems wrong, and it makes the SSL connection fail because you really don't ever want that:

* TLSv1.2 (OUT), TLS handshake, Certificate (11):
* error:2D06D075:FIPS routines:fips_pkey_signature_test:test failure
* Marked for [closure]: Failed HTTPS connection

@nased0 I'm assuming this isn't a problem for you because you're using libcurl and not setting CURLOPT_SSLENGINE_DEFAULT? I can't understand why anyone would want that option, although it's been present in curl(1) since the ENGINE support was first merged in commit af6c394. Can we just make the tool stop doing that?

Next we have to specify the ENGINE explicitly by adding --engine pkcs11. We'll want to automatically infer that from the 'pkcs11:' at the beginning of the command line. Which brings me to the CURLOPT_SSLCERTTYPE and CURLOPT_SSLKEYTYPE options...

I also need to pass --certtype ENG. OK, suboptimal but I can fix that (and let's fix it to automatically determine PKCS#12 vs. PEM files while we're at it, shall we? Just how much do we hate our users, to make them do that manually?).

So now maybe with --engine pkcs11 --cert-type ENG it'll work...

curl: (58) unable to set private key file: 'pkcs11:manufacturer=piv_II;id=%01;pin-value=1234' type PEM

Oh, that's cute. We must really hate our users :)

We "assume" that the key identifier string (usually filename) is the same as the cert, but we don't assume that it's the same type? Really? OK, let's tell it that the key is the same type as the cert...

curl  $URL --engine pkcs11 --cert-type ENG -E 'pkcs11:manufacturer=piv_II;id=%01;pin-value=1234'  --key-type ENG
No keys found.
PKCS11_get_private_key returned NULL
curl: (58) failed to load private key from crypto engine

This is getting surreal now... if I specify the key type you forget what the key was. Finally, if I spell everything out (and patch out the ENGINE_set_default() call as discussed above), it does actually work:

curl  -k https://localhost:4443/data.txt  --engine pkcs11 \
  --cert-type ENG -E 'pkcs11:manufacturer=piv_II;id=%01;pin-value=1234' \
  --key-type ENG --key 'pkcs11:manufacturer=piv_II;id=%01;pin-value=1234'

So... rants about hating users aside, what can we do about this? What part of this utter clusterf*ck do we need to preserve in the name of backward-compatibility, and what can we fix? :)

My suggestion would be to add a cert and key type of 'AUTO', which will be the default, and will work it out automatically. If it's a filename, then look in the file to see if it's PKCS#12 or PEM and act accordingly. And then only if your crypto library is insane enough to make you choose, rather than silently just doing the right thing with the filename you give it.

And if the string starts "pkcs11:" then it's fairly obviously a PKCS#11 URI so treat it as such. Which means setting the engine to 'pkcs11' in the OpenSSL case, but other libraries should cope automatically. Although that's partly because the NSS back end treats it as an NSS identifier even when the type is PEM... but OK, whatever.

It's possible to fix this up in the tool, and have it inspect the --cert argument and manually set the type(s) accordingly... but I think it's much better off in the library.

@dwmw2
Copy link
Contributor Author

dwmw2 commented Aug 22, 2016

I note that the GnuTLS back end doesn't work with PKCS#12 files at all, and accepts PKCS#11 URIs whether you set the file type to PEM or DER. In fact, I'm not sure anything works with PKCS#12 files.

PEM files... are odd. You need to specify the password for the key, in the --cert option:
-E 'cert.pem:key-password' --key 'key.pem'
Which is kind of counter-intuitive. But I think we've already established that we hate users, right?

And the above only actually works for OpenSSL and GnuTLS builds here; with NSS it still fails thus:
curl: (58) unable to load client key: -8178 (SEC_ERROR_BAD_KEY)

@dwmw2 dwmw2 changed the title OpenSSL ENGINE / PKCS#11 issues Inconsistency in cert/key params, OpenSSL ENGINE / PKCS#11 issues Aug 22, 2016
@dwmw2
Copy link
Contributor Author

dwmw2 commented Aug 22, 2016

Turns out the nss-pem module doesn't handle encrypted PKCS#8 PEM files at all. So the latter is at least not curl's fault. https://bugzilla.redhat.com/show_bug.cgi?id=1369251

@nased0
Copy link

nased0 commented Aug 23, 2016

I use curl's PKCS#11 capabilities in a different way:
curl --engine pkcs11 --tlsv1.1 --key-type ENG --key d7f4b99792cc4dd708e408d3eb91b566e0a06c02 --cert-type ENG --cert d7f4b99792cc4dd708e408d3eb91b566e0a06c02 -k https://_..**.**_:443/cepik/api/skp?wsdl
(curl displays WSDL to stdout)

I'm listing private keys on card using command:
pkcs11-tool.exe --module enigmap11.dll --login --login-type user --type privkey -O
Using slot 0 with a present token (0x0)
Logging in to "ENCARD Token kwalifikowany".
Please enter User PIN: Private Key Object; RSA
label:
ID: d7f4b99792cc4dd708e408d3eb91b566e0a06c02
Usage: decrypt, sign

To list certificates I use a command:
pkcs11-tool.exe --module enigmap11.dll -O --type cert
Using slot 0 with a present token (0x0)
Certificate Object, type = X.509 cert
label: WX/999
ID: d7f4b99792cc4dd708e408d3eb91b566e0a06c02
Certificate Object, type = X.509 cert
label: Certyfikat CCK

Then I paste these IDs to the curl command or use them in CURLOPT_SSLCERT and CURLOPT_SSLKEY.

My OpenSSL initialization function is as follows:
`int __fastcall TUploaderThread::SetSSL(void *curl)
{
int res = CURLE_OK;

res = curl_easy_setopt(curl, CURLOPT_SSLVERSION,
    bTLSv1_2Support ? CURL_SSLVERSION_TLSv1_2 : CURL_SSLVERSION_TLSv1_1);
if(res != CURLE_OK)
    return res;

res = curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 0L);
if(res != CURLE_OK)
    return res;

res = curl_easy_setopt(curl, CURLOPT_SSLENGINE, bPKCS11Support ? "pkcs11" : NULL);
if(res != CURLE_OK)
    return res;

res = curl_easy_setopt(curl, CURLOPT_SSLENGINE_DEFAULT, bPKCS11Support ? 1L : 0L);
if(res != CURLE_OK)
    return res;

res = curl_easy_setopt(curl, CURLOPT_SSLKEYTYPE, bPKCS11Support ? "ENG" : "PEM");
if(res != CURLE_OK)
    return res;

res = curl_easy_setopt(curl, CURLOPT_SSLCERTTYPE, bPKCS11Support ? "ENG" : "PEM");
if(res != CURLE_OK)
    return res;

if((strCACert.Length() > 0) && FileExists(strCACert))
{
    res = curl_easy_setopt(curl, CURLOPT_CAINFO, strCACert.c_str());
    if(res != CURLE_OK)
        return res;

    res = curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 1L);
    if(res != CURLE_OK)
        return res;
}
else
{
    res = curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 0L);
    if(res != CURLE_OK)
        return res;
}

if(strCert.Length() > 0)
{
    res = curl_easy_setopt(curl, CURLOPT_SSLCERT, strCert.c_str());
    if(res != CURLE_OK)
        return res;
}
else
if(!bPKCS11Support)
    return -1;

if(strPrivateKey.Length() > 0)
{
    res = curl_easy_setopt(curl, CURLOPT_SSLKEY, strPrivateKey.c_str());
    if(res != CURLE_OK)
        return res;
}
else
if(!bPKCS11Support)
    return -2;

return res;

}
`

@nased0
Copy link

nased0 commented Aug 23, 2016

One remark: I use old versions of engine_pkcs11.dll (0.1.8) and libp11 (0.2.8) compiled with mingw for production purposes because pkcs11-tool.exe 0.13.0 crashes on enumerating certificates with new version of engine pkcs#11.
I'm using new versions compiled with MSVC 2012 only to test memory leaks.

@dwmw2
Copy link
Contributor Author

dwmw2 commented Aug 23, 2016

Thanks for the information. Without actually breaking your code above, my plan is to make most of it unnecessary. Especially the OpenSSL-specific horridness. You shouldn't need to use any of the CURLOPT_SSLENGINE, CURLOPT_SSLCERTTYPE, or CURLOPT_SSLKEYTYPE options, and usually not CURLOPT_SSLKEY either when it's the same as CURLOPT_SSLCERT. And nobody ever wanted to set CURLOPT_SSLENGINEDEFAULT anyway, AFAICT.

All you need to do is set CURLOPT_SSLCERT to a suitable PKCS#11 URI specifying the cert+key you want to use. On the command line, that means instead of

curl --engine pkcs11 --tlsv1.1 --key-type ENG --key d7f4b99792cc4dd708e408d3eb91b566e0a06c02 --cert-type ENG --cert d7f4b99792cc4dd708e408d3eb91b566e0a06c02 -k https://...:443/cepik

You'd have just

curl --tlsv1.1 --cert pkcs11:id=%d7%f4%b9%97%92%cc%4d%d7%08%e4%08%d3%eb%91%b5%66%e0%a0%6c%02 -k https://...:443/cepik

Note that the latter already works when curl is built against GnuTLS, and I've got patches outstanding to NSS which mean that it'll work with NSS too. And now I'll fix curl's use of OpenSSL so it's all consistent.

I am very interested in your comment about pkcs11-tool crashing. Note that it doesn't actually use the engine, or even libp11. It is, however, linked against its own version of OpenSC. What is the token you're using? Is it also OpenSC, and could it be a different version, so you have two different versions of the library being used at the same time? Could you file a separate ticket for that in the OpenSC repository please?

@nased0
Copy link

nased0 commented Aug 23, 2016

Hmm. I probably mismatched versions of opensc.dll and pkcs11-tool.exe in my system. I repaired it and can't repeat this crash.

@dwmw2
Copy link
Contributor Author

dwmw2 commented Aug 23, 2016

For reference and building test cases, here are a bunch of ways you can generate/mangle a key, all of which should Just Work™ when you give the filename to curl. Without having to take any additional information from the user...

PKCS#8 encrypted (PKCS#5 v2.0), HMAC-SHA1

openssl pkcs8 -topk8 -in key.pem -out key-pkcs8-crypt.pem -v2 aes258 -v2prf hmacWithSHA1
-----BEGIN ENCRYPTED PRIVATE KEY-----

PKCS#8 encrypted (PKCS#5 v2.0), HMAC-SHA256

openssl pkcs8 -topk8 -in key.pem -out key-pkcs8-crypt.pem -v2 aes258 -v2prf hmacWithSHA256
-----BEGIN ENCRYPTED PRIVATE KEY-----

PKCS#8 unencrypted

openssl pkcs8 -topk8 -in key.pem -nocrypt
-----BEGIN PRIVATE KEY-----

PKCS#1 unencrypted

openssl rsa -in key-pkcs8-crypt.pem -out key-pkcs1.pem
-----BEGIN RSA PRIVATE KEY----

PKCS#1 (OpenSSL encrypted)

openssl rsa -in key-pkcs8-crypt.pem -out key-pkcs1-crypted.pem -aes128
-----BEGIN RSA PRIVATE KEY-----
Proc-Type: 4,ENCRYPTED
DEK-Info: AES-128-CBC,03C0E0115419AD1EFE79C6B0D304A5EB

openssl rsa -in ke2.pem -out ke3.pem

PKCS#12

openssl pkcs12 -export -out foo.p12 -inkey foo -in cert -certfile ca.cert -chain

We can probably add a test which adds a checkHost option to the stunnel config, then validates that the client is connecting with the correct cert. And try each of the above options in -E arguments.

@dwmw2
Copy link
Contributor Author

dwmw2 commented Sep 26, 2016

Well, that quickly got silly. I accumulated some test cases, realised that my own project doesn't support them all, and went off to get my own house in order first... and also fixed some bugs in GnuTLS and OpenSSL which made it impossible to support them all.

So now I have a nice set of torture tests at http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/tests/Makefile.am which I assert that any well-behaved application capable of using client certificates SHOULD support. And a nascent Internet-Draft describing best practice to that effect: http://david.woodhou.se/draft-woodhouse-cert-best-practice.html

Feedback very much welcome; I will be coming back to prod at curl with a pointy stick later...

@stale stale bot added the stale label Apr 8, 2017
@stale
Copy link

stale bot commented Apr 8, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot closed this as completed Apr 29, 2017
@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

3 participants