engine icon indicating copy to clipboard operation
engine copied to clipboard

Feature request: For OpenSSL 3.0, modify the gost implementation to become an provider module

Open levitte opened this issue 4 years ago • 14 comments

With OpenSSL 3.0, there is this new interface for dealing with extension, termed "provider". This is intended to be a more flexible interface, and most importantly, both upward and downward compatible with different OpenSSL versions (starting with 3.0). The ENGINE API will eventually go away, and you might as well start working on the provider module form now.

levitte avatar Aug 29 '19 06:08 levitte

I've started to implement the provider, taking the legacy provider as an example. md2_functions is defined via the IMPLEMENT_digest_functions macro. It seems to be not a public one.

What is a relevant way to set the implementation for the 3rd-party providers?

beldmit avatar Oct 27 '19 18:10 beldmit

It might be better to look at a simpler example: https://github.com/provider-corner/vigenere

(because openssl's three providers are quite entangled, they are not a good starting example)

levitte avatar Oct 27 '19 18:10 levitte

BTW, I'm working that provider up to be a bit more complete, including individual error reporting, which was more difficult than I had anticipated.

levitte avatar Oct 28 '19 10:10 levitte

Done, please have a look and tell me if there's anything that needs clarification (actually, feel free to raise issues there)

levitte avatar Oct 28 '19 10:10 levitte

I've converted the digests to a provider form but found some problems.

When I try to define micalg as gettable param, I get an error:

/home/beldmit/crypto/engine/gost_md2012.c: In function ‘STREEBOG_gettable_params’:
/home/beldmit/crypto/openssl/include/openssl/params.h:61:49: error: lvalue required as unary ‘&’ operand
     OSSL_PARAM_DEFN((key), OSSL_PARAM_UTF8_PTR, &(addr), sz)
                                                 ^
/home/beldmit/crypto/openssl/include/openssl/params.h:25:23: note: in definition of macro ‘OSSL_PARAM_DEFN’
     { (key), (type), (addr), (sz), 0 }
                       ^~~~
/home/beldmit/crypto/engine/gost_md2012.c:94:9: note: in expansion of macro ‘OSSL_PARAM_utf8_ptr’
         OSSL_PARAM_utf8_ptr("micalg", NULL, strlen(micalg_256)+1),
         ^~~~~~~~~~~~~~~~~~~

So what is the pattern of using the OSSL_PARAM_utf8_ptr here? (https://github.com/gost-engine/engine/blob/011e15b1cee3283c4700c113211f53f4d777860f/gost_md2012.c#L94)

I tried to copy the pattern from fipsprov.c, but did no succeed. See also https://github.com/openssl/openssl/issues/10399

beldmit avatar Nov 09 '19 20:11 beldmit

So parameter will be NULL of size strlen(micalg_256)+1 which is strange.

vt-alt avatar Nov 09 '19 21:11 vt-alt

I've tried many variants but none of them made the compiler happy. It means I do not get the point.

beldmit avatar Nov 09 '19 21:11 beldmit

@beldmit, you need to update your openssl 3.0-dev installation, this is an issue that was fixed quite a while ago, for exactly this sort of reason (ever since we started using the OSSL_PARAM array as a descriptor).

Here's what that macro looks like right now:

https://github.com/openssl/openssl/blob/master/include/openssl/params.h#L56

levitte avatar Nov 10 '19 03:11 levitte

Hmmm. The macro looks exactly as you say, and the copy is up-to-date. Could it be a compiler issue?

Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/8/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 8.3.0-6' --with-bugurl=file:///usr/share/doc/gcc-8/README.Bugs --enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++ --prefix=/usr --with-gcc-major-version-only --program-suffix=-8 --program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --enable-bootstrap --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-vtable-verify --enable-libmpx --enable-plugin --enable-default-pie --with-system-zlib --with-target-system-zlib --enable-objc-gc=auto --enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-offload-targets=nvptx-none --without-cuda-driver --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 8.3.0 (Debian 8.3.0-6) 

beldmit avatar Nov 10 '19 08:11 beldmit

Are you sure you're looking at the right one? This is very specific:

/home/beldmit/crypto/openssl/include/openssl/params.h:61:49: error: lvalue required as unary ‘&’ operand
     OSSL_PARAM_DEFN((key), OSSL_PARAM_UTF8_PTR, &(addr), sz)
                                                 ^

If you look at the line I pointed at, the & is gone, so it is not exactly the same.

levitte avatar Nov 10 '19 12:11 levitte

Sure. The parameter was described as OSSL_PARAM_utf8_ptr, not OSSL_PARAM_utf8_string, and you've provided the link to the line defining the OSSL_PARAM_utf8_string macro.

But after I changed it to OSSL_PARAM_utf8_string, I got an error

/home/beldmit/crypto/engine/gost_md2012.c: In function ‘STREEBOG_gettable_params’:
/home/beldmit/crypto/openssl/include/openssl/params.h:25:22: error: initialization discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
     { (key), (type), (addr), (sz), 0 }
                      ^
/home/beldmit/crypto/openssl/include/openssl/params.h:56:5: note: in expansion of macro ‘OSSL_PARAM_DEFN’
     OSSL_PARAM_DEFN((key), OSSL_PARAM_UTF8_STRING, (addr), sz)
     ^~~~~~~~~~~~~~~
/home/beldmit/crypto/engine/gost_md2012.c:94:9: note: in expansion of macro ‘OSSL_PARAM_utf8_string’
         OSSL_PARAM_utf8_string("micalg", micalg_256, strlen(micalg_256)+1),
         ^~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Casting the string to char * eliminates this error, but it seems wrong for this case:

        OSSL_PARAM_utf8_string("micalg", (char *)micalg_256, strlen(micalg_256)+1),

So it means I do not understand:

  1. When I should use utf8_string and when I should use utf8_ptr
  2. What shell I use when I want to use the constant string. I can just add -Wno-discarded-qualifiers but it seems a wrong solution.

beldmit avatar Nov 10 '19 15:11 beldmit

Oh silly me, I didn't notice that you used the _PTR form. To your question, the _PTR variant is intended to be used when the pointer itself is the data, rather than what it's pointing at. That doesn't seem important in this case.

Either way, if we're still talking about a gettable, this is what I would specify:

    OSSL_PARAM_utf8_string("micalg", NULL, 0),

(the 0 can be replaced with any number if you want to specify a maximum size)

levitte avatar Nov 10 '19 18:11 levitte

GOST ciphers from gostprov don't work.

ssl_load_ciphers() determines whether GOST algos are available by using get_optional_pkey_id(). This works with the engine, but it won't work with the provider.

@levitte Is it possible to rewrite checks in ssl_load_ciphers() to use provider-aware EVP_KEYEXCH_fetch()?

yanovich avatar Jun 06 '22 15:06 yanovich

@yanovich I think this question is worth raising against openssl. Yes, it definitely makes sense.

beldmit avatar Jun 06 '22 17:06 beldmit