Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
levitte opened this issue Aug 29, 2019 · 14 comments
Assignees

Comments

@levitte
Copy link
Contributor

levitte commented Aug 29, 2019

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 levitte changed the title For OpenSSL 3.0, modify the gost implementation to become an provider module Feature request: For OpenSSL 3.0, modify the gost implementation to become an provider module Aug 29, 2019
@beldmit beldmit self-assigned this Aug 29, 2019
@beldmit
Copy link
Contributor

beldmit commented Oct 27, 2019

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 beldmit pinned this issue Oct 27, 2019
@levitte
Copy link
Contributor Author

levitte commented Oct 27, 2019

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
Copy link
Contributor Author

levitte commented Oct 28, 2019

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
Copy link
Contributor Author

levitte commented Oct 28, 2019

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

@beldmit
Copy link
Contributor

beldmit commented Nov 9, 2019

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?
(

/* OSSL_PARAM_utf8_ptr("micalg", NULL, strlen(micalg_256)+1), */
)

I tried to copy the pattern from fipsprov.c, but did no succeed.
See also openssl/openssl#10399

@vt-alt
Copy link
Member

vt-alt commented Nov 9, 2019

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

@beldmit
Copy link
Contributor

beldmit commented Nov 9, 2019

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

@levitte
Copy link
Contributor Author

levitte commented Nov 10, 2019

@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

@beldmit
Copy link
Contributor

beldmit commented Nov 10, 2019

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) 

@levitte
Copy link
Contributor Author

levitte commented Nov 10, 2019

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.

@beldmit
Copy link
Contributor

beldmit commented Nov 10, 2019

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.

@levitte
Copy link
Contributor Author

levitte commented Nov 10, 2019

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)

@yanovich
Copy link

yanovich commented Jun 6, 2022

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()?

@beldmit
Copy link
Contributor

beldmit commented Jun 6, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants