Nokogiri: RFC: Nokogiri should use system libraries by default if they are of acceptable vintage

[flavorjones]

Proposed behavior change

As discussed in #936, I'd like Nokogiri to only use the bundled versions of libxml2/libxslt if the system versions:

1. either don't exist,
2. or they exist, but are of unsupported or known-worse vintage than the bundled libraries.

unsupported vintage would be a new minor release bump, which historically has indicated that libxml2 contains non-backwards-compatible functionality changes. For example, 2.10.0 would be an unsupported version to the Nokogiri that bundles version 2.9.2.

known-worse vintage would be anything older than the bundled libraries, which are likely to contain bugs or vulnerabilities. For example, 2.9.1 would be a known-worse version to the Nokogiri that bundles 2.9.2.

What's an "acceptable vintage"?

Said another way, I'd like Nokogiri to use the system library if its version if greater than the installed version while not being a minor version bump.

Specifically, the logic we should use for Nokogiri 1.6.5, which bundles libxml 2.9.2, would be::

system_library_version > 2.9.2 && system_library_version < 2.10.0

Show me the behavior, please

For example:

Feature: Installation of Nokogiri 1.6.5, which bundles libxml 2.9.2, should defer to newer versions of libxml2 if they are likely contain security patches and are also likely to be functional.
  In order to make my business more secure
  As a system administrator or a developer
  I want Nokogiri to use the most-secure version of libxml2 available
  that won't also break my application

  Scenario: There are no system libraries installed.
    Given libxml2 does not exist on the target system
    When we `gem install nokogiri`
    Then Nokogiri should compile and use its bundled version of libxml 2.9.2

  Scenario: There is a deprecated version of libxml2 installed
    Given libxml 2.8.0 is installed on the target system
    When we `gem install nokogiri`
    Then Nokogiri should compile and use its bundled version of libxml 2.9.2

  Scenario: There is an older version of libxml2 installed
    Given libxml 2.9.0 is installed on the target system
    When we `gem install nokogiri`
    Then Nokogiri should compile and use its bundled version of libxml 2.9.2

  Scenario: There is the same version of libxml2 installed, which doesn't contain our backported security fixes (if any)
    Given stock libxml 2.9.2 is installed on the target system
    When we `gem install nokogiri`
    Then Nokogiri should compile and use its bundled version of libxml 2.9.2

  Scenario: There is a newer version of libxml2 installed
    Given libxml 2.9.3 is installed on the target system
    When we `gem install nokogiri`
    Then Nokogiri should use the system-installed version of libxml 2.9.3

  Scenario: There is an untested version of libxml2 installed
    Given libxml 2.10.0 is installed on the target system
    When we `gem install nokogiri`
    Then Nokogiri should compile and use its bundled version of libxml 2.9.2

What about command-line options to the gem install process?

We should still support the --use-system-libraries option, which will always attempt to use the system libraries, even if they're of known-buggy or untested vintage. We should obviously then emit some warning message.

We should also introduce a --use-bundled-libraries option, which would support the current behavior, which is to always attempt to use the bundled libraries. This information should be available in the output of nokogiri -v.

The default behavior would be as described above.

Will I need to re-install Nokogiri if I upgrade system libraries?

I think it needs to be a requirement that, given Nokogiri decides to use bundled libraries at install time, if acceptable-vintage system libraries are upgraded (or installed), then Nokogiri should decide at runtime to use those system libraries.

Unfortunately, I don't know how easy that will be to do. Currently, Nokogiri statically links libxml2 and libxslt into the C extension, for some valid reasons. We'll need to switch back to loading shared libraries and tackle those other reasons.

I'll try to dig up a link to the email thread that discusses static-versus-shared linking.

[jrochkind]

This is very encouraging -- I think it would improve things greatly for nokogiri users in several ways, take care of a couple different pain points, if it can be pulled off technically. Not just security, but the pain of 4+ minute nokogiri installs, which has become a significant nokogiri pain for me and others.

I can't speak to the feasibility of pulling it off technically, but thanks for looking into it. It does seem like it could be challenging to determine exactly when the installed version is 'good enough', I thought last time I looked the nokogiri build actually had some custom patches and such.

If it does work, nokogiri on install could even warn the user when using it's own build, that they can avoid this by upgrading system libxml to X, consider the a proper system libxml to be the preferred solution where possible.

I am not sure that it needs to be an absolute requirement that a previously installed nokogiri using bundled libraries will automatically use appropriate system libraries once they become available. I think a reinstall of nokogiri is acceptable in that case. On the other hand, it probably does need to be a requirement that a nokogiri using a system install of libxml 2.9.2; when the system upgrades to 2.9.3 the installed nokogiri needs to begin using it (in case it was upgraded for security reasons for instance). And that might amount to the same problem.

