Bug 1299388

Summary: freeradius: libssl version mismatch.
Product: [Fedora] Fedora Reporter: Nikos Mavrogiannopoulos <nmavrogi>
Component: freeradiusAssignee: Nikolai Kondrashov <nikolai.kondrashov>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: rawhideCC: aland, lemenkov, nikolai.kondrashov, nmavrogi, tmraz
Target Milestone: ---Keywords: Reopened
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: freeradius-3.0.13-2.fc26 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2017-04-01 17:07:57 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Nikos Mavrogiannopoulos 2016-01-18 09:21:23 UTC
Description of problem:
Starting freeradius fails with "libssl version mismatch. built: 1000205f linked: 1000204f"

This is a recurring issue which comes up on every update of freeradius or fedora. As this is a _very_ critical package for several setups could you please disable that check on the fedora builds? You are making fedora unusable for several server setups.

Comment 1 Nikolai Kondrashov 2016-03-08 15:20:23 UTC
Hi Nikos, thanks for reporting this. Sorry for the delay only just got round to FreeRADIUS.

I don't see this issue on F23 at the moment and am not quite sure how it came to be as I didn't submit any new packages lately. Am I missing something?

Regardless, please help me understand the issue. I'm not very much into OpenSSL version numbering intricacies, sorry.

However, I can tell that FreeRADIUS fails those version checks when any of major, minor, or fix versions mismatch, or when patch level of the linked version is *greater* than the version FreeRADIUS was built with.

I'd like to achieve balance between maintainability of Fedora and protecting FreeRADIUS against breakage from subtle OpenSSL ABI changes, as that's the upstream intention, and I would like to support them in that, because they're doing the major part of support for us.

