Support external EC cert/key using --management-external-xxx

From: Selva Nair <[email protected]...>

- Move setting of key method callbacks into a function

No change in functionality.

From: Selva Nair <[email protected]...>

- This automatically supports EC certificates through --management-external-cert
- EC signature request from management has the same format as for rsa with >RSA_SIGN replaced by >ECDSA_SIGN Response should be of the form 'ecdsa-sig' followed by DER encoded signature as base64 followed by 'END'

static void

src/openvpn/manage.c +5 -0

+man_ecdsa_sig(struct management *man)
+{
+    struct man_connection *mc = &man->connection;
+    if (mc->ext_key_state == EKS_SOLICIT)
+    {

src/openvpn/ssl_openssl.c +13 -0

+#if OPENSSL_VERSION_NUMBER >= 0x10100001L && !defined(OPENSSL_NO_EC)
+
+/* called when EC_KEY is destroyed */
+static void
+openvpn_extkey_ec_finish(EC_KEY *ec)
+{
+    /* release the method structure */
+    const EC_KEY_METHOD *ec_meth = EC_KEY_get_method(ec);
+    EC_KEY_METHOD_free((EC_KEY_METHOD *) ec_meth);
+}
+
+/* EC_KEY_METHOD callback: sign().
+ * Sign the hash using EC key and return DER encoded signature in sig,

src/openvpn/ssl_openssl.c +26 -0

+
+    /* convert 'from' to base64 *
+    if (openvpn_base64_encode(dgst, dgstlen, &in_b64) <= 0)
+    {
+        goto done;
+    }
+
+    /* call MI for signature */
+    if (management)
+    {
+        out_b64 = management_query_ecdsa_sig(management, in_b64);
+    }
+    if (!out_b64)
+    {
+        goto done;
+    }
+
+    /* decode base64 signature to binary */
+    len = ECDSA_size(ec);
+    *siglen = openvpn_base64_decode(out_b64, sig, len);
+    if (*siglen > 0)
+    {
+        ret = 1;
+    }
+
+done:

src/openvpn/ssl_openssl.c +12 -0

+
+/* EC_KEY_METHOD callback: sign_setup(). We do no precomputations */
+static int
+ecdsa_sign_setup(EC_KEY *ec, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp)
+{
+    return 1;
+}
+
+/* EC_KEY_METHOD callback: sign_sig().
+ * Sign the hash and return the result as a newly allocated ECDS_SIG
+ * struct or NULL on error.
+ */

int
tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
const char *cert_file, const char *cert_file_inline)

src/openvpn/ssl_openssl.c +7 -0

@@ -1156,11 +1310,26 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
             goto err;
         }
     }