Jonathan

[idyll]

Will I need to re-install Nokogiri if I upgrade system libraries? ... Yes...

This really feels like it's going to be dangerous. I can imagine situations where something triggers an upgrade of libxml and you take down apps because nokogiri isn't expecting it.

Image a situation where a person was working on a dozen little apps and then Apple does something that bumps system libxml to a version Nokogiri doesn't support. They would then need to gem update all of them. Even if they are using different versions of Nokogiri.

This behaviour will be hard to track down. Especially for someone just learning Ruby. Which is where one needs the current system of bundled libxml the most.

The burden of complicated installation should be taken on by the people who understand it. If a person wants to use system XML then they should be responsible for configuring that.

[flavorjones]

@jrochkind - Thanks for your comments. Agree that "good enough" may be hard to define for edge cases, but I think that the patches we backport are generally (at least by most *nix vendors or maintainers) backported anyway, and so the heuristic of "same minor release but newer" will likely work in most or all cases.

In fact, we may want to defer to the system library in case of a tie, presuming that the distro vendor/maintainer is doing a more-thorough job than Team Nokogiri (which, frankly, they ought to be).

With respect to the "Should Nokogiri require a re-install", your comments and @idyll's appear to be in agreement. However, I believe that most system admins and people-responsible-for-security would disagree and want an automatic upgrade.

In particular, libxml2 shouldn't require a recompilation to load against a newer version of a shared object library (especially if it's on the same minor-release branch). If it wasn't safe to upgrade dynamic libraries, your machine would stop working every time you updated libxml2, as this gist demonstrates:

https://gist.github.com/flavorjones/cc1b82365d1d29f6fb3f

Keep the comments coming!

[yob]

I use linux in development and production so tend to have system copies of libxml2 available and prefer to use them if possible. If nothing else, it makes bundling and installing nokogiri so much quicker.

This proposal sounds reasonable to me.

[jrochkind]

In particular, libxml2 shouldn't require a recompilation to load against a newer version of a shared object library

Yes, I think you are quite right there.

[hone]

On heroku, when this got changed we started using the NOKOGIRI_USE_SYSTEM_LIBRARIES env var to force using the system libraries. We currently rely on the distro (ubuntu) in this case to provide libxml2 and libxml2-dev packages. I'd like to avoid forcing every app to compile libxml2 on the platform when it's readily available. So, I have some questions with this change:

1. Will NOKOGIRI_USE_SYSTEM_LIBRARIES remain around as an env var we can hook into?
2. If not, do we need to use the flag --use-system-libraries? I assume for using this with bundler we'd need to use RUBYGEMS_OPT or something similar. Do other gems honour this? If so, it seems like we might want to just restrict this to nokogiri.
3. If the user ends up with the system libxml2, they will get a warning message and there's nothing they can really do about it. This would be annoying per build. Is there a way we can phrase it differently that this is expected?

Thanks for raising this issue and all your hard work on nokogiri.
<3 <3 <3

[flavorjones]

Hey @hone!

Thanks for jumping in! As someone who works with the Cloud Foundry buildpacks team, I FEEL YOUR PAIN.

Yes, the env var will continue to be the implementation that's used by the commandline options. If it ever changes, I'm extremely motivated to make sure that Heroku and Cloud Foundry know about it well ahead of time.

As far as the wording of the warning message, we'll of course omit the ominous language if they're on an acceptable vintage. But I think we'll continue to want to make it obvious whether people are using system- or bundled-libraries, both at runtime and at installtime. I'm open to suggestions!

[hone]

@flavorjones thanks for the response. Yeah, message clarity makes sense. What kind of message copy are you thinking of? I'm worried about people filing support tickets based off of the warnings and the fact that they can't really do much about it and it's expected behaviour on our part.

[veillard]

Libxml2/libxslt maintainer here :-) The problem of bundling is then there is no way to get fixes deployed on security issues. Most of the significant and hence risky changes done in the last 5 years of so for libxml2, were done to fix security issues, about entities processing. Avoiding DoS attacks, last one no later than 4 months ago. It did cause regressions, sometime hard to work around, but the balance is apps vulnerability. You prefer an app which may fail in some corner case or an app which stay vulnerable to attacks ? Unless the system libraries are really behind, it makes no sense at this point to not use the system libraries unles your framework provides to carrying updates automatically, which I assume from context is not the case.
There is no big changes planned for either libs, it's really maintainance and security at this point.
So definitely supportive of the change,

Daniel

[flavorjones]

@veillard, Thanks for much for your comments, I really appreciate that you took the time to weigh in!

