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;
engine-patch-engine-1.patch
Description: Binary data
