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;