For historical context, the contributing factors that led us to bundle libxml2 were:

• known-bad, really old versions of libxml2 in the wild (who could forget 2.6.16?)
• new versions that we didn't support yet at the time (2.7 and 2.8 took time before Nokogiri supported them, and 2.9 had xpath curiosities before 2.9.2)

The second case should be reasonably rare, and so let's not optimize for it (though we reacted to it at the time).

To that end, it makes sense for Nokogiri to bundle a modern version of libxml2 for people running on older systems (I'm looking at you, RHEL6), while also maintaining the ability to roll forward to newer versions easily.

I'm so glad you're supportive of the proposal!

Cheers,
-mike

[knu]

This proposal makes much sense to me!

[rklemme]

I suggest considering a small change in the version test example:

system_library_version >= 2.9.2 && system_library_version < 2.10.0

In other words: if the system library version is the _same_ as the bundled version use the system version. This would have the advantage to not load two different instances of the the same version of the shared lib in case other software is using the system library.

There may be a risk that the system version is different despite the same version number. But if we go down that route we can never use a system version.

[b1nary]

So this is the magic why i never ran into any problems with nokogiri, and it just gets better. This sounds good, looking forward to worry even less.

[jirutka]

I completely agree with @veillard about security concerns. This is also the reason why package maintainers in Gentoo generally try to avoid bundled dependencies.

I’m happy for this proposal! :smile_cat:

[flavorjones]

@rklemme -- Agree. As our thinking has evolved on this topic, I agree with you (as I stated above at https://github.com/sparklemotion/nokogiri/issues/1220#issuecomment-69809954).

@jirutka -- Agree with you both. Thanks for the feedback!

For everyone following: this will not be in 1.6.6 (the next point release), but I'm hoping for it to be part of the 1.7.0 release soon thereafter.

[ab]

:+1:

[fulldecent]

Hello all. Since the posting of this issue. GitHub has introduced reactions. If you are in favor of or opposed to the solution and course of action from OP, you are welcome to click the button.

A little encouragement here will go a long way.

[fulldecent]

I see two open problems in this discussion:

1. What happens if we depend on system libxml and Apple updates it?

Programs that depend on nokogiri (and which did not use --use-system-libraries) may break. Also, every other program on the computer that depends on libxml may break. Lot's of stuff will break. Then users will use Google, find this repo and complain on GitHub Issues. Then we will update the error message to be more helpful and those users will update their apps.

Last year Apple introduced "System Integrity Protection". It broke all kinds of shit (like basically every Ruby program). I recommend that we do not hypothesize about the ways that things might get broken in the future, since for sure they will get broken somehow.

PROPOSED BEHAVIOR: After install, naively use system libxml. If something doesn't work then crash with an error message.

2. We don't have a pull request

There is clearly value to this proposal. Just reducing the carbon footprint of Travis CI running thousands of builds per second from developers that forgot to use NOKOGIRI_USE_SYSTEM_LIBRARIES will be enough to add a few seconds to your life and everyone you know.

It is a lot easier to accept a PR than an issue.

[tenderlove]

PROPOSED BEHAVIOR: After install, naively use system libxml. If something doesn't work then crash with an error message.

The upside to this is people can upgrade libxml2 without upgrading Nokogiri. The problem is that if something doesn't work in the new version of libxml2, it crashes with a SEGV. We can't know what future versions will do, so we can't give the user any more useful info than a SEGV. In fact, this issue was so common we introduced the "Nokogiri was built against LibXML version XXX but loaded XXX" warning that people have come to love so much.

We've also had an opposite problem, where Apple didn't upgrade libxml2 for ages and we got stuck supporting that version because we wanted to work with system libraries.

I think Mike's proposal makes sense, and I think we should work with system libraries if they fit the version criteria he's specified. IMO, we should use system libraries and let the user upgrade system libraries, but we shouldn't go out of our way (at runtime) to deal with the situation where upgrading the system libraries breaks something. If the new system library breaks something, they should report the issue and we can deal with. Whether dealing with it means fixing our code, or declaring that version to be unsupported.

Hopefully that makes sense?

[flavorjones]

I'd like to propose closing this with no action being taken.

Given:

• Nokogiri v1.11 ships precompiled binaries for most platforms,
• non-native "ruby" platform gem downloads constitute ~3.5% of Nokogiri 1.11 downloads

... this work feels less urgent and less impactful, and so I'd like to propose closing this with no further action.

I'll close in a few days, to give folks an opportunity to argue that this work would be more impactful than some of the other work queued up for v1.12 and subsequent releases, including HTML5 support, performance work, and better handling of exceptions during operations involving callbacks.