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

openssl: remove most BoringSSL #ifdefs. #640

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@davidben
Contributor

davidben commented Feb 9, 2016

I've tested this against BoringSSL and OpenSSL 1.0.2. make test passes against both on my machine. I'm happy to be more or less aggressive in removing the #ifdefs as you prefer.

Notably, I can see an argument to keep the RAND seeding code out. See https://commondatastorage.googleapis.com/chromium-boringssl-docs/rand.h.html#Deprecated-functions for the various ridiculous tricks we do to no-op everyone's RAND seeding logic. :-) Your call.


As of https://boringssl-review.googlesource.com/#/c/6980/, almost all of BoringSSL #ifdefs in cURL should be unnecessary:

  • BoringSSL provides no-op stubs for compatibility which replaces most #ifdefs.
  • DES_set_odd_parity has been in BoringSSL for nearly a year now. Remove the compatibility codepath.
  • With a small tweak to an extend_key_56_to_64 call, the NTLM code builds fine.
  • Switch OCSP-related #ifdefs to the more generally useful OPENSSL_NO_OCSP.

The only #ifdefs which remain are Curl_ossl_version and the #undefs to work around OpenSSL and wincrypt.h name conflicts. (BoringSSL leaves that to the consumer. The in-header workaround makes things sensitive to include order.)

This change errs on the side of removing conditionals despite many of the restored codepaths being no-ops. (BoringSSL generally adds no-op compatibility stubs when possible. OPENSSL_VERSION_NUMBER #ifdefs are bad enough!)

openssl: remove most BoringSSL #ifdefs.
As of https://boringssl-review.googlesource.com/#/c/6980/, almost all of
BoringSSL #ifdefs in cURL should be unnecessary:

- BoringSSL provides no-op stubs for compatibility which replaces most
  #ifdefs.

- DES_set_odd_parity has been in BoringSSL for nearly a year now. Remove
  the compatibility codepath.

- With a small tweak to an extend_key_56_to_64 call, the NTLM code
  builds fine.

- Switch OCSP-related #ifdefs to the more generally useful
  OPENSSL_NO_OCSP.

The only #ifdefs which remain are Curl_ossl_version and the #undefs to
work around OpenSSL and wincrypt.h name conflicts. (BoringSSL leaves
that to the consumer. The in-header workaround makes things sensitive to
include order.)

This change errs on the side of removing conditionals despite many of
the restored codepaths being no-ops. (BoringSSL generally adds no-op
compatibility stubs when possible. OPENSSL_VERSION_NUMBER #ifdefs are
bad enough!)
#include <openssl/ui.h>
#else
/* ENGINE_load_private_key() takes three arguments */
#undef HAVE_ENGINE_LOAD_FOUR_ARGS

This comment has been minimized.

@davidben

davidben Feb 9, 2016

Contributor

(I'm not sure where this came from. We've never had that function.)

@davidben

davidben Feb 9, 2016

Contributor

(I'm not sure where this came from. We've never had that function.)

This comment has been minimized.

@bagder

bagder Feb 9, 2016

Member

can't recall that specific one, I assume that was just a mistake of mine

@bagder

bagder Feb 9, 2016

Member

can't recall that specific one, I assume that was just a mistake of mine

@@ -143,14 +143,10 @@ static void setup_des_key(const unsigned char *key_56,
DES_cblock key;
/* Expand the 56-bit key to 64-bits */
extend_key_56_to_64(key_56, (char *) key);
extend_key_56_to_64(key_56, (char *) &key);

This comment has been minimized.

@jay

jay Feb 9, 2016

Member

@davidben This looks wrong please review

@jay

jay Feb 9, 2016

Member

@davidben This looks wrong please review

This comment has been minimized.

@davidben

davidben Feb 9, 2016

Contributor

DES_cblock is a struct wrapping an array in BoringSSL and a plain array in OpenSSL. (Array types in C act really surprisingly, even when typedef'd. I wish OpenSSL hadn't done that.) Sometimes code needs the occasional & to smooth it over. :-/ On the plus side, it makes it clearer that you're mutating the key.

@davidben

davidben Feb 9, 2016

Contributor

DES_cblock is a struct wrapping an array in BoringSSL and a plain array in OpenSSL. (Array types in C act really surprisingly, even when typedef'd. I wish OpenSSL hadn't done that.) Sometimes code needs the occasional & to smooth it over. :-/ On the plus side, it makes it clearer that you're mutating the key.

This comment has been minimized.

@jay

jay Feb 9, 2016

Member

Ok makes sense, thanks.

@jay

jay Feb 9, 2016

Member

Ok makes sense, thanks.

@bagder bagder added the SSL/TLS label Feb 9, 2016

@bagder bagder self-assigned this Feb 9, 2016

@bagder bagder closed this in 39c803c Feb 9, 2016

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 9, 2016

Member

Thanks!

Member

bagder commented Feb 9, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment