Allow setting SPN parameter (new in php-krb5 1.1.3)

[knixeur]

The SPN parameter allow specifying the entry to authenticate with in the keytab file. Setting it to null keeps the old behavior where it guesses the FQDN, but this is problematic when running as vhost or behind a reverse proxy such. When set to 0 it authenticates with any entry in the keytab file.

Edit: spelling

[codecov[bot]]

Codecov Report

Merging #2 into master will not change coverage. The diff coverage is 0%.

@@          Coverage Diff           @@
##             master    #2   +/-   ##
======================================
  Coverage         0%    0%           
- Complexity       39    41    +2     
======================================
  Files             1     1           
  Lines           165   171    +6     
======================================
- Misses          165   171    +6

Impacted Files	Coverage Δ	Complexity Δ
lib/Auth/Source/Negotiate.php	0% <0%> (ø)	41 <0> (+2)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6117a8f...cf953bc. Read the comment docs.

[tvdijen]

Thanks @knixeur ! I'm actually fine with forcing the new behaviour and force ext-krb5 >= 1.1.3 in the composer.json file.. Do you think you can change that and perhaps come up with a few lines of documentation on the new setting as well?

[knixeur]

You're welcome! Sure I'll add some lines about this change. Actually if you don't specify a value for 'spn' it will still be compatible with < 1.1.3. Of course there were some other fixes which people might still want :)

[tvdijen]

Do you know if 1.1.3 is supporting PHP 7.0? In that case we could merge this into the 0.9 branch as is, and do the master-branch without the backwards compatibility

[knixeur]

Do you know if 1.1.3 is supporting PHP 7.0? In that case we could merge this into the 0.9 branch as is, and do the master-branch without the backwards compatibility

Yes it does, I'm running it with PHP 7.3 without any problems :+1:

[knixeur]

Ok I'm done with the changes, sorry, I was applying this from a simplesamlphp fork I had. ~Let me know if you want me to rebase them.~ I went ahead and squashed the commits to make this cleaner

[tvdijen]

This is great, thank you! I think you did misunderstand my question though.. would krb5 1.1.3 work on php 7.0 and / or 7.1? If that’s the case, I can backport this patch

[knixeur]

Oh yeah I misunderstood heh, it should as krb5 minimal version in theory is 5.3.0 but let me do a test tomorrow and I'll confirm, it's great if this can be backported!

[tvdijen]

Thanks @knixeur! Your effort is really appreciated!

[knixeur]

I confirm it works on PHP 7.0 and 7.1. I also fixed using GSS_C_NO_NAME as the module expects the second parameter to be a numeric 0 instead of a '0' and I assumed the latter would be infered/casted, it's not. So the config now has to be 'spn' => '0' (a string in the config file) and that will be passed as numeric 0 to the constructor.

https://github.com/simplesamlphp/simplesamlphp-module-negotiate/pull/2/files#diff-7721f09ecbc745ec702ca62ce9c2636cR176

Edit: added link to relevant lines.

[tvdijen]

I took a slightly different approach on master Let me know what you think

[knixeur]

That's perfect! I actually did something similar to that but reverted it to avoid the value having so many different types, but it's the correct approach to match krb5's logic.

[knixeur]

I've just realized that you removed the one parameter constructor call when spn is set to null. That will break compability with php-krb5<1.1.3 I added a comment here: https://github.com/simplesamlphp/simplesamlphp-module-negotiate/commit/d404ec676ca7f69b452d1b21510d29d673422aab#r37522793