Add support for OpenSSL TLS 1.3 when using management-external-key

For TLS 1.0 to 1.2 OpenSSL calls us and requires a PKCS1 padded response, for TLS 1.3 it requires to an unpadded response. Since we can PCKS1 pad an unpadded response, we prefer to always query for an unpadded response from the management interface and add the PCKS1 padding ourselves when needed.

This patch adds an 'unpadded' parameter to the management-external-key option to signal that it is uses the new unpadded API. Since we cannot

support TLS 1.3 without unpadded queries we disable TLS 1.3 otherwise.

We also do the same for cryptoapi since it uses the same API.

Using the management api client version instead might seem like the more logical way but since we only now that version very late, it would extra logic and complexity to deal with this asynchronous behaviour .

Hi,

Not a review, but some thoughts:

Quoted 6 lines · Arne Schwabe

For TLS 1.0 to 1.2 OpenSSL calls us and requires a PKCS1 padded response, for TLS 1.3 it requires to an unpadded response. Since we can PCKS1 pad an unpadded response, we prefer to always query for an unpadded response from the management interface and add the PCKS1 padding ourselves when needed.

First, one clarification:

The reason for unpadded response is that when PSS padding is in use, OpenSSL adds the padding and, thus, asks for raw RSA encryption (unpadded). This is because RSA_sign or rsa_priv_enc callbacks do not support a way of indicating PSS padding. In fact, OpenSSL 1.1.1 client talking to 1.1.1 server prefers PSS padding even for TLS 1.2, so this is not a TLS 1.3-only problem.

Quoted 2 lines

This patch adds an 'unpadded' parameter to the management-external-key option to signal that it is uses the new unpadded API. Since we cannot

I do not particularly like this new option:
"management-external-key-unpadded". Apart from the confusing name, this doesn't really address the basic limitation of the current pkey-sig (old rsa-sig) directive. We should have extended it when the new name was introduced. pkey-sig should pass not just the data (or hash) to sign but additional info like digest type in use, padding mode etc. Or have a way for the management client to query additional parameters that may be necessary for signing.

Using padded data and requiring the external "engine" to do a raw signature may look like a way out of this but not really: not all external libraries may support such a usage. AFAICS, Windows CNG does not. Also some hardware devices may insist on doing the padding inside -- especially non-deterministic padding like PSS.

So what about naming this "management-external-key-version2" and then extend pk-sig as below?

pk-sig <base64-data-to-sign> [signing-algorithm]

where the <signing algorithm> could be, say, SHA256_RSA_PKCS1 or NONE_RSA_PKCS1 or SHA384_RSA_PSS etc. If missing, and the key is RSA, the client can assume it is NONE_RSA_PKCS1 and expect the hash with digest-info header already added and sign as such. Or, if the key is EC, do the ECDSA signing operation as is being done now.

The "unpadded" case could be indicated by NONE_RSA_RAW. These mnemonics are just examples -- we could use PKCS11 identifiers or something else.

To get unpadded data we have to override sign() in EVP_PKEY_METHOD but that would also provide max flexibility.

Now, I think its safe to just update and document this and require that management-interface clients that use the external key feature must update if using client compiled with OpenSSL 1.1.1. Android(?) and iOS (?) clients may be the only one's affected, but those are shipped as one complete package. I'm not aware of any standalone management-client UI that uses the external-key feature.

Quoted 1 line

support TLS 1.3 without unpadded queries we disable TLS 1.3 otherwise.

If we must do this, disabling TLS 1.3 will not enough. One way is to also explicitly set the padding mode to RSA_PKCS1_PADDING (for TLS1.2)

EVP_PKEY_CTX_set_rsa_padding(ctx, pad) can do this.

Quoted 3 lines

We also do the same for cryptoapi since it uses the same API.

Let's just fix cryptoapi --- I'm working on it. Anyway it does
not use the same API as external key and prepadded data (= nopadding) will not work with Windows. This'll have to be fixed before we release binaries linked with OpenSSL 1.1.1.

Quoted 6 lines

Using the management api client version instead might seem like the more logical way but since we only now that version very late, it would extra logic and complexity to deal with this asynchronous behaviour .

IMO, we should explore this further and try to avoid --management-external-key-foo.

Am 17.10.18 um 05:15 schrieb Selva Nair:

Quoted 13 lines

Hi,

Not a review, but some thoughts:

Quoted 6 lines · Arne Schwabe

For TLS 1.0 to 1.2 OpenSSL calls us and requires a PKCS1 padded response, for TLS 1.3 it requires to an unpadded response. Since we can PCKS1 pad an unpadded response, we prefer to always query for an unpadded response from the management interface and add the PCKS1 padding ourselves when needed.

Quoted 8 lines

