Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Will phpCAS work with php 7.1? #224

Closed
johanv opened this issue Mar 14, 2017 · 6 comments
Closed

Will phpCAS work with php 7.1? #224

johanv opened this issue Mar 14, 2017 · 6 comments
Assignees
Labels
Minor Bug PHP 7 Issues related to supporting PHP 7.0, 7.1, and 7.2.
Milestone

Comments

@johanv
Copy link

johanv commented Mar 14, 2017

I recently was looking into issue dokuwiki/dokuwiki#1904 with CAS-authentication in dokuwiki using the authplaincas extension, which uses phpCAS. phpCAS replaces the session ID by a 13-character string, and dokuwiki doesn't like session ID's smaller than 22 characters.

In the discussion on the issue, @mprins pointed out that in php 7.1 session ID's should have a length of at least 22. So I guess php 7.1 might break phpCAS. I didn't try it out though.

See also esn-org/authplaincas#9.

@jfritschi
Copy link
Contributor

I guess this is something that we can fix.

The only reason we this strange mapping of CAS Ticket to session ID is that we need to have a mapping between CAS Ticket ID and PHP session to allow for a single lockout without having and database..

My idea to fix this would be use sha256("ST-YXXXX" + random_seed) to generate the session ID in the future. This would however require everyone to configure their own seed once during setup. We could also just use the domain/host as additional seed value.
Maybe better we use a combination: the domain by default and allow extra hardening via manual seed? This would be better if we want to slip it into the next patch.

@adamfranco: Any thoughts on this?

@jfritschi jfritschi added this to the 1.3.6 milestone Apr 10, 2017
@mmarlett
Copy link

Yes, this breaks on php 7.1

@jfritschi
Copy link
Contributor

I'm working on a patch

@grahamb
Copy link

grahamb commented Jan 30, 2018

Is there a timeframe for a release that works on PHP 7.1?

@kourylape
Copy link

Any update on this? This seems to be generating a lot of similar issues, so it seems pretty desirable.

@adamfranco
Copy link
Contributor

My idea to fix this would be use sha256("ST-YXXXX" + random_seed) to generate the session ID in the future. This would however require everyone to configure their own seed once during setup. We could also just use the domain/host as additional seed value.
Maybe better we use a combination: the domain by default and allow extra hardening via manual seed? This would be better if we want to slip it into the next patch.

Is there a need for the random seed? hash("sha256", "ST-YXXXX") should give an appropriate-length session-id for short tickets as well as long tickets (#248, #224). A random-seed would certainly give increased entropy to make guessing the session-id harder, but might not be necessary when the CAS server returns a longer session-ticket value. Maybe making the random-seed optional would be best: It would be unneeded for CAS servers returning long tickets, would work ok with any ticket-length without modifying the application configuration, and still allow applications to upgrade their entropy if they are faced with a CAS server returning short tickets.

@adamfranco adamfranco added the PHP 7 Issues related to supporting PHP 7.0, 7.1, and 7.2. label Mar 13, 2018
adamfranco added a commit that referenced this issue Mar 13, 2018
…n-ids.

By using a sha256 hash of the ticket, the session-id is guarenteed to be
64 bytes long no matter how short or long the ticket provided by the CAS
server is.

This fixes #248, fixes #244, and partially addresses the comments in #224 with the
exception of an extra salt or random-seed when generating the hash.
adamfranco added a commit that referenced this issue Mar 13, 2018
As suggested in #224, a client-configurable salt will allow a hashed
ticket to have increased unique data, helping to make ensure that the
session id is hard to guess even if the CAS server uses short tickets.

This includes the contents of PR #257.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Minor Bug PHP 7 Issues related to supporting PHP 7.0, 7.1, and 7.2.
Projects
None yet
Development

No branches or pull requests

6 participants