| Message ID | 20191122143315.8564-2-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,v7,1/2] Make tls_version_max return the actual maximum version | expand |
Tiny grammar concern: On 22/11/2019 14:33, Arne Schwabe wrote: > For TLS 1.0 to 1.2 and OpenSSL 1.1.0 calls us and requires a PKCS1 > padded response. As TLS 1.3 mandates RSA-PSS padding support and also > requires an TLS 1.3 implementation to support RSA-PSS for older TLS > version, OpenSSL will query us to sign an already RSA-PSS padded > string. > > This patch adds an 'unpadded' and 'pkcs1' parameter to the > management-external-key option to signal that the client is > able to support pkcs1 as well as unpadded signature requests. > > Since clients that implement the management-external-key interface > are usually rather tightly integrated solutions (OpenVPN Connect in the > past, OpenVPN for Android), it is reasonable to expect that > upgrading the OpenSSL library can be done together with > management interface changes. Therefore we provide no backwards > compatbility for mangement-interface clients not supporting > OpenSSL 1.1.1. > > Using the management api client version instead might seem like the > more logical way but since we only now that version very late, > it would extra logic and complexity to deal with this asynchronous > behaviour. Instead just give an error early if OpenSSL 1.1.1 and > management-external-key without nopadding is detected. > > The interface is prepared for signalling PCKS1 and RSA-PSS support > instead of signalling unpadded support. > > Patch v3: fix overlong lines and few other style patches. Note > two overlong lines concerning mbedtls are not fixed as they > are removed/shortend by the mbed tls patch to avoid conflicts > > Patch v4: Setting minimum TLS version proved to be not enough and > instead of implementing a whole compability layer we require > mangement-clients to implement the new feature when they want > to use OpenSSL 1.1.1 > > Add a padding=ALGORITHM argument to pk-sig to indicate the > algorithm. Drop adding PKCS1 ourselves. > > Patch v5: Send the right version of the patch > Patch v6: rebase on master > Patch v7: change style and reword documentation. Make thing more consistent > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> > --- > doc/management-notes.txt | 21 ++++++++++++- > doc/openvpn.8 | 7 +++-- > src/openvpn/manage.c | 20 +++++++++--- > src/openvpn/manage.h | 19 +++++++----- > src/openvpn/options.c | 36 +++++++++++++++++++++- > src/openvpn/ssl_mbedtls.c | 8 +++-- > src/openvpn/ssl_openssl.c | 64 ++++++++++++++++++++++++++++++--------- > 7 files changed, 143 insertions(+), 32 deletions(-) > > diff --git a/doc/management-notes.txt b/doc/management-notes.txt > index 17645c1d..c64e594d 100644 > --- a/doc/management-notes.txt > +++ b/doc/management-notes.txt > @@ -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,18 @@ 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 signature algorithm is indicated in the PK_SIGN request only if the > +management client-version is >= 2. In particular, to support TLS1.3 and > +TLS1.2 using OpenSSL 1.1.1, not padded signature support is required and this "not padded" -> "unpadded" > +can be indicated in the signing request only if the client version is > 2" > + > +The currently defined padding algorithm are: > + > + - RSA_PKCS1_PADDING - PKCS1 padding and RSA signature > + - RSA_NO_PADDING - No padding may be added for the signature > + - ECDSA - EC signature. > + > + > COMMAND -- certificate (OpenVPN 2.4 or higher) > ---------------------------------------------- > Provides support for external storage of the certificate. Requires the > diff --git a/doc/openvpn.8 b/doc/openvpn.8 > index 457c2667..28d4516a 100644 > --- a/doc/openvpn.8 > +++ b/doc/openvpn.8 > @@ -2708,10 +2708,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 > diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c > index 1d97c2b6..49864c0a 100644 > --- a/src/openvpn/manage.c > +++ b/src/openvpn/manage.c > @@ -3641,18 +3641,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 *algorithm) > { > const char *prompt = "PK_SIGN"; > const char *desc = "pk-sign"; > + struct buffer buf_data = alloc_buf(strlen(b64_data) + strlen(algorithm) + 20); > + > if (man->connection.client_version <= 1) > { > prompt = "RSA_SIGN"; > desc = "rsa-sign"; > } > - return management_query_multiline_flatten(man, b64_data, prompt, desc, > - &man->connection.ext_key_state, &man->connection.ext_key_input); > + > + 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, algorithm, (int) strlen(algorithm)); > + } > + 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 * > diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h > index d24abe09..6f5f34c1 100644 > --- a/src/openvpn/manage.h > +++ b/src/openvpn/manage.h > @@ -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,14 @@ 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) > > bool management_open(struct management *man, > const char *addr, > @@ -430,7 +432,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 *algorithm); > > char *management_query_cert(struct management *man, const char *cert_name); > > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index c282b582..ebe553af 100644 > --- a/src/openvpn/options.c > +++ b/src/openvpn/options.c > @@ -2171,6 +2171,16 @@ options_postprocess_verify_ce(const struct options *options, const struct connec > > #endif /* ifdef ENABLE_MANAGEMENT */ > > +#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"); > + } > +#endif > /* > * Windows-specific options. > */ > @@ -5241,9 +5251,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]) > + 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 (streq(p[j], "pkcs1")) > + { > + options->management_flags |= MF_EXTERNAL_KEY_PKCS1PAD; > + } > + else > + { > + msg(msglevel, "Unknown management-external-key flag: %s", p[j]); > + } > + } > + /* > + * 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; > + } > options->management_flags |= MF_EXTERNAL_KEY; > } > else if (streq(p[0], "management-external-cert") && p[1] && !p[2]) > diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c > index a4197cba..6fdef406 100644 > --- a/src/openvpn/ssl_mbedtls.c > +++ b/src/openvpn/ssl_mbedtls.c > @@ -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,12 @@ 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 support RSA external keys and PKCS1 signatures at the moment > + * in mbed TLS, so the signature parameter is hardcoded to this encoding > + */ > + if (!(dst_b64 = management_query_pk_sig(management, src_b64, > + "RSA_PKCS1_PADDING"))) > { > goto cleanup; > } > diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c > index a080338e..b7cfc77a 100644 > --- a/src/openvpn/ssl_openssl.c > +++ b/src/openvpn/ssl_openssl.c > @@ -225,7 +225,9 @@ tls_version_max(void) > * However, the library we are *linked* against might be 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 > + * upgraded to 1.1.1. > + * We only need need to this check for OpenSSL versions that can be > + * upgraded to 1.1.1 without recompile (>= 1.1.0) > */ > if (OpenSSL_version_num() >= 0x1010100fL) > { > @@ -1133,24 +1135,52 @@ openvpn_extkey_rsa_finish(RSA *rsa) > return 1; > } > > -/* 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. > - * Return value is signature length or -1 on error. > +/* > + * Convert OpenSSL's constant to the strings used in the management > + * interface query > + */ > +const char * > +get_rsa_padding_name (const int padding) > +{ > + switch (padding) > + { > + case RSA_PKCS1_PADDING: > + return "RSA_PKCS1_PADDING"; > + > + case RSA_NO_PADDING: > + return "RSA_NO_PADDING"; > + > + default: > + return "UNKNOWN"; > + } > +} > + > +/** > + * Pass the input hash in 'dgst' to management and get the signature back. > + * > + * @param dgst hash to be signed > + * @param dgstlen len of data in dgst > + * @param sig On successful return signature is in sig. > + * @param siglen length of buffer sig > + * @param algorithm padding/hasing algorithm for the signature > + * > + * @return 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, > + const char *algorithm) > { > char *in_b64 = NULL; > char *out_b64 = NULL; > int len = -1; > > - /* 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, algorithm); > + > } > if (out_b64) > { > @@ -1164,18 +1194,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; > > - 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, get_rsa_padding_name (padding)); > > return (ret == len) ? ret : -1; > } > @@ -1271,7 +1302,12 @@ 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); > + /* > + * ECDSA does not seem to have proper constants for paddings since > + * there are only signatures without padding at the moment, use > + * a generic ECDSA for the moment > + */ > + int len = get_sig_from_man(dgst, dgstlen, sig, capacity, "ECDSA"); > > if (len > 0) > { >
Hi, Thanks for the updates. In spite of several nits below, I'm ACKing this. All remarks are typos or grammar, important only for docs and some comments. I suggest to handle these as a minor follow up patch. I'm also ignoring most typos in commit message except a few that could be corrected during merge to add clarity. On Fri, Nov 22, 2019 at 9:34 AM Arne Schwabe <arne@rfc2549.org> wrote: > 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, > now --> know it would extra logic and complexity to deal with this asynchronous > would --> would require 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 > Patch v6: rebase on master > Patch v7: change style and reword documentation. Make thing more consistent > Signed-off-by: Arne Schwabe <arne@rfc2549.org> > --- > doc/management-notes.txt | 21 ++++++++++++- > doc/openvpn.8 | 7 +++-- > src/openvpn/manage.c | 20 +++++++++--- > src/openvpn/manage.h | 19 +++++++----- > src/openvpn/options.c | 36 +++++++++++++++++++++- > src/openvpn/ssl_mbedtls.c | 8 +++-- > src/openvpn/ssl_openssl.c | 64 ++++++++++++++++++++++++++++++--------- > 7 files changed, 143 insertions(+), 32 deletions(-) > > diff --git a/doc/management-notes.txt b/doc/management-notes.txt > index 17645c1d..c64e594d 100644 > --- a/doc/management-notes.txt > +++ b/doc/management-notes.txt > @@ -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 > "interface expects" or" interfaces expect" +(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. > Remove this statement. Its no longer correct after the latest changes. Anyway this info is repeated below with correct keywords. > This capability is intended to allow the use of arbitrary cryptographic > service providers with OpenVPN via the management interface. > @@ -840,6 +847,18 @@ 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 signature algorithm is indicated in the PK_SIGN request only if the > +management client-version is >= 2. In particular, to support TLS1.3 and > I think that should be "is > 2", not "is >= 2" +TLS1.2 using OpenSSL 1.1.1, not padded signature support is required and > this > "not padded" --> "unpadded" or "nonpadded" (former is more consistent with the rest) > +can be indicated in the signing request only if the client version is > 2" > + > +The currently defined padding algorithm are: > algorithm --> algorithms + > + - RSA_PKCS1_PADDING - PKCS1 padding and RSA signature > + - RSA_NO_PADDING - No padding may be added for the signature > + - ECDSA - EC signature. > + > + > COMMAND -- certificate (OpenVPN 2.4 or higher) > ---------------------------------------------- > Provides support for external storage of the certificate. Requires the > diff --git a/doc/openvpn.8 b/doc/openvpn.8 > index 457c2667..28d4516a 100644 > --- a/doc/openvpn.8 > +++ b/doc/openvpn.8 > @@ -2708,10 +2708,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 > format that as The optional parameters .B nopadding and .B pkcs1 signal support for .... +doc/mangement-notes.txt for a complete description of this > +feature. > .\"********************************************************* > .TP > .B \-\-management\-external\-cert certificate\-hint > diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c > index 1d97c2b6..49864c0a 100644 > --- a/src/openvpn/manage.c > +++ b/src/openvpn/manage.c > @@ -3641,18 +3641,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 *algorithm) > { > const char *prompt = "PK_SIGN"; > const char *desc = "pk-sign"; > + struct buffer buf_data = alloc_buf(strlen(b64_data) + > strlen(algorithm) + 20); > + > if (man->connection.client_version <= 1) > { > prompt = "RSA_SIGN"; > desc = "rsa-sign"; > } > - return management_query_multiline_flatten(man, b64_data, prompt, desc, > - > &man->connection.ext_key_state, &man->connection.ext_key_input); > + > + 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, algorithm, (int) strlen(algorithm)); > + } > + 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 * > diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h > index d24abe09..6f5f34c1 100644 > --- a/src/openvpn/manage.h > +++ b/src/openvpn/manage.h > @@ -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,14 @@ 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) > > bool management_open(struct management *man, > const char *addr, > @@ -430,7 +432,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 *algorithm); > > char *management_query_cert(struct management *man, const char > *cert_name); > > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index c282b582..ebe553af 100644 > --- a/src/openvpn/options.c > +++ b/src/openvpn/options.c > @@ -2171,6 +2171,16 @@ options_postprocess_verify_ce(const struct options > *options, const struct connec > > #endif /* ifdef ENABLE_MANAGEMENT */ > > +#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"); > + } > +#endif > /* > * Windows-specific options. > */ > @@ -5241,9 +5251,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]) > + 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 (streq(p[j], "pkcs1")) > + { > + options->management_flags |= MF_EXTERNAL_KEY_PKCS1PAD; > + } > + else > + { > + msg(msglevel, "Unknown management-external-key flag: %s", > p[j]); > + } > + } > + /* > + * 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; > + } > options->management_flags |= MF_EXTERNAL_KEY; > } > else if (streq(p[0], "management-external-cert") && p[1] && !p[2]) > diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c > index a4197cba..6fdef406 100644 > --- a/src/openvpn/ssl_mbedtls.c > +++ b/src/openvpn/ssl_mbedtls.c > @@ -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,12 @@ 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 support RSA external keys and PKCS1 signatures at the > moment > + * in mbed TLS, so the signature parameter is hardcoded to this > encoding > + */ > + if (!(dst_b64 = management_query_pk_sig(management, src_b64, > + "RSA_PKCS1_PADDING"))) > { > goto cleanup; > } > diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c > index a080338e..b7cfc77a 100644 > --- a/src/openvpn/ssl_openssl.c > +++ b/src/openvpn/ssl_openssl.c > @@ -225,7 +225,9 @@ tls_version_max(void) > * However, the library we are *linked* against might be 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 > + * upgraded to 1.1.1. > + * We only need need to this check for OpenSSL versions that can be > need need to this check -> need to check this + * upgraded to 1.1.1 without recompile (>= 1.1.0) > */ > if (OpenSSL_version_num() >= 0x1010100fL) > { > @@ -1133,24 +1135,52 @@ openvpn_extkey_rsa_finish(RSA *rsa) > return 1; > } > > -/* 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. > - * Return value is signature length or -1 on error. > +/* > + * Convert OpenSSL's constant to the strings used in the management > + * interface query > + */ > +const char * > +get_rsa_padding_name (const int padding) > +{ > + switch (padding) > + { > + case RSA_PKCS1_PADDING: > + return "RSA_PKCS1_PADDING"; > + > + case RSA_NO_PADDING: > + return "RSA_NO_PADDING"; > + > + default: > + return "UNKNOWN"; > + } > +} > + > +/** > + * Pass the input hash in 'dgst' to management and get the signature back. > + * > + * @param dgst hash to be signed > + * @param dgstlen len of data in dgst > + * @param sig On successful return signature is in sig. > + * @param siglen length of buffer sig > + * @param algorithm padding/hasing algorithm for the signature > hasing -> hashing + * > + * @return 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, > + const char *algorithm) > { > char *in_b64 = NULL; > char *out_b64 = NULL; > int len = -1; > > - /* 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, algorithm); > + > } > if (out_b64) > { > @@ -1164,18 +1194,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; > > - 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, get_rsa_padding_name > (padding)); > > return (ret == len) ? ret : -1; > } > @@ -1271,7 +1302,12 @@ 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); > + /* > + * ECDSA does not seem to have proper constants for paddings since > + * there are only signatures without padding at the moment, use > + * a generic ECDSA for the moment > + */ > + int len = get_sig_from_man(dgst, dgstlen, sig, capacity, "ECDSA"); > > if (len > 0) > { > > Tested for RSA and EC signatures using (i) an OpenSSL 1.1.1 build (ii) an OpenSSL 1.1.0 build upgraded to 1.1.1 at run time. Acked-by: selva.nair@gmail.com <div dir="ltr"><div dir="ltr"><div>Hi,</div><div><br></div><div>Thanks for the updates.<br></div><div><br></div><div>In spite of several nits below, I'm ACKing this. <br></div><div><br></div><div>All remarks are typos or grammar, important only for docs</div><div>and some comments. I suggest to handle these as a minor follow</div><div>up patch. </div></div><div dir="ltr"><div><br></div><div>I'm also ignoring most typos in commit message except a few that</div><div>could be corrected during merge to add clarity.</div><div><br></div></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Nov 22, 2019 at 9:34 AM Arne Schwabe <<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">For TLS 1.0 to 1.2 and OpenSSL 1.1.0 calls us and requires a PKCS1 </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> padded response. As TLS 1.3 mandates RSA-PSS padding support and also<br> requires an TLS 1.3 implementation to support RSA-PSS for older TLS </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> version, OpenSSL will query us to sign an already RSA-PSS padded</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> string.<br> <br> This patch adds an 'unpadded' and 'pkcs1' parameter to the</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> management-external-key option to signal that the client is<br> able to support pkcs1 as well as unpadded signature requests.<br> <br> Since clients that implement the management-external-key interface<br> are usually rather tightly integrated solutions (OpenVPN Connect in the<br> past, OpenVPN for Android), it is reasonable to expect that<br> upgrading the OpenSSL library can be done together with<br> management interface changes. Therefore we provide no backwards<br> compatbility for mangement-interface clients not supporting<br> OpenSSL 1.1.1.<br> <br> Using the management api client version instead might seem like the<br> more logical way but since we only now that version very late,<br></blockquote><div><br></div><div>now --> know</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> it would extra logic and complexity to deal with this asynchronous<br></blockquote><div><br></div><div>would --> would require</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> behaviour. Instead just give an error early if OpenSSL 1.1.1 and<br> management-external-key without nopadding is detected.<br> <br> The interface is prepared for signalling PCKS1 and RSA-PSS support<br> instead of signalling unpadded support.<br> <br> Patch v3: fix overlong lines and few other style patches. Note<br> two overlong lines concerning mbedtls are not fixed as they<br> are removed/shortend by the mbed tls patch to avoid conflicts<br> <br> Patch v4: Setting minimum TLS version proved to be not enough and<br> instead of implementing a whole compability layer we require </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> mangement-clients to implement the new feature when they want<br> to use OpenSSL 1.1.1<br> <br> Add a padding=ALGORITHM argument to pk-sig to indicate the<br> algorithm. Drop adding PKCS1 ourselves.</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> Patch v5: Send the right version of the patch<br> Patch v6: rebase on master<br> Patch v7: change style and reword documentation. Make thing more consistent</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> Signed-off-by: Arne Schwabe <<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>><br> ---<br> doc/management-notes.txt | 21 ++++++++++++-<br> doc/openvpn.8 | 7 +++--<br> src/openvpn/manage.c | 20 +++++++++---<br> src/openvpn/manage.h | 19 +++++++-----<br> src/openvpn/options.c | 36 +++++++++++++++++++++-<br> src/openvpn/ssl_mbedtls.c | 8 +++--<br> src/openvpn/ssl_openssl.c | 64 ++++++++++++++++++++++++++++++---------<br> 7 files changed, 143 insertions(+), 32 deletions(-)<br> <br> diff --git a/doc/management-notes.txt b/doc/management-notes.txt<br> index 17645c1d..c64e594d 100644<br> --- a/doc/management-notes.txt<br> +++ b/doc/management-notes.txt<br> @@ -816,6 +816,7 @@ actual private key. When the SSL protocol needs to perform a sign<br> operation, the data to be signed will be sent to the management interface<br> via a notification as follows:<br> <br> +>PK_SIGN:[BASE64_DATA],[ALG] (if client announces support for management version > 2)<br> >PK_SIGN:[BASE64_DATA] (if client announces support for management version > 1)<br> >RSA_SIGN:[BASE64_DATA] (only older clients will be prompted like this)<br> <br> @@ -823,7 +824,7 @@ The management interface client should then create an appropriate signature of<br> the (decoded) BASE64_DATA using the private key and return the SSL signature as<br> follows:<br> <br> -pk-sig (or rsa-sig)<br> +pk-sig (or rsa-sig)<br> [BASE64_SIG_LINE]<br> .<br> .<br> @@ -833,6 +834,12 @@ END<br> Base 64 encoded output of RSA_private_encrypt for RSA or ECDSA_sign()<br> for EC using OpenSSL or mbedtls_pk_sign() using mbed TLS will provide a<br> correct signature.<br> +The rsa-sig interfaces expects PKCS1 padded signatures for RSA keys<br></blockquote><div><br></div><div>"interface expects" or" interfaces expect"<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +(RSA_PKCS1_PADDING). EC signatures are always unpadded.<br> +<br> +The padding field is only present when pk-sig is used and<br> +currently the following values can be requested PCKS1 and NOPADDING for RSA<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +certificates and NOPADDING for EC certificates.<br></blockquote><div><br></div><div>Remove this statement. Its no longer correct after the latest</div><div>changes. Anyway this info is repeated below with correct keywords.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> This capability is intended to allow the use of arbitrary cryptographic<br> service providers with OpenVPN via the management interface.<br> @@ -840,6 +847,18 @@ service providers with OpenVPN via the management interface.<br> New and updated clients are expected to use the version command to announce<br> a version > 1 and handle '>PK_SIGN' prompt and respond with 'pk-sig'.<br> <br> +The signature algorithm is indicated in the PK_SIGN request only if the<br> +management client-version is >= 2. In particular, to support TLS1.3 and<br></blockquote><div><br></div><div>I think that should be "is > 2", not "is >= 2"<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +TLS1.2 using OpenSSL 1.1.1, not padded signature support is required and this<br></blockquote><div><br></div><div>"not padded" --> "unpadded" or "nonpadded" (former is more consistent with the rest)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +can be indicated in the signing request only if the client version is > 2"<br> +<br> +The currently defined padding algorithm are:<br></blockquote><div><br></div><div>algorithm --> algorithms</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +<br> + - RSA_PKCS1_PADDING - PKCS1 padding and RSA signature<br> + - RSA_NO_PADDING - No padding may be added for the signature <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + - ECDSA - EC signature.<br> +<br> +<br> COMMAND -- certificate (OpenVPN 2.4 or higher)<br> ----------------------------------------------<br> Provides support for external storage of the certificate. Requires the<br> diff --git a/doc/openvpn.8 b/doc/openvpn.8<br> index 457c2667..28d4516a 100644<br> --- a/doc/openvpn.8<br> +++ b/doc/openvpn.8<br> @@ -2708,10 +2708,13 @@ Allow management interface to override<br> directives (client\-only).<br> .\"*********************************************************<br> .TP<br> -.B \-\-management\-external\-key<br> +.B \-\-management\-external\-key [nopadding] [pkcs1]<br> Allows usage for external private key file instead of<br> .B \-\-key<br> -option (client\-only).<br> +option (client\-only). The optional parameters nopadding and<br> +pkcs1 signal support for different padding algorithms. See<br></blockquote><div><br></div><div>format that as</div><div><br></div><div>The optional parameters<br></div><div>.B nopadding</div><div>and<br></div><div>.B pkcs1<br></div><div>signal support for ....<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +doc/mangement-notes.txt for a complete description of this<br> +feature.<br> .\"*********************************************************<br> .TP<br> .B \-\-management\-external\-cert certificate\-hint<br> diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c<br> index 1d97c2b6..49864c0a 100644<br> --- a/src/openvpn/manage.c<br> +++ b/src/openvpn/manage.c<br> @@ -3641,18 +3641,30 @@ management_query_multiline_flatten(struct management *man,<br> <br> char *<br> /* returns allocated base64 signature */<br> -management_query_pk_sig(struct management *man,<br> - const char *b64_data)<br> +management_query_pk_sig(struct management *man, const char *b64_data,<br> + const char *algorithm)<br> {<br> const char *prompt = "PK_SIGN";<br> const char *desc = "pk-sign";<br> + struct buffer buf_data = alloc_buf(strlen(b64_data) + strlen(algorithm) + 20);<br> +<br> if (man->connection.client_version <= 1)<br> {<br> prompt = "RSA_SIGN";<br> desc = "rsa-sign";<br> }<br> - return management_query_multiline_flatten(man, b64_data, prompt, desc,<br> - &man->connection.ext_key_state, &man->connection.ext_key_input);<br> +<br> + buf_write(&buf_data, b64_data, (int) strlen(b64_data));<br> + if (man->connection.client_version > 2)<br> + {<br> + buf_write(&buf_data, ",", (int) strlen(","));<br> + buf_write(&buf_data, algorithm, (int) strlen(algorithm));<br> + }<br> + char* ret = management_query_multiline_flatten(man,<br> + (char *)buf_bptr(&buf_data), prompt, desc,<br> + &man->connection.ext_key_state, &man->connection.ext_key_input);<br> + free_buf(&buf_data);<br> + return ret;<br> }<br> <br> char *<br> diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h<br> index d24abe09..6f5f34c1 100644<br> --- a/src/openvpn/manage.h<br> +++ b/src/openvpn/manage.h<br> @@ -31,7 +31,7 @@<br> #include "socket.h"<br> #include "mroute.h"<br> <br> -#define MANAGEMENT_VERSION 2<br> +#define MANAGEMENT_VERSION 3<br> #define MANAGEMENT_N_PASSWORD_RETRIES 3<br> #define MANAGEMENT_LOG_HISTORY_INITIAL_SIZE 100<br> #define MANAGEMENT_ECHO_BUFFER_SIZE 100<br> @@ -341,12 +341,14 @@ struct management *management_init(void);<br> #ifdef MANAGEMENT_PF<br> #define MF_CLIENT_PF (1<<7)<br> #endif<br> -#define MF_UNIX_SOCK (1<<8)<br> -#define MF_EXTERNAL_KEY (1<<9)<br> -#define MF_UP_DOWN (1<<10)<br> -#define MF_QUERY_REMOTE (1<<11)<br> -#define MF_QUERY_PROXY (1<<12)<br> -#define MF_EXTERNAL_CERT (1<<13)<br> +#define MF_UNIX_SOCK (1<<8)<br> +#define MF_EXTERNAL_KEY (1<<9)<br> +#define MF_EXTERNAL_KEY_NOPADDING (1<<10)<br> +#define MF_EXTERNAL_KEY_PKCS1PAD (1<<11)<br> +#define MF_UP_DOWN (1<<12)<br> +#define MF_QUERY_REMOTE (1<<13)<br> +#define MF_QUERY_PROXY (1<<14)<br> +#define MF_EXTERNAL_CERT (1<<15)<br> <br> bool management_open(struct management *man,<br> const char *addr,<br> @@ -430,7 +432,8 @@ void management_learn_addr(struct management *management,<br> <br> #endif<br> <br> -char *management_query_pk_sig(struct management *man, const char *b64_data);<br> +char *management_query_pk_sig(struct management *man, const char *b64_data,<br> + const char *algorithm);<br> <br> char *management_query_cert(struct management *man, const char *cert_name);<br> <br> diff --git a/src/openvpn/options.c b/src/openvpn/options.c<br> index c282b582..ebe553af 100644<br> --- a/src/openvpn/options.c<br> +++ b/src/openvpn/options.c<br> @@ -2171,6 +2171,16 @@ options_postprocess_verify_ce(const struct options *options, const struct connec<br> <br> #endif /* ifdef ENABLE_MANAGEMENT */<br> <br> +#if defined(ENABLE_MANAGEMENT)<br> + if ((tls_version_max() >= TLS_VER_1_3)<br> + && (options->management_flags & MF_EXTERNAL_KEY)<br> + && !(options->management_flags & (MF_EXTERNAL_KEY_NOPADDING))<br> + )<br> + {<br> + msg(M_ERR, "management-external-key with OpenSSL 1.1.1 requires "<br> + "the nopadding argument/support");<br> + }<br> +#endif<br> /*<br> * Windows-specific options.<br> */<br> @@ -5241,9 +5251,33 @@ add_option(struct options *options,<br> options->management_write_peer_info_file = p[1];<br> }<br> #ifdef ENABLE_MANAGEMENT<br> - else if (streq(p[0], "management-external-key") && !p[1])<br> + else if (streq(p[0], "management-external-key"))<br> {<br> VERIFY_PERMISSION(OPT_P_GENERAL);<br> + for (int j = 1; j < MAX_PARMS && p[j] != NULL; ++j)<br> + {<br> + if (streq(p[j], "nopadding"))<br> + {<br> + options->management_flags |= MF_EXTERNAL_KEY_NOPADDING;<br> + }<br> + else if (streq(p[j], "pkcs1"))<br> + {<br> + options->management_flags |= MF_EXTERNAL_KEY_PKCS1PAD;<br> + }<br> + else<br> + {<br> + msg(msglevel, "Unknown management-external-key flag: %s", p[j]);<br> + }<br> + }<br> + /*<br> + * When no option is present, assume that only PKCS1<br> + * padding is supported<br> + */<br> + if (!(options->management_flags<br> + &(MF_EXTERNAL_KEY_NOPADDING | MF_EXTERNAL_KEY_PKCS1PAD)))<br> + {<br> + options->management_flags |= MF_EXTERNAL_KEY_PKCS1PAD;<br> + }<br> options->management_flags |= MF_EXTERNAL_KEY;<br> }<br> else if (streq(p[0], "management-external-cert") && p[1] && !p[2])<br> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c<br> index a4197cba..6fdef406 100644<br> --- a/src/openvpn/ssl_mbedtls.c<br> +++ b/src/openvpn/ssl_mbedtls.c<br> @@ -626,7 +626,6 @@ tls_ctx_use_external_signing_func(struct tls_root_ctx *ctx,<br> }<br> <br> #ifdef ENABLE_MANAGEMENT<br> -<br> /** Query the management interface for a signature, see external_sign_func. */<br> static bool<br> management_sign_func(void *sign_ctx, const void *src, size_t src_len,<br> @@ -641,7 +640,12 @@ management_sign_func(void *sign_ctx, const void *src, size_t src_len,<br> goto cleanup;<br> }<br> <br> - if (!(dst_b64 = management_query_pk_sig(management, src_b64)))<br> + /*<br> + * We only support RSA external keys and PKCS1 signatures at the moment<br> + * in mbed TLS, so the signature parameter is hardcoded to this encoding<br> + */<br> + if (!(dst_b64 = management_query_pk_sig(management, src_b64,<br> + "RSA_PKCS1_PADDING")))<br> {<br> goto cleanup;<br> }<br> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c<br> index a080338e..b7cfc77a 100644<br> --- a/src/openvpn/ssl_openssl.c<br> +++ b/src/openvpn/ssl_openssl.c<br> @@ -225,7 +225,9 @@ tls_version_max(void)<br> * However, the library we are *linked* against might be OpenSSL 1.1.1<br> * and therefore supports TLS 1.3. This needs to be checked at runtime<br> * since we can be compiled against 1.1.0 and then the library can be<br> - * upgraded to 1.1.1<br> + * upgraded to 1.1.1.<br> + * We only need need to this check for OpenSSL versions that can be<br></blockquote><div><br></div><div>need need to this check -> need to check this</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + * upgraded to 1.1.1 without recompile (>= 1.1.0)<br> */<br> if (OpenSSL_version_num() >= 0x1010100fL)<br> {<br> @@ -1133,24 +1135,52 @@ openvpn_extkey_rsa_finish(RSA *rsa)<br> return 1;<br> }<br> <br> -/* Pass the input hash in 'dgst' to management and get the signature back.<br> - * On input siglen contains the capacity of the buffer 'sig'.<br> - * On return signature is in sig.<br> - * Return value is signature length or -1 on error.<br> +/*<br> + * Convert OpenSSL's constant to the strings used in the management<br> + * interface query<br> + */<br> +const char *<br> +get_rsa_padding_name (const int padding)<br> +{<br> + switch (padding)<br> + {<br> + case RSA_PKCS1_PADDING:<br> + return "RSA_PKCS1_PADDING";<br> +<br> + case RSA_NO_PADDING:<br> + return "RSA_NO_PADDING";<br> +<br> + default:<br> + return "UNKNOWN";<br> + }<br> +}<br> +<br> +/**<br> + * Pass the input hash in 'dgst' to management and get the signature back.<br> + *<br> + * @param dgst hash to be signed<br> + * @param dgstlen len of data in dgst<br> + * @param sig On successful return signature is in sig.<br> + * @param siglen length of buffer sig<br> + * @param algorithm padding/hasing algorithm for the signature<br></blockquote><div><br></div><div>hasing -> hashing</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + *<br> + * @return signature length or -1 on error.<br> */<br> static int<br> get_sig_from_man(const unsigned char *dgst, unsigned int dgstlen,<br> - unsigned char *sig, unsigned int siglen)<br> + unsigned char *sig, unsigned int siglen,<br> + const char *algorithm)<br> {<br> char *in_b64 = NULL;<br> char *out_b64 = NULL;<br> int len = -1;<br> <br> - /* convert 'dgst' to base64 */<br> - if (management<br> - && openvpn_base64_encode(dgst, dgstlen, &in_b64) > 0)<br> + int bencret = openvpn_base64_encode(dgst, dgstlen, &in_b64);<br> +<br> + if (management && bencret > 0)<br> {<br> - out_b64 = management_query_pk_sig(management, in_b64);<br> + out_b64 = management_query_pk_sig(management, in_b64, algorithm);<br> +<br> }<br> if (out_b64)<br> {<br> @@ -1164,18 +1194,19 @@ get_sig_from_man(const unsigned char *dgst, unsigned int dgstlen,<br> <br> /* sign arbitrary data */<br> static int<br> -rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, int padding)<br> +rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa,<br> + int padding)<br> {<br> unsigned int len = RSA_size(rsa);<br> int ret = -1;<br> <br> - if (padding != RSA_PKCS1_PADDING)<br> + if (padding != RSA_PKCS1_PADDING && padding != RSA_NO_PADDING)<br> {<br> RSAerr(RSA_F_RSA_OSSL_PRIVATE_ENCRYPT, RSA_R_UNKNOWN_PADDING_TYPE);<br> return -1;<br> }<br> <br> - ret = get_sig_from_man(from, flen, to, len);<br> + ret = get_sig_from_man(from, flen, to, len, get_rsa_padding_name (padding));<br> <br> return (ret == len) ? ret : -1;<br> }<br> @@ -1271,7 +1302,12 @@ ecdsa_sign(int type, const unsigned char *dgst, int dgstlen, unsigned char *sig,<br> unsigned int *siglen, const BIGNUM *kinv, const BIGNUM *r, EC_KEY *ec)<br> {<br> int capacity = ECDSA_size(ec);<br> - int len = get_sig_from_man(dgst, dgstlen, sig, capacity);<br> + /*<br> + * ECDSA does not seem to have proper constants for paddings since<br> + * there are only signatures without padding at the moment, use<br> + * a generic ECDSA for the moment<br> + */<br> + int len = get_sig_from_man(dgst, dgstlen, sig, capacity, "ECDSA");<br> <br> if (len > 0)<br> {<br><br></blockquote><div><br></div><div>Tested for RSA and EC signatures using </div><div>(i) an OpenSSL 1.1.1 build</div><div>(ii) an OpenSSL 1.1.0 build upgraded to 1.1.1 at run time.</div><div><br></div><div>Acked-by: <a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a> <br></div></div></div>
Am 23.11.19 um 03:07 schrieb Selva Nair: > Hi, > > Thanks for the updates. > > In spite of several nits below, I'm ACKing this. > > All remarks are typos or grammar, important only for docs > and some comments. I suggest to handle these as a minor follow > up patch. > > I'm also ignoring most typos in commit message except a few that > could be corrected during merge to add clarity. > Thanks again for all the patience with me. I added those corrections to a v8 to make Gert's/David's job easier. Arne
diff --git a/doc/management-notes.txt b/doc/management-notes.txt index 17645c1d..c64e594d 100644 --- a/doc/management-notes.txt +++ b/doc/management-notes.txt @@ -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,18 @@ 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 signature algorithm is indicated in the PK_SIGN request only if the +management client-version is >= 2. In particular, to support TLS1.3 and +TLS1.2 using OpenSSL 1.1.1, not padded signature support is required and this +can be indicated in the signing request only if the client version is > 2" + +The currently defined padding algorithm are: + + - RSA_PKCS1_PADDING - PKCS1 padding and RSA signature + - RSA_NO_PADDING - No padding may be added for the signature + - ECDSA - EC signature. + + COMMAND -- certificate (OpenVPN 2.4 or higher) ---------------------------------------------- Provides support for external storage of the certificate. Requires the diff --git a/doc/openvpn.8 b/doc/openvpn.8 index 457c2667..28d4516a 100644 --- a/doc/openvpn.8 +++ b/doc/openvpn.8 @@ -2708,10 +2708,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 diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c index 1d97c2b6..49864c0a 100644 --- a/src/openvpn/manage.c +++ b/src/openvpn/manage.c @@ -3641,18 +3641,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 *algorithm) { const char *prompt = "PK_SIGN"; const char *desc = "pk-sign"; + struct buffer buf_data = alloc_buf(strlen(b64_data) + strlen(algorithm) + 20); + if (man->connection.client_version <= 1) { prompt = "RSA_SIGN"; desc = "rsa-sign"; } - return management_query_multiline_flatten(man, b64_data, prompt, desc, - &man->connection.ext_key_state, &man->connection.ext_key_input); + + 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, algorithm, (int) strlen(algorithm)); + } + 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 * diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h index d24abe09..6f5f34c1 100644 --- a/src/openvpn/manage.h +++ b/src/openvpn/manage.h @@ -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,14 @@ 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) bool management_open(struct management *man, const char *addr, @@ -430,7 +432,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 *algorithm); char *management_query_cert(struct management *man, const char *cert_name); diff --git a/src/openvpn/options.c b/src/openvpn/options.c index c282b582..ebe553af 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -2171,6 +2171,16 @@ options_postprocess_verify_ce(const struct options *options, const struct connec #endif /* ifdef ENABLE_MANAGEMENT */ +#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"); + } +#endif /* * Windows-specific options. */ @@ -5241,9 +5251,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]) + 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 (streq(p[j], "pkcs1")) + { + options->management_flags |= MF_EXTERNAL_KEY_PKCS1PAD; + } + else + { + msg(msglevel, "Unknown management-external-key flag: %s", p[j]); + } + } + /* + * 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; + } options->management_flags |= MF_EXTERNAL_KEY; } else if (streq(p[0], "management-external-cert") && p[1] && !p[2]) diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c index a4197cba..6fdef406 100644 --- a/src/openvpn/ssl_mbedtls.c +++ b/src/openvpn/ssl_mbedtls.c @@ -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,12 @@ 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 support RSA external keys and PKCS1 signatures at the moment + * in mbed TLS, so the signature parameter is hardcoded to this encoding + */ + if (!(dst_b64 = management_query_pk_sig(management, src_b64, + "RSA_PKCS1_PADDING"))) { goto cleanup; } diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c index a080338e..b7cfc77a 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -225,7 +225,9 @@ tls_version_max(void) * However, the library we are *linked* against might be 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 + * upgraded to 1.1.1. + * We only need need to this check for OpenSSL versions that can be + * upgraded to 1.1.1 without recompile (>= 1.1.0) */ if (OpenSSL_version_num() >= 0x1010100fL) { @@ -1133,24 +1135,52 @@ openvpn_extkey_rsa_finish(RSA *rsa) return 1; } -/* 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. - * Return value is signature length or -1 on error. +/* + * Convert OpenSSL's constant to the strings used in the management + * interface query + */ +const char * +get_rsa_padding_name (const int padding) +{ + switch (padding) + { + case RSA_PKCS1_PADDING: + return "RSA_PKCS1_PADDING"; + + case RSA_NO_PADDING: + return "RSA_NO_PADDING"; + + default: + return "UNKNOWN"; + } +} + +/** + * Pass the input hash in 'dgst' to management and get the signature back. + * + * @param dgst hash to be signed + * @param dgstlen len of data in dgst + * @param sig On successful return signature is in sig. + * @param siglen length of buffer sig + * @param algorithm padding/hasing algorithm for the signature + * + * @return 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, + const char *algorithm) { char *in_b64 = NULL; char *out_b64 = NULL; int len = -1; - /* 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, algorithm); + } if (out_b64) { @@ -1164,18 +1194,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; - 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, get_rsa_padding_name (padding)); return (ret == len) ? ret : -1; } @@ -1271,7 +1302,12 @@ 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); + /* + * ECDSA does not seem to have proper constants for paddings since + * there are only signatures without padding at the moment, use + * a generic ECDSA for the moment + */ + int len = get_sig_from_man(dgst, dgstlen, sig, capacity, "ECDSA"); if (len > 0) {
For TLS 1.0 to 1.2 and OpenSSL 1.1.0 calls us and requires a PKCS1 padded response. As TLS 1.3 mandates RSA-PSS padding support and also requires an TLS 1.3 implementation to support RSA-PSS for older TLS version, OpenSSL will query us to sign an already RSA-PSS padded string. This patch adds an 'unpadded' and 'pkcs1' parameter to the management-external-key option to signal that the client is able to support pkcs1 as well as unpadded signature requests. Since clients that implement the management-external-key interface are usually rather tightly integrated solutions (OpenVPN Connect in the past, OpenVPN for Android), it is reasonable to expect that upgrading the OpenSSL library can be done together with management interface changes. Therefore we provide no backwards compatbility for mangement-interface clients not supporting OpenSSL 1.1.1. Using the management api client version instead might seem like the more logical way but since we only now that version very late, it would extra logic and complexity to deal with this asynchronous behaviour. Instead just give an error early if OpenSSL 1.1.1 and management-external-key without nopadding is detected. The interface is prepared for signalling PCKS1 and RSA-PSS support instead of signalling unpadded support. Patch v3: fix overlong lines and few other style patches. Note two overlong lines concerning mbedtls are not fixed as they are removed/shortend by the mbed tls patch to avoid conflicts Patch v4: Setting minimum TLS version proved to be not enough and instead of implementing a whole compability layer we require mangement-clients to implement the new feature when they want to use OpenSSL 1.1.1 Add a padding=ALGORITHM argument to pk-sig to indicate the algorithm. Drop adding PKCS1 ourselves. Patch v5: Send the right version of the patch Patch v6: rebase on master Patch v7: change style and reword documentation. Make thing more consistent Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- doc/management-notes.txt | 21 ++++++++++++- doc/openvpn.8 | 7 +++-- src/openvpn/manage.c | 20 +++++++++--- src/openvpn/manage.h | 19 +++++++----- src/openvpn/options.c | 36 +++++++++++++++++++++- src/openvpn/ssl_mbedtls.c | 8 +++-- src/openvpn/ssl_openssl.c | 64 ++++++++++++++++++++++++++++++--------- 7 files changed, 143 insertions(+), 32 deletions(-)