| Message ID | 1515959073-10376-3-git-send-email-selva.nair@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support external EC cert/key using --management-external-xxx | expand |
Am 14.01.18 um 20:44 schrieb selva.nair@gmail.com: > From: Selva Nair <selva.nair@gmail.com> > > - This automatically supports EC certificates through > --management-external-cert > - EC signature request from management has the same format > as for rsa with >RSA_SIGN replaced by >ECDSA_SIGN > Response should be of the form 'ecdsa-sig' followed > by DER encoded signature as base64 followed by 'END' > Thanks for the code. I implemented and tested this on my Android client. It works very well. However I found some minor issues (see below). Tested-By: Arne Schwabe <arne@rfc2549.org> But first a high level issue. I think if you enable manange-external-key with an "old" ui that only understand rsa keys the OpenVPN client will hang after ECDSA prompt since the ui will never reply with anything. We might need an argument to management-external-key to say "yes I am ecdsa ready". > > static void > +man_ecdsa_sig(struct management *man) > +{ > + struct man_connection *mc = &man->connection; > + if (mc->ext_key_state == EKS_SOLICIT) > + { > + mc->ext_key_state = EKS_INPUT; > + mc->in_extra_cmd = IEC_ECDSA_SIGN; > + in_extra_reset(mc, IER_NEW); > + } > + else > + { > + msg(M_CLIENT, "ERROR: The ecdsa-sig command is not currently available"); > + } > +} > + This function is almost identical to man_rsa_sign. I would like to have them both combined into one and then called by man_ecdsa_sig/man_rsa_sig. > > > +char * > +management_query_ecdsa_sig(struct management *man, > + const char *b64_data) > +{ > + return management_query_multiline_flatten(man, b64_data, "ECDSA_SIGN", "ecdsa-sign", > + &man->connection.ext_key_state, &man->connection.ext_key_input); > +} Hm, could be merged with management_query_rsa_sig but since it a one liner I have no strong preference. > +#if OPENSSL_VERSION_NUMBER >= 0x10100001L && !defined(OPENSSL_NO_EC) > + > +/* called when EC_KEY is destroyed */ > +static void > +openvpn_extkey_ec_finish(EC_KEY *ec) > +{ > + /* release the method structure */ > + const EC_KEY_METHOD *ec_meth = EC_KEY_get_method(ec); > + EC_KEY_METHOD_free((EC_KEY_METHOD *) ec_meth); > +} > + > +/* EC_KEY_METHOD callback: sign(). > + * Sign the hash using EC key and return DER encoded signature in sig, > + * its length in siglen. Return value is 1 on success, 0 on error. > + */ > +static int > +ecdsa_sign(int type, const unsigned char *dgst, int dgstlen, unsigned char *sig, > + unsigned int *siglen, const BIGNUM *kinv, const BIGNUM *r, EC_KEY *ec) > +{ > + char *in_b64 = NULL; > + char *out_b64 = NULL; > + int ret = 0; > + int len; len could just as well be done als inline C99 decleration. But no problem. (Also I am not so firm on our new style here) > + > + /* convert 'from' to base64 * > + if (openvpn_base64_encode(dgst, dgstlen, &in_b64) <= 0) > + { > + goto done; > + } > + > + /* call MI for signature */ > + if (management) > + { > + out_b64 = management_query_ecdsa_sig(management, in_b64); > + } > + if (!out_b64) > + { > + goto done; > + } > + > + /* decode base64 signature to binary */ > + len = ECDSA_size(ec); > + *siglen = openvpn_base64_decode(out_b64, sig, len); > + if (*siglen > 0) > + { > + ret = 1; > + } > + > +done: > + if (in_b64) > + { > + free(in_b64); > + } > + if (out_b64) > + { > + free(out_b64); > + } > + return ret; > +} This method has a bit code duplication to rsa_priv_enc but I think it is still okay here since you have an rather ugly looking function that is hard to read. > + > +/* EC_KEY_METHOD callback: sign_setup(). We do no precomputations */ > +static int > +ecdsa_sign_setup(EC_KEY *ec, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp) > +{ > + return 1; > +} > + > +/* EC_KEY_METHOD callback: sign_sig(). > + * Sign the hash and return the result as a newly allocated ECDS_SIG > + * struct or NULL on error. > + */ > +static ECDSA_SIG * > +ecdsa_sign_sig(const unsigned char *dgst, int dgstlen, const BIGNUM *in_kinv, > + const BIGNUM *in_r, EC_KEY *ec) > +{ > + ECDSA_SIG *ecsig = NULL; > + int len = ECDSA_size(ec); > + struct gc_arena gc = gc_new(); > + > + unsigned char *buf = gc_malloc(len, false, &gc); > + if (1 != ecdsa_sign(0, dgst, dgstlen, buf, &len, NULL, NULL, ec)) I am not sure about 1!= style here. Might be okay. I would try to avoid this. len should be size_t. My compiler agrees: ssl_openssl.c:1215:48: warning: passing 'int *' to parameter of type 'unsigned int *' converts between pointers to integer types with different sign [-Wpointer-sign] if (1 != ecdsa_sign(0, dgst, dgstlen, buf, &len, NULL, NULL, ec)) ^~~~ ssl_openssl.c:1152:26: note: passing argument to parameter 'siglen' here unsigned int *siglen, const BIGNUM *kinv, const BIGNUM *r, EC_KEY *ec) ^ 1 warning generated. > int > tls_ctx_use_external_private_key(struct tls_root_ctx *ctx, > const char *cert_file, const char *cert_file_inline) > @@ -1156,11 +1310,26 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx, > goto err; > } > } > +#if OPENSSL_VERSION_NUMBER >= 0x10100001L && !defined(OPENSSL_NO_EC) > + else if (EVP_PKEY_get0_EC_KEY(pkey)) > + { > + if (!tls_ctx_use_external_ec_key(ctx, pkey)) > + { > + goto err; > + } > + } > + else > + { > + crypto_msg(M_WARN, "management-external-key requires an RSA or EC certificate"); > + goto err; > + } > +#else > else > { > crypto_msg(M_WARN, "management-external-key requires a RSA certificate"); If you touch that line you can as well fix the grammar in the existing string that you fixed in the new string :) Arne ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, Wow that was fast :) Thanks for the review. I'll provide a v2, but some quick remarks/rfc On Tue, Jan 16, 2018 at 5:40 PM, Arne Schwabe <arne@rfc2549.org> wrote: > Am 14.01.18 um 20:44 schrieb selva.nair@gmail.com: >> From: Selva Nair <selva.nair@gmail.com> >> >> - This automatically supports EC certificates through >> --management-external-cert >> - EC signature request from management has the same format >> as for rsa with >RSA_SIGN replaced by >ECDSA_SIGN >> Response should be of the form 'ecdsa-sig' followed >> by DER encoded signature as base64 followed by 'END' >> > > > Thanks for the code. I implemented and tested this on my Android client. > It works very well. However I found some minor issues (see below). > > Tested-By: Arne Schwabe <arne@rfc2549.org> > > But first a high level issue. I think if you enable manange-external-key > with an "old" ui that only understand rsa keys the OpenVPN client will > hang after ECDSA prompt since the ui will never reply with anything. > > We might need an argument to management-external-key to say "yes I am > ecdsa ready". This needs some thought as config option is not really a way of indicating client capabilities, or so it seems. I can see this could pose a problem for clients that currently support external key. If there are only a few such known clients we can handle this transparently by looking at IV_GUI_VER? >> >> static void >> +man_ecdsa_sig(struct management *man) >> +{ >> + struct man_connection *mc = &man->connection; >> + if (mc->ext_key_state == EKS_SOLICIT) >> + { >> + mc->ext_key_state = EKS_INPUT; >> + mc->in_extra_cmd = IEC_ECDSA_SIGN; >> + in_extra_reset(mc, IER_NEW); >> + } >> + else >> + { >> + msg(M_CLIENT, "ERROR: The ecdsa-sig command is not currently available"); >> + } >> +} >> + > > This function is almost identical to man_rsa_sign. I would like to have > them both combined into one and then called by man_ecdsa_sig/man_rsa_sig. Could combine these into one function named man_external_sig with an extra parameter, say, cmd = IEC_ECDSA_SIGN or IEC_RSA_SIGN > >> >> >> +char * >> +management_query_ecdsa_sig(struct management *man, >> + const char *b64_data) >> +{ >> + return management_query_multiline_flatten(man, b64_data, "ECDSA_SIGN", "ecdsa-sign", >> + &man->connection.ext_key_state, &man->connection.ext_key_input); >> +} > > Hm, could be merged with management_query_rsa_sig but since it a one > liner I have no strong preference. > > >> +#if OPENSSL_VERSION_NUMBER >= 0x10100001L && !defined(OPENSSL_NO_EC) >> + >> +/* called when EC_KEY is destroyed */ >> +static void >> +openvpn_extkey_ec_finish(EC_KEY *ec) >> +{ >> + /* release the method structure */ >> + const EC_KEY_METHOD *ec_meth = EC_KEY_get_method(ec); >> + EC_KEY_METHOD_free((EC_KEY_METHOD *) ec_meth); >> +} >> + >> +/* EC_KEY_METHOD callback: sign(). >> + * Sign the hash using EC key and return DER encoded signature in sig, >> + * its length in siglen. Return value is 1 on success, 0 on error. >> + */ >> +static int >> +ecdsa_sign(int type, const unsigned char *dgst, int dgstlen, unsigned char *sig, >> + unsigned int *siglen, const BIGNUM *kinv, const BIGNUM *r, EC_KEY *ec) >> +{ >> + char *in_b64 = NULL; >> + char *out_b64 = NULL; >> + int ret = 0; >> + int len; > > len could just as well be done als inline C99 decleration. But no > problem. (Also I am not so firm on our new style here) > >> + >> + /* convert 'from' to base64 * >> + if (openvpn_base64_encode(dgst, dgstlen, &in_b64) <= 0) >> + { >> + goto done; >> + } >> + >> + /* call MI for signature */ >> + if (management) >> + { >> + out_b64 = management_query_ecdsa_sig(management, in_b64); >> + } >> + if (!out_b64) >> + { >> + goto done; >> + } >> + >> + /* decode base64 signature to binary */ >> + len = ECDSA_size(ec); >> + *siglen = openvpn_base64_decode(out_b64, sig, len); >> + if (*siglen > 0) >> + { >> + ret = 1; >> + } >> + >> +done: >> + if (in_b64) >> + { >> + free(in_b64); >> + } >> + if (out_b64) >> + { >> + free(out_b64); >> + } >> + return ret; >> +} > > > This method has a bit code duplication to rsa_priv_enc but I think it is > still okay here since you have an rather ugly looking function that is > hard to read. Will give it another try to see whether those can be combined without uglifying things further. > >> + >> +/* EC_KEY_METHOD callback: sign_setup(). We do no precomputations */ >> +static int >> +ecdsa_sign_setup(EC_KEY *ec, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp) >> +{ >> + return 1; >> +} >> + >> +/* EC_KEY_METHOD callback: sign_sig(). >> + * Sign the hash and return the result as a newly allocated ECDS_SIG >> + * struct or NULL on error. >> + */ >> +static ECDSA_SIG * >> +ecdsa_sign_sig(const unsigned char *dgst, int dgstlen, const BIGNUM *in_kinv, >> + const BIGNUM *in_r, EC_KEY *ec) >> +{ >> + ECDSA_SIG *ecsig = NULL; >> + int len = ECDSA_size(ec); >> + struct gc_arena gc = gc_new(); >> + >> + unsigned char *buf = gc_malloc(len, false, &gc); >> + if (1 != ecdsa_sign(0, dgst, dgstlen, buf, &len, NULL, NULL, ec)) > > I am not sure about 1!= style here. Might be okay. I would try to avoid > this. Not my favourite either, but for some reason I thought that style was preferred in ssl_xx files (may be so in ssl_mbedtls.c?). So much "easier" to write that as xxx != 1 :) Will do. > > len should be size_t. My compiler agrees: > > ssl_openssl.c:1215:48: warning: passing 'int *' to parameter of type > 'unsigned int *' converts between pointers to integer types with > different sign [-Wpointer-sign] > if (1 != ecdsa_sign(0, dgst, dgstlen, buf, &len, NULL, NULL, ec)) > ^~~~ > > ssl_openssl.c:1152:26: note: passing argument to parameter 'siglen' here > unsigned int *siglen, const BIGNUM *kinv, const BIGNUM *r, > EC_KEY *ec) > ^ > 1 warning generated. Yeah, it should be unsigned int. My bad. >> int >> tls_ctx_use_external_private_key(struct tls_root_ctx *ctx, >> const char *cert_file, const char *cert_file_inline) >> @@ -1156,11 +1310,26 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx, >> goto err; >> } >> } >> +#if OPENSSL_VERSION_NUMBER >= 0x10100001L && !defined(OPENSSL_NO_EC) >> + else if (EVP_PKEY_get0_EC_KEY(pkey)) >> + { >> + if (!tls_ctx_use_external_ec_key(ctx, pkey)) >> + { >> + goto err; >> + } >> + } >> + else >> + { >> + crypto_msg(M_WARN, "management-external-key requires an RSA or EC certificate"); >> + goto err; >> + } >> +#else >> else >> { >> crypto_msg(M_WARN, "management-external-key requires a RSA certificate"); > > If you touch that line you can as well fix the grammar in the existing > string that you fixed in the new string :) > > Arne Thanks again, Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, > On Tue, Jan 16, 2018 at 5:40 PM, Arne Schwabe <arne@rfc2549.org> wrote: >> Am 14.01.18 um 20:44 schrieb selva.nair@gmail.com: >>> From: Selva Nair <selva.nair@gmail.com> >>> >>> - This automatically supports EC certificates through >>> --management-external-cert >>> - EC signature request from management has the same format >>> as for rsa with >RSA_SIGN replaced by >ECDSA_SIGN >>> Response should be of the form 'ecdsa-sig' followed >>> by DER encoded signature as base64 followed by 'END' >>> >> snipped.. >> >>> static void >>> +man_ecdsa_sig(struct management *man) >>> +{ >>> + struct man_connection *mc = &man->connection; >>> + if (mc->ext_key_state == EKS_SOLICIT) >>> + { >>> + mc->ext_key_state = EKS_INPUT; >>> + mc->in_extra_cmd = IEC_ECDSA_SIGN; >>> + in_extra_reset(mc, IER_NEW); >>> + } >>> + else >>> + { >>> + msg(M_CLIENT, "ERROR: The ecdsa-sig command is not currently available"); >>> + } >>> +} >>> + >> >> This function is almost identical to man_rsa_sign. I would like to have >> them both combined into one and then called by man_ecdsa_sig/man_rsa_sig. Refactored code that addresses this and other suggestions is here https://github.com/selvanair/openvpn/commits/external-ec-cert (last 3 commits left unsquashed for now). Will send in v2 after testing and squashing but comments welcome. Regarding amending --management-external-cert command, better to address it separately, so not handled here. Also I'm toying with the idea of renaming ecdsa-sig/ECDSA-SIGN by pkey-sig/PKEY-SIGN so that eventually we may be able to use it for all types of keys and retire rsa-sig. Any thoughts on that? Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On 17-01-18 05:24, Selva Nair wrote: > Also I'm toying with the idea of renaming ecdsa-sig/ECDSA-SIGN by > pkey-sig/PKEY-SIGN so that eventually we may be able to use it for > all types of keys and retire rsa-sig. Any thoughts on that? This was my first though when looking at these patches. Haven't looked at the code yet, but functionally I think that's the way to go. Even if it were that Ed25519 / Ed448 sigs are technically not ECDSA sigs. I'll look into these great patches more later. -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi On Wed, Jan 17, 2018 at 3:53 AM, Steffan Karger <steffan.karger@fox-it.com> wrote: > Hi, > > On 17-01-18 05:24, Selva Nair wrote: > > Also I'm toying with the idea of renaming ecdsa-sig/ECDSA-SIGN by > > pkey-sig/PKEY-SIGN so that eventually we may be able to use it for > > all types of keys and retire rsa-sig. Any thoughts on that? > > This was my first though when looking at these patches. Haven't looked > at the code yet, but functionally I think that's the way to go. Even if > it were that Ed25519 / Ed448 sigs are technically not ECDSA sigs. > I was waiting for the chatlog before finalizing v2. Looks like there are a number of opinions that seem to converge towards the same view. This is what I have in my current v2 ready to go out: (i) Current behaviour for rsa signatures stays for now: daemon sends >RSA_SIGN client responds with rsa-sig (ii) Daemon sends >PK_SIG client responds with pk-sig [*] Initially this will be used only for EC, but trivial to switch RSA to this in future. So document that new mgmt clients should be prepared to handle all supported key types via >PK_SIGN/pk-sig and >RSA_SIGN will be eventually removed. As a nice side effect, clients are allowed to respond with pk-sig even if the challenge was >RSA_SIGN. As a not so nice side effect, the daemon will reply with "SUCCESS rsa-sign succeeded" because that's what it asked for -- I suppose we can live with that. We can deprecate and remove >RSA_SIGN at a later date by a hard switch when all known clients are capable of handling >PK_SIGN Does this sound like an acceptable solution? Question: while at it should we alter the syntax to >PK_SIGN,xxx,<base64> where xxx will be empty for now, but it reserves space for extra info that may come handy for some future signature types (like use PSS padding for RSA or use PureEdDSA or whatever). Thanks, Selva <div dir="ltr">Hi<br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 17, 2018 at 3:53 AM, Steffan Karger <span dir="ltr"><<a href="mailto:steffan.karger@fox-it.com" target="_blank">steffan.karger@fox-it.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br> <span class="gmail-"><br> On 17-01-18 05:24, Selva Nair wrote:<br> > Also I'm toying with the idea of renaming ecdsa-sig/ECDSA-SIGN by<br> > pkey-sig/PKEY-SIGN so that eventually we may be able to use it for<br> > all types of keys and retire rsa-sig. Any thoughts on that?<br> <br> </span>This was my first though when looking at these patches. Haven't looked<br> at the code yet, but functionally I think that's the way to go. Even if<br> it were that Ed25519 / Ed448 sigs are technically not ECDSA sigs.<br></blockquote><div><br></div><div>I was waiting for the chatlog before finalizing v2. Looks like there are a number of opinions</div><div>that seem to converge towards the same view.</div><div><br></div><div>This is what I have in my current v2 ready to go out:</div><div><br></div><div>(i) Current behaviour for rsa signatures stays for now: daemon sends >RSA_SIGN client responds</div><div>with rsa-sig</div><div>(ii) Daemon sends >PK_SIG client responds with pk-sig [*] </div><div>Initially this will be used only for EC, but trivial to switch RSA to this in future.</div><div>So document that new mgmt clients should be prepared to handle all supported</div><div>key types via >PK_SIGN/pk-sig and >RSA_SIGN will be eventually removed.</div><div><br></div><div>As a nice side effect, clients are allowed to respond with pk-sig even if the challenge was</div><div>>RSA_SIGN. As a not so nice side effect, the daemon will reply with "SUCCESS rsa-sign</div><div>succeeded" because that's what it asked for -- I suppose we can live with that.</div><div><br></div><div>We can deprecate and remove >RSA_SIGN at a later date by a hard switch when</div><div>all known clients are capable of handling >PK_SIGN</div><div><div><br></div><div>Does this sound like an acceptable solution?</div></div><div><br></div><div>Question: while at it should we alter the syntax to</div><div>>PK_SIGN,xxx,<base64></div><div><br></div><div>where xxx will be empty for now, but it reserves space for extra info that may come handy for some future signature types (like use PSS padding for RSA or use PureEdDSA or whatever). </div><div><br></div><div>Thanks,</div><div><br></div><div>Selva</div></div></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 18/01/18 16:49, Selva Nair wrote: > Hi > > On Wed, Jan 17, 2018 at 3:53 AM, Steffan Karger <steffan.karger@fox-it.com > <mailto:steffan.karger@fox-it.com>> wrote: > > Hi, > > On 17-01-18 05:24, Selva Nair wrote: > > Also I'm toying with the idea of renaming ecdsa-sig/ECDSA-SIGN by > > pkey-sig/PKEY-SIGN so that eventually we may be able to use it for > > all types of keys and retire rsa-sig. Any thoughts on that? > > This was my first though when looking at these patches. Haven't looked > at the code yet, but functionally I think that's the way to go. Even if > it were that Ed25519 / Ed448 sigs are technically not ECDSA sigs. > > > I was waiting for the chatlog before finalizing v2. Looks like there are a > number of opinions > that seem to converge towards the same view. > > This is what I have in my current v2 ready to go out: > > (i) Current behaviour for rsa signatures stays for now: daemon sends >>RSA_SIGN client responds > with rsa-sig > (ii) Daemon sends >PK_SIG client responds with pk-sig [*] > Initially this will be used only for EC, but trivial to switch RSA to this in > future. > So document that new mgmt clients should be prepared to handle all supported > key types via >PK_SIGN/pk-sig and >RSA_SIGN will be eventually removed. > > As a nice side effect, clients are allowed to respond with pk-sig even if the > challenge was >>RSA_SIGN. As a not so nice side effect, the daemon will reply with "SUCCESS > rsa-sign > succeeded" because that's what it asked for -- I suppose we can live with that. > > We can deprecate and remove >RSA_SIGN at a later date by a hard switch when > all known clients are capable of handling >PK_SIGN > > Does this sound like an acceptable solution? > > Question: while at it should we alter the syntax to >>PK_SIGN,xxx,<base64> > > where xxx will be empty for now, but it reserves space for extra info that may > come handy for some future signature types (like use PSS padding for RSA or > use PureEdDSA or whatever). Hi, I've discussed this quickly with James quite recently. We are concerned about breaking existing front-end integrations by doing too much funky stuff with RSA_SIGN for EC support. That in particular touches the signal renaming to PK_SIGN. So we have another suggestion. For external keys to be used, --management-external-key is needed. Add an optional argument to this option, which indicates a version. If this argument is not provided, everything works as it is today - without EC support. If using: --management-external-key 2 ... for version 2, do a hard-shift to PK_SIGN with RSA and EC support. This way we ensure we don't break any existing implementations, we can allow a period of transition to the new version, and we can in the future when we are sure PK_SIGN works well and has seen deployment elsewhere start a deprecation process if the legacy version 1. But we're not put in a situation where anything changes suddenly and abruptly. Any thoughts?
On Mon, Jan 22, 2018 at 8:21 AM, David Sommerseth <openvpn@sf.lists.topphemmelig.net> wrote: > > On 18/01/18 16:49, Selva Nair wrote: > > Hi > > > > On Wed, Jan 17, 2018 at 3:53 AM, Steffan Karger <steffan.karger@fox-it.com > > <mailto:steffan.karger@fox-it.com>> wrote: > > > > Hi, > > > > On 17-01-18 05:24, Selva Nair wrote: > > > Also I'm toying with the idea of renaming ecdsa-sig/ECDSA-SIGN by > > > pkey-sig/PKEY-SIGN so that eventually we may be able to use it for > > > all types of keys and retire rsa-sig. Any thoughts on that? > > > > This was my first though when looking at these patches. Haven't looked > > at the code yet, but functionally I think that's the way to go. Even if > > it were that Ed25519 / Ed448 sigs are technically not ECDSA sigs. > > > > > > I was waiting for the chatlog before finalizing v2. Looks like there are a > > number of opinions > > that seem to converge towards the same view. > > > > This is what I have in my current v2 ready to go out: > > > > (i) Current behaviour for rsa signatures stays for now: daemon sends > >>RSA_SIGN client responds > > with rsa-sig > > (ii) Daemon sends >PK_SIG client responds with pk-sig [*] > > Initially this will be used only for EC, but trivial to switch RSA to this in > > future. > > So document that new mgmt clients should be prepared to handle all supported > > key types via >PK_SIGN/pk-sig and >RSA_SIGN will be eventually removed. > > > > As a nice side effect, clients are allowed to respond with pk-sig even if the > > challenge was > >>RSA_SIGN. As a not so nice side effect, the daemon will reply with "SUCCESS > > rsa-sign > > succeeded" because that's what it asked for -- I suppose we can live with that. > > > > We can deprecate and remove >RSA_SIGN at a later date by a hard switch when > > all known clients are capable of handling >PK_SIGN > > > > Does this sound like an acceptable solution? > > > > Question: while at it should we alter the syntax to > >>PK_SIGN,xxx,<base64> > > > > where xxx will be empty for now, but it reserves space for extra info that may > > come handy for some future signature types (like use PSS padding for RSA or > > use PureEdDSA or whatever). > > Hi, > > I've discussed this quickly with James quite recently. We are concerned about > breaking existing front-end integrations by doing too much funky stuff with > RSA_SIGN for EC support. That in particular touches the signal renaming to > PK_SIGN. > > So we have another suggestion. For external keys to be used, > --management-external-key is needed. Add an optional argument to this option, > which indicates a version. If this argument is not provided, everything works > as it is today - without EC support. If using: --management-external-key 2 > ... for version 2, do a hard-shift to PK_SIGN with RSA and EC support. > > This way we ensure we don't break any existing implementations, we can allow a > period of transition to the new version, and we can in the future when we are > sure PK_SIGN works well and has seen deployment elsewhere start a deprecation > process if the legacy version 1. But we're not put in a situation where > anything changes suddenly and abruptly. > > Any thoughts? This is similar to what Arne proposed while reviewing version 1 of the patch. But I was not excited about encoding client capability into a config option. On further thought, this may offer a little less unpleasant UX to some users and I'm ok with it. I guess tunnelblick supports this options so Jonathan (and Arne) may have more insight. The current patch also allows a delayed deprecation of RSA_SIGN giving time for clients to prepare to handle PK_SIGN. But the UX differs a little: - lay user: everything just works always if the provider/admin is good; or someone fixes things for them. - client developers: as long as we deprecate commands/options responsibly (no hard switching), and document it, they/we will cope So its about average/advanced users changing their RSA cert to EC with --management-external-key and a client that doesn't yet support PK_SIGN: - Present patch: connection process appears stuck (but UI is still responsive) and logs show the daemon is waiting for signature - This proposal: connection fails with: "External EC cert/key not supported in this config. Try using --management-external-key 2" User edits the config option and the connection process appears stuck ..... I suppose the latter is a bit better. Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 22/01/18 16:27, Selva Nair wrote: > - Present patch: connection process appears stuck (but UI is still > responsive) and logs show the daemon is waiting for signature > > - This proposal: connection fails with: "External EC cert/key not > supported in this config. Try using --management-external-key 2" > User edits the config option and the connection process appears stuck ..... > > I suppose the latter is a bit better. Well, it should probably be tweaked slightly better. First of all, if run via a GUI front-end, it's the GUI which should set this argument. We could consider to add a "fail-safe" on this option, so once set - it can't be changed again. The more advanced rabbit whole fix would be that the command line provided --management-external-key option overrides whatever is in the configuration file; doing this will require more work though. This will make the VPN connection will still fail, but it won't be stuck any more. The user may complain "but I did add that option!?" which then is a better starting point for support cases ... "Yes, it is most likely ignored as your user interface is not capable of this feature". Another alternative is to extend an already longer error log entry, by mentioning "also ensure that your management interface front-end supports version 2."
Hi, On Mon, Jan 22, 2018 at 12:18 PM, David Sommerseth <openvpn@sf.lists.topphemmelig.net> wrote: > On 22/01/18 16:27, Selva Nair wrote: >> - Present patch: connection process appears stuck (but UI is still >> responsive) and logs show the daemon is waiting for signature >> >> - This proposal: connection fails with: "External EC cert/key not >> supported in this config. Try using --management-external-key 2" >> User edits the config option and the connection process appears stuck ..... >> >> I suppose the latter is a bit better. > > Well, it should probably be tweaked slightly better. First of all, if run via > a GUI front-end, it's the GUI which should set this argument. We could > consider to add a "fail-safe" on this option, so once set - it can't be > changed again. The more advanced rabbit whole fix would be that the command > line provided --management-external-key option overrides whatever is in the > configuration file; doing this will require more work though. Problem with GUI specifying the option is that it has to first check whether the config uses management-external-key (this option interferes with --key, --pkcs12 etc.). Probably not an issue with most UI's out there but Windows GUI doesn't currently parse the config at all. That said, Windows GUI doesn't support management-external-key so that's not a strong argument. > > This will make the VPN connection will still fail, but it won't be stuck any > more. The user may complain "but I did add that option!?" which then is a > better starting point for support cases ... "Yes, it is most likely ignored as > your user interface is not capable of this feature". > > Another alternative is to extend an already longer error log entry, by > mentioning "also ensure that your management interface front-end supports > version 2." What about extending the current "version" command with an argument where the client states the version of "management-speak" that it supports. Current management version is 1, we increase it to 1.1 and unless the client says "version 1.1" or more we do not send PK_SIGN. The client could do that when it gets the version message or any time later. The response to version command (current management version and openvpn daemon's version stays the same). No full-fledged cap negotiation, but good enough. The UX would be much better that way. Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> The client could do that when it gets the version message or any time > later. Sorry for the type: read that as The client could do that when it gets the welcome message or any time later. Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On Mon, Jan 22, 2018 at 12:31 PM, Selva Nair <selva.nair@gmail.com> wrote: > Hi, > > On Mon, Jan 22, 2018 at 12:18 PM, David Sommerseth > <openvpn@sf.lists.topphemmelig.net> wrote: >> On 22/01/18 16:27, Selva Nair wrote: >>> - Present patch: connection process appears stuck (but UI is still >>> responsive) and logs show the daemon is waiting for signature >>> >>> - This proposal: connection fails with: "External EC cert/key not >>> supported in this config. Try using --management-external-key 2" >>> User edits the config option and the connection process appears stuck ..... >>> >>> I suppose the latter is a bit better. >> >> Well, it should probably be tweaked slightly better. First of all, if run via >> a GUI front-end, it's the GUI which should set this argument. We could >> consider to add a "fail-safe" on this option, so once set - it can't be >> changed again. The more advanced rabbit whole fix would be that the command >> line provided --management-external-key option overrides whatever is in the >> configuration file; doing this will require more work though. > > Problem with GUI specifying the option is that it has to first check > whether the config uses management-external-key (this option > interferes with --key, --pkcs12 etc.). Probably not an issue with most > UI's out there but Windows GUI doesn't currently parse the config at > all. That said, Windows GUI doesn't support management-external-key so > that's not a strong argument. > >> >> This will make the VPN connection will still fail, but it won't be stuck any >> more. The user may complain "but I did add that option!?" which then is a >> better starting point for support cases ... "Yes, it is most likely ignored as >> your user interface is not capable of this feature". >> >> Another alternative is to extend an already longer error log entry, by >> mentioning "also ensure that your management interface front-end supports >> version 2." > > What about extending the current "version" command with an argument > where the client states the version of "management-speak" that it > supports. Current management version is 1, we increase it to 1.1 and > unless the client says "version 1.1" or more we do not send PK_SIGN. > The client could do that when it gets the version message or any time > later. The response to version command (current management version and > openvpn daemon's version stays the same). No full-fledged cap > negotiation, but good enough. > > The UX would be much better that way. I would appreciate some feedback. Thanks, Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi. On Mon, Jan 22, 2018 at 12:31 PM, Selva Nair <selva.nair@gmail.com> wrote: > What about extending the current "version" command with an argument > where the client states the version of "management-speak" that it > supports. Current management version is 1, we increase it to 1.1 and > unless the client says "version 1.1" or more we do not send PK_SIGN. > The client could do that when it gets the version message or any time > later. The response to version command (current management version and > openvpn daemon's version stays the same). No full-fledged cap > negotiation, but good enough. That sounds reasonable; easy to implement in Tunnelblick > The UX would be much better that way. Absolutely. Best regards, Jon Bullard ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On Thu, Jan 25, 2018 at 1:36 PM, Jonathan K. Bullard <jkbullard@gmail.com> wrote: > Hi. > > On Mon, Jan 22, 2018 at 12:31 PM, Selva Nair <selva.nair@gmail.com> wrote: >> What about extending the current "version" command with an argument >> where the client states the version of "management-speak" that it >> supports. Current management version is 1, we increase it to 1.1 and >> unless the client says "version 1.1" or more we do not send PK_SIGN. >> The client could do that when it gets the version message or any time >> later. The response to version command (current management version and >> openvpn daemon's version stays the same). No full-fledged cap >> negotiation, but good enough. > > That sounds reasonable; easy to implement in Tunnelblick > > >> The UX would be much better that way. > > Absolutely. > Encouraged by Jonathan's reply I have made a patch to rename RSA_SIGN to PK_SIGN if client announces a version > 1. Will send it and a modified EC key patch soon. Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 25/01/18 19:46, Selva Nair wrote: > Hi, > > On Thu, Jan 25, 2018 at 1:36 PM, Jonathan K. Bullard > <jkbullard@gmail.com> wrote: >> Hi. >> >> On Mon, Jan 22, 2018 at 12:31 PM, Selva Nair <selva.nair@gmail.com> wrote: >>> What about extending the current "version" command with an argument >>> where the client states the version of "management-speak" that it >>> supports. Current management version is 1, we increase it to 1.1 and >>> unless the client says "version 1.1" or more we do not send PK_SIGN. >>> The client could do that when it gets the version message or any time >>> later. The response to version command (current management version and >>> openvpn daemon's version stays the same). No full-fledged cap >>> negotiation, but good enough. >> >> That sounds reasonable; easy to implement in Tunnelblick >> >> >>> The UX would be much better that way. >> >> Absolutely. >> > > Encouraged by Jonathan's reply I have made a patch to rename RSA_SIGN > to PK_SIGN if client announces a version > 1. Will send it and a > modified EC key patch soon. Sounds good! Just one question ... any reason why to complicate the version number with decimals? Why not just version 1, 2,...X? Coding wise, this is much easier too, as we don't need to do more complicated conversion steps from strings to float.
Hi, On Thu, Jan 25, 2018 at 6:01 PM, David Sommerseth <openvpn@sf.lists.topphemmelig.net> wrote: > On 25/01/18 19:46, Selva Nair wrote: >> Hi, >> >> On Thu, Jan 25, 2018 at 1:36 PM, Jonathan K. Bullard >> <jkbullard@gmail.com> wrote: >>> Hi. >>> >>> On Mon, Jan 22, 2018 at 12:31 PM, Selva Nair <selva.nair@gmail.com> wrote: >>>> What about extending the current "version" command with an argument >>>> where the client states the version of "management-speak" that it >>>> supports. Current management version is 1, we increase it to 1.1 and >>>> unless the client says "version 1.1" or more we do not send PK_SIGN. >>>> The client could do that when it gets the version message or any time >>>> later. The response to version command (current management version and >>>> openvpn daemon's version stays the same). No full-fledged cap >>>> negotiation, but good enough. >>> >>> That sounds reasonable; easy to implement in Tunnelblick >>> >>> >>>> The UX would be much better that way. >>> >>> Absolutely. >>> >> >> Encouraged by Jonathan's reply I have made a patch to rename RSA_SIGN >> to PK_SIGN if client announces a version > 1. Will send it and a >> modified EC key patch soon. > > Sounds good! Just one question ... any reason why to complicate the version > number with decimals? Why not just version 1, 2,...X? Coding wise, this is > much easier too, as we don't need to do more complicated conversion steps from > strings to float. Yes, patch submitted this afternoon uses just a single integer -- so the version goes up to 2. My plan was to split "1.1" into major + minor (float in the RFC patch was just for amusement) but as you say a single integer is so much easier and good enough for this. Thanks, Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c index 55b106c..0152dee 100644 --- a/src/openvpn/manage.c +++ b/src/openvpn/manage.c @@ -113,6 +113,8 @@ man_help(void) #ifdef MANAGMENT_EXTERNAL_KEY msg(M_CLIENT, "rsa-sig : Enter an RSA signature in response to >RSA_SIGN challenge"); msg(M_CLIENT, " Enter signature base64 on subsequent lines followed by END"); + msg(M_CLIENT, "ecdsa-sig : Enter an ECDSA signature in response to >ECDSA_SIGN challenge"); + msg(M_CLIENT, " Enter signature base64 on subsequent lines followed by END"); msg(M_CLIENT, "certificate : Enter a client certificate in response to >NEED-CERT challenge"); msg(M_CLIENT, " Enter certificate base64 on subsequent lines followed by END"); #endif @@ -936,6 +938,7 @@ in_extra_dispatch(struct management *man) #endif /* ifdef MANAGEMENT_PF */ #ifdef MANAGMENT_EXTERNAL_KEY case IEC_RSA_SIGN: + case IEC_ECDSA_SIGN: man->connection.ext_key_state = EKS_READY; buffer_list_free(man->connection.ext_key_input); man->connection.ext_key_input = man->connection.in_extra; @@ -1119,6 +1122,22 @@ man_rsa_sig(struct management *man) } static void +man_ecdsa_sig(struct management *man) +{ + struct man_connection *mc = &man->connection; + if (mc->ext_key_state == EKS_SOLICIT) + { + mc->ext_key_state = EKS_INPUT; + mc->in_extra_cmd = IEC_ECDSA_SIGN; + in_extra_reset(mc, IER_NEW); + } + else + { + msg(M_CLIENT, "ERROR: The ecdsa-sig command is not currently available"); + } +} + +static void man_certificate(struct management *man) { struct man_connection *mc = &man->connection; @@ -1516,6 +1535,10 @@ man_dispatch_command(struct management *man, struct status_output *so, const cha { man_rsa_sig(man); } + else if (streq(p[0], "ecdsa-sig")) + { + man_ecdsa_sig(man); + } else if (streq(p[0], "certificate")) { man_certificate(man); @@ -3655,6 +3678,13 @@ management_query_rsa_sig(struct management *man, &man->connection.ext_key_state, &man->connection.ext_key_input); } +char * +management_query_ecdsa_sig(struct management *man, + const char *b64_data) +{ + return management_query_multiline_flatten(man, b64_data, "ECDSA_SIGN", "ecdsa-sign", + &man->connection.ext_key_state, &man->connection.ext_key_input); +} char * management_query_cert(struct management *man, const char *cert_name) diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h index 676be64..0b1ae5b 100644 --- a/src/openvpn/manage.h +++ b/src/openvpn/manage.h @@ -281,6 +281,7 @@ struct man_connection { #define IEC_CLIENT_PF 2 #define IEC_RSA_SIGN 3 #define IEC_CERTIFICATE 4 +#define IEC_ECDSA_SIGN 5 int in_extra_cmd; struct buffer_list *in_extra; #ifdef MANAGEMENT_DEF_AUTH @@ -441,6 +442,8 @@ void management_learn_addr(struct management *management, char *management_query_rsa_sig(struct management *man, const char *b64_data); +char *management_query_ecdsa_sig(struct management *man, const char *b64_data); + char *management_query_cert(struct management *man, const char *cert_name); #endif diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c index c29dbcf..9379784 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -1132,6 +1132,160 @@ err: return 0; } +#if OPENSSL_VERSION_NUMBER >= 0x10100001L && !defined(OPENSSL_NO_EC) + +/* called when EC_KEY is destroyed */ +static void +openvpn_extkey_ec_finish(EC_KEY *ec) +{ + /* release the method structure */ + const EC_KEY_METHOD *ec_meth = EC_KEY_get_method(ec); + EC_KEY_METHOD_free((EC_KEY_METHOD *) ec_meth); +} + +/* EC_KEY_METHOD callback: sign(). + * Sign the hash using EC key and return DER encoded signature in sig, + * its length in siglen. Return value is 1 on success, 0 on error. + */ +static int +ecdsa_sign(int type, const unsigned char *dgst, int dgstlen, unsigned char *sig, + unsigned int *siglen, const BIGNUM *kinv, const BIGNUM *r, EC_KEY *ec) +{ + char *in_b64 = NULL; + char *out_b64 = NULL; + int ret = 0; + int len; + + /* convert 'from' to base64 */ + if (openvpn_base64_encode(dgst, dgstlen, &in_b64) <= 0) + { + goto done; + } + + /* call MI for signature */ + if (management) + { + out_b64 = management_query_ecdsa_sig(management, in_b64); + } + if (!out_b64) + { + goto done; + } + + /* decode base64 signature to binary */ + len = ECDSA_size(ec); + *siglen = openvpn_base64_decode(out_b64, sig, len); + if (*siglen > 0) + { + ret = 1; + } + +done: + if (in_b64) + { + free(in_b64); + } + if (out_b64) + { + free(out_b64); + } + return ret; +} + +/* EC_KEY_METHOD callback: sign_setup(). We do no precomputations */ +static int +ecdsa_sign_setup(EC_KEY *ec, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp) +{ + return 1; +} + +/* EC_KEY_METHOD callback: sign_sig(). + * Sign the hash and return the result as a newly allocated ECDS_SIG + * struct or NULL on error. + */ +static ECDSA_SIG * +ecdsa_sign_sig(const unsigned char *dgst, int dgstlen, const BIGNUM *in_kinv, + const BIGNUM *in_r, EC_KEY *ec) +{ + ECDSA_SIG *ecsig = NULL; + int len = ECDSA_size(ec); + struct gc_arena gc = gc_new(); + + unsigned char *buf = gc_malloc(len, false, &gc); + if (1 != ecdsa_sign(0, dgst, dgstlen, buf, &len, NULL, NULL, ec)) + { + goto out; + } + /* const char ** should be avoided: not up to us, so we cast our way through */ + ecsig = d2i_ECDSA_SIG(NULL, (const unsigned char **)&buf, len); + +out: + gc_free(&gc); + return ecsig; +} + +static int +tls_ctx_use_external_ec_key(struct tls_root_ctx *ctx, EVP_PKEY *pkey) +{ + EC_KEY *ec = NULL; + EVP_PKEY *privkey = NULL; + EC_KEY_METHOD *ec_method; + + ASSERT(ctx); + + ec_method = EC_KEY_METHOD_new(EC_KEY_OpenSSL()); + if (!ec_method) + { + goto err; + } + + /* Among init methods, we only need the finish method */ + EC_KEY_METHOD_set_init(ec_method, NULL, openvpn_extkey_ec_finish, NULL, NULL, NULL, NULL); + EC_KEY_METHOD_set_sign(ec_method, ecdsa_sign, ecdsa_sign_setup, ecdsa_sign_sig); + + ec = EC_KEY_dup(EVP_PKEY_get0_EC_KEY(pkey)); + if (!ec) + { + EC_KEY_METHOD_free(ec_method); + goto err; + } + if (!EC_KEY_set_method(ec, ec_method)) + { + EC_KEY_METHOD_free(ec_method); + goto err; + } + /* from this point ec_method will get freed when ec is freed */ + + privkey = EVP_PKEY_new(); + if (!EVP_PKEY_assign_EC_KEY(privkey, ec)) + { + goto err; + } + /* from this point ec will get freed when privkey is freed */ + + if (!SSL_CTX_use_PrivateKey(ctx->ctx, privkey)) + { + ec = NULL; /* avoid double freeing it below */ + goto err; + } + + EVP_PKEY_free(privkey); /* this will down ref privkey and ec */ + return 1; + +err: + /* Reach here only when ec and privkey can be independenly freed */ + if (privkey) + { + EVP_PKEY_free(privkey); + } + if(ec) + { + EC_KEY_free(ec); + } + return 0; +} +#endif // OPENSSL_VERSION_NUMBER > 1.1.0 dev + int tls_ctx_use_external_private_key(struct tls_root_ctx *ctx, const char *cert_file, const char *cert_file_inline) @@ -1156,11 +1310,26 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx, goto err; } } +#if OPENSSL_VERSION_NUMBER >= 0x10100001L && !defined(OPENSSL_NO_EC) + else if (EVP_PKEY_get0_EC_KEY(pkey)) + { + if (!tls_ctx_use_external_ec_key(ctx, pkey)) + { + goto err; + } + } + else + { + crypto_msg(M_WARN, "management-external-key requires an RSA or EC certificate"); + goto err; + } +#else else { crypto_msg(M_WARN, "management-external-key requires a RSA certificate"); goto err; } +#endif return 1; err: