Remove mcrypt dependency

[skymeyer]

• Mcrypt removal based on Remove mcrypt as a dependency #101 without any other fixes
• This PR is PHP 5.6 compatible and up, fixes Remove dependency on mcrypt #55

Changes which are not included from the previous PR:

• HHVM seg fault during tests HHVM seg fault #87
• mb_str issue as per xmlseclibs fails when using mb_str functions #2
• Remove md5 usage for generateGUID::generateGUID
• Removal of deprecated methods XMLSecurityDSig::generate_GUID and XMLSecurityKey::getAlgorith
• Unit test frame work restructure
• Removal of xmlseclibs.php relying on PSR-4 autoloading

[skymeyer]

@robrichards tests are passing except for HHVM (see #87) which was addressed in my earlier PR. Let me know when this can be reviewed and merged in favor of the code you added in the remove_mcrypt branch.

[pitbulk]

@skymeyer I used that PR on php-saml and my tests past on php5.6 and php7.0 with mcrypt not installed on the system, also on Travis CI.

[skymeyer]

@pitbulk Updated the PR, forgot to remove encryptOpenSSL and decryptOpenSSL which are no longer used.

[pitbulk]

@skymeyer Thanks, I updated it ;)

[zsprackett]

@robrichards any next steps required in order to get this one merged?

[rajesh2011]

We need this urgently. Please get it in asap.

[csezheng]

👍

[robrichards]

I will take a look this week. I've been out of pocket for past month so I've had very limited time to look at things. Got back the other day so will make sure this tops my priority list to review the changes.

…

On Apr 17, 2017, at 2:32 PM, rajesh2011 ***@***.***> wrote: We need this urgently. Please get it in asap. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[sergiofcasado]

If it helps, I had integrated @skymeyer version for the past 5 months and it is still working well with SimpleSAMLPHP as SP with external IdP.

In this sense we are very interested in having it officially integrated into the project, with hopes that next version of simplesaml will have it integrated as well.

[robrichards]

@skymeyer I went ahead and merged this (manually as I questioned a change until I realized it was fixing a regression in my branch). Can you take a look and make sure it tests out ok on your end. @jaimeperez would appreciate it if you could do the same. Will be unavailable for the next 10 days just a heads up on timing of getting it into master

[skymeyer]

@robrichards I'm a bit confused on the process here. Why are you creating a separate branch manually taking the changes manually from this PR instead of reviewing this PR by itself ? This duplicates the work for testing, code verification and we cannot comment on code in your branch. Why can't we just work from this PR directly ? The code in both is now exactly the same.

By merging your branch instead of this PR also implies you are throwing away the credits for the contributed code which does not make sense for open source development.

[robrichards]

Originally the changes to the PR was so significant that I found it easier to start off a clean branch. As for these last changes in the end you are right and it was really just me rushing to fit this in. As I mentioned I won't get to merge it over next 10 days so feel free to create a clean branch off master with all the changes if you want to insure proper credits for all changes as all I did was remove the non mcrypt related items and not worried about that. Will try to avoid this issue in future

Rob

[Hikariii]

Travis timed out...
@robrichards Any idea when you're going to merge this?

[robrichards]

Change has been merged into master. Am looking at releasing a 3.0 version with it

[skymeyer]

@robrichards this PR is a clean branch against master - I really do not understand your way of working here. I insist you use this PR as that's the one which has been reviewed and tested and has the proper credit as mentioned above.

This topic has been going on for way too long with 3 different PR's: the original one, then you made me do one for PHP 7 only for which you changed you mind, and then I had to make this one to correct your mistakes of trying to manually digest some parts of my second PR.

[Hikariii]

@robrichards, thanks for the work, although I agree with @skymeyer about the ownership of these changes.

[ghost]

@robrichards - I have a favour to ask you.

To explain: PHP 7.1 is throwing many many "Function mcrypt... is deprecated" errors at us and our preference is not to hide such errors but to update our application as necessary to remove them. The challenge we have currently is that we are using PHP-SAML which in turn uses xmlseclibs, and as I understand from SAML-Toolkits/php-saml#79 PHP-SAML is keen to remove dependency on mcrypt but is unable to until xmlseclibs provides an alternative.

