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

Add support for multiple upstream CAS servers #36

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bnoordhuis
Copy link
Contributor

This PR makes it possible to talk to more than one CAS server. It works by changing a number of config directives from per-server to per-directory.

It's a rough draft (a RC1 if you will) but it's been successfully tested by Swiss Post. Feedback appreciated.

This commit makes it possible to use mod_auth_cas with more than one one CAS
server.

It is now possible to configure CASLoginURL, CASValidateURL and other directives
on a per-directory level, not just on the per-server level.
@dotmjs
Copy link

dotmjs commented May 12, 2012

Will review after attz work is merged

@dune73
Copy link

dune73 commented Sep 26, 2012

Is there any progress on this front?

@bnoordhuis
Copy link
Contributor Author

We seem to be having a reviewer shortage.

@dotmjs
Copy link

dotmjs commented Sep 26, 2012

Need to merge PR#42 for 1.0.10 release (like to fix CASAuthNHeader bug first). Will target this for 1.0.11.

And yeah -- we need to recruit additional reviewers .....

@dune73
Copy link

dune73 commented Sep 27, 2012

Thanks.

@bnoordhuis
Copy link
Contributor Author

Any objections to merging this?

return(c);
}

static char *check_cookie_domain(apr_pool_t *pool, const char *value)
Copy link
Contributor

Choose a reason for hiding this comment

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

If/when we merge this, feel free to substitute this in lieu of the function I just added in PR47.

@pames
Copy link
Contributor

pames commented Nov 8, 2012

OK, this LGTM modulo that TODO assuming it doesn't break any existing tests.

Other random thoughts (don't necessarily have to be addressed in this PR, but just things to talk about) - the CASValidateServer directive -> should we keep it at the server level, make it per-dir, or remove it entirely? (I'm leaning towards 'remove entirely')

@pames
Copy link
Contributor

pames commented Nov 9, 2012

Another thought came to me. If you have different CAS servers for different URLs, then that means the authentication cookie should be bound to the CAS server it came from. Right now, you can take a cookie from path A/CAS server A, and use it on path B, configured for CAS server B. We should add the CAS server/login URL to the ticket metadata, and check that when the cookie comes in against the expected one for the path.

@dune73
Copy link

dune73 commented Nov 11, 2012

While you guys are at it ...
AFAIK mod_auth_cas does not check the path in the ticket metadata against the path of the request. I think it would be more secure if the module would make sure session can only be used for paths they were issued for.

@fretscha
Copy link

I really would appreciate if somebody could merge this feature into the master branch, because it would be very handy in my enterprise environment (multiple LDAP Servers :). Is there still any reason for refusing this pull request?

@dotmjs
Copy link

dotmjs commented Jan 30, 2013

I've been tied up for an unforgivable period, due to job change. I hope to
free up a little bit in a few weeks to review and merge; plus my new
employer is .... very open-source friendly ... so I should be able to
jump on it soon.

In the meantime, Frederic -- could you confirm some successful testing of
the patch in #36?

Thanks,
-Matt

On Mon, Jan 28, 2013 at 2:51 AM, Frederic Tschannen <
notifications@github.com> wrote:

I really would appreciate if somebody could merge this feature into the
master branch, because it would be very handy in my enterprise environment
(multiple LDAP Servers :). Is there still any reason for refusing this pull
request?


Reply to this email directly or view it on GitHubhttps://github.com//pull/36#issuecomment-12771427.

matt@forsetti.com
PGP: E2144AD8

@pames
Copy link
Contributor

pames commented Jan 30, 2013

I do think we should resolve #36 (comment) before merging this. Path might be nice-to-have, but since the path of a URI is not the strongest security boundary (XSS anywhere else on an origin can initiate requests to the protected path, and the cookie is most likely going to be stolen via XSS) it really only adds value in edge cases IMHO.

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.

6 participants