On Tue, Jun 19, 2012 at 1:27 AM, Thomas Habets <tho...@habets.se> wrote:
> When I specify --enable-password-save to ./configure askpass is able
> to read the password from a file.
Right, this is the idea, and if you use the management interface you
can specify the password via that interface.
> Seems despite what the --help says it actually defaults to off. :-(
Right...
> Shouldn't it on be the default?
This is a very looooooooooooong argument... weather to allow unsecured
setup by default...
>
> 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;