+#if OPENSSL_VERSION_NUMBER >= 0x10100001L && !defined(OPENSSL_NO_EC)
+    else if (EVP_PKEY_get0_EC_KEY(pkey))
+    {
+        if (!tls_ctx_use_external_ec_key(ctx, pkey))
+        {
+            goto err;
+        }

Am 14.01.18 um 20:44 schrieb [email protected]...:

Quoted 9 lines

From: Selva Nair <[email protected]...>

- This automatically supports EC certificates through --management-external-cert
- EC signature request from management has the same format as for rsa with >RSA_SIGN replaced by >ECDSA_SIGN Response should be of the form 'ecdsa-sig' followed by DER encoded signature as base64 followed by 'END'

Thanks for the code. I implemented and tested this on my Android client. It works very well. However I found some minor issues (see below).

Tested-by Arne Schwabe <[email protected]...>

But first a high level issue. I think if you enable manange-external-key with an "old" ui that only understand rsa keys the OpenVPN client will hang after ECDSA prompt since the ui will never reply with anything.

We might need an argument to management-external-key to say "yes I am ecdsa ready".

Quoted 12 lines

static void

src/openvpn/manage.c +10 -0

+man_ecdsa_sig(struct management *man)
+{
+    struct man_connection *mc = &man->connection;
+    if (mc->ext_key_state == EKS_SOLICIT)
+    {
+        mc->ext_key_state = EKS_INPUT;
+        mc->in_extra_cmd = IEC_ECDSA_SIGN;
+        in_extra_reset(mc, IER_NEW);
+    }
+    else

src/openvpn/manage.c man_rsa_sig(struct management *man) · line 1139

Quoted 5 lines

src/openvpn/manage.c +5 -0

+    {
+        msg(M_CLIENT, "ERROR: The ecdsa-sig command is not currently available");
+    }
+}
+

This function is almost identical to man_rsa_sign. I would like to have them both combined into one and then called by man_ecdsa_sig/man_rsa_sig.

src/openvpn/manage.c management_query_rsa_sig(struct management *man, · line 3687

Quoted 10 lines

src/openvpn/manage.c +7 -0

+char *
+management_query_ecdsa_sig(struct management *man,
+                           const char *b64_data)
+{
+    return management_query_multiline_flatten(man, b64_data, "ECDSA_SIGN", "ecdsa-sign",
+                                              &man->connection.ext_key_state, &man->connection.ext_key_input);
+}

Hm, could be merged with management_query_rsa_sig but since it a one liner I have no strong preference.

Quoted 13 lines

src/openvpn/ssl_openssl.c +13 -0

+#if OPENSSL_VERSION_NUMBER >= 0x10100001L && !defined(OPENSSL_NO_EC)
+
+/* called when EC_KEY is destroyed */
+static void
+openvpn_extkey_ec_finish(EC_KEY *ec)
+{
+    /* release the method structure */
+    const EC_KEY_METHOD *ec_meth = EC_KEY_get_method(ec);
+    EC_KEY_METHOD_free((EC_KEY_METHOD *) ec_meth);
+}
+
+/* EC_KEY_METHOD callback: sign().
+ * Sign the hash using EC key and return DER encoded signature in sig,

src/openvpn/ssl_openssl.c err: · line 1157

Quoted 10 lines

src/openvpn/ssl_openssl.c +10 -0

+ * its length in siglen. Return value is 1 on success, 0 on error.
+ */
+static int
+ecdsa_sign(int type, const unsigned char *dgst, int dgstlen, unsigned char *sig,
+           unsigned int *siglen, const BIGNUM *kinv, const BIGNUM *r, EC_KEY *ec)
+{
+    char *in_b64 = NULL;
+    char *out_b64 = NULL;
+    int ret = 0;
+    int len;

len could just as well be done als inline C99 decleration. But no problem. (Also I am not so firm on our new style here)

Quoted 33 lines

src/openvpn/ssl_openssl.c +33 -0

+
+    /* convert 'from' to base64 *
+    if (openvpn_base64_encode(dgst, dgstlen, &in_b64) <= 0)
+    {
+        goto done;
+    }
+
+    /* call MI for signature */
+    if (management)
+    {
+        out_b64 = management_query_ecdsa_sig(management, in_b64);
+    }
+    if (!out_b64)
+    {
+        goto done;
+    }
+
+    /* decode base64 signature to binary */
+    len = ECDSA_size(ec);
+    *siglen = openvpn_base64_decode(out_b64, sig, len);
+    if (*siglen > 0)
+    {
+        ret = 1;
+    }
+
+done:
+    if (in_b64)
+    {
+        free(in_b64);
+    }
+    if (out_b64)
+    {
+        free(out_b64);

src/openvpn/ssl_openssl.c err: · line 1193

Quoted 3 lines

src/openvpn/ssl_openssl.c +3 -0

+    }
+    return ret;
+}

This method has a bit code duplication to rsa_priv_enc but I think it is still okay here since you have an rather ugly looking function that is hard to read.

Quoted 18 lines

src/openvpn/ssl_openssl.c +17 -0

+
+/* EC_KEY_METHOD callback: sign_setup(). We do no precomputations */
+static int
+ecdsa_sign_setup(EC_KEY *ec, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp)
+{
+    return 1;
+}
+
+/* EC_KEY_METHOD callback: sign_sig().
+ * Sign the hash and return the result as a newly allocated ECDS_SIG
+ * struct or NULL on error.
+ */
+static ECDSA_SIG *
+ecdsa_sign_sig(const unsigned char *dgst, int dgstlen, const BIGNUM *in_kinv,
+               const BIGNUM *in_r, EC_KEY *ec)
+{
+    ECDSA_SIG *ecsig = NULL;

src/openvpn/ssl_openssl.c err: · line 1215

Quoted 5 lines

src/openvpn/ssl_openssl.c +5 -0

+    int len = ECDSA_size(ec);
+    struct gc_arena gc = gc_new();
+
+    unsigned char *buf = gc_malloc(len, false, &gc);
+    if (1 != ecdsa_sign(0, dgst, dgstlen, buf, &len, NULL, NULL, ec))

I am not sure about 1!= style here. Might be okay. I would try to avoid this.

len should be size_t. My compiler agrees:

ssl_openssl.c:1215:48: warning: passing 'int *' to parameter of type 'unsigned int *' converts between pointers to integer types with different sign [-Wpointer-sign]

    if (1 != ecdsa_sign(0, dgst, dgstlen, buf, &len, NULL, NULL, ec))
                                               ^~~~

ssl_openssl.c:1152:26: note: passing argument to parameter 'siglen' here
unsigned int *siglen, const BIGNUM *kinv, const BIGNUM *r,
EC_KEY *ec)

                         ^

1 warning generated.

Quoted 14 lines

int
tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
const char *cert_file, const char *cert_file_inline)

src/openvpn/ssl_openssl.c +7 -0

@@ -1156,11 +1310,26 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
             goto err;
         }
     }
