-
Notifications
You must be signed in to change notification settings - Fork 97
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
Replace bespoke cookie parsing with ap_cookie_read() #202
Conversation
I'm all for this but want to note that this will leave Apache 2.2 support behind. I'm personally for that since it's been end-of-life for going on 5 years, but still wanted to note it. I'll review and test. Thanks for the patch! |
Yeah, it seems safe to no longer care about Apache 2.2. In addition to the upstream EOL date I also had a look at what the various LTS Linux distros ship (in their oldest still supported release):
|
The %%%x format string resolves to the literal "%" and the hex representation of the character to be encoded, but is always asssumed to return three characters. However for a small value like e.g. 7 it would return "%7" instead. None of the current two call sites of the function use such a small value, but apply correct padding just in case the function might be used elsewhere in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks good, I just added one suggestion to get rid of compile warnings.
I would like to update the README to note that Apache 2.4 is the only supported version. I can take that (simple) task.
I'm not sure if you meant for the urlEncode() changes to go with this PR, but no objections from me.
Co-authored-by: David Hawes <dhawes@gmail.com>
The upstream support for Apache 2.2.x ended on 2018-01-01 and also none of the long term Linux distros still support it, looking at the latest still supported releases: * Debian 8 ELTS has Apache httpd 2.4.10 * Ubuntu 14.4 has Apache httpd 2.4.5 * RHEL 7 has Apache httpd 2.4.6 * SLES 11 has Apache httpd 2.4.23
I've added a documentation entry to the README to mention that 2.4 is now required and did another smoke test on two CAS-enabled services using the latest version of the PR. (It also includes a small correctness fix which we had in our internal .deb package already and which is also included in the PR, it's a NOP for all current call sites of urlEncode(). With Apache 2.4 officially the base line, this also obsoletes CASAuthoritative. I can make a separate PR for that in the next days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, thanks for doing the legwork to look at supported distro versions.
I'll likely do a clean up pass in the near future to remove some Apache 2.2-specific #ifdefs as well.
WIP pull request to remove Apache 2.2: |
We may need to revisit using ap_cookie_read(). A user noted that they were getting into a redirect loop, and it turns out it's because of that change:
If multiple mod_auth_cas cookies are sent, APR_EGENERAL is returned, which results in a redirect. This looks to be a common situation depending on a user's Apache config, and certainly is a change from previous versions. Perhaps we can grab the cookie directly with apr_table_do()? |
Simplify the code by moving the cookie parsing to use ap_cookie_read(). We're running that patch on several servers in Wikimedia's CAS installation without problems.