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

Hash ticket strings to generate valid-length session-ids. #257

Merged
merged 1 commit into from
Mar 15, 2018

Conversation

adamfranco
Copy link
Contributor

By using a sha256 hash of the ticket, the session-id is guaranteed 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.

…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 adamfranco requested a review from jfritschi March 13, 2018 20:07
@adamfranco
Copy link
Contributor Author

@jfritschi Let me know if you'd like to integrate this fix as-is in this PR or would prefer to add a additional configuration for a salt/seed for the hash. My only hesitancy with that is that it further pollutes the API even as we're thinking of cleaning it up. I'll submit a separate PR with this change plus the configurable salt/seed.

adamfranco added a commit that referenced this pull request 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.
@jfritschi
Copy link
Contributor

The reason I would like at least the option to include a user configured seed is hardening of the session token.
With the simple hash solution we artificially increase the length of the token without including real entropy. In a sense we are undermining the new and improved php session security standards that demand a long and random session string. Also the CAS tokens were never aimed at longer term usage. They are usually limited to a few minutes which means a small size is acceptable since any brute force is only doable in a short interval. This is not really true for a website session which may last hours or even days....
By having a seed option we allow the user to manually increase security. In my view we need it needs to be a random value set by the user (or a pseudo random but static value which however may have some issues in multi system setups). This seeding could be part of our optional hardening recommendations.

Your thoughts?

@jfritschi
Copy link
Contributor

Sorry did not see the other pull before. I really think we should go for the other pull request that includes the salt.

Copy link
Contributor

@jfritschi jfritschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry did not see the other pull before. I really think we should go for the other pull request that includes the salt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for longer tickets Endless Redirect Loop
2 participants