On 18 June 2012 23:40, Alon Bar-Lev <alon.bar...@gmail.com> wrote:
>> Shouldn't it on be the default?
> This is a very looooooooooooong argument... weather to allow unsecured
> setup by default...
[googled a bit for it. I see.]
Should be noted that the prompt is now "Enter Private Key Password:"
instead of the engine-module generated "SRK authorization:".
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"
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
>
>>
>> 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;
