Hi,
Thanks for the comments
On Wed, Jan 17, 2018 at 9:20 AM, Steffan Karger
<steffan.kar...@fox-it.com> wrote:
> Hi,
>
> Some preliminary comments:
>
> On 08-01-18 03:21, selva.n...@gmail.com wrote:
>> From: Selva Nair <selva.n...@gmail.com>
>>
>> - If an NCRYPT handle for the private key can be obtained, use
>> NCryptSignHash from the Cryptography NG API to sign the hash.
>>
>> This should work for all keys in the Windows certifiate stores
>> but may fail for keys in a legacy token, for example. In such
>> cases, we disable TLS v1.2 and fall back to the current
>> behaviour. A warning is logged unless TLS version is already
>> restricted to <= 1.1
>>
>>
.. snipped..
>> +static int
>> +priv_enc_CNG(const CAPI_DATA *cd, const unsigned char *from, int flen,
>> + unsigned char *to, int tlen, int padding)
>> +{
>> + NCRYPT_KEY_HANDLE hkey = cd->crypt_prov;
>> + DWORD len;
>> + ASSERT(cd->key_spec == CERT_NCRYPT_KEY_SPEC);
>> +
>> + msg(M_INFO, "Signing hash using CNG: data size = %d", flen);
>
> M_INFO is probably a bit verbose here.
Yeah, that was more for testing -- will use one of the TLS debug flags -- we
really need to make M_DEBUG print only at some high verbosity.
...
>> /* Unfortunately, there is no "CryptSign()" function in CryptoAPI, that
>> would
>> * be way to straightforward for M$, I guess... So we have to do it this
>> * tricky way instead, by creating a "Hash", and load the already-made
>> hash
>> @@ -322,7 +373,14 @@ finish(RSA *rsa)
>> }
>> if (cd->crypt_prov && cd->free_crypt_prov)
>> {
>> - CryptReleaseContext(cd->crypt_prov, 0);
>> + if (cd->key_spec == CERT_NCRYPT_KEY_SPEC)
>> + {
>> + NCryptFreeObject(cd->crypt_prov);
>> + }
>> + else
>> + {
>> + CryptReleaseContext(cd->crypt_prov, 0);
>> + }
>> }
>> if (cd->cert_context)
>> {
>> @@ -458,7 +516,9 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx,
>> const char *cert_prop)
>> }
>>
>> /* set up stuff to use the private key */
>> - if (!CryptAcquireCertificatePrivateKey(cd->cert_context,
>> CRYPT_ACQUIRE_COMPARE_KEY_FLAG,
>> + /* We prefer to get an NCRYPT key handle so that TLS1.2 can be
>> supported */
>> + DWORD flags =
>> CRYPT_ACQUIRE_COMPARE_KEY_FLAG|CRYPT_ACQUIRE_PREFER_NCRYPT_KEY_FLAG;
>> + if (!CryptAcquireCertificatePrivateKey(cd->cert_context, flags,
>> NULL, &cd->crypt_prov,
>> &cd->key_spec, &cd->free_crypt_prov))
>
> Some rewrapping of the arguments to no longer exceed 80 chars would be
> nice if you touch this anyway.
done :)
>
>> {
>> /* if we don't have a smart card reader here, and we try to access a
>> @@ -470,6 +530,17 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx,
>> const char *cert_prop)
>> /* here we don't need to do CryptGetUserKey() or anything; all
>> necessary key
>> * info is in cd->cert_context, and then, in cd->crypt_prov. */
>>
>> + /* If we do not have an NCRYPT key handle disable TLS v1.2 */
>> + if (cd->key_spec != CERT_NCRYPT_KEY_SPEC)
>> + {
>> + if (!(SSL_CTX_get_options(ssl_ctx) & SSL_OP_NO_TLSv1_2))
>> + {
>
> This no longer works for OpenSSL 1.1, we'll need the patch from
> https://patchwork.openvpn.net/patch/160/ (awaiting review), and then use
> the SSL_CTX_{get,set}_max_proto_version functions instead.
Good point.
Strangely enough it works in some cases -- I had tested with a non-NCRYPT key in
a legacy token and got restricted to TLS1.1. But point noted. And, anyway,
set_max_proto_version is better, as otherwise we'll have to touch this again
when TLS 1.3 is out.
Will take care of it in v2.
Selva
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
