On Fri, 12 Oct 2018, Jakub Jelen wrote:> Something like this can be used to properly initialize new OpenSSL > versions: > > @@ -70,12 +70,19 @@ ssh_compatible_openssl(long headerver, long libver) > void > ssh_OpenSSL_add_all_algorithms(void) > { > +#if OPENSSL_VERSION_NUMBER < 0x10100000L > OpenSSL_add_all_algorithms(); > > /* Enable use of crypto hardware */ > ENGINE_load_builtin_engines(); > +#if OPENSSL_VERSION_NUMBER < 0x10001000L > ENGINE_register_all_complete(); > +#endif > OPENSSL_config(NULL); > +#else > + OPENSSL_init_crypto(OPENSSL_INIT_ADD_ALL_DIGESTS | > + OPENSSL_INIT_ADD_ALL_DIGESTS | OPENSSL_INIT_LOAD_CONFIG, > NULL); > +#endifI don't think the #ifs match the #endifs properly here - it leaves the OPENSSL_init_crypto() call inside a #if OPENSSL_VERSION_NUMBER < 0x10100000L... IMO this is simpler and better preserves the current flow of the code. OpenSSL_add_all_algorithms() isn't marked as deprecated in the OpenSSL headers, is used elsewhere in OpenSSH and is still used in most of OpenSSL's demos/*, so I don't see any need to skip that ATM. diff --git a/openbsd-compat/openssl-compat.c b/openbsd-compat/openssl-compat.c index 259fccbe..762358f0 100644 --- a/openbsd-compat/openssl-compat.c +++ b/openbsd-compat/openssl-compat.c @@ -75,7 +75,13 @@ ssh_OpenSSL_add_all_algorithms(void) /* Enable use of crypto hardware */ ENGINE_load_builtin_engines(); ENGINE_register_all_complete(); + +#if OPENSSL_VERSION_NUMBER < 0x10001000L OPENSSL_config(NULL); +#else + OPENSSL_init_crypto(OPENSSL_INIT_ADD_ALL_CIPHERS | + OPENSSL_INIT_ADD_ALL_DIGESTS | OPENSSL_INIT_LOAD_CONFIG); +#endif } #endif
On Oct 15 10:18, Damien Miller wrote:> On Fri, 12 Oct 2018, Jakub Jelen wrote: > > > Something like this can be used to properly initialize new OpenSSL > > versions: > > > > @@ -70,12 +70,19 @@ ssh_compatible_openssl(long headerver, long libver) > > void > > ssh_OpenSSL_add_all_algorithms(void) > > { > > +#if OPENSSL_VERSION_NUMBER < 0x10100000L > > OpenSSL_add_all_algorithms(); > > > > /* Enable use of crypto hardware */ > > ENGINE_load_builtin_engines(); > > +#if OPENSSL_VERSION_NUMBER < 0x10001000L > > ENGINE_register_all_complete(); > > +#endif > > OPENSSL_config(NULL); > > +#else > > + OPENSSL_init_crypto(OPENSSL_INIT_ADD_ALL_DIGESTS | > > + OPENSSL_INIT_ADD_ALL_DIGESTS | OPENSSL_INIT_LOAD_CONFIG, > > NULL); > > +#endif > > I don't think the #ifs match the #endifs properly here - it leaves > the OPENSSL_init_crypto() call inside a #if OPENSSL_VERSION_NUMBER < > 0x10100000L...#if bracketing is correct, afaics: #if OPENSSL_VERSION_NUMBER < 0x10100000L #if OPENSSL_VERSION_NUMBER < 0x10001000L #endif #else #endif There's only one OPENSSL_INIT_ADD_ALL_DIGESTS too many. HTH, Corinna -- Corinna Vinschen Cygwin Maintainer Red Hat -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 833 bytes Desc: not available URL: <http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20181015/58e63299/attachment.asc>
On Mon, 2018-10-15 at 08:32 +0200, Corinna Vinschen wrote:> On Oct 15 10:18, Damien Miller wrote: > > On Fri, 12 Oct 2018, Jakub Jelen wrote: > > > > > Something like this can be used to properly initialize new > > > OpenSSL > > > versions: > > > > > > @@ -70,12 +70,19 @@ ssh_compatible_openssl(long headerver, long > > > libver) > > > void > > > ssh_OpenSSL_add_all_algorithms(void) > > > { > > > +#if OPENSSL_VERSION_NUMBER < 0x10100000L > > > OpenSSL_add_all_algorithms(); > > > > > > /* Enable use of crypto hardware */ > > > ENGINE_load_builtin_engines(); > > > +#if OPENSSL_VERSION_NUMBER < 0x10001000L > > > ENGINE_register_all_complete(); > > > +#endif > > > OPENSSL_config(NULL); > > > +#else > > > + OPENSSL_init_crypto(OPENSSL_INIT_ADD_ALL_DIGESTS | > > > + OPENSSL_INIT_ADD_ALL_DIGESTS | OPENSSL_INIT_LOAD_CONFIG, > > > NULL); > > > +#endif > > > > I don't think the #ifs match the #endifs properly here - it leaves > > the OPENSSL_init_crypto() call inside a #if OPENSSL_VERSION_NUMBER > > < > > 0x10100000L... > > #if bracketing is correct, afaics: > > #if OPENSSL_VERSION_NUMBER < 0x10100000L > #if OPENSSL_VERSION_NUMBER < 0x10001000L > #endif > #else > #endifYou are right.> There's only one OPENSSL_INIT_ADD_ALL_DIGESTS too many.Good catch. The one of them should probably have been OPENSSL_INIT_ENGINE_ALL_BUILTIN. The OpenSSL_add_all_algorithms() is described as deprecated in the official documentation [1] and matches the functionality of the new call OPENSSL_init_crypto(). [1] https://www.openssl.org/docs/man1.1.0/crypto/OpenSSL_add_all_algorithms.html -- Jakub Jelen Software Engineer Security Technologies Red Hat, Inc.
On Mon, 2018-10-15 at 10:18 +1100, Damien Miller wrote:> On Fri, 12 Oct 2018, Jakub Jelen wrote: > > > Something like this can be used to properly initialize new OpenSSL > > versions: > > > > @@ -70,12 +70,19 @@ ssh_compatible_openssl(long headerver, long > > libver) > > void > > ssh_OpenSSL_add_all_algorithms(void) > > { > > +#if OPENSSL_VERSION_NUMBER < 0x10100000L > > OpenSSL_add_all_algorithms(); > > > > /* Enable use of crypto hardware */ > > ENGINE_load_builtin_engines(); > > +#if OPENSSL_VERSION_NUMBER < 0x10001000L > > ENGINE_register_all_complete(); > > +#endif > > OPENSSL_config(NULL); > > +#else > > + OPENSSL_init_crypto(OPENSSL_INIT_ADD_ALL_DIGESTS | > > + OPENSSL_INIT_ADD_ALL_DIGESTS | OPENSSL_INIT_LOAD_CONFIG, > > NULL); > > +#endif > > I don't think the #ifs match the #endifs properly here - it leaves > the OPENSSL_init_crypto() call inside a #if OPENSSL_VERSION_NUMBER < > 0x10100000L... > > IMO this is simpler and better preserves the current flow of the > code. > OpenSSL_add_all_algorithms() isn't marked as deprecated in the > OpenSSL > headers, is used elsewhere in OpenSSH and is still used in most of > OpenSSL's demos/*, so I don't see any need to skip that ATM. > > diff --git a/openbsd-compat/openssl-compat.c b/openbsd- > compat/openssl-compat.c > index 259fccbe..762358f0 100644 > --- a/openbsd-compat/openssl-compat.c > +++ b/openbsd-compat/openssl-compat.c > @@ -75,7 +75,13 @@ ssh_OpenSSL_add_all_algorithms(void) > /* Enable use of crypto hardware */ > ENGINE_load_builtin_engines(); > ENGINE_register_all_complete(); > + > +#if OPENSSL_VERSION_NUMBER < 0x10001000L > OPENSSL_config(NULL); > +#else > + OPENSSL_init_crypto(OPENSSL_INIT_ADD_ALL_CIPHERS | > + OPENSSL_INIT_ADD_ALL_DIGESTS | OPENSSL_INIT_LOAD_CONFIG); > +#endif > } > #endifThe version in the last snap 20181017 (master commit [1]) is actually missing the last (NULL) argument so the master/snap does not compile at all now with new OpenSSL. [1] https://github.com/openssh/openssh-portable/commit/4e23deef Regards, -- Jakub Jelen Software Engineer Security Technologies Red Hat, Inc.
On Tue, 2018-10-16 at 20:13 +0200, Jakub Jelen wrote:> On Mon, 2018-10-15 at 10:18 +1100, Damien Miller wrote: > > On Fri, 12 Oct 2018, Jakub Jelen wrote: > > > > > Something like this can be used to properly initialize new > > > OpenSSL > > > versions: > > > > > > @@ -70,12 +70,19 @@ ssh_compatible_openssl(long headerver, long > > > libver) > > > void > > > ssh_OpenSSL_add_all_algorithms(void) > > > { > > > +#if OPENSSL_VERSION_NUMBER < 0x10100000L > > > OpenSSL_add_all_algorithms(); > > > > > > /* Enable use of crypto hardware */ > > > ENGINE_load_builtin_engines(); > > > +#if OPENSSL_VERSION_NUMBER < 0x10001000L > > > ENGINE_register_all_complete(); > > > +#endif > > > OPENSSL_config(NULL); > > > +#else > > > + OPENSSL_init_crypto(OPENSSL_INIT_ADD_ALL_DIGESTS | > > > + OPENSSL_INIT_ADD_ALL_DIGESTS | OPENSSL_INIT_LOAD_CONFIG, > > > NULL); > > > +#endif > > > > I don't think the #ifs match the #endifs properly here - it leaves > > the OPENSSL_init_crypto() call inside a #if OPENSSL_VERSION_NUMBER > > < > > 0x10100000L... > > > > IMO this is simpler and better preserves the current flow of the > > code. > > OpenSSL_add_all_algorithms() isn't marked as deprecated in the > > OpenSSL > > headers, is used elsewhere in OpenSSH and is still used in most of > > OpenSSL's demos/*, so I don't see any need to skip that ATM. > > > > diff --git a/openbsd-compat/openssl-compat.c b/openbsd- > > compat/openssl-compat.c > > index 259fccbe..762358f0 100644 > > --- a/openbsd-compat/openssl-compat.c > > +++ b/openbsd-compat/openssl-compat.c > > @@ -75,7 +75,13 @@ ssh_OpenSSL_add_all_algorithms(void) > > /* Enable use of crypto hardware */ > > ENGINE_load_builtin_engines(); > > ENGINE_register_all_complete(); > > + > > +#if OPENSSL_VERSION_NUMBER < 0x10001000L > > OPENSSL_config(NULL); > > +#else > > + OPENSSL_init_crypto(OPENSSL_INIT_ADD_ALL_CIPHERS | > > + OPENSSL_INIT_ADD_ALL_DIGESTS | OPENSSL_INIT_LOAD_CONFIG); > > +#endif > > } > > #endif > > The version in the last snap 20181017 (master commit [1]) is actually > missing the last (NULL) argument so the master/snap does not compile > at > all now with new OpenSSL. > > [1] https://github.com/openssh/openssh-portable/commit/4e23deefAfter fixing the missing argument above, all the tests pass for me on Fedora 28 with OpenSSL 1.1.0 Thanks, Jakub