First, one clarification:

The reason for unpadded response is that when PSS padding is in use, OpenSSL adds the padding and, thus, asks for raw RSA encryption (unpadded). This is because RSA_sign or rsa_priv_enc callbacks do not support a way of indicating PSS padding. In fact, OpenSSL 1.1.1 client talking to 1.1.1 server prefers PSS padding even for TLS 1.2, so this is not a TLS 1.3-only problem.

Ah did not get that extra problem.

Quoted 11 lines

Quoted 2 lines

This patch adds an 'unpadded' parameter to the management-external-key option to signal that it is uses the new unpadded API. Since we cannot

I do not particularly like this new option:
"management-external-key-unpadded". Apart from the confusing name, this doesn't really address the basic limitation of the current pkey-sig (old rsa-sig) directive. We should have extended it when the new name was introduced. pkey-sig should pass not just the data (or hash) to sign but additional info like digest type in use, padding mode etc. Or have a way for the management client to query additional parameters that may be necessary for signing.

Yes. But we still need a way to signal the capabilities of the management client to OpenVPN, otherwise we will query for a signature that we might not support. How about adding the supported signatures as parameters?

E.g. management-external-key pkcs1 raw pss

pk-sig <data> RSA-RAW

Quoted 7 lines

Using padded data and requiring the external "engine" to do a raw signature may look like a way out of this but not really: not all external libraries may support such a usage. AFAICS, Windows CNG does not. Also some hardware devices may insist on doing the padding inside -- especially non-deterministic padding like PSS.

I have kind of the opposite problem. At least older Android version do not expose signing with PSS. So I have to sign with a raw signature anyway. And I would like to avoid implementing PSS in OpenVPN.

Quoted 6 lines

So what about naming this "management-external-key-version2" and then extend pk-sig as below?

pk-sig <base64-data-to-sign> [signing-algorithm]

Sure, not a problem with me.

Quoted 13 lines

where the <signing algorithm> could be, say, SHA256_RSA_PKCS1 or NONE_RSA_PKCS1 or SHA384_RSA_PSS etc. If missing, and the key is RSA, the client can assume it is NONE_RSA_PKCS1 and expect the hash with digest-info header already added and sign as such. Or, if the key is EC, do the ECDSA signing operation as is being done now.

The "unpadded" case could be indicated by NONE_RSA_RAW. These mnemonics are just examples -- we could use PKCS11 identifiers or something else.

To get unpadded data we have to override sign() in EVP_PKEY_METHOD but that would also provide max flexibility.

Quoted 6 lines

Now, I think its safe to just update and document this and require that management-interface clients that use the external key feature must update if using client compiled with OpenSSL 1.1.1. Android(?) and iOS (?) clients may be the only one's affected, but those are shipped as one complete package. I'm not aware of any standalone management-client UI that uses the external-key feature.

Might be just my client.

Quoted 7 lines

Quoted 2 lines

support TLS 1.3 without unpadded queries we disable TLS 1.3 otherwise.

If we must do this, disabling TLS 1.3 will not enough. One way is to also explicitly set the padding mode to RSA_PKCS1_PADDING (for TLS1.2)

EVP_PKEY_CTX_set_rsa_padding(ctx, pad) can do this.

Okay, I will add this to the patch to force pkcs1 padding. Thanks for looking it up/pointing it out.

Quoted 9 lines

Quoted 4 lines

We also do the same for cryptoapi since it uses the same API.

Let's just fix cryptoapi --- I'm working on it. Anyway it does
not use the same API as external key and prepadded data (= nopadding) will not work with Windows. This'll have to be fixed before we release binaries linked with OpenSSL 1.1.1.

When I wrote that patch I did not know you would dive so fast into it :)

Quoted 9 lines

Quoted 6 lines

Using the management api client version instead might seem like the more logical way but since we only now that version very late, it would extra logic and complexity to deal with this asynchronous behaviour .

IMO, we should explore this further and try to avoid --management-external-key-foo.

The biggest problem is that connect to the management interface is not strictly enforced. If you don't have management-hold, the client might start connecting before the management interface version is known and start with TLS 1.3 while the management client is not yet connected and might not support PSS signatures.

This management-external-key is a special feature I would like to opt for simplicity here. I would rather just drop support for older management clients that do not support the new padding but in the last discussion of pk-sig vs rsa-sig we decided to keep support for older clients.

Before OpenSSL 1.1.1 there could be no mismatch between compiled and actual OpenSSL version. With OpenSSL 1.1.1 we need runtime detection to detect the actual best TLS version supported.

Allowing this runtime detection also allows removing some of the TLS 1.3/OpenSSL 1.1.1 #ifdefs

