search

robrichards / xmlseclibs

A PHP library for XML Security
BSD 3-Clause "New" or "Revised" License
387 stars 181 forks source link

Remove mcrypt dependency #123

Closed skymeyer closed 7 years ago

skymeyer commented 7 years ago

Changes which are not included from the previous PR:

skymeyer commented 7 years ago

@robrichards tests are passing except for HHVM (see https://github.com/robrichards/xmlseclibs/issues/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 commented 7 years ago

@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 commented 7 years ago

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

pitbulk commented 7 years ago

@skymeyer Thanks, I updated it ;)

zsprackett commented 7 years ago

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

rajesh2011 commented 7 years ago

We need this urgently. Please get it in asap.

csezheng commented 7 years ago

👍

robrichards commented 7 years ago

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 notifications@github.com 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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

robrichards commented 7 years ago

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

skymeyer commented 7 years ago

@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 commented 7 years ago

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

ghost commented 7 years ago

@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 https://github.com/onelogin/php-saml/issues/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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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:

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:

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 commented 7 years ago

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:

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.