[PATCH] ima-evm-utils: remove redundant call to OpenSSL_add_all_algorithms

[PATCH] ima-evm-utils: remove redundant call to OpenSSL_add_all_algorithms

[Vitaly Chikunov]

Because of call to OPENSSL_add_all_algorithms_conf() calling
OpenSSL_add_all_algorithms() is not needed. There was not be any
problems though because double initialization is permitted.
---
 src/libimaevm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index 7501303..b038d0c 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -995,7 +995,6 @@ int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const c
 
 static void libinit()
 {
-	OpenSSL_add_all_algorithms();
 	OPENSSL_add_all_algorithms_conf();
 	ERR_load_crypto_strings();
 }
-- 
2.11.0

Re: [PATCH] ima-evm-utils: remove redundant call to OpenSSL_add_all_algorithms

[Mimi Zohar]

On Sun, 2019-01-27 at 05:39 +0300, Vitaly Chikunov wrote:
> Because of call to OPENSSL_add_all_algorithms_conf() calling
> OpenSSL_add_all_algorithms() is not needed. There was not be any
> problems though because double initialization is permitted.
> ---
>  src/libimaevm.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index 7501303..b038d0c 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -995,7 +995,6 @@ int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const c
> 
>  static void libinit()
>  {
> -	OpenSSL_add_all_algorithms();
>  	OPENSSL_add_all_algorithms_conf();
>  	ERR_load_crypto_strings();
>  }

The only difference between the two calls seems to be reading the
system openssl.cnf file.  In the original call that is dependent on
OPENSSL_LOAD_CONF being defined.  Calling
OPENSSL_add_all_algorithms_conf(), forces reading the system
openssl.cnf.

Mimi

Re: [PATCH] ima-evm-utils: remove redundant call to OpenSSL_add_all_algorithms

[Vitaly Chikunov]

On Wed, Jan 30, 2019 at 07:34:57AM -0500, Mimi Zohar wrote:
> On Sun, 2019-01-27 at 05:39 +0300, Vitaly Chikunov wrote:
> > Because of call to OPENSSL_add_all_algorithms_conf() calling
> > OpenSSL_add_all_algorithms() is not needed. There was not be any
> > problems though because double initialization is permitted.
> > ---
> >  src/libimaevm.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > index 7501303..b038d0c 100644
> > --- a/src/libimaevm.c
> > +++ b/src/libimaevm.c
> > @@ -995,7 +995,6 @@ int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const c
> > 
> >  static void libinit()
> >  {
> > -	OpenSSL_add_all_algorithms();
> >  	OPENSSL_add_all_algorithms_conf();
> >  	ERR_load_crypto_strings();
> >  }
> 
> The only difference between the two calls seems to be reading the
> system openssl.cnf file.  In the original call that is dependent on
> OPENSSL_LOAD_CONF being defined.  Calling
> OPENSSL_add_all_algorithms_conf(), forces reading the system
> openssl.cnf.

Yes. OPENSSL_LOAD_CONF is per application define, which is by default
undefined. And instead of defining it, we could just call
OPENSSL_add_all_algorithms_conf(), which is required for GOST support.
Otherwise enabling Streebog via OPENSSL_CONF will not work.

Thanks,

> 
> Mimi

Re: [PATCH] ima-evm-utils: remove redundant call to OpenSSL_add_all_algorithms

[Mimi Zohar]