Without this patch tls-min-version 1.3 or-highest will actually downgrade to TLS 1.3 in the "compiled with 1.1.0 and linked against 1.1.1" scenario.

Hi,

Quoted 10 lines · Arne Schwabe

Before OpenSSL 1.1.1 there could be no mismatch between compiled and actual OpenSSL version. With OpenSSL 1.1.1 we need runtime detection to detect the actual best TLS version supported.

Allowing this runtime detection also allows removing some of the TLS 1.3/OpenSSL 1.1.1 #ifdefs

Without this patch tls-min-version 1.3 or-highest will actually downgrade to TLS 1.3 in the "compiled with 1.1.0 and linked against 1.1.1" scenario.

"Downgrade to TLS 1.2", I guess?

But more fundamental: do want to support runtime-upgrading the TLS library? Are we sure that this is the only place where this will create unexpected behaviour? Does it even make sense to upgrade a dependency to a version that contains all sorts of API/ABI changes and then expect that you do not have to recompile? Honest questions; I don't understand why one would want or do this.

Quoted 30 lines · Arne Schwabe

Am 05.12.18 um 11:51 schrieb Steffan Karger:

Quoted 12 lines · Arne Schwabe

Quoted 10 lines

Before OpenSSL 1.1.1 there could be no mismatch between compiled and actual OpenSSL version. With OpenSSL 1.1.1 we need runtime detection to detect the actual best TLS version supported.

Allowing this runtime detection also allows removing some of the TLS 1.3/OpenSSL 1.1.1 #ifdefs

Without this patch tls-min-version 1.3 or-highest will actually downgrade to TLS 1.3 in the "compiled with 1.1.0 and linked against 1.1.1" scenario.

Quoted 8 lines

"Downgrade to TLS 1.2", I guess?

But more fundamental: do want to support runtime-upgrading the TLS library? Are we sure that this is the only place where this will create unexpected behaviour? Does it even make sense to upgrade a dependency to a version that contains all sorts of API/ABI changes and then expect that you do not have to recompile? Honest questions; I don't understand why one would want or do this.

I think on some installation, especially when the user compiled OpenVPN him/herself, we just need to live with the fact that the OpenSSL library got upgraded from OpenSSL 1.1.0 to OpenSSL 1.1.1.

But one art of decting the newer library here is that we can bail out on some of the corner cases instead of continuing, like refusing external auth if the OpenSSL libary is too new.

Quoted 5 lines

I still think is an unsupported scenario and we might add an warning message aka like

"Detected OpenSSL 1.1.1 but compiled against OpenSSL 1.1.0, recompile to use all OpenSSL 1.1.1 features"

Do you mean "add a warning message *in addition to* the patch workarounds", or "add a warning message "instead of* the patch workarounds"?

I'm definitely in favour of adding the warning, but would need to think a bit more about whether adding the workarounds will give net-positive results.

For TLS 1.0 to 1.2 and OpenSSL 1.1.0 calls us and requires a PKCS1 padded response. As TLS 1.3 mandates RSA-PSS padding support and also requires an TLS 1.3 implementation to support RSA-PSS for older TLS version, OpenSSL will query us to sign an already RSA-PSS padded string.

This patch adds an 'unpadded' and 'pkcs1' parameter to the

management-external-key option to signal that the client is able to support pkcs1 as well as unpadded signature requests.

Since clients that implement the management-external-key interface are usually rather tightly integrated solutions (OpenVPN Connect in the past, OpenVPN for Android), it is reasonable to expect that upgrading the OpenSSL library can be done together with management interface changes. Therefore we provide no backwards compatbility for mangement-interface clients not supporting OpenSSL 1.1.1.

Using the management api client version instead might seem like the more logical way but since we only now that version very late, it would extra logic and complexity to deal with this asynchronous behaviour. Instead just give an error early if OpenSSL 1.1.1 and management-external-key without nopadding is detected.

The interface is prepared for signalling PCKS1 and RSA-PSS support instead of signalling unpadded support.

Patch v3: fix overlong lines and few other style patches. Note
two overlong lines concerning mbedtls are not fixed as they are removed/shortend by the mbed tls patch to avoid conflicts

Patch v4: Setting minimum TLS version proved to be not enough and
instead of implementing a whole compability layer we require mangement-clients to implement the new feature when they want to use OpenSSL 1.1.1

Add a padding=ALGORITHM argument to pk-sig to indicate the algorithm. Drop adding PKCS1 ourselves.

Patch v5: Send the right version of the patch

Signed-off-by Arne Schwabe <[email protected]...>