+#if OPENSSL_VERSION_NUMBER >= 0x10100001L && !defined(OPENSSL_NO_EC)
+    else if (EVP_PKEY_get0_EC_KEY(pkey))
+    {
+        if (!tls_ctx_use_external_ec_key(ctx, pkey))
+        {
+            goto err;
+        }

src/openvpn/ssl_openssl.c tls_ctx_use_external_private_key(struct tls_root_ctx *ctx, · line 1329

Quoted 10 lines

src/openvpn/ssl_openssl.c +7 -0

+    }
+    else
+    {
+        crypto_msg(M_WARN, "management-external-key requires an RSA or EC certificate");
+        goto err;
+    }
+#else
     else
     {
         crypto_msg(M_WARN, "management-external-key requires a RSA certificate");

If you touch that line you can as well fix the grammar in the existing string that you fixed in the new string :)

Arne

Quoted 11 lines

Am 14.01.18 um 20:44 schrieb [email protected]...:

Quoted 9 lines

From: Selva Nair <[email protected]...>

- This automatically supports EC certificates through --management-external-cert
- EC signature request from management has the same format as for rsa with >RSA_SIGN replaced by >ECDSA_SIGN Response should be of the form 'ecdsa-sig' followed by DER encoded signature as base64 followed by 'END'

Quoted 12 lines

Quoted 12 lines

static void

src/openvpn/manage.c +10 -0

+man_ecdsa_sig(struct management *man)
+{
+    struct man_connection *mc = &man->connection;
+    if (mc->ext_key_state == EKS_SOLICIT)
+    {
+        mc->ext_key_state = EKS_INPUT;
+        mc->in_extra_cmd = IEC_ECDSA_SIGN;
+        in_extra_reset(mc, IER_NEW);
+    }
+    else

Quoted 8 lines

src/openvpn/manage.c man_rsa_sig(struct management *man) · line 1139

Quoted 5 lines

src/openvpn/manage.c +5 -0

+    {
+        msg(M_CLIENT, "ERROR: The ecdsa-sig command is not currently available");
+    }
+}
+

This function is almost identical to man_rsa_sign. I would like to have them both combined into one and then called by man_ecdsa_sig/man_rsa_sig.

Also I'm toying with the idea of renaming ecdsa-sig/ECDSA-SIGN by pkey-sig/PKEY-SIGN so that eventually we may be able to use it for all types of keys and retire rsa-sig. Any thoughts on that?

Hi,

Quoted 3 lines · Selva Nair

Also I'm toying with the idea of renaming ecdsa-sig/ECDSA-SIGN by pkey-sig/PKEY-SIGN so that eventually we may be able to use it for all types of keys and retire rsa-sig. Any thoughts on that?

This was my first though when looking at these patches. Haven't looked at the code yet, but functionally I think that's the way to go. Even if it were that Ed25519 / Ed448 sigs are technically not ECDSA sigs.

RSA_SIGN client responds

RSA_SIGN. As a not so nice side effect, the daemon will reply with

PK_SIGN,xxx,<base64>

Hi

On Wed, Jan 17, 2018 at 3:53 AM, Steffan Karger <[email protected]... <mailto:[email protected]...>> wrote:

Hi,

On 17-01-18 05:24, Selva Nair wrote:
> Also I'm toying with the idea of renaming ecdsa-sig/ECDSA-SIGN by > pkey-sig/PKEY-SIGN so that eventually we may be able to use it for > all types of keys and retire rsa-sig. Any thoughts on that?

This was my first though when looking at these patches.  Haven't looked at the code yet, but functionally I think that's the way to go.  Even if it were that Ed25519 / Ed448 sigs are technically not ECDSA sigs.

I was waiting for the chatlog before finalizing v2. Looks like there are a number of opinions
that seem to converge towards the same view.

This is what I have in my current v2 ready to go out:

(i) Current behaviour for rsa signatures stays for now: daemon sends

Quoted 1 line

RSA_SIGN  client responds

with rsa-sig
(ii) Daemon sends >PK_SIG client responds with pk-sig [*] Initially this will be used only for EC, but trivial to switch RSA to this in future.
So document that new mgmt clients should be prepared to handle all supported key types via >PK_SIGN/pk-sig and >RSA_SIGN will be eventually removed.

As a nice side effect, clients are allowed to respond with pk-sig even if the challenge was

Quoted 1 line

RSA_SIGN. As a not so nice side effect, the daemon will reply with "SUCCESS

rsa-sign
succeeded" because that's what it asked for -- I suppose we can live with that.

We can deprecate and remove >RSA_SIGN at a later date by a hard switch when all known clients are capable of handling >PK_SIGN

Does this sound like an acceptable solution?

Question: while at it should we alter the syntax to

Quoted 1 line

PK_SIGN,xxx,<base64>

where xxx will be empty for now, but it reserves space for extra info that may come handy for some future signature types (like use PSS padding for RSA or use PureEdDSA or whatever).

Quoted 41 lines · Selva Nair

Hi

On Wed, Jan 17, 2018 at 3:53 AM, Steffan Karger <[email protected]... <mailto:[email protected]...>> wrote:

Hi,

On 17-01-18 05:24, Selva Nair wrote:
> Also I'm toying with the idea of renaming ecdsa-sig/ECDSA-SIGN by > pkey-sig/PKEY-SIGN so that eventually we may be able to use it for > all types of keys and retire rsa-sig. Any thoughts on that?

This was my first though when looking at these patches. Haven't looked at the code yet, but functionally I think that's the way to go. Even if it were that Ed25519 / Ed448 sigs are technically not ECDSA sigs.

I was waiting for the chatlog before finalizing v2. Looks like there are a number of opinions
that seem to converge towards the same view.

This is what I have in my current v2 ready to go out:

(i) Current behaviour for rsa signatures stays for now: daemon sends

Quoted 1 line

RSA_SIGN client responds

with rsa-sig
(ii) Daemon sends >PK_SIG client responds with pk-sig [*] Initially this will be used only for EC, but trivial to switch RSA to this in future.
So document that new mgmt clients should be prepared to handle all supported key types via >PK_SIGN/pk-sig and >RSA_SIGN will be eventually removed.

As a nice side effect, clients are allowed to respond with pk-sig even if the challenge was

Quoted 1 line

RSA_SIGN. As a not so nice side effect, the daemon will reply with "SUCCESS

rsa-sign
succeeded" because that's what it asked for -- I suppose we can live with that.

We can deprecate and remove >RSA_SIGN at a later date by a hard switch when all known clients are capable of handling >PK_SIGN

Quoted 8 lines

Does this sound like an acceptable solution?

Question: while at it should we alter the syntax to

Quoted 1 line

PK_SIGN,xxx,<base64>

where xxx will be empty for now, but it reserves space for extra info that may come handy for some future signature types (like use PSS padding for RSA or use PureEdDSA or whatever).

Hi,

I've discussed this quickly with James quite recently. We are concerned about breaking existing front-end integrations by doing too much funky stuff with RSA_SIGN for EC support. That in particular touches the signal renaming to PK_SIGN.

So we have another suggestion. For external keys to be used, --management-external-key is needed. Add an optional argument to this option, which indicates a version. If this argument is not provided, everything works as it is today - without EC support. If using: --management-external-key 2 ... for version 2, do a hard-shift to PK_SIGN with RSA and EC support.

This way we ensure we don't break any existing implementations, we can allow a period of transition to the new version, and we can in the future when we are sure PK_SIGN works well and has seen deployment elsewhere start a deprecation process if the legacy version 1. But we're not put in a situation where anything changes suddenly and abruptly.

Any thoughts?

- Present patch: connection process appears stuck (but UI is still responsive) and logs show the daemon is waiting for signature

- This proposal: connection fails with: "External EC cert/key not supported in this config. Try using --management-external-key 2" User edits the config option and the connection process appears stuck .....

I suppose the latter is a bit better.

Quoted 7 lines · Selva Nair

- Present patch: connection process appears stuck (but UI is still responsive) and logs show the daemon is waiting for signature

- This proposal: connection fails with: "External EC cert/key not supported in this config. Try using --management-external-key 2" User edits the config option and the connection process appears stuck .....

Quoted 1 line

I suppose the latter is a bit better.

Well, it should probably be tweaked slightly better. First of all, if run via a GUI front-end, it's the GUI which should set this argument. We could consider to add a "fail-safe" on this option, so once set - it can't be changed again. The more advanced rabbit whole fix would be that the command line provided --management-external-key option overrides whatever is in the configuration file; doing this will require more work though.

This will make the VPN connection will still fail, but it won't be stuck any more. The user may complain "but I did add that option!?" which then is a better starting point for support cases ... "Yes, it is most likely ignored as your user interface is not capable of this feature".

Another alternative is to extend an already longer error log entry, by mentioning "also ensure that your management interface front-end supports version 2."

The client could do that when it gets the version message or any time later.

Hi,

Quoted 8 lines · David Sommerseth

Quoted 7 lines · Selva Nair

- Present patch: connection process appears stuck (but UI is still responsive) and logs show the daemon is waiting for signature

- This proposal: connection fails with: "External EC cert/key not supported in this config. Try using --management-external-key 2" User edits the config option and the connection process appears stuck .....

Quoted 8 lines

Quoted 1 line

I suppose the latter is a bit better.

Well, it should probably be tweaked slightly better. First of all, if run via a GUI front-end, it's the GUI which should set this argument. We could consider to add a "fail-safe" on this option, so once set - it can't be changed again. The more advanced rabbit whole fix would be that the command line provided --management-external-key option overrides whatever is in the configuration file; doing this will require more work though.

Problem with GUI specifying the option is that it has to first check whether the config uses management-external-key (this option interferes with --key, --pkcs12 etc.). Probably not an issue with most UI's out there but Windows GUI doesn't currently parse the config at all. That said, Windows GUI doesn't support management-external-key so that's not a strong argument.

Quoted 9 lines

This will make the VPN connection will still fail, but it won't be stuck any more. The user may complain "but I did add that option!?" which then is a better starting point for support cases ... "Yes, it is most likely ignored as your user interface is not capable of this feature".

Another alternative is to extend an already longer error log entry, by mentioning "also ensure that your management interface front-end supports version 2."

What about extending the current "version" command with an argument where the client states the version of "management-speak" that it supports. Current management version is 1, we increase it to 1.1 and unless the client says "version 1.1" or more we do not send PK_SIGN. The client could do that when it gets the version message or any time later. The response to version command (current management version and openvpn daemon's version stays the same). No full-fledged cap negotiation, but good enough.

The UX would be much better that way.

What about extending the current "version" command with an argument where the client states the version of "management-speak" that it supports. Current management version is 1, we increase it to 1.1 and unless the client says "version 1.1" or more we do not send PK_SIGN. The client could do that when it gets the version message or any time later. The response to version command (current management version and openvpn daemon's version stays the same). No full-fledged cap negotiation, but good enough.

The UX would be much better that way.

Hi.

Quoted 8 lines · Selva Nair

What about extending the current "version" command with an argument where the client states the version of "management-speak" that it supports. Current management version is 1, we increase it to 1.1 and unless the client says "version 1.1" or more we do not send PK_SIGN. The client could do that when it gets the version message or any time later. The response to version command (current management version and openvpn daemon's version stays the same). No full-fledged cap negotiation, but good enough.

That sounds reasonable; easy to implement in Tunnelblick

Quoted 1 line

The UX would be much better that way.

Absolutely.

Hi,

Quoted 15 lines · Jonathan K. Bullard

Hi.

Quoted 8 lines · Selva Nair

What about extending the current "version" command with an argument where the client states the version of "management-speak" that it supports. Current management version is 1, we increase it to 1.1 and unless the client says "version 1.1" or more we do not send PK_SIGN. The client could do that when it gets the version message or any time later. The response to version command (current management version and openvpn daemon's version stays the same). No full-fledged cap negotiation, but good enough.

That sounds reasonable; easy to implement in Tunnelblick

Quoted 4 lines

Quoted 1 line

The UX would be much better that way.

Absolutely.

Encouraged by Jonathan's reply I have made a patch to rename RSA_SIGN to PK_SIGN if client announces a version > 1. Will send it and a modified EC key patch soon.

Quoted 24 lines · Selva Nair

Hi,

Quoted 19 lines · Jonathan K. Bullard

Hi.

Quoted 8 lines · Selva Nair

What about extending the current "version" command with an argument where the client states the version of "management-speak" that it supports. Current management version is 1, we increase it to 1.1 and unless the client says "version 1.1" or more we do not send PK_SIGN. The client could do that when it gets the version message or any time later. The response to version command (current management version and openvpn daemon's version stays the same). No full-fledged cap negotiation, but good enough.

That sounds reasonable; easy to implement in Tunnelblick

Quoted 1 line

The UX would be much better that way.

Absolutely.

Quoted 3 lines

Encouraged by Jonathan's reply I have made a patch to rename RSA_SIGN to PK_SIGN if client announces a version > 1. Will send it and a modified EC key patch soon.

Sounds good! Just one question ... any reason why to complicate the version number with decimals? Why not just version 1, 2,...X? Coding wise, this is much easier too, as we don't need to do more complicated conversion steps from strings to float.

PK_SIGN if the client supports it (or >RSA_SIGN)

From: Selva Nair <[email protected]...>

- This automatically supports EC certificates through --management-external-cert
- EC signature request from management is prompted by

Quoted 1 line

PK_SIGN if the client supports it (or >RSA_SIGN)

Response should be of the form 'pk-sig' (or rsa-sig by older clients) followed by DER encoded signature as base64 terminated by 'END' on a new line.

Am 25.01.18 um 20:45 schrieb [email protected]...:

Quoted 11 lines

From: Selva Nair <[email protected]...>

- This automatically supports EC certificates through --management-external-cert
- EC signature request from management is prompted by

Quoted 1 line

PK_SIGN if the client supports it (or >RSA_SIGN)

Response should be of the form 'pk-sig' (or rsa-sig by older clients) followed by DER encoded signature as base64 terminated by 'END' on a new line.

Acked-by Arne Schwabe <[email protected]...>

Ack from my side. Note that this patch will ask for a ec signatre with > RSA-SIGN when the user has put a EC certificate in the config and the ui does not send "version 2". I am not sure that we need to address this tough. That kind of configuration will break anyway. It just now does at a different point. And putting an EC certificate is not something that should happen on accident.

Your patch has been applied to the master branch.

(I have not done any real review on it, except "compile test" and "stare a bit at the code for really obvious issues" - trusting Arne since his GUI seems to be the only thing really using it today, so if he's happy, I am as well)

commit 7eca140c70ff76177371dc94c19aeb8644c2c3b5

Author: Selva Nair
Date:   Thu Jan 25 14:45:13 2018 -0500

Allow external EC key through --management-external-key

Hi,

Quoted 7 lines · Gert Doering

Your patch has been applied to the master branch.

(I have not done any real review on it, except "compile test" and "stare a bit at the code for really obvious issues" - trusting Arne since his GUI seems to be the only thing really using it today, so if he's happy, I am as well)

Quoted 5 lines

commit 7eca140c70ff76177371dc94c19aeb8644c2c3b5

Author: Selva Nair
Date:   Thu Jan 25 14:45:13 2018 -0500

Allow external EC key through --management-external-key

*sigh*

Today's the day of buildbot exploisions.

*This* one breaks LibreSSL compilation (namely, the OpenBSD buildbot), because

ssl_openssl.c: In function 'tls_ctx_use_external_ec_key':
ssl_openssl.c:1229: error: 'EC_KEY_METHOD' undeclared (first use in this function) ssl_openssl.c:1229: error: 'ec_method' undeclared (first use in this function) ssl_openssl.c:1233: warning: implicit declaration of function 'EC_KEY_METHOD_new' ssl_openssl.c:1233: warning: implicit declaration of function 'EC_KEY_OpenSSL' ssl_openssl.c:1240: warning: implicit declaration of function 'EC_KEY_METHOD_set_init' ssl_openssl.c:1241: warning: implicit declaration of function 'EC_KEY_METHOD_set_sign' ssl_openssl.c:1249: warning: implicit declaration of function 'EC_KEY_set_method'

So we need another "&& !defined(LIBRESSL)" here, I think.

(And this, dear OpenBSD folks, is why application developers are just a tiny little bit unhappy with the way LibreSSL claims OpenSSL "recent API" compatibility by means of OPENSSL_VERSION but then... doesn't)

Quoted 2 lines

Quoted 2 lines

*This* one breaks LibreSSL compilation (namely, the OpenBSD buildbot), because

Tested using the freebsd-11 box in openvpn-vagrant and did pkg install libressl-2.6.4 (that replaces openssl 1.1.0).

This particular error gets fixed by adding
!defined(LIBRESSL_VERSION_NUMBER) in two places.

I can send a patch for this.

But there are other errors:

In file included from crypto_openssl.c:44:
./openssl_compat.h:699:1: error: static declaration of 'SSL_CTX_set_min_proto_version' follows

./openssl_compat.h:728:1: error: static declaration of 'SSL_CTX_set_max_proto_version' follows
non-static declaration

In openssl those are macros but not in libressl so the current openssl_compat.h defs don't work.

Hi,

Quoted 2 lines · Selva Nair

Quoted 2 lines

Quoted 2 lines

*This* one breaks LibreSSL compilation (namely, the OpenBSD buildbot), because

[..]

Quoted 7 lines

Tested using the freebsd-11 box in openvpn-vagrant and did pkg install libressl-2.6.4 (that replaces openssl 1.1.0).

This particular error gets fixed by adding
!defined(LIBRESSL_VERSION_NUMBER) in two places.

I can send a patch for this.

This would be appreciated (please --cc me directly as the list is still not working properly).

Quoted 5 lines

But there are other errors:

In file included from crypto_openssl.c:44:
./openssl_compat.h:699:1: error: static declaration of 'SSL_CTX_set_min_proto_version' follows

[..]

Quoted 3 lines

./openssl_compat.h:728:1: error: static declaration of 'SSL_CTX_set_max_proto_version' follows
non-static declaration

[..]

Quoted 2 lines

In openssl those are macros but not in libressl so the current openssl_compat.h defs don't work.

This is interesting. Seems my OpenBSD version is too old, as I've not seen these errors in my buildbots.

From: Selva Nair <[email protected]...>

Signed-off-by Selva Nair <[email protected]...>

---
 doc/management-notes.txt | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

doc/management-notes.txt +17 -0

index a9ba18a..e2e8249 100644
@@ -795,6 +795,36 @@ Base64 encoded output of RSA_private_encrypt() (OpenSSL) or mbedtls_pk_sign()
 This capability is intended to allow the use of arbitrary cryptographic
 service providers with OpenVPN via the management interface.
 
+COMMAND -- ecdsa-sig (OpenVPN 2.5 or higher)
+------------------------------------------
+Same as rsa-sig but for EC keys: requires openssl 1.1
+
+Provides support for external storage of the EC private key. Requires the
+--management-external-key option. This option can be used instead of "key"
+in client mode, and allows the client to run without the need to load the
+actual private key. When the SSL protocol needs to perform a sign
+operation, the data to be signed will be sent to the management interface
+via a notification as follows:
+
+>ECDSA_SIGN:[BASE64_DATA]
+
+The management interface client should then create a DER encoded signature of
+the (decoded) BASE64_DATA using the private key and return the SSL signature as
+follows:
+

Am 14.01.18 um 20:44 schrieb [email protected]...:

Quoted 38 lines

From: Selva Nair <[email protected]...>

Signed-off-by Selva Nair <[email protected]...>

---
 doc/management-notes.txt | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

doc/management-notes.txt +23 -0

index a9ba18a..e2e8249 100644
@@ -795,6 +795,36 @@ Base64 encoded output of RSA_private_encrypt() (OpenSSL) or mbedtls_pk_sign()
 This capability is intended to allow the use of arbitrary cryptographic
 service providers with OpenVPN via the management interface.
 
+COMMAND -- ecdsa-sig (OpenVPN 2.5 or higher)
+------------------------------------------
+Same as rsa-sig but for EC keys: requires openssl 1.1
+
+Provides support for external storage of the EC private key. Requires the
+--management-external-key option. This option can be used instead of "key"
+in client mode, and allows the client to run without the need to load the
+actual private key. When the SSL protocol needs to perform a sign
+operation, the data to be signed will be sent to the management interface
+via a notification as follows:
+
+>ECDSA_SIGN:[BASE64_DATA]
+
+The management interface client should then create a DER encoded signature of
+the (decoded) BASE64_DATA using the private key and return the SSL signature as
+follows:
+
+ecdsa-sig
+[BASE64_SIG_LINE]
+.
+.
+.
+END

doc/management-notes.txt Base64 encoded output of RSA_private_encrypt() (OpenSSL) or mbedtls_pk_sign() · line 824

Quoted 4 lines

doc/management-notes.txt +4 -0

+
+Base64 encoded output of ECDSA_sign() (OpenSSL) or mbedtls_pk_sign()
+(mbed TLS) will provide a correct signature.
+

Signature.getInstance("NONEwithECDSA") worked for me in Java for this. Any other signature algorithm did _NOT_ work e.g. SHA384withECDSA. I think ecdsa-sign might already provided a hash to sign.

On the other hand mbedtls documentation states:

For RSA, md_alg may be MBEDTLS_MD_NONE if hash_len != 0. For ECDSA md_alg may never be MBEDTLS_MD_NONE.

So this interface might not work with mbedtls.

FWIW, some remarks on the hash and the signature below.

Quoted 39 lines · Arne Schwabe

Am 14.01.18 um 20:44 schrieb [email protected]...:

Quoted 38 lines

From: Selva Nair <[email protected]...>

Signed-off-by Selva Nair <[email protected]...>

---
 doc/management-notes.txt | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

doc/management-notes.txt +23 -0

index a9ba18a..e2e8249 100644
@@ -795,6 +795,36 @@ Base64 encoded output of RSA_private_encrypt() (OpenSSL) or mbedtls_pk_sign()
 This capability is intended to allow the use of arbitrary cryptographic
 service providers with OpenVPN via the management interface.
 
+COMMAND -- ecdsa-sig (OpenVPN 2.5 or higher)
+------------------------------------------
+Same as rsa-sig but for EC keys: requires openssl 1.1
+
+Provides support for external storage of the EC private key. Requires the
+--management-external-key option. This option can be used instead of "key"
+in client mode, and allows the client to run without the need to load the
+actual private key. When the SSL protocol needs to perform a sign
+operation, the data to be signed will be sent to the management interface
+via a notification as follows:
+
+>ECDSA_SIGN:[BASE64_DATA]
+
+The management interface client should then create a DER encoded signature of
+the (decoded) BASE64_DATA using the private key and return the SSL signature as
+follows:
+
+ecdsa-sig
+[BASE64_SIG_LINE]
+.
+.
+.
+END

Quoted 8 lines

doc/management-notes.txt Base64 encoded output of RSA_private_encrypt() (OpenSSL) or mbedtls_pk_sign() · line 824

Quoted 4 lines

doc/management-notes.txt +4 -0

+
+Base64 encoded output of ECDSA_sign() (OpenSSL) or mbedtls_pk_sign()
+(mbed TLS) will provide a correct signature.
+

Signature.getInstance("NONEwithECDSA") worked for me in Java for this. Any other signature algorithm did _NOT_ work e.g. SHA384withECDSA. I think ecdsa-sign might already provided a hash to sign.

Yes, the hash is what is provided in this signature request. In this particular case,
the data is handshake history and may not be even saved. Its the accumulated hash that is passed in by the SSL/TLS library to the callback.

So "NONEwithECDSA" should be the right choice for Java (Sun/Oracle) -- or ECDSAforSSL in case of IBM Java.

Quoted 5 lines

On the other hand mbedtls documentation states:

For RSA, md_alg may be MBEDTLS_MD_NONE if hash_len != 0. For ECDSA md_alg may never be MBEDTLS_MD_NONE.

External RSA keys currently work with both openssl and mbedtls.

To elaborate, for RSA, its more complicated as the hash to be encoded with the OID of the hash algo prepended to it as specified in PKCS1 v1.5, then padded and signed. Unless the pre TLS1.2 MD5+SHA1 mixed hash is in use. In the latter case only padding is needed and that's the case where mbedtls specifies hash type as none.

For RSA, openssl callback gets the encoded hash (not padded) and signing involves padding + encryption only (hash type is not needed). But mbedtls provides the hash and hash algorithm and the signing routine has to encode it. However, the pkcs1 v1.5 encoding part is already present in ssl_mbedtls.c (implemented by Stefan, I suppose) so what the management interface gets is the same data for both openssl and mbedtls builds as far as RSA is concerned.

Quoted 1 line

So this interface might not work with mbedtls.

It should work if only we could find a way to set a callback for this in mbedtls.
It seems they provide that facility only for RSA keys. Stefan may know. Not sure how pkcs11-helper hooks on to it -- does it support EC keys?

From: Selva Nair <[email protected]...>

Signed-off-by Selva Nair <[email protected]...>

---
 doc/management-notes.txt | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)