Add support for OpenSSL TLS 1.3 when using management-external-key
PATCH 1/1 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 .
--- doc/management-notes.txt | 7 ++++- src/openvpn/manage.h | 9 ++++--- src/openvpn/options.c | 54 ++++++++++++++++++++++++++++++++++++++- src/openvpn/ssl_openssl.c | 26 ++++++++++++++----- 4 files changed, 83 insertions(+), 13 deletions(-)
doc/management-notes.txt +6 -1
@@ -832,7 +832,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. +correct signature. With the 'nopadding' argument to the +external-management-interface the interface expects unpadded signatures +(RSA_NO_PADDING in OpenSSL). When the 'nopadding' keyword is missing the +interfaces expects PKCS1 padded signatures for RSA keys (RSA_PKCS1_PADDING). +EC signatures are always unpadded. To support TLS 1.3 using unpadded +signatures is required. This capability is intended to allow the use of arbitrary cryptographic service providers with OpenVPN via the management interface.
src/openvpn/manage.h +5 -4
@@ -348,11 +348,12 @@ struct management *management_init(void);#define MF_UNIX_SOCK (1<<8)#ifdef MANAGMENT_EXTERNAL_KEY #define MF_EXTERNAL_KEY (1<<9) +#define MF_EXTERNAL_KEY_NOPADDING (1<<10)#endif -#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_UP_DOWN (1<<11) +#define MF_QUERY_REMOTE (1<<12) +#define MF_QUERY_PROXY (1<<13) +#define MF_EXTERNAL_CERT (1<<14) bool management_open(struct management *man, const char *addr,
src/openvpn/options.c +53 -1
74 lines not on this page · raw message
src/openvpn/ssl_openssl.c +19 -7
@@ -1099,15 +1099,27 @@ openvpn_extkey_rsa_finish(RSA *rsa)*/ static int get_sig_from_man (const unsigned char *dgst,unsigned int dgstlen, -unsigned char *sig,unsigned int siglen) +unsigned char *sig,unsigned int siglen,bool pkcs1pad) {char *in_b64 =NULL ;char *out_b64 =NULL ;int len =-1 ; +int bencret =-1 ; -/* convert 'dgst' to base64 */ -if (management - &&openvpn_base64_encode (dgst, dgstlen, &in_b64) >0 ) +if ((management->settings.flags &MF_EXTERNAL_KEY_NOPADDING ) >2 && pkcs1pad) + { +/* +* Add PKCS1 signature and replace input with it +* Use our output buffer also als temporary buffer +*/ +RSA_padding_add_PKCS1_type_1 (sig, siglen, dgst, dgstlen); + bencret =openvpn_base64_encode (sig, siglen, &in_b64); + } +else + { + bencret =openvpn_base64_encode (dgst, dgstlen, &in_b64); + } +if (management && bencret >0 ) { out_b64 =management_query_pk_sig (management, in_b64); }
@@ -1128,13 +1140,13 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, iunsigned int len =RSA_size (rsa);int ret =-1 ; -if (padding !=RSA_PKCS1_PADDING ) +if (padding !=RSA_PKCS1_PADDING && padding !=RSA_NO_PADDING ) {RSAerr (RSA_F_RSA_OSSL_PRIVATE_ENCRYPT ,RSA_R_UNKNOWN_PADDING_TYPE );return -1 ; } - ret =get_sig_from_man (from, flen, to, len); + ret =get_sig_from_man (from, flen, to, len, padding ==RSA_PKCS1_PADDING );return (ret == len)? ret :-1 ; }
@@ -1228,7 +1240,7 @@ 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) { int capacity = ECDSA_size(ec); -int len =get_sig_from_man (dgst, dgstlen, sig, capacity); +int len =get_sig_from_man (dgst, dgstlen, sig, capacity, false);if (len >0 ) {
Signature · 1 line
2.19.0
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 3 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 2 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 5 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.
Selva
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_signorrsa_priv_enccallbacks 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
Quoted 5 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 5 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_PKCS1orNONE_RSA_PKCS1orSHA384_RSA_PSSetc. If missing, and the key is RSA, the client can assume it isNONE_RSA_PKCS1and 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()inEVP_PKEY_METHODbut 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 6 lines
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.
Okay, I will add this to the patch to force pkcs1 padding. Thanks for looking it up/pointing it out.
Quoted 8 lines
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.
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.
Arne
Hi,
Quoted 38 lines · Arne Schwabe
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_signorrsa_priv_enccallbacks 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.
Quoted 6 lines
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
This sounds good. None specified would imply rsa-pkcs1 and ecdsa to match the status-quo.
So if pss is not included but raw is and we need PSS, we pad and pass it as something like
Quoted 1 line
pk-sig <data> RSA-RAW
In fact its easy to detect raw requests as the data will be exactly the size of the key so there is no room for padding. However, in the current code that would be an error as the external signer is supposed to do PKCS1 padding.
If pss and raw are supported we have an option to pass the hash with the argument as hash_type-RSA-PSS or RSA-RAW.
Sounds good to me.
Should we make the arguments name value pairs thinking of possible future extensions. Like
pk-sig <data> signature_algorithm=SHA256-RSA-PSS saltlen=32
Either way, the additional option to pk-sig will help us as we can use it to indicate whether the digest-info header is added for pkcs1 (using hash type = None) as is done presently or not etc.
Quoted 9 lines
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.
Quoted 3 lines
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.
Ideally, when an engine is in use (external signer in our case) OpenSSL should not do the padding and let the engine do that. Due to limitations of the RSA_sign() call back this is not the case now when padding is not PKCS1. Hopefully that situation will change in future versions of OpenSSL. (see issue 7341 in openssl-github) or using the EVP_PKEY interface to get hold of the hash and signature algorithm details may become the preferred approach.
But I understand -- legacy "engines" have to cope too and implementing PSS padding within the UI is not a good option. So we need the ability to ask for raw signature.
Quoted 22 lines
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_PKCS1orNONE_RSA_PKCS1orSHA384_RSA_PSSetc. If missing, and the key is RSA, the client can assume it isNONE_RSA_PKCS1and 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()inEVP_PKEY_METHODbut that would also provide max flexibility.
Quoted 8 lines
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.
Yeah. So we need not really worry about backwards compatibility as you client is shipped as one package including openssl and the UI.
Quoted 11 lines
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.
Sorry, I wrote that without testing. It wont work: by the time we can get hold of EVP_PKEY context the signature algorithm is already negotiated with the peer. We need to use something like:
SSL_CTX_set1_sigalgs_list(ctx, "ECDSA+SHA256:RSA+SHA256");
That is, exclude RSA-PSS+hash type entries included by default by OpenSSL 1.1.1. I tested this with cryptoapicert and works.
Quoted 28 lines
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.
Quoted 5 lines
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.
I recall that. But, in reality, I think there are no such clients, so we could easily extend pk-sig without breaking any. But that's just my opinion.
Thanks,
Selva
PATCH v2 1/2 Make tls_version_max return the actual maximum version
In reply to Arne SchwabeBefore 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.
--- src/openvpn/ssl.c | 11 +++++------ src/openvpn/ssl_openssl.c | 33 ++++++++++++++++++++++++++++----- 2 files changed, 33 insertions(+), 11 deletions(-)
src/openvpn/ssl.c +5 -6
@@ -4182,12 +4182,11 @@ show_available_tls_ciphers(const char *cipher_list, {printf ("Available TLS Ciphers, listed in order of preference:\n" ); -#if (ENABLE_CRYPTO_OPENSSL &&OPENSSL_VERSION_NUMBER >=0x1010100fL ) -printf ("\nFor TLS 1.3 and newer (--tls-ciphersuites):\n\n" ); -show_available_tls_ciphers_list (cipher_list_tls13, tls_cert_profile, true); -#else - (void ) cipher_list_tls13;/* Avoid unused warning */ -#endif +if (tls_version_max () >=TLS_VER_1_3 ) + { +printf ("\nFor TLS 1.3 and newer (--tls-ciphersuites):\n\n" ); +show_available_tls_ciphers_list (cipher_list_tls13, tls_cert_profile, true); + }printf ("\nFor TLS 1.2 and older (--tls-cipher):\n\n" );show_available_tls_ciphers_list (cipher_list, tls_cert_profile, false);
src/openvpn/ssl_openssl.c +28 -5
@@ -210,7 +210,23 @@ inttls_version_max (void) {#if defined(TLS1_3_VERSION ) +/* If this is defined we can safely assume TLS 1.3 support */ return TLS_VER_1_3 ; +#elif OPENSSL_VERSION_NUMBER >=0x10100000L +/* +* The library we are *linked* against is OpenSSL 1.1.1 +* and therefore supports TLS 1.3. This needs to be checked at runtime +* since we can be compiled against 1.1.0 and then the library can be +* upgraded to 1.1.1 +*/ +if (OpenSSL_version_num () >=0x1010100fL ) + { +return TLS_VER_1_3 ; + } +else + { +return TLS_VER_1_2 ; + }#elif defined(TLS1_2_VERSION ) || defined(SSL_OP_NO_TLSv1_2)return TLS_VER_1_2 ;#elif defined(TLS1_1_VERSION ) || defined(SSL_OP_NO_TLSv1_1)
@@ -236,12 +252,20 @@ openssl_tls_version(int ver) {return TLS1_2_VERSION ; } -#if defined(TLS1_3_VERSION ) else if (ver == TLS_VER_1_3) { +/* +* Supporting the library upgraded to TLS1.3 without recompile +* is enough to support here with a simple constant that the same +* as in the TLS 1.3, so spec it is very unlikely that OpenSSL +* will change this constant +*/ +#ifndef TLS1_3_VERSION +return 0x0304 ; +#else return TLS1_3_VERSION ; - }#endif + }return 0 ; }
@@ -1948,14 +1972,13 @@ show_available_tls_ciphers_list(const char *cipher_list,crypto_msg (M_FATAL ,"Cannot create SSL_CTX object" ); } -#if (OPENSSL_VERSION_NUMBER >=0x1010100fL )if (tls13) { -SSL_CTX_set_min_proto_version (tls_ctx.ctx,TLS1_3_VERSION ); +SSL_CTX_set_min_proto_version (tls_ctx.ctx, +openssl_tls_version (TLS_VER_1_3 ));tls_ctx_restrict_ciphers_tls13 (&tls_ctx, cipher_list); }else -#endif {SSL_CTX_set_max_proto_version (tls_ctx.ctx,TLS1_2_VERSION );tls_ctx_restrict_ciphers (&tls_ctx, cipher_list);
Signature · 1 line
2.19.1
PATCH v2 2/2 Add support for OpenSSL TLS 1.3 when using management-external-key
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.
--- doc/management-notes.txt | 13 ++++++++++- doc/openvpn.8 | 7 ++++-- src/openvpn/manage.c | 18 +++++++++++--- src/openvpn/manage.h | 26 ++++++++++++++------- src/openvpn/options.c | 45 ++++++++++++++++++++++++++++++++--- src/openvpn/ssl_mbedtls.c | 7 ++++-- src/openvpn/ssl_openssl.c | 49 +++++++++++++++++++++++++++++++-------- 7 files changed, 136 insertions(+), 29 deletions(-)
doc/management-notes.txt +12 -1
@@ -816,6 +816,7 @@ 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: +>PK_SIGN:[BASE64_DATA],[ALG] (if client announces support for management version > 2) >PK_SIGN:[BASE64_DATA] (if client announces support for management version > 1) >RSA_SIGN:[BASE64_DATA] (only older clients will be prompted like this)
@@ -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] . .
@@ -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. + +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 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'. +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. + COMMAND -- certificate (OpenVPN 2.4 or higher) ---------------------------------------------- Provides support for external storage of the certificate. Requires the
doc/openvpn.8 +5 -2
@@ -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 +15 -3
@@ -3639,18 +3639,30 @@ management_query_multiline_flatten(struct management *man,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) {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 +18 -8
@@ -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) +#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 + bool management_open(struct management *man, const char *addr,
@@ -430,7 +436,11 @@ void management_learn_addr(struct management *management,#endif -char *management_query_pk_sig (struct management *man,const char *b64_data); +#define PKSIG_PKCS1 1 +#define PK + +char *management_query_pk_sig (struct management *man,const char *b64_data, +const char * pading);char *management_query_cert (struct management *man,const char *cert_name);
src/openvpn/options.c +42 -3
69 lines not on this page · raw message
src/openvpn/ssl_mbedtls.c +5 -2
@@ -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,
@@ -641,7 +640,11 @@ management_sign_func(void *sign_ctx, const void *src, size_t src_len, goto cleanup; } -if (!(dst_b64 =management_query_pk_sig (management, src_b64))) +/* +* We only use PKCS1 signatures at the moment in mbed TLS, +* there the signature parameter is hardcoded +*/ +if (!(dst_b64 =management_query_pk_sig (management, src_b64,"PKCS1" ))) { goto cleanup; }
src/openvpn/ssl_openssl.c +39 -10
88 lines not on this page · raw message
Signature · 1 line
2.19.1
PATCH v5 2/2 Add support for OpenSSL TLS 1.3 when using management-external-key
In reply to Arne SchwabeFor 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
--- 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 +12 -1
@@ -816,6 +816,7 @@ 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: +>PK_SIGN:[BASE64_DATA],[ALG] (if client announces support for management version > 2) >PK_SIGN:[BASE64_DATA] (if client announces support for management version > 1) >RSA_SIGN:[BASE64_DATA] (only older clients will be prompted like this)
@@ -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] . .
@@ -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. + +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 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'. +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. + COMMAND -- certificate (OpenVPN 2.4 or higher) ---------------------------------------------- Provides support for external storage of the certificate. Requires the
doc/openvpn.8 +5 -2
@@ -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 +15 -3
@@ -3639,18 +3639,30 @@ management_query_multiline_flatten(struct management *man,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) {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 +15 -8
@@ -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) +#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 + 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);char *management_query_cert (struct management *man,const char *cert_name);
src/openvpn/options.c +43 -2
63 lines not on this page · raw message
src/openvpn/ssl_mbedtls.c +5 -2
@@ -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,
@@ -641,7 +640,11 @@ management_sign_func(void *sign_ctx, const void *src, size_t src_len, goto cleanup; } -if (!(dst_b64 =management_query_pk_sig (management, src_b64))) +/* +* We only use PKCS1 signatures at the moment in mbed TLS, +* there the signature parameter is hardcoded +*/ +if (!(dst_b64 =management_query_pk_sig (management, src_b64,"PKCS1" ))) { goto cleanup; }
src/openvpn/ssl_openssl.c +39 -10
88 lines not on this page · raw message
Signature · 1 line
2.19.1
Hi,
My comments below has grown too long so first a summary for those who TLDR;
My suggestion:
- Leave management-external-key as is (there is not much gained by adding a parameter to it)
- Append a fairly flexible signature algorithm specifier to PK_SIGN request to management
(nopadding or pkcs1 is not good enough in the long run)
Do not take cue from OpenSSL's design of RSA_private_encrypt() and rsa_sign() both of which fall short for historic reasons.
- require version 3 clients to support at least certain algorithms
Some of the comments below were written assuming --management-external-key will get a new optional parameter and some are based on my suggestion not to do so. So the rest is sometimes a bit of flux and/or repetitive. But I spent some
time on this so here goes:
Quoted 11 lines · Arne Schwabe
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.
(ii) Taking a long view, the real problem is not just "padding" but "signature algorithm" in general. That is, PK_SIGN (or RSA_SIGN) does not provide enough details for generating a signature. In good old days when we supported only TLS 1.1 or older and RSA keys, there was only one kind of signature. Today one needs to know the hash algorithm and the padding scheme to generate a signature. We get away without the hash algorithm as the digestinfo header is prepended to the hash by the time rsa_priv_enc is called (not so in mbedTLS so we add it in that handler). But that will not work for PSS if we were to support external PSS padding -- which we should do in the long term.
Part of the reason for this is we have been playing along with OpenSSL which has been trying hard to preserve a dated API. Now with 1.1.1, they have broken it for good and the rsa_sign() callback does not even get called anymore as padding cannot be assumend to be PKCS1.
So we have to stop taking cue from OpenSSL especially since the purpose of external key is to support arbitrary external signing agents/libraries.
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).
Here <hash_lag> could be SHA256, SHA1 etc. and indicates that the data is just the hash and the signer should add digestinfo header to the signature. We do not have to support this as yet but will come handy to have a way to do a complete signature algorithm specifier in ALG.
And require that version 3 clients are expected to support at least RSA_NONE_PKCS1, RSA_RAW and ECDSA.
Anyway, I did review the code considering both options -- not all comments below are relevant if we decide not to modify --management-external-key. That would be my preference.
Quoted 8 lines
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.
Looks perfectly acceptable to me..
Quoted 10 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 6 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
Quoted 7 lines
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.1Add 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 20 lines
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
doc/management-notes.txt actual private key. When the SSL protocol needs to perform a sign · line 821
Quoted 6 lines
doc/management-notes.txt +1
-0@@ -816,6 +816,7 @@ 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: +>PK_SIGN:[BASE64_DATA],[ALG] (if client announces support for management version > 2) >PK_SIGN:[BASE64_DATA] (if client announces support for management version > 1) >RSA_SIGN:[BASE64_DATA] (only older clients will be prompted like this)
List values of ALG that a version 3 client should support. And add that if ALG is missing it should be interpreted as ECDSA for EC keys and RSA_NONE_PKCS1 for RSA keys. That nicely covers the two possible forms of PK_SIGN directive.
Quoted 12 lines
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 END · line 843
Quoted 10 lines
doc/management-notes.txt +6
-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. + +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.
doc/management-notes.txt service providers with OpenVPN via the management interface. · line 853
Quoted 12 lines
doc/management-notes.txt +4
-0This 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'. +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."
Quoted 28 lines
doc/management-notes.txt +0
-0COMMAND -- certificate (OpenVPN 2.4 or higher) ---------------------------------------------- Provides support for external storage of the certificate. Requires thedoc/openvpn.8 +5
-2@@ -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\-hintsrc/openvpn/manage.c +0
-0
src/openvpn/manage.c management_query_multiline_flatten(struct management *man, · line 3643
Quoted 7 lines
src/openvpn/manage.c +2
-2@@ -3639,18 +3639,30 @@ management_query_multiline_flatten(struct management *man,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
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@@ -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
-1bool 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 9 lines
src/openvpn/manage.h +0
-0char *management_query_cert (struct management *man,const char *cert_name);src/openvpn/options.c +0
-0
src/openvpn/options.c options_postprocess_verify_ce(const struct options *options, const struct connec · line 2149
Quoted 9 lines
src/openvpn/options.c +6
-0@@ -2141,6 +2141,23 @@ options_postprocess_verify_ce(const struct options *options, const struct connec#endif +#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..
src/openvpn/options.c options_postprocess_verify_ce(const struct options *options, const struct connec · line 2159
Quoted 12 lines
src/openvpn/options.c +10
-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 )) + ) + { +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..
Quoted 10 lines
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 add_option(struct options *options, · line 5180
Quoted 10 lines
src/openvpn/options.c +8
-0+else if (streq (p[0 ],"management-external-key" )) {VERIFY_PERMISSION (OPT_P_GENERAL ); +for (int j =1 ; j <MAX_PARMS && p[j] !=NULL ; ++j) + { +if (streq (p[j],"nopadding" )) + { + options->management_flags |=MF_EXTERNAL_KEY_NOPADDING ; + } +else if (p[1 ] &&streq (p[1 ],"pkcs1" ))
That should be
else if (streq(p[j], "pkcs1"))
Note the [j]
But the whole chunk could be removed..
Quoted 8 lines
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 add_option(struct options *options, · line 5197
Quoted 10 lines
src/openvpn/options.c +10
-0+ } +/* +* When no option is present, assume that only PKCS1 +* padding is supported +*/ +if (! (options->management_flags & + (MF_EXTERNAL_KEY_NOPADDING |MF_EXTERNAL_KEY_PKCS1PAD ))) + { + options->management_flags |=MF_EXTERNAL_KEY_PKCS1PAD ; + }
As we will anyway prompt for PKCS1 signatures even if the client does not announce it, we can always set that flag unconditionally and assume/require that all clients support PKCS1. Then the above is a simple
options->management_flags |= MF_EXTERNAL_KEY_PKCS1PAD
Again the above is irrelevant if we go with my suggestion to not touch this option.
Quoted 23 lines
src/openvpn/options.c +1
-1options->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); -} +}src/openvpn/ssl_mbedtls.c +0
-1@@ -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 management_sign_func(void *sign_ctx, const void *src, size_t src_len, · line 647
Quoted 9 lines
src/openvpn/ssl_mbedtls.c +5
-1@@ -641,7 +640,11 @@ management_sign_func(void *sign_ctx, const void *src, size_t src_len, goto cleanup; } -if (!(dst_b64 =management_query_pk_sig (management, src_b64))) +/* +* We only use PKCS1 signatures at the moment in mbed TLS, +* there the signature parameter is hardcoded +*/ +if (!(dst_b64 =management_query_pk_sig (management, src_b64,"PKCS1" )))
PKCS1 --> RSA_NONE_PKCS1 :)
Quoted 19 lines
src/openvpn/ssl_mbedtls.c +0
-0{ goto cleanup; }src/openvpn/ssl_openssl.c +7
-1@@ -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 openvpn_extkey_rsa_finish(RSA *rsa) · line 1098
Quoted 10 lines
src/openvpn/ssl_openssl.c +10
-0+switch (padding) + { +case RSA_PKCS1_PADDING : +return "PKCS1" ; +case RSA_NO_PADDING : +return "NOPADDING" ; +default : +return "UNKNOWN" ; + } +}
Change the above to sign_alg_name instead of padding_name
src/openvpn/ssl_openssl.c openvpn_extkey_rsa_finish(RSA *rsa) · line 1104
Quoted 6 lines
src/openvpn/ssl_openssl.c +4
-0+ +/* +* Pass the input hash in 'dgst' to management and get the signature back. * On input siglen contains the capacity of the buffer 'sig'. * On return signature is in sig. +* pkcs1 controls if pkcs1 padding is required
That should be "The parameter padding controls the type of padding to use."
But I suggest sign_alg controls the signature algorithm to use.
The caller need only translate padding (not hash algorithm) to signature algorithm
as the hash_alg is currently not available in rsa_priv_enc() callback. So for now
hash-alg will be always indicated as none. However, we want the management interface protocol be more flexible and hence we indicate the signature algorithm
using more descriptive mnemonics like RSA_NONE_PKCS1 etc in PK_SIGN.
src/openvpn/ssl_openssl.c openvpn_extkey_rsa_finish(RSA *rsa) · line 1110
Quoted 7 lines
src/openvpn/ssl_openssl.c +2
-1* Return value is signature length or -1 on error. */ static int get_sig_from_man (const unsigned char *dgst,unsigned int dgstlen, -unsigned char *sig,unsigned int siglen) +unsigned char *sig,unsigned int siglen, +int padding)
I suggest replace that "int padding" by "const char *padding" and pass in the translated name --- see below for a rationale.
Actually "const char *sign_alg" is my preference.
Quoted 6 lines
src/openvpn/ssl_openssl.c +0
-0{char *in_b64 =NULL ;char *out_b64 =NULL ;int len =-1 ;
src/openvpn/ssl_openssl.c openvpn_extkey_rsa_finish(RSA *rsa) · line 1121
Quoted 10 lines
src/openvpn/ssl_openssl.c +5
-4-/* convert 'dgst' to base64 */ -if (management - &&openvpn_base64_encode (dgst, dgstlen, &in_b64) >0 ) +int bencret =openvpn_base64_encode (dgst, dgstlen, &in_b64); + +if (management && bencret >0 ) { - out_b64 =management_query_pk_sig (management, in_b64); + out_b64 =management_query_pk_sig (management, in_b64, +get_sig_padding_name (padding));
Then that would be just padding or sign_alg
Quoted 14 lines
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 get_sig_from_man(const unsigned char *dgst, unsigned int dgstlen, · line 1147
Quoted 10 lines
src/openvpn/ssl_openssl.c +2
-2-if (padding !=RSA_PKCS1_PADDING ) +if (padding !=RSA_PKCS1_PADDING && padding !=RSA_NO_PADDING ) {RSAerr (RSA_F_RSA_OSSL_PRIVATE_ENCRYPT ,RSA_R_UNKNOWN_PADDING_TYPE );return -1 ; } - ret =get_sig_from_man (from, flen, to, len); + ret =get_sig_from_man (from, flen, to, len, padding);
And use "get_sig_padding_name(padding)" or preferably "get_sign_alg_name(padding)" here.
Quoted 6 lines
src/openvpn/ssl_openssl.c +0
-0return (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 ecdsa_sign(int type, const unsigned char *dgst, int dgstlen, unsigned char *sig, · line 1248
Quoted 10 lines
src/openvpn/ssl_openssl.c +7
-1int capacity =ECDSA_size (ec); -int len =get_sig_from_man (dgst, dgstlen, sig, capacity); +/* +* ECDSA does not seem to have proper constants for paddings since +* there are only signatures without padding at the moment, reuse +* RSA_NO_PADDING for now as it will trigger querying for "NOPADDING" in the +* management interface +*/ +int len =get_sig_from_man (dgst, dgstlen, sig, capacity,RSA_NO_PADDING );
That comment reads like a hack. As suggested above, passing the padding or sign_alg name avoids it. And one could instead say:
/* ECDSA signature involves no padding */
int len = get_sig_from_man(dgst, dgstlen, sig, capacity, "ECDSA");
Just as "PKCS1" (or "RSA_NONE_PKCS1") is hard coded in the request when using mbedTLS.
Quoted 6 lines
src/openvpn/ssl_openssl.c +0
-1if (len >0 ) { --2.19.1
Selva
Quoted 12 lines
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.
Quoted 8 lines
"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)
Note, will use nopadding consistently in the next patch.
Quoted 10 lines
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.
I can add an extra warning to print if the client will not announce a new version. But I think that config will not really happen (the tightly coupled argument)
Quoted 7 lines
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 leastRSA_PADDING_NONEandRSA_PADDING_PKCS1inrsa_priv_enc()for now)
Quoted 8 lines
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.
I get what you saying here but that breaks compability with management clients that are on the old pkcs1/mangement interface.
Quoted 8 lines
In any case, both here (if we retain this) and in
PK_SIGNa 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.
Quoted 9 lines
I would suggest:
PKS_SIGN<base64data>,ALG
where
ALG =RSA_NONE_PKCS1(current default),
RSA_RAW(orRSA_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).
My patch adds exactly that but with slightly different names for the algorithms.
Quoted 24 lines
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 conflictsPatch 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 5 lines
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.
Yeah, the commit message is wrong. Noted.
Quoted 8 lines
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.
Quoted 8 lines
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:
That comes from the inflexibility of our management code. I only accepts pk-sig when quering PKSIG and only accepts rsa-sig when querying RSSIG.
Quoted 18 lines
"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 isRSA_RAW, a raw RSA sign operation should be performed. Base 64 encoded output of OpenSSL functionRSA_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 ofECDSA_sign()provides the correct signature."And remove those references to
ECDSA_sign()andRSA_private_encrypt()earlier.Quoted 6 lines
doc/management-notes.txt +0
-0This 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'.
Quoted 12 lines
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 thePK_SIGNrequest 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."
Yes, your wording is better.
Quoted 9 lines
src/openvpn/manage.c management_query_multiline_flatten(struct management *man, · line 3643
Quoted 6 lines
src/openvpn/manage.c +2
-2char */* 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
Okay.
Quoted 96 lines
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@@ -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
-1bool 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_algis my preference.Quoted 12 lines
src/openvpn/manage.h +0
-0char *management_query_cert (struct management *man,const char *cert_name);src/openvpn/options.c +0
-0@@ -2141,6 +2141,23 @@ options_postprocess_verify_ce(const struct options *options, const struct connec#endif
Quoted 11 lines
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..
I added these few lines as they quick to remove and point an error in configuration at the moment. And what tls_version() does NOT report the version that is currently enabled but the maximum TLS version that the SSL library supports. So it will always error out on OpenSSL 1.1.1 and crypto api use.
Quoted 8 lines
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))
Quoted 8 lines
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..
The check can be translated as "If you use management-external-key with OpenSSL 1.1.1 you need to support the new API"
I removed the rest of comment as I agree with them.
But in summary.
My patch basically amounts to: Support old style management-external-key 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 would 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 opinion) Supporting a half working RSA_SIG option that breaks left and right is not desirable in my opinion.
I would be really happy to drop the old support if no one objects.
Arne
PATCH v5 2/2 Add support for OpenSSL TLS 1.3 when using management-external-key
In reply to a message not in the archiveSomehow this didn't get copied to the list
---------- Forwarded message ---------
From: Selva Nair <[email protected]...>
Date: Wed, Nov 14, 2018 at 11:06 AM
Subject: Re: [Openvpn-devel] [PATCH v5 2/2] Add support for OpenSSL TLS 1.3 when using management-external-key
To: Arne Schwabe <[email protected]...>
Hi,
Quoted 10 lines · Arne Schwabe
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
management-external-key
Quoted 2 lines
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
would
Quoted 3 lines
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
opinion)
Quoted 15 lines
Quoted 10 lines
Quoted 2 lines
Supporting a half working
RSA_SIGoption 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.
Quoted 5 lines
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.
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?
Selva
Hi,
A thought: why not split this patch into two:
(i) extend PK_SIGN to optionally signal ALG (signalled only if client_version > 2). Include all the changes to rsa_priv_enc() etc to to handle PSS sign requests from OpenSSL 1.1.1.
If client version is <= 2 continue to use PK_SIGN as before provided the signature required is PKCS1 for RSA or ECDSA. Else error out manage.c.
This ensures backward compat to the extent possible.
(ii) Amend management-external-key to take an additional option and do whatever one can do with it for an early error report.
Anyway, my suggestion is not even bother with (ii) but this way we can quickly get (i) finalized.
Unless you already decided to drop (ii) :)
Only downside (or upside depending on your pov) to this is once (i) is merged in we will start including ALG in PK_SIGN for new clients (version 3+) so if merging (ii), that should happen before a subsequent release.
Selva
Quoted 9 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
2. We will not error out but send the signature request asPK_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 9 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.
Arne
Quoted 6 lines · Arne Schwabe
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
Quoted 9 lines
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 failWe 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?
True, but by same argument then we need not worry about backward compatibility
of management protocol at all, do we? We could say, why would any one use openvpn.exe that speaks MI version 3 with a client that cannot support it.
Anyway, for backward compat its enough to check client version and then either error out if ALG is not PKCS1 or ECDSA or add ALG to PK_SIGN.
I fail to appreciate the utility of early erroring out feature -- it was meaningful
in v1 where you had tried to downgrade to TLS 1.2 to plough along without causing an error. But that turned out to be not enough.
Quoted 6 lines
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
Quoted 9 lines
worth it.
Quoted 3 lines
Am I missing something?
tls_version_maxwill 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.
Selva
Quoted 19 lines
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_maxwill 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 4 lines
Aha, I missed that. Still I really do not understand the need for erroring here
instead of when prompting forPK_SIGNbased on client version. Much simpler.
It did this because my initial OpenSSL 1.1.1 client did not have any problem, only after I upgraded the server to 1.1.1. I want to avoid the situation that OpenSSL 1.1.1 is used and then just "breaks for no reason" and erroring out early and tell the "This will not really work" is a better approach.
Arne
Re: [Openvpn-devel] [PATCH v2 1/2] Make tls_version_max return the actual maximum version
In reply to Arne SchwabeHi,
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.
-Steffan
Re: [Openvpn-devel] [PATCH v2 1/2] Make tls_version_max return the actual maximum version
Am 05.12.18 um 11:51 schrieb Steffan Karger:
Quoted 14 lines
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.
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.
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"
Arne
Re: [Openvpn-devel] [PATCH v2 1/2] Make tls_version_max return the actual maximum version
In reply to a message not in the archiveHi,
Quoted 38 lines · Steffan Karger
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"
Quoted 6 lines
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.
I was of the view that we should support upgrade to OpenSSL 1.1.1 without recompiling, but on closer encounters with 1.1.1, I have a change of mind.
In our case, the only real problem is: if OpenSSL 1.1.1 is in use at run time, external certificate support is broken and needs fixing. But does that need this patch? Two cases to consider:
(i) compiled against 1.1.0 but the dynamic linker loads 1.1.1 (ii) compiled and run against 1.1.1
For fixing "management-external-cert", case (i) may need this but case (ii) doesn't. That said, the ABI compat between 1.1.0 and 1.1.1 that OpenSSL claims has caveats, so we need not promote use case (i). Just warn this may not work and let things fail if and when it fails.
Even in case of (ii) my view is that we should just let external signing fail if the management client is not capable of handling the signature request (cases where the data is either prepadded or requires PSS padding). Such an approach still requires some changes to the code to support non PKCS1 padding but that's the topic of a different set of patches.
Selva
Selva
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)
Arne
Forgot copy this to the list -- sending again
Quoted 20 lines · Arne Schwabe
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.
Quoted 8 lines
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)
Well, I can live with that --- at least we'll be able to tell the users to update their client to get the signature request, which is not the case now.
Selva