[Cc'ing Bruno, Petr]

On Wed, 2019-01-30 at 16:25 +0300, Vitaly Chikunov wrote:
> On Wed, Jan 30, 2019 at 07:34:57AM -0500, Mimi Zohar wrote:
> > On Sun, 2019-01-27 at 05:39 +0300, Vitaly Chikunov wrote:
> > > Because of call to OPENSSL_add_all_algorithms_conf() calling
> > > OpenSSL_add_all_algorithms() is not needed. There was not be any
> > > problems though because double initialization is permitted.
> > > ---
> > >  src/libimaevm.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > > index 7501303..b038d0c 100644
> > > --- a/src/libimaevm.c
> > > +++ b/src/libimaevm.c
> > > @@ -995,7 +995,6 @@ int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const c
> > > 
> > >  static void libinit()
> > >  {
> > > -	OpenSSL_add_all_algorithms();
> > >  	OPENSSL_add_all_algorithms_conf();
> > >  	ERR_load_crypto_strings();
> > >  }
> > 
> > The only difference between the two calls seems to be reading the
> > system openssl.cnf file.  In the original call that is dependent on
> > OPENSSL_LOAD_CONF being defined.  Calling
> > OPENSSL_add_all_algorithms_conf(), forces reading the system
> > openssl.cnf.
> 
> Yes. OPENSSL_LOAD_CONF is per application define, which is by default
> undefined. And instead of defining it, we could just call
> OPENSSL_add_all_algorithms_conf(), which is required for GOST support.
> Otherwise enabling Streebog via OPENSSL_CONF will not work.

I think this is basically a packaging question.  Instead of hard
coding this change, it could be an autoconf option (eg.
enable/disable-gost).

Bruno, Petr, any preference?

Thanks,

Mimi

Re: [PATCH] ima-evm-utils: remove redundant call to OpenSSL_add_all_algorithms

[Vitaly Chikunov]

On Wed, Jan 30, 2019 at 08:41:13AM -0500, Mimi Zohar wrote:
> [Cc'ing Bruno, Petr]
> 
> On Wed, 2019-01-30 at 16:25 +0300, Vitaly Chikunov wrote:
> > On Wed, Jan 30, 2019 at 07:34:57AM -0500, Mimi Zohar wrote:
> > > On Sun, 2019-01-27 at 05:39 +0300, Vitaly Chikunov wrote:
> > > > Because of call to OPENSSL_add_all_algorithms_conf() calling
> > > > OpenSSL_add_all_algorithms() is not needed. There was not be any
> > > > problems though because double initialization is permitted.
> > > > ---
> > > >  src/libimaevm.c | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > > 
> > > > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > > > index 7501303..b038d0c 100644
> > > > --- a/src/libimaevm.c
> > > > +++ b/src/libimaevm.c
> > > > @@ -995,7 +995,6 @@ int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const c
> > > > 
> > > >  static void libinit()
> > > >  {
> > > > -	OpenSSL_add_all_algorithms();
> > > >  	OPENSSL_add_all_algorithms_conf();
> > > >  	ERR_load_crypto_strings();
> > > >  }
> > > 
> > > The only difference between the two calls seems to be reading the
> > > system openssl.cnf file.  In the original call that is dependent on
> > > OPENSSL_LOAD_CONF being defined.  Calling
> > > OPENSSL_add_all_algorithms_conf(), forces reading the system
> > > openssl.cnf.
> > 
> > Yes. OPENSSL_LOAD_CONF is per application define, which is by default
> > undefined. And instead of defining it, we could just call
> > OPENSSL_add_all_algorithms_conf(), which is required for GOST support.
> > Otherwise enabling Streebog via OPENSSL_CONF will not work.
> 
> I think this is basically a packaging question.  Instead of hard
> coding this change, it could be an autoconf option (eg.
> enable/disable-gost).

User already have control to load or not engines by editing or not the
config. But, if OpenSSL_add_all_algorithms() is used without defining
OPENSSL_LOAD_CONF (default) user will lose such option.

I think loading config is quite normal operation which is also default
for the openssl binary.

> 
> Bruno, Petr, any preference?
> 
> Thanks,
> 
> Mimi

Re: [PATCH] ima-evm-utils: remove redundant call to OpenSSL_add_all_algorithms

[James Bottomley]

On Wed, 2019-01-30 at 16:25 +0300, Vitaly Chikunov wrote:
> On Wed, Jan 30, 2019 at 07:34:57AM -0500, Mimi Zohar wrote:
> > On Sun, 2019-01-27 at 05:39 +0300, Vitaly Chikunov wrote:
> > > Because of call to OPENSSL_add_all_algorithms_conf() calling
> > > OpenSSL_add_all_algorithms() is not needed. There was not be any
> > > problems though because double initialization is permitted.
> > > ---
> > >  src/libimaevm.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > > index 7501303..b038d0c 100644
> > > --- a/src/libimaevm.c
> > > +++ b/src/libimaevm.c
> > > @@ -995,7 +995,6 @@ int sign_hash(const char *hashalgo, const
> > > unsigned char *hash, int size, const c
> > > 
> > >  static void libinit()
> > >  {
> > > -	OpenSSL_add_all_algorithms();
> > >  	OPENSSL_add_all_algorithms_conf();
> > >  	ERR_load_crypto_strings();
> > >  }
> > 
> > The only difference between the two calls seems to be reading the
> > system openssl.cnf file.  In the original call that is dependent on
> > OPENSSL_LOAD_CONF being defined.  Calling
> > OPENSSL_add_all_algorithms_conf(), forces reading the system
> > openssl.cnf.
> 
> Yes. OPENSSL_LOAD_CONF is per application define, which is by default
> undefined. And instead of defining it, we could just call
> OPENSSL_add_all_algorithms_conf(), which is required for GOST
> support.
> Otherwise enabling Streebog via OPENSSL_CONF will not work.

It will if you call

ENGINE_set_default(e, ENGINE_METHOD_ALL);

after ENGINE_init

That's all the conf file is covering up for.

James

Re: [PATCH] ima-evm-utils: remove redundant call to OpenSSL_add_all_algorithms

[Petr Vorel]

Hi,

[ Cc Marcus and Jonathan ]
> On Wed, Jan 30, 2019 at 08:41:13AM -0500, Mimi Zohar wrote:
> > [Cc'ing Bruno, Petr]

> > On Wed, 2019-01-30 at 16:25 +0300, Vitaly Chikunov wrote:
> > > On Wed, Jan 30, 2019 at 07:34:57AM -0500, Mimi Zohar wrote:
> > > > On Sun, 2019-01-27 at 05:39 +0300, Vitaly Chikunov wrote:
> > > > > Because of call to OPENSSL_add_all_algorithms_conf() calling
> > > > > OpenSSL_add_all_algorithms() is not needed. There was not be any
> > > > > problems though because double initialization is permitted.
> > > > > ---
> > > > >  src/libimaevm.c | 1 -
> > > > >  1 file changed, 1 deletion(-)

> > > > > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > > > > index 7501303..b038d0c 100644
> > > > > --- a/src/libimaevm.c
> > > > > +++ b/src/libimaevm.c
> > > > > @@ -995,7 +995,6 @@ int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const c

> > > > >  static void libinit()
> > > > >  {
> > > > > -	OpenSSL_add_all_algorithms();
> > > > >  	OPENSSL_add_all_algorithms_conf();
> > > > >  	ERR_load_crypto_strings();
> > > > >  }

> > > > The only difference between the two calls seems to be reading the
> > > > system openssl.cnf file.  In the original call that is dependent on
> > > > OPENSSL_LOAD_CONF being defined.  Calling
> > > > OPENSSL_add_all_algorithms_conf(), forces reading the system
> > > > openssl.cnf.

> > > Yes. OPENSSL_LOAD_CONF is per application define, which is by default
> > > undefined. And instead of defining it, we could just call
> > > OPENSSL_add_all_algorithms_conf(), which is required for GOST support.
> > > Otherwise enabling Streebog via OPENSSL_CONF will not work.

> > I think this is basically a packaging question.  Instead of hard
> > coding this change, it could be an autoconf option (eg.
> > enable/disable-gost).

> User already have control to load or not engines by editing or not the
> config. But, if OpenSSL_add_all_algorithms() is used without defining
> OPENSSL_LOAD_CONF (default) user will lose such option.

> I think loading config is quite normal operation which is also default
> for the openssl binary.


> > Bruno, Petr, any preference?
Not sure (I Cc more experienced packagers), but allowing distros to control it
via autoconf certainly does not harm.

BTW OpenSSL_add_all_algorithms() is deprecated since OpenSSL 1.1.0.


Kind regards,
Petr

Re: [PATCH] ima-evm-utils: remove redundant call to OpenSSL_add_all_algorithms

[Vitaly Chikunov]

On Wed, Jan 30, 2019 at 07:35:49AM -0800, James Bottomley wrote:
> On Wed, 2019-01-30 at 16:25 +0300, Vitaly Chikunov wrote:
> > On Wed, Jan 30, 2019 at 07:34:57AM -0500, Mimi Zohar wrote:
> > > On Sun, 2019-01-27 at 05:39 +0300, Vitaly Chikunov wrote:
> > > > Because of call to OPENSSL_add_all_algorithms_conf() calling
> > > > OpenSSL_add_all_algorithms() is not needed. There was not be any
> > > > problems though because double initialization is permitted.
> > > > ---
> > > >  src/libimaevm.c | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > > 
> > > > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > > > index 7501303..b038d0c 100644
> > > > --- a/src/libimaevm.c
> > > > +++ b/src/libimaevm.c
> > > > @@ -995,7 +995,6 @@ int sign_hash(const char *hashalgo, const
> > > > unsigned char *hash, int size, const c
> > > > 
> > > >  static void libinit()
> > > >  {
> > > > -	OpenSSL_add_all_algorithms();
> > > >  	OPENSSL_add_all_algorithms_conf();
> > > >  	ERR_load_crypto_strings();
> > > >  }
> > > 
> > > The only difference between the two calls seems to be reading the
> > > system openssl.cnf file.  In the original call that is dependent on
> > > OPENSSL_LOAD_CONF being defined.  Calling
> > > OPENSSL_add_all_algorithms_conf(), forces reading the system
> > > openssl.cnf.
> > 
> > Yes. OPENSSL_LOAD_CONF is per application define, which is by default
> > undefined. And instead of defining it, we could just call
> > OPENSSL_add_all_algorithms_conf(), which is required for GOST
> > support.
> > Otherwise enabling Streebog via OPENSSL_CONF will not work.
> 
> It will if you call

There is preferred "easy" method of [system wide] loading of gost-engine
"by default" just by changing openssl.cnf like this:

  https://github.com/gost-engine/engine/blob/master/example.conf

After that change all openssl (and linked) tools understand GOST
algorithms without needing options like `-engine gost`.

This works unless tool is compiled without OPENSSL_LOAD_CONF and it calls
OpenSSL_add_all_algorithms() instead of
OPENSSL_add_all_algorithms_conf(). Which is frequently the default,
(because there is too much methods of openssl initialization and people
may not understand all intrications for all options.) In that case we try
to persuade tool author to change the way openssl is initialized.

> 
> ENGINE_set_default(e, ENGINE_METHOD_ALL);
> 
> after ENGINE_init
> 
> That's all the conf file is covering up for.
> 
> James
> 

Re: [PATCH] ima-evm-utils: remove redundant call to OpenSSL_add_all_algorithms

[James Bottomley]

On Wed, 2019-01-30 at 19:12 +0300, Vitaly Chikunov wrote:
> On Wed, Jan 30, 2019 at 07:35:49AM -0800, James Bottomley wrote:
> > On Wed, 2019-01-30 at 16:25 +0300, Vitaly Chikunov wrote:
> > > On Wed, Jan 30, 2019 at 07:34:57AM -0500, Mimi Zohar wrote:
> > > > On Sun, 2019-01-27 at 05:39 +0300, Vitaly Chikunov wrote:
> > > > > Because of call to OPENSSL_add_all_algorithms_conf() calling
> > > > > OpenSSL_add_all_algorithms() is not needed. There was not be
> > > > > any
> > > > > problems though because double initialization is permitted.
> > > > > ---
> > > > >  src/libimaevm.c | 1 -
> > > > >  1 file changed, 1 deletion(-)
> > > > > 
> > > > > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > > > > index 7501303..b038d0c 100644
> > > > > --- a/src/libimaevm.c
> > > > > +++ b/src/libimaevm.c
> > > > > @@ -995,7 +995,6 @@ int sign_hash(const char *hashalgo, const
> > > > > unsigned char *hash, int size, const c
> > > > > 
> > > > >  static void libinit()
> > > > >  {
> > > > > -	OpenSSL_add_all_algorithms();
> > > > >  	OPENSSL_add_all_algorithms_conf();
> > > > >  	ERR_load_crypto_strings();
> > > > >  }
> > > > 
> > > > The only difference between the two calls seems to be reading
> > > > the
> > > > system openssl.cnf file.  In the original call that is
> > > > dependent on
> > > > OPENSSL_LOAD_CONF being defined.  Calling
> > > > OPENSSL_add_all_algorithms_conf(), forces reading the system
> > > > openssl.cnf.
> > > 
> > > Yes. OPENSSL_LOAD_CONF is per application define, which is by
> > > default
> > > undefined. And instead of defining it, we could just call
> > > OPENSSL_add_all_algorithms_conf(), which is required for GOST
> > > support.
> > > Otherwise enabling Streebog via OPENSSL_CONF will not work.
> > 
> > It will if you call
> 
> There is preferred "easy" method of [system wide] loading of gost-
> engine
> "by default" just by changing openssl.cnf like this:
> 
>   https://github.com/gost-engine/engine/blob/master/example.conf
> 
> After that change all openssl (and linked) tools understand GOST
> algorithms without needing options like `-engine gost`.

This means that you turn the gost engine on by default in the file

> This works unless tool is compiled without OPENSSL_LOAD_CONF and it
> calls OpenSSL_add_all_algorithms() instead of
> OPENSSL_add_all_algorithms_conf(). Which is frequently the default,
> (because there is too much methods of openssl initialization and
> people may not understand all intrications for all options.) In that
> case we try to persuade tool author to change the way openssl is
> initialized.

What I'm saying is that modifying config files is really difficult for
most users.  So, if you want ima-evm-utils to work out of the box with
the Streebog hash it needs to have no dependent requirement on config
files, which means you need to add the call to ENGINE_set_default()

This isn't about how you usually do it, it's about making these
additions usable for average users.

That doesn't stop you from adding further mods via the config files,
but it does mean that if a random user installs evmctl and types

evmctl -e gost -a streebog256 ...

it will just work instead of failing with an obscure error because the
default config file is wrong.  I actually think for usability you
should tie the hash and the engine together so a user can just type

evmctl -a streebog256 ...

And the tool will automatically try to load the gost engine and tell
you if it's missing.

James

Re: [PATCH] ima-evm-utils: remove redundant call to OpenSSL_add_all_algorithms

[Vitaly Chikunov]

On Wed, Jan 30, 2019 at 08:44:25AM -0800, James Bottomley wrote:
> On Wed, 2019-01-30 at 19:12 +0300, Vitaly Chikunov wrote:
> > On Wed, Jan 30, 2019 at 07:35:49AM -0800, James Bottomley wrote:
> > > On Wed, 2019-01-30 at 16:25 +0300, Vitaly Chikunov wrote:
> > > > On Wed, Jan 30, 2019 at 07:34:57AM -0500, Mimi Zohar wrote:
> > > > > On Sun, 2019-01-27 at 05:39 +0300, Vitaly Chikunov wrote:
> > > > > > Because of call to OPENSSL_add_all_algorithms_conf() calling
> > > > > > OpenSSL_add_all_algorithms() is not needed. There was not be
> > > > > > any
> > > > > > problems though because double initialization is permitted.
> > > > > > ---
> > > > > >  src/libimaevm.c | 1 -
> > > > > >  1 file changed, 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > > > > > index 7501303..b038d0c 100644
> > > > > > --- a/src/libimaevm.c
> > > > > > +++ b/src/libimaevm.c
> > > > > > @@ -995,7 +995,6 @@ int sign_hash(const char *hashalgo, const
> > > > > > unsigned char *hash, int size, const c
> > > > > > 
> > > > > >  static void libinit()
> > > > > >  {
> > > > > > -	OpenSSL_add_all_algorithms();
> > > > > >  	OPENSSL_add_all_algorithms_conf();
> > > > > >  	ERR_load_crypto_strings();
> > > > > >  }
> > > > > 
> > > > > The only difference between the two calls seems to be reading
> > > > > the
> > > > > system openssl.cnf file.  In the original call that is
> > > > > dependent on
> > > > > OPENSSL_LOAD_CONF being defined.  Calling
> > > > > OPENSSL_add_all_algorithms_conf(), forces reading the system
> > > > > openssl.cnf.
> > > > 
> > > > Yes. OPENSSL_LOAD_CONF is per application define, which is by
> > > > default
> > > > undefined. And instead of defining it, we could just call
> > > > OPENSSL_add_all_algorithms_conf(), which is required for GOST
> > > > support.
> > > > Otherwise enabling Streebog via OPENSSL_CONF will not work.
> > > 
> > > It will if you call
> > 
> > There is preferred "easy" method of [system wide] loading of gost-
> > engine
> > "by default" just by changing openssl.cnf like this:
> > 
> >   https://github.com/gost-engine/engine/blob/master/example.conf
> > 
> > After that change all openssl (and linked) tools understand GOST
> > algorithms without needing options like `-engine gost`.
> 
> This means that you turn the gost engine on by default in the file

This is how most gost users are suggested to use it.

> > This works unless tool is compiled without OPENSSL_LOAD_CONF and it
> > calls OpenSSL_add_all_algorithms() instead of
> > OPENSSL_add_all_algorithms_conf(). Which is frequently the default,
> > (because there is too much methods of openssl initialization and
> > people may not understand all intrications for all options.) In that
> > case we try to persuade tool author to change the way openssl is
> > initialized.
> 
> What I'm saying is that modifying config files is really difficult for
> most users.  So, if you want ima-evm-utils to work out of the box with
> the Streebog hash it needs to have no dependent requirement on config
> files, which means you need to add the call to ENGINE_set_default()

I implemented two methods of loading engine for evmctl (via config and
via --engine option).  There is no problem with --engine option for
Streebog, AFAIK.  We are talking about config method, which would be
default for the most users.

(Your suggestion is still good for my next patch what implements PKEY
instead of RSA, for EC-RDSA signatures. Thanks! I will add it.)

> This isn't about how you usually do it, it's about making these
> additions usable for average users.

Config method of loading gost engine is not how I usually do it, but how
most gost users use it (and suggested to use it). This isn't some marginal
use case. They just modify `openssl.cnf` once (manually of via script)
and all relevant ssl tools "magically" start to understand gost
algorithms without any additional options. This would be very not user
friendly if for every network tool that supports ssl they should specify
`--someoption gost`.

`--engine` option is just supplementary for advanced or occasional
users. It continues to work (for Streebog) if OpenSSL_add_all_algorithms
call is removed, but breaks if OPENSSL_add_all_algorithms_conf is
removed, probably, need to call ENGINE_load_builtin_engines in that case.

All this make initialization more complicated instead of single call to
OPENSSL_add_all_algorithms_conf().

> That doesn't stop you from adding further mods via the config files,
> but it does mean that if a random user installs evmctl and types
> 
> evmctl -e gost -a streebog256 ...
> 
> it will just work instead of failing with an obscure error because the
> default config file is wrong.  I actually think for usability you
> should tie the hash and the engine together so a user can just type
> 
> evmctl -a streebog256 ...
> 
> And the tool will automatically try to load the gost engine and tell
> you if it's missing.

This is interesting idea. (But this probably will fail for GOST
keys/certificates.)

> 
> James
> 
> 

Re: [PATCH] ima-evm-utils: remove redundant call to OpenSSL_add_all_algorithms

[James Bottomley]

On Wed, 2019-01-30 at 20:54 +0300, Vitaly Chikunov wrote:
> On Wed, Jan 30, 2019 at 08:44:25AM -0800, James Bottomley wrote:
> > On Wed, 2019-01-30 at 19:12 +0300, Vitaly Chikunov wrote:
[...]
> > > There is preferred "easy" method of [system wide] loading of
> > > gost-engine "by default" just by changing openssl.cnf like this:
> > > 
> > >   https://github.com/gost-engine/engine/blob/master/example.conf
> > > 
> > > After that change all openssl (and linked) tools understand GOST
> > > algorithms without needing options like `-engine gost`.
> > 
> > This means that you turn the gost engine on by default in the file
> 
> This is how most gost users are suggested to use it.

Well, no, this is how Russian Government people use it because gost
implements the mandatory aspects of their encryption standard.  If you
work for them you want gost all the time for everything.  So it's the
suggested way for that class of users.

However, that's not how a casual non-Russian user would want it. 
They'd only want gost if they specified the streebog hash.  And if we
advertise the hash (as we do because you added it to the help) they
should have a reasonable expectation of its working easily.

> > > This works unless tool is compiled without OPENSSL_LOAD_CONF and
> > > it calls OpenSSL_add_all_algorithms() instead of
> > > OPENSSL_add_all_algorithms_conf(). Which is frequently the
> > > default, (because there is too much methods of openssl
> > > initialization and people may not understand all intrications for
> > > all options.) In that case we try to persuade tool author to
> > > change the way openssl is initialized.
> > 
> > What I'm saying is that modifying config files is really difficult
> > for most users.  So, if you want ima-evm-utils to work out of the
> > box with the Streebog hash it needs to have no dependent
> > requirement on config files, which means you need to add the call
> > to ENGINE_set_default()
> 
> I implemented two methods of loading engine for evmctl (via config
> and via --engine option).  There is no problem with --engine option
> for Streebog, AFAIK.

Can you try it with a vanilla (non gost modified) openssl.cnf file to
verify?  I think you require the ENGINE_set_default() call but it may
be that a non-standard hash name will cause a search of the engine
added hashes.  OpenSSL has badly documented defaults, so I usually
chase that through the code, but in this case a simple experiment will
tell us.

>   We are talking about config method, which would be
> default for the most users.
> 
> (Your suggestion is still good for my next patch what implements PKEY
> instead of RSA, for EC-RDSA signatures. Thanks! I will add it.)
> 
> > This isn't about how you usually do it, it's about making these
> > additions usable for average users.
> 
> Config method of loading gost engine is not how I usually do it, but
> how most gost users use it (and suggested to use it). This isn't some
> marginal use case. They just modify `openssl.cnf` once (manually of
> via script) and all relevant ssl tools "magically" start to
> understand gost algorithms without any additional options. This would
> be very not user friendly if for every network tool that supports ssl
> they should specify `--someoption gost`.

Right, I'm not saying disallow that.  The Russian crypto case is
certainly a valid class of user.  What I'm saying is *don't* require
this config setup for other non-Russian crypto users because they won't
be familiar with it.

> `--engine` option is just supplementary for advanced or occasional
> users. It continues to work (for Streebog) if
> OpenSSL_add_all_algorithms call is removed, but breaks if
> OPENSSL_add_all_algorithms_conf is removed, probably, need to call
> ENGINE_load_builtin_engines in that case.
> 
> All this make initialization more complicated instead of single call
> to OPENSSL_add_all_algorithms_conf().

Yes, but it's acceptable to make our life more complex if it makes life
simpler for the end user.

> > That doesn't stop you from adding further mods via the config
> > files, but it does mean that if a random user installs evmctl and
> > types
> > 
> > evmctl -e gost -a streebog256 ...
> > 
> > it will just work instead of failing with an obscure error because
> > the default config file is wrong.  I actually think for usability
> > you should tie the hash and the engine together so a user can just
> > type
> > 
> > evmctl -a streebog256 ...
> > 
> > And the tool will automatically try to load the gost engine and
> > tell you if it's missing.
> 
> This is interesting idea. (But this probably will fail for GOST
> keys/certificates.)

Gost certs should identify the streebog hash, so in theory we can
examine the OIDs and see we need the gost engine to process them.  I'm
certainly not saying do this, I'm just giving it as an example of how
to make life easier for users.

James

Re: [PATCH] ima-evm-utils: remove redundant call to OpenSSL_add_all_algorithms

[Vitaly Chikunov]

On Wed, Jan 30, 2019 at 10:36:00AM -0800, James Bottomley wrote:
> On Wed, 2019-01-30 at 20:54 +0300, Vitaly Chikunov wrote:
> > On Wed, Jan 30, 2019 at 08:44:25AM -0800, James Bottomley wrote:
> > > On Wed, 2019-01-30 at 19:12 +0300, Vitaly Chikunov wrote:
> [...]
> > > > There is preferred "easy" method of [system wide] loading of
> > > > gost-engine "by default" just by changing openssl.cnf like this:
> > > > 
> > > >   https://github.com/gost-engine/engine/blob/master/example.conf
> > > > 
> > > > After that change all openssl (and linked) tools understand GOST
> > > > algorithms without needing options like `-engine gost`.
> > > 
> > > This means that you turn the gost engine on by default in the file
> > 
> > This is how most gost users are suggested to use it.
> 
> Well, no, this is how Russian Government people use it because gost
> implements the mandatory aspects of their encryption standard.  If you
> work for them you want gost all the time for everything.  So it's the
> suggested way for that class of users.

Well, no, this is not "how Russian Government people use it", but is how
normal people use it, and gost-engine does not make GOST everything,
everywhere, always, but just enables GOST algorithms to use when they
need. They may need to access gov portals, APIs, tax and communal
systems, banks, etc.

The root of the problem is GOST algorithms was split from mainstream OpenSSL
project into separate subproject gost-engine. Unlike, for example, to
SM2 or whatever you can use natively just by linking with openssl.
Linking openssl does not make you work for Chinese Government and use SM
algorithms all the time for everything.

Just to make GOST algorithms "automatically" accessible by software
which is linked to openssl, openssl.cnf trick is used. From the user point
of view these algorithms just become "inside" of openssl. And there is
no other way of extending openssl besides engines. And there are no other
ways of loading engine except by 1) making software and their users
aware of openssl engines, 2) config trick.

> However, that's not how a casual non-Russian user would want it. 
> They'd only want gost if they specified the streebog hash.  And if we
> advertise the hash (as we do because you added it to the help) they
> should have a reasonable expectation of its working easily.

It will. I support both methods of use. For occasional user there is
option --engine and for the frequent user there is config trick.

> > > > This works unless tool is compiled without OPENSSL_LOAD_CONF and
> > > > it calls OpenSSL_add_all_algorithms() instead of
> > > > OPENSSL_add_all_algorithms_conf(). Which is frequently the
> > > > default, (because there is too much methods of openssl
> > > > initialization and people may not understand all intrications for
> > > > all options.) In that case we try to persuade tool author to
> > > > change the way openssl is initialized.
> > > 
> > > What I'm saying is that modifying config files is really difficult
> > > for most users.  So, if you want ima-evm-utils to work out of the
> > > box with the Streebog hash it needs to have no dependent
> > > requirement on config files, which means you need to add the call
> > > to ENGINE_set_default()
> > 
> > I implemented two methods of loading engine for evmctl (via config
> > and via --engine option).  There is no problem with --engine option
> > for Streebog, AFAIK.
> 
> Can you try it with a vanilla (non gost modified) openssl.cnf file to
> verify?  I think you require the ENGINE_set_default() call but it may
> be that a non-standard hash name will cause a search of the engine
> added hashes.  OpenSSL has badly documented defaults, so I usually
> chase that through the code, but in this case a simple experiment will
> tell us.

Of course, I tried and tested that both ways are working independently.
Just for Streebog ENGINE_set_default is not required, but to support
GOST signatures (patch is RFCed) it will be required.

> >   We are talking about config method, which would be
> > default for the most users.
> > 
> > (Your suggestion is still good for my next patch what implements PKEY
> > instead of RSA, for EC-RDSA signatures. Thanks! I will add it.)
> > 
> > > This isn't about how you usually do it, it's about making these
> > > additions usable for average users.
> > 
> > Config method of loading gost engine is not how I usually do it, but
> > how most gost users use it (and suggested to use it). This isn't some
> > marginal use case. They just modify `openssl.cnf` once (manually of
> > via script) and all relevant ssl tools "magically" start to
> > understand gost algorithms without any additional options. This would
> > be very not user friendly if for every network tool that supports ssl
> > they should specify `--someoption gost`.
> 
> Right, I'm not saying disallow that.  The Russian crypto case is
> certainly a valid class of user.  What I'm saying is *don't* require
> this config setup for other non-Russian crypto users because they won't
> be familiar with it.

Config method is not required as there are two ways of use. But engine
does not load properly without a call to OPENSSL_add_all_algorithms_conf.
So, I am curious why to disallow it.

Also, calling OPENSSL_add_all_algorithms_conf will not cause any engine
load if a user does not edit config on her system.

Thanks,

> > `--engine` option is just supplementary for advanced or occasional
> > users. It continues to work (for Streebog) if
> > OpenSSL_add_all_algorithms call is removed, but breaks if
> > OPENSSL_add_all_algorithms_conf is removed, probably, need to call
> > ENGINE_load_builtin_engines in that case.
> > 
> > All this make initialization more complicated instead of single call
> > to OPENSSL_add_all_algorithms_conf().
> 
> Yes, but it's acceptable to make our life more complex if it makes life
> simpler for the end user.
> 
> > > That doesn't stop you from adding further mods via the config
> > > files, but it does mean that if a random user installs evmctl and
> > > types
> > > 
> > > evmctl -e gost -a streebog256 ...
> > > 
> > > it will just work instead of failing with an obscure error because
> > > the default config file is wrong.  I actually think for usability
> > > you should tie the hash and the engine together so a user can just
> > > type
> > > 
> > > evmctl -a streebog256 ...
> > > 
> > > And the tool will automatically try to load the gost engine and
> > > tell you if it's missing.
> > 
> > This is interesting idea. (But this probably will fail for GOST
> > keys/certificates.)
> 
> Gost certs should identify the streebog hash, so in theory we can
> examine the OIDs and see we need the gost engine to process them.  I'm
> certainly not saying do this, I'm just giving it as an example of how
> to make life easier for users.
> 
> James

Re: [PATCH] ima-evm-utils: remove redundant call to OpenSSL_add_all_algorithms

[James Bottomley]

On Thu, 2019-01-31 at 12:22 +0300, Vitaly Chikunov wrote:
> On Wed, Jan 30, 2019 at 10:36:00AM -0800, James Bottomley wrote:
[...]
> > However, that's not how a casual non-Russian user would want it. 
> > They'd only want gost if they specified the streebog hash.  And if
> > we advertise the hash (as we do because you added it to the help)
> > they should have a reasonable expectation of its working easily.
> 
> It will. I support both methods of use. For occasional user there is
> option --engine and for the frequent user there is config trick.

OK, as long as users can use it without modifying the config file, I'm
happy.

[...]
> > > I implemented two methods of loading engine for evmctl (via
> > > config and via --engine option).  There is no problem with --
> > > engine option for Streebog, AFAIK.
> > 
> > Can you try it with a vanilla (non gost modified) openssl.cnf file
> > to verify?  I think you require the ENGINE_set_default() call but
> > it may be that a non-standard hash name will cause a search of the
> > engine added hashes.  OpenSSL has badly documented defaults, so I
> > usually chase that through the code, but in this case a simple
> > experiment will tell us.
> 
> Of course, I tried and tested that both ways are working
> independently. Just for Streebog ENGINE_set_default is not required,
> but to support GOST signatures (patch is RFCed) it will be required.

I agree, I tried it with the openssl gost engine and you get this weird
behaviour (I have to use md_gost94 because 1.0.2 gost doesn't have
streebog):

jejb@mulgrave:~/git/ima-evm-utils/src> ./evmctl -n --hashalgo md_gost94  ima_hash ~/tmp.ppt
01945d562c031c262563b026d8cc53e070140ad101
jejb@mulgrave:~/git/ima-evm-utils/src> ./evmctl -n --engine gost --hashalgo md_gost94  ima_hash ~/tmp.ppt
01a930a87289b548c2744fbb183a22196b1f651a727d84021d0eeb80cb4dddbb5d

Because IMA silently falls back on sha1 if it can't find the hash.  But
the test proves it will use the gost hash when the engine is provided
without ENGINE_set_default().

James