---
 doc/management-notes.txt  | 13 ++++++++++-
 doc/openvpn.8             |  7 ++++--
 src/openvpn/manage.c      | 18 +++++++++++---
 src/openvpn/manage.h      | 23 +++++++++++-------
 src/openvpn/options.c     | 45 +++++++++++++++++++++++++++++++++--
 src/openvpn/ssl_mbedtls.c |  7 ++++--
 src/openvpn/ssl_openssl.c | 49 +++++++++++++++++++++++++++++++--------
 7 files changed, 134 insertions(+), 28 deletions(-)

doc/management-notes.txt +0 -0

index 17645c1d..f685af28 100644

doc/management-notes.txt +1 -1

@@ -823,7 +824,7 @@ The management interface client should then create an appropriate signature of
 the (decoded) BASE64_DATA using the private key and return the SSL signature as
 follows:
 
-pk-sig   (or rsa-sig)
+pk-sig (or rsa-sig)
 [BASE64_SIG_LINE]
 .
 .

doc/management-notes.txt +0 -0

 COMMAND -- certificate (OpenVPN 2.4 or higher)
 ----------------------------------------------
 Provides support for external storage of the certificate. Requires the

doc/openvpn.8 +5 -2

index e80d4696..3cd5d944 100644
@@ -2739,10 +2739,13 @@ Allow management interface to override
 directives (client\-only).
 .\"*********************************************************
 .TP
-.B \-\-management\-external\-key
+.B \-\-management\-external\-key [nopadding] [pkcs1]
 Allows usage for external private key file instead of
 .B \-\-key
-option (client\-only).
+option (client\-only). The optional parameters nopadding and
+pkcs1 signal support for different padding algorithms. See
+doc/mangement-notes.txt for a complete description of this
+feature.
 .\"*********************************************************
 .TP
 .B \-\-management\-external\-cert certificate\-hint

src/openvpn/manage.c +0 -0

index 8b633f20..62d4bc7b 100644

src/openvpn/manage.c +13 -1

 {
     const char *prompt = "PK_SIGN";
     const char *desc = "pk-sign";
+    struct buffer buf_data = alloc_buf(strlen(b64_data) + strlen(padding) + 20);
+
     if (man->connection.client_version <= 1)
     {
         prompt = "RSA_SIGN";
         desc = "rsa-sign";
     }
-    return management_query_multiline_flatten(man, b64_data, prompt, desc,
+
+    buf_write(&buf_data, b64_data, (int) strlen(b64_data));
+    if (man->connection.client_version > 2)
 
+    {
+        buf_write(&buf_data, ",", (int) strlen(","));
+        buf_write(&buf_data, padding, (int) strlen(padding));
+    }
+    char* ret = management_query_multiline_flatten(man,
+            (char *)buf_bptr(&buf_data), prompt, desc,
             &man->connection.ext_key_state, &man->connection.ext_key_input);
+    free_buf(&buf_data);
+    return ret;
 }
 

char *

src/openvpn/manage.h +3 -7

index d24abe09..4bfcfdbe 100644
@@ -31,7 +31,7 @@
 #include "socket.h"
 #include "mroute.h"
 
-#define MANAGEMENT_VERSION                      2
+#define MANAGEMENT_VERSION                      3
 #define MANAGEMENT_N_PASSWORD_RETRIES           3
 #define MANAGEMENT_LOG_HISTORY_INITIAL_SIZE   100
 #define MANAGEMENT_ECHO_BUFFER_SIZE           100

@@ -341,12 +341,18 @@ struct management *management_init(void);
 #ifdef MANAGEMENT_PF
 #define MF_CLIENT_PF         (1<<7)
 #endif
-#define MF_UNIX_SOCK       (1<<8)
-#define MF_EXTERNAL_KEY    (1<<9)
-#define MF_UP_DOWN          (1<<10)
-#define MF_QUERY_REMOTE     (1<<11)
-#define MF_QUERY_PROXY      (1<<12)
-#define MF_EXTERNAL_CERT    (1<<13)
+#define MF_UNIX_SOCK                (1<<8)
+#define MF_EXTERNAL_KEY             (1<<9)

src/openvpn/manage.h +0 -0

 char *management_query_cert(struct management *man, const char *cert_name);
 

src/openvpn/options.c +0 -0

index 406669a3..2291dd9d 100644

src/openvpn/options.c +1 -0

+#endif
     /*

* Windows-specific options.
*/

src/openvpn/options.c +0 -1

@@ -5151,9 +5168,33 @@ add_option(struct options *options,
         options->management_write_peer_info_file = p[1];
     }
 #ifdef ENABLE_MANAGEMENT
-    else if (streq(p[0], "management-external-key") && !p[1])

src/openvpn/options.c +7 -0

+            {
+                options->management_flags |= MF_EXTERNAL_KEY_PKCS1PAD;
+            }
+            else
+            {
+                msg(msglevel, "Unknown management-external-key flag: %s" , p[j]);
+            }

src/openvpn/options.c +1 -1

         options->management_flags |= MF_EXTERNAL_KEY;
     }
     else if (streq(p[0], "management-external-cert") && p[1] && !p[2])