So, on to my favour: This is not my area of expertise and reading all the comments I'm really struggling to understand:

1. if xmlseclibs is going to offer PHP-SAML the alternative it requires; and
2. if so, when this is expected to happen

And I would greatly appreciate it if you would be willing to provide some insight into these two items.

thanks so much in advance

ricky

[robrichards]

@skymeyer Again apologies for how this ended up going. With the amount of changes being made (there were significant amounts of changes unrelated to crypt removal in many of the initial patches - yours were not the only ones submitted to me), versions of this change from various folk, and multiple branches of branches, it just ended up easier for me to do what I did to get this out the door ASAP with all the pressure from everyone waiting on this change. No ill will intended here.

[jaimeperez]

Hi all!

@skymeyer, is there any particular reason why this PR dropped support for PHP < 5.6? I was quite eager myself to see this merged, as plenty of other people, but I suspect throwing support for 3 versions of PHP out the window will be a significant (and unexpected) side effect for everybody.

I've reviewed the code of the PR and couldn't see anything in particular that justifies such a big bump in the requirements. Is there anything I am missing?

@robrichards, if there was no obvious reason to bump the PHP requirements, would it be possible to have a 3.0.1 release with support for PHP versions 5.3, 5.4 and 5.5 restored in the composer.json file?

[thijskh]

I've created PR #135 to demonstrate that the testsuite of xmlseclibs passes just fine with PHP 5.4 and 5.5; so (given that it sufficiently covers the code which I think is the case) the library will work fine with these versions.

[jaimeperez]

Wonderful, thanks Thijs!

I'd definitely restore support for PHP 5.4 and 5.5 in travis, and relax the PHP version requirement in composer to 5.4.

[skymeyer]

I know the code passes on 5.4 and 5.5 perfectly. You need to discuss with @robrichards - this one has a lot of back and forward ... On the other hand - without starting yet another version flame war - just move away from 5.x. There is no incentive for hosters and packagers by insisting on supporting libraries for non-supported php versions.

[jaimeperez]

I'm afraid that's not very realistic, @skymeyer.

Even if one could argue that using PHP 5.3 nowadays could even be dangerous, the fact is that 5.x is definitely not unsupported. 5.6 will be getting security fixes until 2019. Even older versions such as 5.3 are still supported by Red Hat and it gets security fixes. RHEL 7.4 has just been released shipping PHP 5.4, meaning it will continue to get security fixes for many years ahead.

There are lots of people using versions of PHP older than 5.6. As much as we would like them to use something up to date, we can't force them, it's not up to us. In many cases, that doesn't even mean they are running insecure versions, or that they can even upgrade to something else.

Considering this, I think it was a mistake to bump the PHP requirement to 5.6 where the only real requirement was 5.4. Doing so might actually prevent people from upgrading, and as such they will continue to depend on mcrypt.

[skymeyer]

Again, this was not my call. The original PR had no such restrictions. You'll need to talk to the repo owner for this who originally only wanted to fix this for PHP 7 only.

[robrichards]

@jaimeperez I am really on the fence on this one even though it technically runs on 5.4 right now. I have a few issues with still supporting 5.4 and 5.5. If users are still using these older versions - while security fixes may be back ported there is no guarantee that other future needed bug fixes will be back ported. Also if they are using such older versions while newer ones are available them and even supported by their OS vendors, who's to say they would even consider altering their PHP configuration to drop the mcrypt support. Lastly starting with 5.6.13 we know the security issues have been addressed so why not enforce that rather than leaving it up to a vendor to determine wether the fixes are security related in their view and/or insuring those not using non-supported versions have some incentive to upgrade.

[jaimeperez]

Hi @robrichards!

I have a few issues with still supporting 5.4 and 5.5.

What are those? It's not like I wouldn't like myself to drop support for anything older than 7.0, it's just that it's not realistic.