Comment 2 Nikos Mavrogiannopoulos 2016-03-08 15:41:07 UTC
(In reply to Nikolai Kondrashov from comment #1)
> Hi Nikos, thanks for reporting this. Sorry for the delay only just got round
> to FreeRADIUS.
> 
> I don't see this issue on F23 at the moment and am not quite sure how it
> came to be as I didn't submit any new packages lately. Am I missing
> something?

Yes. My understanding is that the problem occurs if one updates freeradius and not openssl or vice versa. Most likely if you downgrade your openssl in your system you'll trigger that issue.

Comment 3 Nikolai Kondrashov 2016-03-08 19:45:55 UTC
I would assume that upgrading either of them is a good idea most of the time.

In which cases you wouldn't want to do that and how often do they happen?

If they don't happen often, can people just rebuild FreeRADIUS themselves from SRPM in these cases?

Comment 4 Nikos Mavrogiannopoulos 2016-03-09 09:39:03 UTC
(In reply to Nikolai Kondrashov from comment #3)
> I would assume that upgrading either of them is a good idea most of the time. 
> In which cases you wouldn't want to do that and how often do they happen?

That's not the right question. If the packages have interdependency (i.e., you require a fixed version of openssl to work) you should encode that in the spec file.

> If they don't happen often, can people just rebuild FreeRADIUS themselves
> from SRPM in these cases?

This is not how we provide packages in fedora.

Comment 5 Nikolai Kondrashov 2016-03-21 17:07:10 UTC
Alright, I assume you mean the RPM packages should express the requirements,
but it should still be possible to install and run the incompatible (by
upstream reconing) packages with "--nodeps". Please correct me if I
misunderstood.

However, supposing I could somehow substitute the OpenSSL version FreeRADIUS
is built with into the .spec file (any hints?), I'm not quite sure how to
express the requirements upstream FreeRADIUS imposes. AFAIK, RPM doesn't
support version ranges, or logic operators within "Requires:", whereas I will
need to express this:

    openssl >= <major>.<minor>.<fix> && openssl <= <major>.<minor>.<fix><patch>

e.g.:

    openssl >= 1.0.1 && openssl <= 1.0.1k

Can I perhaps use "Requires:" for the minimum requirement and "Conflicts:" for
the minimum, or vice versa? E.g.:

    Requires: openssl >= 1.0.1
    Conflicts: openssl > 1.0.1k

Or perhaps you can suggest something else?

Thank you.

Comment 6 Nikos Mavrogiannopoulos 2016-03-31 07:20:43 UTC
Why not simply remove the version check for openssl in freeradius source code? The libraries we ship on each fedora release are compatible with each other and such checks are only causing harm.

The culprit seems to be ssl_check_consistency() in version.c. The whole function is questionable in the presence of shared libraries. Adding Tomas, the maintainer of openssl; Tomas do such checks have any merit in our Fedora releases?

http://doc.freeradius.org/main_2version_8c_source.html

Comment 7 Tomas Mraz 2016-03-31 07:57:25 UTC
As we do not support forward compatibility, the check that the openssl version (including patch level) which the freeradius was built with is not newer than the version that is present on the system has its merit. The major.minor should be also equal but that is handled by SONAME versioning as well, so that is not critically needed.

The requires from comment 5 should be actually the other way around and it is sufficient to express just the one requiring the greater or equal patch level.

I.E. if freeradius was built with openssl-1.0.1k the requires should be:
Requires: openssl >= 1:1.0.1k

(Note the Epoch == 1)

It should be possible to query the openssl NVR programatically during build and generate the version automatically.

However also note that the not-being-forward-compatible condition applies to practically all libraries in Fedora and we do not bother to handle it usually neither within SPECs nor within runtimes. It is basically unsupported practice to update only parts of the system and not all the packages. We would have to be much much more strict on the buildroots and buildroot overrides to make the partial updates supportable.

Comment 8 Nikolai Kondrashov 2016-04-11 15:27:18 UTC
Nikos, I was trying to suggest we remove the runtime version check and add the requirements to the .spec file instead. My concern was how to express it there.

However, how specifically do you ensure the libraries we ship in Fedora are compatible with each other, provided the subtle ABI differences OpenSSL introduces sometimes?

Tomas,

The upstream requirements are that the linked version should *NOT* be newer than the linked version. It can have lower patch level, but otherwise should be the same. I'd like to express that in the .spec file.

Comment 9 Nikolai Kondrashov 2016-04-11 15:28:20 UTC
The upstream does this to prevent unnecessary questions on the maillist. I will still have to negotiate the solution with them. I.e. if the .spec requirements will be sufficient.

Comment 10 Tomas Mraz 2016-04-11 15:31:48 UTC
(In reply to Nikolai Kondrashov from comment #8)
> The upstream requirements are that the linked version should *NOT* be newer
> than the linked version.

I can't parse this sentence, please clarify.

Comment 11 Nikolai Kondrashov 2016-04-11 15:50:06 UTC
(In reply to Tomas Mraz from comment #10)
> (In reply to Nikolai Kondrashov from comment #8)
> > The upstream requirements are that the linked version should *NOT* be newer
> > than the linked version.
> 
> I can't parse this sentence, please clarify.

A-ha-ha, sorry :)

The upstream requirements are that the run time linked version should *NOT* be newer than the build time linked version.

Or, in other words, the OpenSSL version FreeRADIUS runs with should *NOT* be newer than the OpenSSL version it was built with.

Here's the code in question:

    if ((ssl_built & 0xfffff000) != (ssl_linked & 0xfffff000) ||
        (ssl_built & 0x00000ff0) > (ssl_linked & 0x00000ff0)) goto mismatch;

Comment 12 Tomas Mraz 2016-04-11 16:13:51 UTC
Hmm, that is bogus check but only slightly. With the recent rules on what fixes are applicable to which branches in OpenSSL the check should be:

if ((ssl_built & 0xfff00000) != (ssl_linked & 0xfff00000) ||
    (ssl_built & 0x000fffff) > (ssl_linked & 0x000fffff)) goto mismatch;

- note that this applies only to versions 1.0.0 and newer. The old check was more applicable to 0.9.x versions.

For Fedora RPMs the first condition is solved by the automatic SONAME dependency generation and the second condition is exactly what I propose in comment 7 ->

I.E. if freeradius was built with openssl-1.0.1k the requires should be:
Requires: openssl >= 1:1.0.1k

Version it was built with has to be older or same as the version it runs with.

Comment 13 Nikolai Kondrashov 2016-04-11 18:02:56 UTC
It handles the pre-1.0.0 versions as well. See the full function at https://github.com/FreeRADIUS/freeradius-server/blob/release_3_0_11/src/main/version.c#L51

Aargh, I think I was misinterpreting the code all this time and confusing you all, sorry. Thanks for your patient corrections :)

So, yes, at the moment upstream requires that major, minor, fix and status values match, but patch value can be equal or greater in the runtime-linked version. That's with 1.0.0 versions and greater, which we're concerned with.

Now, if my new understanding is correct then we need those (still two) conditions:

    openssl >= <major>.<minor>.<fix><patch> && openssl <= <major>.<minor>.<fix + 1>

Which can possibly be expressed as such, for the sake of example:

    Requires: openssl >= 1:1.0.1k
    Conflicts: openssl >= 1:1.0.2

Am I correctly interpreting it now?

Now, granted major and minor equality is handled by SONAME dependency generation, why allowing fix and status values be greater at run time is safe, and if it is, how can we convince upstream?

Thank you.

Comment 14 Nikolai Kondrashov 2016-04-11 18:59:45 UTC
The condition is slightly incorrect, I meant this, of course:

    openssl >= <major>.<minor>.<fix><patch> && openssl < <major>.<minor>.<fix + 1>

(Note strictly less than in the second branch)

Comment 15 Tomas Mraz 2016-04-12 08:29:32 UTC
Except the second condition is simply too aggressive. Starting from the 1.0.0 version these are ABI backwards compatible between the <fix> versions. So the conflicts is sufficient to be:
Conflicts: openssl >= 1:<major>.<minor + 1>.0

But that conflicts is actually unnecessary as this dependency is equivalently well satisfied via the SONAME automatic dependency if the freeradius is properly linked against openssl and not using dlopen() instead.

Comment 16 Fedora End Of Life 2016-11-24 15:01:23 UTC
This message is a reminder that Fedora 23 is nearing its end of life.
Approximately 4 (four) weeks from now Fedora will stop maintaining
and issuing updates for Fedora 23. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as EOL if it remains open with a Fedora  'version'
of '23'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora 23 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora, you are encouraged  change the 'version' to a later Fedora 
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events. Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

Comment 17 Fedora End Of Life 2016-12-20 17:58:32 UTC
Fedora 23 changed to end-of-life (EOL) status on 2016-12-20. Fedora 23 is
no longer maintained, which means that it will not receive any further
security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of
Fedora please feel free to reopen this bug against that version. If you
are unable to reopen this bug, please file a new report against the
current release. If you experience problems, please add a comment to this
bug.

Thank you for reporting this bug and we are sorry it could not be fixed.

Comment 18 Nikolai Kondrashov 2017-03-13 07:06:07 UTC
This is still a problem, reopening.

Comment 19 Tomas Mraz 2017-03-13 07:56:46 UTC
The version check is bogus and should be simply patched out. There is no point in such check in RHEL/Fedora because we track ABI changes and change the soname accordingly.

Comment 20 Nikolai Kondrashov 2017-03-13 08:02:30 UTC
Hi Tomas, thank you for your patience explaining OpenSSL versioning before.

I would really like to get rid of that runtime check in FreeRADIUS and not even have one in the .spec file. However, I must be sure I understand everything here, and am able to negotiate with upstream.

So, you're saying that we don't need this condition:

    openssl < <major>.<minor>.<fix + 1>

because openssl promises backwards compatibility between "fix" numbers (applications built with lower numbers can run with higher numbers), and the rest is handled by the dynamic linker. I failed to find any official information on the former. Could you please provide a source or some proof that is the case?

You are also saying that this condition:

    openssl >= <major>.<minor>.<fix><patch>

is practically equivalent to:

    openssl >= 1:<major>.<minor + 1>.0

Is this really true? I.e. can applications built with higher "fix" and "patch" numbers run with openssl having lower "fix" and "patch" numbers?

Please correct me if I understood you wrong.

Thank you.

Comment 21 Nikos Mavrogiannopoulos 2017-03-13 09:05:31 UTC
(In reply to Nikolai Kondrashov from comment #20)
> Hi Tomas, thank you for your patience explaining OpenSSL versioning before.
> 
> I would really like to get rid of that runtime check in FreeRADIUS and not
> even have one in the .spec file. However, I must be sure I understand
> everything here, and am able to negotiate with upstream.
> 
> So, you're saying that we don't need this condition:
> 
>     openssl < <major>.<minor>.<fix + 1>
> because openssl promises backwards compatibility between "fix" numbers

Nikolai I think Tomas is going further than that. What you describe above (the freeradius test) is a test for openssl _upstream_ version numbers. What Tomas is saying, is that the downstream openssl we ship _always_ provides ABI compatibility, irrespective of any upstream version numbers. If your application is linked with whichever openssl library, and it that library gets upgraded we guarrantee compatibility, no matter what version numbers are there. That is not really fedora/rhel specific, the majority of distributions have that promise, and the freeradius check for openssl versions is quite bogus and only applicable for people compiling freeradius outside a distribution.

If we ever ship a new and incompatible openssl library (e.g., as we do in F26 with 1.1.0), that is be done with another shared object version, so it wouldn't affect applications compiled with the previous library.

Comment 22 Nikolai Kondrashov 2017-03-13 09:23:13 UTC
Thank you, Nikos! Yes, that makes sense to me as a guarantee a distribution would provide. I'll be talking to upstream next and hopefully will remove the check.

BTW, out of curiosity, what do you do if upstream introduces an API incompatibility, and keeps the major/minor numbers. Which values would you use then?

Comment 23 Tomas Mraz 2017-03-13 10:58:45 UTC
That's now a completely theoretical question as upstream is now very precise about not breaking API/ABI rules starting with the 1.1.0 and 1.0.2 versions. That means the 1.1.* (and 1.0.2*) versions are promised to be always API/ABI backwards compatible and future ABI compatibility break will mean the release with the break will also bump the <major> or <minor> part of the version. This will also automatically imply change of the SONAME of the library as the SONAME of the 1.1.* is lib(crypto|ssl).so.1.1.

BTW: Saying that this condition:

Conflicts:    openssl >= <major>.<minor>.<fix><patch>

is practically equivalent to:

Conflicts:    openssl >= 1:<major>.<minor + 1>.0

is incorrect. What I am saying is that the first condition is bogus, because there will never be conflict with newer openssl that bumps only fix or patch part. And the second condition is unneeded because that is already tracked by the automatical dependency based on the libcrypto/libssl SONAME.

Also note that the openssl library API/ABI is depending on the build time options so the version check is bogus even from this point of view - it would not track the removal of some API calls that are optional based on build time options anyway. You still depend on the distribution to update the library sanely and not disable random build time options of openssl that would break the ABI in comparison to the previous versions without also modifying the SONAME.

Comment 24 Nikolai Kondrashov 2017-03-13 16:18:39 UTC
Thank you for the explanation, once again, Tomas. I have posted a pull request
upstream, introducing a configure option which disables the check, which was
promptly closed:

    https://github.com/FreeRADIUS/freeradius-server/pull/1939

Can I ask you to perhaps visit the link above and convey your point of view?
It would greatly help us find a solution, as I'm not so experienced or
knowledgable in the whole matter.

Thank you.

Comment 25 Tomas Mraz 2017-03-13 17:54:10 UTC
I am not going to spend long time arguing with the FreeRadius upstream. I am not saying that there were never subtle problems of that kind in the past however the current situation is very much different, the upstream is much more careful and caring about API/ABI breakage, and the version check when applied to the current situation is bogus. That's all I can say.

Comment 26 Nikolai Kondrashov 2017-03-13 18:02:10 UTC
I understand. Thank you, Tomas.

Comment 27 Nikolai Kondrashov 2017-03-14 13:58:13 UTC
Unfortunately, upstream still doesn't trust OpenSSL enough to remove the checks and we don't want to remove them in our packaging only, as that might affect upstream support load.

We'll have to close this until OpenSSL is deemed reliable by upstream.

Comment 28 Tomas Mraz 2017-03-14 14:25:05 UTC
If they at least dropped the (ssl_built & 0x00000ff0) > (ssl_linked & 0x00000ff0) part from the condition it would actually solve the original problem from this bug report.

Or you have to autogenerate 
Requires: openssl-libs >= <version that is installed on the build system>

You should be able to do that with some script.

This way the openssl is always updated with freeradius when you do dnf update freeradius. Fortunately we do not put testing updates into build roots by default.

Comment 29 Nikos Mavrogiannopoulos 2017-03-15 06:24:30 UTC
While it is understandable that upstream has that opinion, their product is freeradius, and everything around it is just dependencies they can consider distractions, and freeze their version. However, our product is Fedora (and RHEL), and thus all packages must be able to operate in that dynamic environment, i.e., with an openssl library that _will be_ upgraded. A WONTFIX means shipping a problematic package, and thus postponing the problem. Tomas' solution above is quite to the point.

Comment 30 Nikolai Kondrashov 2017-03-15 09:48:55 UTC
I understand your desire to fix this, and your reasons, Nikos, Tomas.

I will try to write a patch for the .spec file adding the Requires Tomas mentions. I assume that if we add that we won't need to change the upstream code.

If that doesn't work I'll try to ask upstream to remove the check for older patch level Tomas mentions, and I'd greatly appreciate it if you could help me explain the reasoning to upstream, and provide some evidence of OpenSSL commitment to compatibility between those.

Comment 31 Nikolai Kondrashov 2017-03-15 10:42:35 UTC
Tomas, as far as I could figure out, the package version doesn't include the patch version, which that condition you mention checks. Does it mean that we have to check for package release as well, in Requires?

I.e. do we have to do this:

    Requires: openssl >= %(rpm -q --queryformat '%%{VERSION}-%%{RELEASE}' openssl)

Or just this:

    Requires: openssl >= %(rpm -q --queryformat '%%{VERSION}' openssl)

Comment 32 Nikolai Kondrashov 2017-03-15 10:50:38 UTC
I don't quite understand how that would help us with the changing fix number, though, because FreeRADIUS code goes:

    if ((ssl_built & 0xfffff000) != (ssl_linked & 0xfffff000) ||
        (ssl_built & 0x00000ff0) > (ssl_linked & 0x00000ff0)) goto mismatch;

I.e. if the fix increases in an update, FreeRADIUS stops working.

Comment 33 Tomas Mraz 2017-03-15 11:01:00 UTC
That's incorrect. The patch part of version is the letter after the 1.0.2 version number. So it is part of the version.

On the other hand I believe you have to include the Epoch otherwise the condition would be always fulfilled even with an older patch release. You can test that.

So either use 
Requires: openssl >= %(rpm -q --queryformat '%%{EPOCH}:%%{VERSION}-%%{RELEASE}' openssl)

or
Requires: openssl >= %(rpm -q --queryformat '%%{EPOCH}:%%{VERSION}' openssl)

Comment 34 Tomas Mraz 2017-03-15 11:05:07 UTC
(In reply to Nikolai Kondrashov from comment #32)
> I don't quite understand how that would help us with the changing fix
> number, though, because FreeRADIUS code goes:
> 
>     if ((ssl_built & 0xfffff000) != (ssl_linked & 0xfffff000) ||
>         (ssl_built & 0x00000ff0) > (ssl_linked & 0x00000ff0)) goto mismatch;
> 
> I.e. if the fix increases in an update, FreeRADIUS stops working.

This is not so critical for us as we include a special versioning hack in our OpenSSL build that actually masks the current version in that case. We included it a long ago to actually prevent these misguided version checks from working. The original reported issue from this bug is coming from the second condition from the if statement above and it should be solved by the autogenerated Requires.

Comment 35 Nikolai Kondrashov 2017-03-15 11:15:17 UTC
Aargh, I got lost in the mapping between the binary constant and the package versions. Right, epoch needs to be there too, but I'll drop the release.

The versioning hack situation is ironic, but I'm relieved it could help us.

Alright, I'll get this into the package:

    Requires: openssl >= %(rpm -q --queryformat '%%{EPOCH}:%%{VERSION}' openssl)

and we'll see how it works.

Thanks!

Comment 36 Fedora Update System 2017-03-15 15:21:19 UTC
freeradius-3.0.13-2.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-97cc5cb6d6

Comment 37 Fedora Update System 2017-03-16 00:50:57 UTC
freeradius-3.0.13-2.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-97cc5cb6d6

Comment 38 aland 2017-03-16 13:59:33 UTC
See commit b7b5493c61aeb4e

OpenSSL 1.0.2 changed the way they called our callback, and violated their public documentation.  The ABI was the same (i.e. data structures and function parameters), but the *values* passed were different.

Earlier versions of OpenSSL would change data structure contents and layouts in minor minor releases, and sometimes in letter releases.  The checks we added to FreeRADIUS actively prevented crashes due to bad library versions.

Now, RedHat may manually check OpenSSL and ensure that these things don't happen.  But that's only on *your* platform.  There are (still) people using non-RedHat systems who *we* need to support.

Calling these checks "misguided" or "bogus" is entirely inappropriate and anti-social.  If the entire world is RedHat, maybe those statements would be true.  But it isn't.  So we have to support our customers, and ensure that we get *meaningful* error messages instead of opaque crashes.

If you want to remove these checks for RedHat, you can do so via a local patch.  We cannot add such patches to the main release, because the world isn't RedHat.

OpenSSL 1.1.0 has (mostly) fixed the problem, by removing access to the underlying data structures, and adding accessor functions.

I'd be OK with updating the code so that for OpenSSL greater than 1.1.0, FreeRADIUS just prints a warning, but still starts.  If something goes wrong, we would still have that notice in the debug output to help.

Comment 39 aland 2017-03-16 14:01:54 UTC
> That is not really fedora/rhel specific, the majority of distributions have that promise, 

That just hasn't been my experience.

> and the freeradius check for openssl versions is quite bogus and only applicable for people compiling freeradius outside a distribution.

That also hasn't been my experience.

This kind of negative comment makes me inclined to think that RedHat people live in their own little bubble, and simply don't give a shit about anyone else.

Comment 40 Fedora Update System 2017-04-01 17:07:57 UTC
freeradius-3.0.13-2.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.