@@ -8456,4 +8497,4 @@ add_option(struct options *options,
     }
 err:
     gc_free(&gc);
-}
\ No newline at end of file
+}

src/openvpn/ssl_mbedtls.c +0 -1

index f7e8c2d0..09b1a8fa 100644
@@ -626,7 +626,6 @@ tls_ctx_use_external_signing_func(struct tls_root_ctx *ctx,
 }
 
 #ifdef ENABLE_MANAGEMENT
-
 /** Query the management interface for a signature, see external_sign_func. */
 static bool
 management_sign_func(void *sign_ctx, const void *src, size_t src_len,

src/openvpn/ssl_mbedtls.c +0 -0

     {
         goto cleanup;
     }

src/openvpn/ssl_openssl.c +7 -1

index c2c8fdc0..237c2b55 100644
@@ -1079,24 +1079,46 @@ openvpn_extkey_rsa_finish(RSA *rsa)
     return 1;
 }
 
-/* Pass the input hash in 'dgst' to management and get the signature back.
+/*
+ * Convert OpenSSL's constant to the strings used in the management
+ * interface query
+ */
+const char*
+get_sig_padding_name(const int padding)
+{

src/openvpn/ssl_openssl.c +0 -0

 {
     char *in_b64 = NULL;
     char *out_b64 = NULL;
     int len = -1;
 

src/openvpn/ssl_openssl.c +2 -1

     }
     if (out_b64)
     {

@@ -1110,18 +1132,19 @@ get_sig_from_man(const unsigned char *dgst, unsigned int dgstlen,
 
 /* sign arbitrary data */
 static int
-rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, int padding)
+rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa,
+             int padding)
 {
     unsigned int len = RSA_size(rsa);
     int ret = -1;

src/openvpn/ssl_openssl.c +0 -0

     return (ret == len)? ret : -1;
 }

@@ -1215,7 +1238,13 @@ 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)
 {

src/openvpn/ssl_openssl.c +0 -1

     if (len > 0)
     {
--

2.19.1

Quoted 10 lines

For TLS 1.0 to 1.2 and OpenSSL 1.1.0 calls us and requires a PKCS1 padded response. As TLS 1.3 mandates RSA-PSS padding support and also requires an TLS 1.3 implementation to support RSA-PSS for older TLS version, OpenSSL will query us to sign an already RSA-PSS padded string.

This patch adds an 'unpadded' and 'pkcs1' parameter to the

management-external-key option to signal that the client is able to support pkcs1 as well as unpadded signature requests.

"unpadded" in past tense is confusing -- I suggest "nopadding" or "raw" or "rsa-raw" to indicate that what it expects to do is a raw signature (or no padding will be added) which means the caller should do the padding. In such cases the input will be invariably of the same size as the modulus and there is no room for padding anyway.

(NB: in the code proper, you do use "nopadding" and not "unpadded" which is better -- but also see below)

That said, I still fail to warm up to the idea of adding a parameter to management-external-key. First, even with this precaution, signing can fail in two cases:

(i) the config file has "--management-external-key nopadding" but the client version is <= 2. In such cases no warning will be printed but management will get prepadded data with no ALG indicator which the external signer will definitely fail to treat as PKCS1. Either signing will fail (not sure how an existing client would handle that) or connection will restart with verification error.

So, in short, I suggest to just leave the management-external-key option alone and only add a new parameter to PK_SIGN (more on that below) and make sure the external sign dispatcher does not error out when padding is not PKCS1 (ie. support at least RSA_PADDING_NONE and RSA_PADDING_PKCS1 in rsa_priv_enc() for now)

The early erroring out is only marginally useful as it does not catch all cases as argued above. Also a user may try to deal with the error by setting tls version max to 1.2 but that will also fail when openssl 1.1.1 is use -- this time without triggering the early error-out. So we gain little by mutilating this
option and all the complications it brings.

The patch will be considerably simple once this change is taken out.

In any case, both here (if we retain this) and in PK_SIGN a more generic signalling is required. Else we wont be able to support external PSS padding later -- just padding = PSS is not enough to do that, the hash algorithm is
also required. The same is true for PKCS1 too but currently we do not support external signer to add digest info for PKCS1 signatures. But going forward, some external signers would need that option.

I would suggest: PKS_SIGN <base64data>,ALG
where
ALG = RSA_NONE_PKCS1 (current default),
RSA_RAW (or RSA_NONE_NOPADDING)
RSA_<hash-alg>_PKCS1
ECDSA (current default if key is EC)
with ALG missing to be interpreted as current defaults (PKCS1 with digest info already prepended for RSA and ECDSA for EC).

Quoted 9 lines

Using the management api client version instead might seem like the more logical way but since we only now that version very late, it would extra logic and complexity to deal with this asynchronous behaviour. Instead just give an error early if OpenSSL 1.1.1 and management-external-key without nopadding is detected.

The interface is prepared for signalling PCKS1 and RSA-PSS support instead of signalling unpadded support.

"instead of" --> "in addition to" ?
unpadded support --> "signature algorithm to use"

Quoted 11 lines

Patch v3: fix overlong lines and few other style patches. Note
two overlong lines concerning mbedtls are not fixed as they are removed/shortend by the mbed tls patch to avoid conflicts

Patch v4: Setting minimum TLS version proved to be not enough and
instead of implementing a whole compability layer we require mangement-clients to implement the new feature when they want to use OpenSSL 1.1.1

Quoted 2 lines

Add a padding=ALGORITHM argument to pk-sig to indicate the algorithm. Drop adding PKCS1 ourselves.

The code below adds ",ALG" to PK_SIGN, not padding=ALG to pk-sig. Better make the above consistent with that.

Quoted 8 lines

.

doc/management-notes.txt +2 -0

@@ -833,6 +834,12 @@ END
 Base 64 encoded output of RSA_private_encrypt for RSA or ECDSA_sign()
 for EC using OpenSSL or mbedtls_pk_sign() using mbed TLS will provide a
 correct signature.
+The rsa-sig interfaces expects PKCS1 padded signatures for RSA keys
+(RSA_PKCS1_PADDING). EC signatures are always unpadded.
 

doc/management-notes.txt END · line 842

Quoted 4 lines

doc/management-notes.txt +4 -0

+
+The padding field is only present when pk-sig is used and
+currently the following values can be requested PCKS1 and NOPADDING for RSA
+certificates and NOPADDING for EC certificates.

This is totally confusing. There is no reason to distinguish between rsa-sig and pk-sig as used by the client although we should encourage all new clients to use pk-sig only. These added lines may be replaced by:

"For RSA keys, if signature algorithm is "RSA_NONE_PKCS1" or unspecified, the data to sign contains the hash with digest info header added. If
the algorithm is RSA_RAW, a raw RSA sign operation should be performed. Base 64 encoded output of OpenSSL function RSA_private_encrypt() with padding=RSA_PADDING_PKCS1 for the first case and padding=RSA_PADDING_NONE in the second case creates the correct signature. In case of EC keys Base 64 encoded output of ECDSA_sign() provides the correct signature."

And remove those references to ECDSA_sign() and RSA_private_encrypt() earlier.

Quoted 6 lines

doc/management-notes.txt +0 -0

 This capability is intended to allow the use of arbitrary cryptographic
 service providers with OpenVPN via the management interface.

@@ -840,6 +847,10 @@ service providers with OpenVPN via the management interface.
 New and updated clients are expected to use the version command to announce
 a version > 1 and handle '>PK_SIGN' prompt and respond with 'pk-sig'.
 

doc/management-notes.txt service providers with OpenVPN via the management interface. · line 853

Quoted 4 lines

doc/management-notes.txt +4 -0

+The older rsa-sig and pk-sig interfaces hav no capability to indidicate the
+requested padding algorithm. When the 'nopadding' using version >= 2 is required.
+To support TLS 1.3 with OpenSSL 1.1.1 supporting unpadded signatures is required.
+

Again, confusing. If you want to reiterate this what about:
"The signature algorithm is indicated in the PK_SIGN request only if the or management client-version is > 2. In particular, to support TLS1.3 and TLS1.2 using OpenSSL 1.1.1, non-pkcs1 padded signature support is required and this can be indicated in the signing request only if the client version is > 2."

src/openvpn/manage.c management_query_multiline_flatten(struct management *man, · line 3643

Quoted 6 lines

src/openvpn/manage.c +2 -2

 char *
 /* returns allocated base64 signature */
-management_query_pk_sig(struct management *man,
-                         const char *b64_data)
+management_query_pk_sig(struct management *man, const char *b64_data,
+                        const char *padding)

Suggest to change padding -> sign_alg

Quoted 53 lines

 {
     const char *prompt = "PK_SIGN";
     const char *desc = "pk-sign";

src/openvpn/manage.c +13 -1

+    struct buffer buf_data = alloc_buf(strlen(b64_data) + strlen(padding) + 20);
+
     if (man->connection.client_version <= 1)
     {
         prompt = "RSA_SIGN";
         desc = "rsa-sign";
     }
-    return management_query_multiline_flatten(man, b64_data, prompt, desc,
+
+    buf_write(&buf_data, b64_data, (int) strlen(b64_data));
+    if (man->connection.client_version > 2)
 
+    {
+        buf_write(&buf_data, ",", (int) strlen(","));
+        buf_write(&buf_data, padding, (int) strlen(padding));
+    }
+    char* ret = management_query_multiline_flatten(man,
+            (char *)buf_bptr(&buf_data), prompt, desc,
             &man->connection.ext_key_state, &man->connection.ext_key_input);
+    free_buf(&buf_data);
+    return ret;
 }
 
 char *

src/openvpn/manage.h +3 -7

index d24abe09..4bfcfdbe 100644
@@ -31,7 +31,7 @@
 #include "socket.h"
 #include "mroute.h"
 
-#define MANAGEMENT_VERSION                      2
+#define MANAGEMENT_VERSION                      3
 #define MANAGEMENT_N_PASSWORD_RETRIES           3
 #define MANAGEMENT_LOG_HISTORY_INITIAL_SIZE   100
 #define MANAGEMENT_ECHO_BUFFER_SIZE           100

@@ -341,12 +341,18 @@ struct management *management_init(void);
 #ifdef MANAGEMENT_PF
 #define MF_CLIENT_PF         (1<<7)
 #endif
-#define MF_UNIX_SOCK       (1<<8)
-#define MF_EXTERNAL_KEY    (1<<9)
-#define MF_UP_DOWN          (1<<10)
-#define MF_QUERY_REMOTE     (1<<11)
-#define MF_QUERY_PROXY      (1<<12)
-#define MF_EXTERNAL_CERT    (1<<13)
+#define MF_UNIX_SOCK                (1<<8)
+#define MF_EXTERNAL_KEY             (1<<9)

src/openvpn/manage.h struct management *management_init(void); · line 355

Quoted 10 lines

src/openvpn/manage.h +10 -0

+#define MF_EXTERNAL_KEY_NOPADDING   (1<<10)
+#define MF_EXTERNAL_KEY_PKCS1PAD    (1<<11)
+#define MF_UP_DOWN                  (1<<12)
+#define MF_QUERY_REMOTE             (1<<13)
+#define MF_QUERY_PROXY              (1<<14)
+#define MF_EXTERNAL_CERT            (1<<15)
+
+#define MF_RSA_PKCS1_PADDING       1
+#define MF_RSA_NO_PADDING          2
+

These extra defines and associated code can go if --management-external-key is not changed.

src/openvpn/manage.h void management_learn_addr(struct management *management, · line 440

Quoted 11 lines

src/openvpn/manage.h +2 -1

 bool management_open(struct management *man,
                      const char *addr,

@@ -430,7 +436,8 @@ void management_learn_addr(struct management *management,
 
 #endif
 
-char *management_query_pk_sig(struct management *man, const char *b64_data);
+char *management_query_pk_sig(struct management *man, const char *b64_data,
+                              const char* pading);

pading -> padding for consistency..
But padding -> sign_alg is my preference.

Quoted 12 lines

src/openvpn/manage.h +0 -0

 char *management_query_cert(struct management *man, const char *cert_name);
 

src/openvpn/options.c +0 -0

index 406669a3..2291dd9d 100644
@@ -2141,6 +2141,23 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
 
 #endif
 

src/openvpn/options.c options_postprocess_verify_ce(const struct options *options, const struct connec · line 2149

Quoted 6 lines

src/openvpn/options.c +6 -0

+#if defined(ENABLE_CRYPTOAPI)
+    if (o->cryptoapi_cert && (tls_version_max() >= TLS_VER_1_3))
+    {
+        msg(M_ERR, "Cryptoapi support currently is incompatible "
+                   "with OpenSSL 1.1.1/TLS 1.3");
+    }

TLS1.2 is also affected but for now ok like this.
I have a fix to support PSS in cryptoapi, just have to find some time to test it thoroughly. Sigh..

Quoted 8 lines

src/openvpn/options.c +5 -0

+#endif
+#if  defined(ENABLE_MANAGEMENT)
+    if ((tls_version_max() >= TLS_VER_1_3) &&
 
+        (options->management_flags & MF_EXTERNAL_KEY) &&
+        !(options->management_flags & (MF_EXTERNAL_KEY_NOPADDING))
 

src/openvpn/options.c options_postprocess_verify_ce(const struct options *options, const struct connec · line 2159

Quoted 5 lines

src/openvpn/options.c +5 -0

+        )
+    {
+        msg(M_ERR, "management-external-key with OpenSSL 1.1.1 requires "
+                   "the nopadding argument/support");
+    }

Same as above but I think this check is not that useful..

Am 14.11.18 um 16:18 schrieb Selva Nair:

Quoted 8 lines

Hi,

snipping a lot of useful stuff as we tend agree on those.. ..

Quoted 3 lines

But in summary.

My patch basically amounts to: Support old style

Quoted 2 lines

Quoted 2 lines

with <= OpenSSL 1.1.0 and use and require a new API if we have OpenSSL 1.1.1 and to really move and have external key have a sane API, we

Quoted 3 lines

Quoted 3 lines

drop rsa-sig and support for older clients, even with <= OpenSSL 1.1.0 as you oppose my proposl for signalling that a new API should be used. (version in managemnet protocol is too late and not feasible in my

Quoted 10 lines

Quoted 2 lines

Supporting a half working RSA_SIG option that breaks left and right is not desirable in my opinion.

My point is that even with "--management-external-key foo bar", we will be sending
a non workable signature request to old clients so changing that option is not really
useful. In other words the early erroring out works only half the time and all that code
complication is not worth it.

Okay, I think you misunderstood my code. What it does is basically if we detect having TLS 1.3 support (max_tls_version >= TLS 1.3) and not a nopadding argument we error out, saying that this combination cannot work.

Yes that means that if you upgrade to OpenSSL 1.1.1 you need to support the new API but I think that is acceptable.

Unless I overlooked something, I don't see any situation in which we ask for an unsupported signature.

Quoted 3 lines

Unless I overlooked something, I don't see any situation in which we ask for an unsupported signature.

Consider this:
(i) config has --management-external-key nopadding but client announces version
2. We will not error out but send the signature request as
PK_SIGN <base64data>
without the ALG as client version is not 3 and fail

(ii) tls version max is set 1.2 and openssl 1.1.1 is in use both on server and client.
PSS signing will get negotiated but we will not error out early as TLS 1.3 is not in use.

That's why I say that this extension of management-external-key is not worth it.

Am I missing something?

Quoted 6 lines

Quoted 3 lines

Unless I overlooked something, I don't see any situation in which we ask for an unsupported signature.

Consider this:
(i) config has --management-external-key nopadding but client announces

version

Quoted 3 lines

2. We will not error out but send the signature request as PK_SIGN <base64data>
without the ALG as client version is not 3 and fail

We can error out on version on the management line. But this is also a kind of obscure misconfiguration since why would add nopadding to the config if you cannot support it?

Quoted 6 lines

(ii) tls version max is set 1.2 and openssl 1.1.1 is in use both on server and client.
PSS signing will get negotiated but we will not error out early as TLS 1.3 is not in use.

That's why I say that this extension of management-external-key is not

worth it.

Quoted 3 lines

Am I missing something?

tls_version_max will still report TLS 1.3 as it not affected by the version set in config but really the max the library is capable of irrespectable tls min/max version.

Quoted 6 lines

(ii) tls version max is set 1.2 and openssl 1.1.1 is in use both on server and client.
PSS signing will get negotiated but we will not error out early as TLS 1.3 is not in  use.

That's why I say that this extension of management-external-key is

not worth it.
>
> Am I missing something?
>

tls_version_max will still report TLS 1.3 as it not affected by the version set in config but really the max the library is capable of irrespectable tls min/max version.

Aha, I missed that. Still I really do not understand the need for erroring here
instead of when prompting for PK_SIGN based on client version. Much simpler.

Hi,

Reviving this thread/patch as now users are running into this padding issue (trac 1216 <https://community.openvpn.net/openvpn/ticket/1216>).

IIRC, we more-or-less agreed upon adding an argument (nopadding, pss etc..) to >PK_SIGN for new clients and erroring out with old clients that cannot sign with PSS padding.

Selva

Am 20.09.19 um 22:55 schrieb Selva Nair:

Quoted 11 lines

Hi,

Reviving this thread/patch as now users are running into this padding issue (trac 1216 <https://community.openvpn.net/openvpn/ticket/1216>).

IIRC, we more-or-less agreed upon adding an argument (nopadding, pss etc..) to >PK_SIGN for new clients and erroring out with old clients that cannot sign with PSS padding.

Selva

Yeah.

We did not really to a conclusion if we wanted backwards compatibility or not. Since it seems that OpenSSL 1.1.1 requires the management-client to understand the new way of signatures anyway, I would say we require the management client to be able to understand the signature in any case.

I think the missing bit of piece for the patch is if we want to error out early if we detect a config that *might* not work (the nopadding argument or any other argument to the management-external-key) or if we do not error at this point and fail then when we actually require PSS signature. I am more for the first version because otherwise you end up with configurations that work fine until the server is upgraded to OpenSSL 1.1.1 and then the client stops working without anything being change (yes I realise that is already the case at the moment)