If users are still using these older versions - while security fixes may be back ported there is no guarantee that other future needed bug fixes will be back ported

Red Hat (which is the vendor shipping horribly old versions of PHP to their paid customers) guarantees that all security fixes are backported to the versions of PHP shipping with their Red Hat Enterprise Linux OS, which in turn has an incredibly long support period. How many bug fixes have you noticed so far that you absolutely needed in place and weren't backported? I think the fact that this library was working just fine with PHP 5.3 is a good indication that there aren't so many cases. Besides, if that case ever happens, we can worry about it and bump the minimum requirements then. What you are suggesting is basically putting a bandage before we get a wound.

Also if they are using such older versions while newer ones are available them and even supported by their OS vendors, who's to say they would even consider altering their PHP configuration to drop the mcrypt support.

There are a couple of wrong assumptions here:

• There are no newer versions of PHP available for them. There is for users of older RHEL versions, as they can upgrade to RHEL 7.4 and get PHP 5.4. But that's all.
• The same way they can't easily upgrade PHP, they can't install mcrypt. That's not altering their PHP configuration, that's finding a way to install packages that are unavailable. This is why this is so important. Removing the mcrypt dependency makes it possible for those users to install xmlseclibs without depending on external repos maintained by third parties.

Lastly starting with 5.6.13 we know the security issues have been addressed so why not enforce that rather than leaving it up to a vendor to determine wether the fixes are security related in their view and/or insuring those not using non-supported versions have some incentive to upgrade.

Well, first because it's not our call. It really is that simple, we can't decide for others, the same way we can't be held responsible for the security of people using old versions of PHP (or any other software). It's the same principle why things like random_compat exist. We can't assume that if people don't upgrade that's because they don't want to. Refusing to provide a secure alternative for them downgrades security as they won't upgrade anyway. Instead, the responsible approach here is to provide secure alternatives where possible to those people not upgrading.

Second, because it's not that simple. Most people using those ancient versions of PHP have no easy way to upgrade. It's not just that they don't want. If they could do that easily, they would do it.

Let me try to summarize the pros and cons:

• If you don't modify the minimum requirements, we gain nothing. There's nothing in this PR that requires PHP >= 5.6. You might have in mind some other developments of xmlseclibs that would require either 5.6 or even 7.0, but in that case, you can increase the required PHP version when those developments are in place, not now. Furthermore, what we get by requiring PHP 5.6 here is worse security for those users, as they'll be forced to trust third-party repositories that could get compromised or provide software that's not as well-tested as the available in official repos.

• If, on the other hand, you restore the minimum requirements to the actual minimum, 5.4, we provide better security:

  • People using RHEL can use the library without depending on third-party, unofficial repositories (repos that could be unmaintained, or compromised, or whatever).
  • People will no longer need to compile mcrypt if unavailable.
  • In general, people will no longer need to depend on a library that's been unmaintained for many, many years (mcrypt).

So, if we care about the security of people using those old versions of PHP, the right thing here is to lessen the requirements. I can provide a PR with the changes needed if that makes it easier for you. If you also want to bump the version of PHP required to support some other thing you have in mind, that's fine, but do that in a different 4.0 release. Meanwhile, please, restore support for PHP 5.4 and 5.5.

[thijskh]

I agree with Jaime that by setting the technical requirements so strictly people may actually be upgrading xmlseclibs less. People may have a blocking reason to stay on e.g. HEL supported PHP 5.4, say one app they cannot upgrade yet, may be willing to upgrade other things like xmlseclibs but are unable to bevause of the strict dependency. If other libraries would do the same, this means they only can start upgrading when the last app is migrated, and not start their upgades way earlier.

May I suggest that:

• In the technical metadata, you describe the technical minimum requirements of the current code (i.e. PHP 5.4 in composer.json);
• In the README, next to mentioning the above you also clearly document what the best/preferred PHP version is (i.e. >= 5.6.13).

And of course there's nothing that stands in the way of raising the technical minimum in the next version as soon as there's something you want to use that's 5.6 only.

This would be really helpful.