Skip to content

Conversation

@jonasschnelli
Copy link
Contributor

Alternative solution for #7585. Re-Enables LibreSSL compatibility.
Commit count and credit goes to @AliceWonderMiscreations

@jonasschnelli
Copy link
Contributor Author

Added a commit that fixes a logical text-issue when compiling with LibreSSL.
Current masters shows: Using OpenSSL Version: OpenSSL 1.0.2d 9 Jul 2015 (or similar).
After this PR: Using SSL Version: OpenSSL 1.0.2d 9 Jul 2015 (or similar).

@laanwj
Copy link
Member

laanwj commented Feb 24, 2016

In main.cpp we explicitly cede for LibreSSL, do we want a similar construction here? (or even: factor it out somehow so it exists only in one place, so that we don't have this inconsistency next time)

#if (OPENSSL_VERSION_NUMBER < 0x10100000L)
    LogPrintf("Using OpenSSL version %s\n", SSLeay_version(SSLEAY_VERSION));
#elif defined OPENSSL_VERSION
    LogPrintf("Using OpenSSL version %s\n", OpenSSL_version(OPENSSL_VERSION));
#elif defined LIBRESSL_VERSION_TEXT
    LogPrintf("Using %s\n", LIBRESSL_VERSION_TEXT);
#endif

@jonasschnelli
Copy link
Contributor Author

Yes. Let me factor it out.

But not sure about extra treating LibreSSL, IMO we should just use SSLeay_version() (which will result in returning LIBRESSL_VERSION_TEXT in the background when compiling against LibreSSL). Any OpenSSL compatible layer should probably implement SSLeay_version().

I could imaging allow compiling against different OpenSSL alternatives (Apple also has it's own IIRC).

@laanwj
Copy link
Member

laanwj commented Feb 24, 2016

IIRC support for SSLeay_version() was dropped in recent OpenSSL, that's why the version check was added. But apparently LibreSSL didn't follow that convention, even though they report as an OpenSSL version that would no longer have SSLeay_version(). So it is, sort of, their mistake, as they aren't a perfect drop-in replacement for OpenSSL anymore.

(or we have the OpenSSL_version versus SSLeay_version threshold configured wrongly in the first place... but I think that was checked after I remarked that before on "OpenSSL 1.1 - SSLEAY_VERSION Not Declared In This Scope #7080")

@AliceWonderMiscreations
Copy link
Contributor

It's not LibreSSL's mistake. They forked at a specific version of OpenSSL that they guarantee API compatibility with. That version uses SSLEAY_VERSION. So LibreSSL has to keep that in order to keep the API compatibility promise.

The change in OpenSSL is after the fork.

@jonasschnelli
Copy link
Contributor Author

Added a commit that refactors the SSL detection.
Needs testing on CentOS (or other LibreSSL distribution)

@laanwj
Copy link
Member

laanwj commented Feb 24, 2016

It's not LibreSSL's mistake. They forked at a specific version of OpenSSL that they guarantee API compatibility with. That version uses SSLEAY_VERSION. So LibreSSL has to keep that in order to keep the API compatibility promise.

That's true but they do report a newer OpenSSL version in the OPENSSL_VERSION_NUMBER macro, which, in case of OpenSSL means that there is no longer a SSLeay_version.
So they forked from an older release, then bumped the version, without changing the API to be the same as that newer release of OpenSSL.

@AliceWonderMiscreations
Copy link
Contributor

@jonasschnelli just a note, CentOS is not a LibreSSL distribution. I built and run LibreSSL on it, as the OpenSSL it has is old and has poor ECC support.

@jonasschnelli jonasschnelli changed the title [Qt] fix for building against LibreSSL fixes/refactoring for building against LibreSSL Feb 24, 2016
@laanwj
Copy link
Member

laanwj commented Feb 24, 2016

as the OpenSSL it has is old and has poor ECC support.

Aside, as it is no reason to have worse support for LibreSSL, but as of 0.12, ECC support in OpenSSL is no longer a requirement for Bitcoin Core.

@AliceWonderMiscreations
Copy link
Contributor

@laanwj yeah, but while I can have both openssl and libressl installed at the same time I can't have the header files to both installed at the same time, at least not with both installed in /usr/include

<widget class="QLabel" name="label_14">
<property name="text">
<string>Using OpenSSL version</string>
<string>Using SSL version</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add library to make clear it's not the protocol?

@maflcko
Copy link
Member

maflcko commented Feb 24, 2016

Concept ACK

#elif defined(SSLEAY_VERSION_NUMBER) || defined(OPENSSL_VERSION_NUMBER)
return SSLeay_version(SSLEAY_VERSION);
#else
return NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about returning _("unknown") and further simplify the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Soon, there could be an option to run without SSL (after rand and aes implementation).
Then we might want to skip/hide the SSL log info?

@paveljanik
Copy link
Contributor

Debug log:

before:

2016-02-24 16:42:18 Using OpenSSL version OpenSSL 1.0.x 9 Apr 20xx

after:

2016-02-24 16:44:02 Using SSL version: OpenSSL 1.0.x 9 Apr 20xx

I propose to change the text to "Using SSL library"


std::string CopyrightHolders(const std::string& strPrefix);

/* returns the current SSL product and version info */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this file is completely documented, can you please extend the comments? E.g. like this:

/**
  * Return the product name with the version number of the SSL library being used.
  * @note Bitcoin Core supports OpenSSL and LibreSSL.
  */

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Will use you comment.

@gmaxwell
Copy link
Contributor

Since its no longer part of consensus does it make sense to log it? Bitcoind doesn't log the boost version or the QT version.

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Feb 25, 2016
@maflcko
Copy link
Member

maflcko commented Feb 25, 2016

Agree with @gmaxwell. This way we could get rid of the hacky logic and also tidy up the GUI a bit, keeping in mind there are other ways to detect the library version, if this is ever desired.

@paveljanik
Copy link
Contributor

I think it could be useful in the debug log. But I agree it can be removed in the UI.

@laanwj
Copy link
Member

laanwj commented Mar 3, 2016

Closing (#7605 merged)

@laanwj laanwj closed this Mar 3, 2016
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants