On Tue, Jun 19, 2012 at 2:20 AM, Thomas Habets <tho...@habets.se> wrote:
> Should be noted that the prompt is now "Enter Private Key Password:"
> instead of the engine-module generated "SRK authorization:".
Right. I can live with this... maybe in future the key names for all
kind of keys will be modified...
> But all is well because I can just send the SRK password as a command
> to the module:
> engine-pvk-cmd-post PIN "foo bar baz"
Great!
Always go with best practices when programming something... There is a
reason for having best practices... :)
Don't know when the engine was added and why it was added this way...
> Just two small typos that need to be fixed, and I'm happy:
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index be0bc7c..a8ccd54 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -6470,7 +6470,7 @@ add_option (struct options *options,
> VERIFY_PERMISSION (OPT_P_GENERAL);
>
> for (j = 1; j < MAX_PARMS && p[j] != NULL; j++)
> - options->engine_cmd_pre[j-1] = strcmp(p[j], "(null)") ? p[j] : NULL;
> + options->engine_cmd_post[j-1] = strcmp(p[j], "(null)") ? p[j] : NULL;
> }
> else if (streq (p[0], "engine-pvk") && p[1])
> {
> @@ -6493,7 +6493,7 @@ add_option (struct options *options,
> VERIFY_PERMISSION (OPT_P_GENERAL);
>
> for (j = 1; j < MAX_PARMS && p[j] != NULL; j++)
> - options->engine_pvk_cmd_pre[j-1] = strcmp(p[j], "(null)") ? p[j] :
> NULL;
> + options->engine_pvk_cmd_post[j-1] = strcmp(p[j], "(null)") ? p[j] :
> NULL
> }
> #endif /* ENABLE_CRYPTO_OPENSSL */
> #ifdef HAVE_EVP_CIPHER_CTX_SET_KEY_LENGTH
>
Right!
Fixed.
Thanks!
>
>
>>
>>>
>>> I'm not an OpenSSL ninja, but this looks suspicious:
>>> ------
>>> if (!ENGINE_init(e))
>>> msg (M_SSLERR, "Cannot initialize engine '%s'", engine);
>>>
>>> ENGINE_free(e); /* free +1 reference count of the above two */
>>> [... more uses of e ...]
>>> ------
>>> Seems all the M_SSLERR lines there should set e=NULL and stop
>>> processing, to keep the same behaviour.
>>
>> When msg with M_SSLERR is issued, the program terminates...
>> ---
>> #define M_SSLERR (M_FATAL | M_SSL)
>>
>> if (flags & M_FATAL)
>> openvpn_exit (OPENVPN_EXIT_STATUS_ERROR); /* exit point */
>> ----
>>
>>> But yes, it works.
>>
>> Thanks!
>> Any more comments regarding engine usage and cleanups?
>>
>> Alon.
>>
>>>
>>> On 18 June 2012 21:40, Alon Bar-Lev <alon.bar...@gmail.com> wrote:
>>>> Hello Thomas,
>>>>
>>>> Thank you for your comments and help.
>>>> I've updated the branch based on your work, but with changes...
>>>> The password is secret, and there is a standard mechanism in openvpn
>>>> to handle password...
>>>> So I tried to use it.
>>>>
>>>> For the conditionals, I wanted to get rid of the openssl engine
>>>> conditionals... anyway, the conditionals is not for the ENGINE type as
>>>> it does exists in 0.9.7 initial release. So I think that reducing use
>>>> of conditionals is better for reading the code.
>>>>
>>>> Can you please re-test and comment?
>>>>
>>>> Thanks!
>>>> Alon.
>>>>
>>>> On Mon, Jun 18, 2012 at 10:12 PM, Thomas Habets <tho...@habets.se> wrote:
>>>>> I can confirm that it works. I need to specify both engine and
>>>>> engine-pvk in the config though. If "engine" is not specified then
>>>>> ENGINE_load_builtin_engines() is never called. If you had this in mind
>>>>> then I think "engine-pvk" should require "engine". (just putting
>>>>> "engine" in the config file is enough, without parameter)
>>>>>
>>>>> Other nits:
>>>>> The call to ENGINE_load_private_key() should probably be surrounded by
>>>>> #ifdef HAVE_OPENSSL_ENGINE.
>>>>>
>>>>> Presumably without HAVE_OPENSSL_ENGINE the ENGINE struct and
>>>>> openssl/engine.h doesn't exist, so they should be #ifdeffed too.
>>>>>
>>>>> Fixed this, and added SRK password option, in attached patch to be
>>>>> applied on top of your engine branch. If option is not given then the
>>>>> default UI functions are called (for linux at console that means
>>>>> prompt the user).
>>>>>
>>>>> (the SRK password is not really a secret. If you have it then you're
>>>>> allowed to do operations with the TPM chip. But you have to supply
>>>>> the tpm-sealed data when doing the operation. I've seen several
>>>>> recommendations to just leave it blank)
>>>>>
>>>>> On 18 June 2012 15:22, Alon Bar-Lev <alon.bar...@gmail.com> wrote:
>>>>>> Oh...
>>>>>> And I forgot mentioning that the UI method should be solved, using the
>>>>>> default is not something that is usable for openvpn.
>>>>>> Can you please take care of this?
>>>>>>
>>>>>> Alon.
>>>>>>
>>>>>> On Mon, Jun 18, 2012 at 3:25 PM, Alon Bar-Lev <alon.bar...@gmail.com>
>>>>>> wrote:
>>>>>>> Hello Thomas,
>>>>>>>
>>>>>>> I did not have the global variable in mind :)
>>>>>>>
>>>>>>> I thought about your initial suggestion of specific private key
>>>>>>> engine, and it has value, so I added a new option.
>>>>>>>
>>>>>>> I propose the following [1], the problem is that I cannot test this out.
>>>>>>>
>>>>>>> While looking on the current engine implementation, there is a need to
>>>>>>> pass the pre/post strings, so there is more work to be done here, I
>>>>>>> will do this later.
>>>>>>>
>>>>>>> I also don't understand the need of using the dynamic in
>>>>>>> try_load_engine, but this will be resolved by supporting pre/post
>>>>>>> strings.
>>>>>>>
>>>>>>> Please review and test, should reach the same functionality as you
>>>>>>> require.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Alon.
>>>>>>>
>>>>>>> [1] https://github.com/alonbl/openvpn/compare/master...engine
>>>>>>>
>>>>>>> On Mon, Jun 18, 2012 at 12:45 AM, Thomas Habets <tho...@habets.se>
>>>>>>> wrote:
>>>>>>>> Those questions are why I'd prefer to reuse the already loaded ENGINE
>>>>>>>> (engine_persist in crypto_openssl), but it didn't appear to be
>>>>>>>> exported from the crypto backend (crypto_backend.h), which is why my
>>>>>>>> previous patch added exporting of it (by means of the init function).
>>>>>>>>
>>>>>>>> All versions of the patch works with non-static modules. The TPM one
>>>>>>>> I'm using is a .so file.
>>>>>>>> http://packages.debian.org/squeeze/amd64/libengine-tpm-openssl/filelist
>>>>>>>>
>>>>>>>> Do you want a separate ENGINE struct or not?
>>>>>>>> If same, how should it be exported from crypto_openssl to ssl_openssl?
>>>>>>>> I don't see any non-static accessor.
>>>>>>>> If different, call setup_engine() from ssl_openssl.c (meaning turning
>>>>>>>> setup_engine() non-static)?
>>>>>>>>
>>>>>>>> I was hesitant to create new non-statics in crypto_openssl, but one
>>>>>>>> easy solution is of course to just make
>>>>>>>> crypto_openssl.c::engine_persist non-static and use it directly, as
>>>>>>>> seen in attached patch.
>>>>>>>>
>>>>>>>> Seems I don't need to call ENGINE_init() at all. The attached patch
>>>>>>>> works, at least.
>>>>>>>>
>>>>>>>> I appreciate the code discipline. Really I do. :-)
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Thomas
>>>>>>>>
>>>>>>>> On 17 June 2012 22:04, Alon Bar-Lev <alon.bar...@gmail.com> wrote:
>>>>>>>>> Yes, almost :)
>>>>>>>>>
>>>>>>>>> Won't it better to call ENGINE_init at setup_engine() or at
>>>>>>>>> try_load_engine() instead of at tls_ctx_load_priv_file()? It is just
>>>>>>>>> that tls_ctx_load_priv_file() can be called more than once, while the
>>>>>>>>> init should be called once, right?
>>>>>>>>> Are you sure all works well if engine is not statically linked?
>>>>>>>>> What about ENGINE_finish() at proper place?
>>>>>>>>>
>>>>>>>>> Thank you for your patience,
>>>>>>>>> Alon Bar-Lev.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Sun, Jun 17, 2012 at 11:53 PM, Thomas Habets <tho...@habets.se>
>>>>>>>>> wrote:
>>>>>>>>>> Hi.
>>>>>>>>>>
>>>>>>>>>> Need? No. I thought you preferred reusing the loaded/inited ENGINE
>>>>>>>>>> struct cached by existing code instead of creating a new one.
>>>>>>>>>>
>>>>>>>>>> Is the attached patch what you had in mind?
>>>>>>>>>>
>>>>>>>>>> (same description/sign-off)
>>>>>>>>>>
>>>>>>>>>> Regards,
>>>>>>>>>> Thomas
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 17 June 2012 12:12, Alon Bar-Lev <alon.bar...@gmail.com> wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> Why do we need to crypto_init_lib_engine() twice? Can you please
>>>>>>>>>>> take
>>>>>>>>>>> a look at init_crypto_pre:: init_crypto_pre()?
>>>>>>>>>>>
>>>>>>>>>>> I also think crypto_init_lib_engine() should not return the
>>>>>>>>>>> engine...
>>>>>>>>>>> as won't it simpler to use ENGINE_by_id() at
>>>>>>>>>>> ssl_openssl.c::tls_ctx_load_priv_file()?
>>>>>>>>>>>
>>>>>>>>>>> Alon.
>>>>>>>>>>>
>>>>>>>>>>> On Sun, Jun 17, 2012 at 1:02 PM, Thomas Habets <tho...@habets.se>
>>>>>>>>>>> wrote:
>>>>>>>>>>>> Hi.
>>>>>>>>>>>>
>>>>>>>>>>>> Ah yes, I first made the patch to an older version where some of
>>>>>>>>>>>> these
>>>>>>>>>>>> things don't apply, and then forward-ported it.
>>>>>>>>>>>>
>>>>>>>>>>>> How about this?
>>>>>>>>>>>> ---------
>>>>>>>>>>>> Add support for SSL engine loading the private key.
>>>>>>>>>>>>
>>>>>>>>>>>> Option 'engine' is used to specify the name of the engine that
>>>>>>>>>>>> will load the private key.
>>>>>>>>>>>>
>>>>>>>>>>>> For example this can be "tpm" to use the OpenSSL TPM engine module
>>>>>>>>>>>> (libengine-tpm-openssl in Debian).
>>>>>>>>>>>>
>>>>>>>>>>>> It defaults to the built-in UI methods because openssl-tpm-engine
>>>>>>>>>>>> doesn't yet support user data being sent to the callback functions.
>>>>>>>>>>>> A patch for that on its way to them.
>>>>>>>>>>>>
>>>>>>>>>>>> Some more details:
>>>>>>>>>>>> http://blog.habets.pp.se/2012/02/TPM-backed-SSL
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Thomas Habets <hab...@google.com>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 17 June 2012 01:11, Alon Bar-Lev <alon.bar...@gmail.com> wrote:
>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>
>>>>>>>>>>>>> It is a good idea.
>>>>>>>>>>>>> But first, please remove the emacs stuff.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Now, I see that the ENGINE_load_builtin_engines() is already
>>>>>>>>>>>>> called at
>>>>>>>>>>>>> crypto_openssl.c::crypto_init_lib_engine, is there any require to
>>>>>>>>>>>>> duplicate this?
>>>>>>>>>>>>>
>>>>>>>>>>>>> There is already "engine" option, available only to polarssl, it
>>>>>>>>>>>>> can
>>>>>>>>>>>>> easily and correct way be used also for openssl, instead of having
>>>>>>>>>>>>> another option.
>>>>>>>>>>>>>
>>>>>>>>>>>>> What do you think?
>>>>>>>>>>>>> Alon.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Sun, Jun 17, 2012 at 2:50 AM, Thomas Habets <tho...@habets.se>
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>> Patch attached.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Add support for SSL engine loading the private key.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Added option 'key-engine' specifying the name of the engine that
>>>>>>>>>>>>>> will load the private key.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> For example this can be "tpm" to use the OpenSSL TPM engine
>>>>>>>>>>>>>> module
>>>>>>>>>>>>>> (libengine-tpm-openssl in Debian).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> It defaults to the built-in UI methods because openssl-tpm-engine
>>>>>>>>>>>>>> doesn't yet support user data being sent to the callback
>>>>>>>>>>>>>> functions.
>>>>>>>>>>>>>> A patch for that on its way to them.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Some more details:
>>>>>>>>>>>>>> http://blog.habets.pp.se/2012/02/TPM-backed-SSL
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Thomas Habets <hab...@google.com>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>> typedef struct me_s {
>>>>>>>>>>>>>> char name[] = { "Thomas Habets" };
>>>>>>>>>>>>>> char email[] = { "tho...@habets.pp.se" };
>>>>>>>>>>>>>> char kernel[] = { "Linux" };
>>>>>>>>>>>>>> char *pgpKey[] = { "http://www.habets.pp.se/pubkey.txt"; };
>>>>>>>>>>>>>> char pgp[] = { "A8A3 D1DD 4AE0 8467 7FDE 0945 286A E90A AD48
>>>>>>>>>>>>>> E854" };
>>>>>>>>>>>>>> char coolcmd[] = { "echo '. ./_&. ./_'>_;. ./_" };
>>>>>>>>>>>>>> } me_t;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ------------------------------------------------------------------------------
>>>>>>>>>>>>>> Live Security Virtual Conference
>>>>>>>>>>>>>> Exclusive live event will cover all the ways today's security and
>>>>>>>>>>>>>> threat landscape has changed and how IT managers can respond.
>>>>>>>>>>>>>> Discussions
>>>>>>>>>>>>>> will include endpoint security, mobile security and the latest
>>>>>>>>>>>>>> in malware
>>>>>>>>>>>>>> threats.
>>>>>>>>>>>>>> http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
>>>>>>>>>>>>>> _______________________________________________
>>>>>>>>>>>>>> Openvpn-devel mailing list
>>>>>>>>>>>>>> Openvpn-devel@lists.sourceforge.net
>>>>>>>>>>>>>> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> --
>>>>>>>>>>>> typedef struct me_s {
>>>>>>>>>>>> char name[] = { "Thomas Habets" };
>>>>>>>>>>>> char email[] = { "tho...@habets.pp.se" };
>>>>>>>>>>>> char kernel[] = { "Linux" };
>>>>>>>>>>>> char *pgpKey[] = { "http://www.habets.pp.se/pubkey.txt"; };
>>>>>>>>>>>> char pgp[] = { "A8A3 D1DD 4AE0 8467 7FDE 0945 286A E90A AD48
>>>>>>>>>>>> E854" };
>>>>>>>>>>>> char coolcmd[] = { "echo '. ./_&. ./_'>_;. ./_" };
>>>>>>>>>>>> } me_t;
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> typedef struct me_s {
>>>>>>>> char name[] = { "Thomas Habets" };
>>>>>>>> char email[] = { "tho...@habets.pp.se" };
>>>>>>>> char kernel[] = { "Linux" };
>>>>>>>> char *pgpKey[] = { "http://www.habets.pp.se/pubkey.txt"; };
>>>>>>>> char pgp[] = { "A8A3 D1DD 4AE0 8467 7FDE 0945 286A E90A AD48 E854" };
>>>>>>>> char coolcmd[] = { "echo '. ./_&. ./_'>_;. ./_" };
>>>>>>>> } me_t;
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> typedef struct me_s {
>>>>> char name[] = { "Thomas Habets" };
>>>>> char email[] = { "tho...@habets.pp.se" };
>>>>> char kernel[] = { "Linux" };
>>>>> char *pgpKey[] = { "http://www.habets.pp.se/pubkey.txt"; };
>>>>> char pgp[] = { "A8A3 D1DD 4AE0 8467 7FDE 0945 286A E90A AD48 E854" };
>>>>> char coolcmd[] = { "echo '. ./_&. ./_'>_;. ./_" };
>>>>> } me_t;
>>>
>>>
>>>
>>> --
>>> typedef struct me_s {
>>> char name[] = { "Thomas Habets" };
>>> char email[] = { "tho...@habets.pp.se" };
>>> char kernel[] = { "Linux" };
>>> char *pgpKey[] = { "http://www.habets.pp.se/pubkey.txt"; };
>>> char pgp[] = { "A8A3 D1DD 4AE0 8467 7FDE 0945 286A E90A AD48 E854" };
>>> char coolcmd[] = { "echo '. ./_&. ./_'>_;. ./_" };
>>> } me_t;
>
>
>
> --
> typedef struct me_s {
> char name[] = { "Thomas Habets" };
> char email[] = { "tho...@habets.pp.se" };
> char kernel[] = { "Linux" };
> char *pgpKey[] = { "http://www.habets.pp.se/pubkey.txt"; };
> char pgp[] = { "A8A3 D1DD 4AE0 8467 7FDE 0945 286A E90A AD48 E854" };
> char coolcmd[] = { "echo '. ./_&. ./_'>_;. ./_" };
> } me_t;
