If the cookie is invalid, send an expired cookie value #63

jgoguen · 2014-01-21T19:53:35Z

If a user's browser has a mod_auth_cas cookie that is not mapped to a valid cache file, currently it redirects in a loop back to CAS and the application. This PR expires the cookie before sending the user back to CAS so they're forced to get a new mod_auth_cas cookie when they return.

bnoordhuis · 2014-01-24T17:16:17Z

Sounds like #52... I don't have a vested interest in mod_auth_cas anymore but maybe @pames can take this one?

@jgoguen Are you using mod_auth_cas in production? If yes, maybe we should make you a committer. I think most of the current maintainers have moved on.

jgoguen · 2014-01-24T17:23:03Z

Yes, we're using mod_auth_cas in production for almost all of our Apache servers.

bnoordhuis · 2014-01-24T17:35:41Z

@forsetti @pames Your call.

pames · 2014-01-24T17:40:22Z

And, re: giving @jgoguen commit access -- I think as long as we keep up the general practice of code review, no concerns. @forsetti do you have any more time in your new role to help shepherd mod_auth_cas changes?

jgoguen · 2014-02-24T12:02:48Z

Is this one good to go? Could someone please review?

pames · 2014-02-24T13:34:39Z

OK, upon further thought, I do have some suggestions here. It seems like this largely duplicates setCASCookie, but with the exception that it adds support for Expires and simply sets an empty value. It seems like the better approach would be to modify setCASCookie to accept an expiration timestamp and call that instead of duplicating the code.

jgoguen · 2014-02-24T18:51:40Z

How's this? Seems to work fine for issuing new cookies and for when the server-side XML file is somehow corrupt (invalid XML, not present).

dotmjs · 2014-02-24T23:03:14Z

I note I never replied publicly to the above committer comments. I have reached out to Marvin for guidance on current process for adding committers. Minimally, I understand an ICLA is required. Will let you know when I have something more definitive.

jgoguen · 2014-03-09T22:13:47Z

Just forgot all about apr_time_t and the standard time_t too. All fixed up now, I think. Not sure about the style, and I couldn't get the tests to run on my setup for some reason (CentOS 6 x64) but I wrote a quick program separately and I think it'll come out correctly.

jgoguen · 2014-04-03T13:28:07Z

Is there anything else to be changed with this?

pames · 2014-04-03T16:22:27Z

src/mod_auth_cas.c

@@ -763,7 +764,24 @@ void setCASCookie(request_rec *r, char *cookieName, char *cookieValue, apr_byte_
 	if(c->CASRootProxiedAs.is_initialized)
 		pathPrefix = urlEncode(r, c->CASRootProxiedAs.path, " ");

-	headerString = apr_psprintf(r->pool, "%s=%s%s;Path=%s%s%s%s%s", cookieName, cookieValue, (secure ? ";Secure" : ""), pathPrefix, urlEncode(r, getCASScope(r), " "), (c->CASCookieDomain != NULL ? ";Domain=" : ""), (c->CASCookieDomain != NULL ? c->CASCookieDomain : ""), (c->CASCookieHttpOnly != FALSE ? "; HttpOnly" : ""));
+	if(-1 != expireTime) {


Maybe #define this as a constant, or declare a global "apr_time_t CAS_SESSION_EXPIRE_COOKIE_TIMEOUT = -1;" to improve readability?

jgoguen · 2014-04-04T14:04:14Z

I've got these changes made, but I still can't figure out why the tests won't run. What I'm running is:

autoreconf
./configure --with-check --with-apxs=/usr/sbin/apxs
make check

And what I'm getting is:

[jgoguen@opal mod_auth_cas]$ make check
Making check in src
make[1]: Entering directory `/home/jgoguen/Code/mod_auth_cas/src'
/usr/sbin/apxs -c -lpcre -lssl -lcrypto  -Wc," -D_LARGEFILE64_SOURCE -g -O2 -Wall -Wextra -Wdeclaration-after-statement -Wformat -Wformat-security -Wmissing-declarations -Wno-unused-parameter -Wpointer-arith -Wstrict-prototypes -fprofile-arcs -ftest-coverage " -Wl,"  -lcurl   -lpcre -lssl -lcrypto " mod_auth_cas.c cas_saml_attr.c
/usr/lib64/apr-1/build/libtool --silent --mode=compile gcc -prefer-pic -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Wformat-security -fno-strict-aliasing  -DLINUX=2 -D_REENTRANT -D_GNU_SOURCE -pthread -I/usr/include/httpd  -I/usr/include/apr-1   -I/usr/include/apr-1   -D_LARGEFILE64_SOURCE -g -O2 -Wall -Wextra -Wdeclaration-after-statement -Wformat -Wformat-security -Wmissing-declarations -Wno-unused-parameter -Wpointer-arith -Wstrict-prototypes -fprofile-arcs -ftest-coverage   -c -o mod_auth_cas.lo mod_auth_cas.c && touch mod_auth_cas.slo
/usr/lib64/apr-1/build/libtool --silent --mode=compile gcc -prefer-pic -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Wformat-security -fno-strict-aliasing  -DLINUX=2 -D_REENTRANT -D_GNU_SOURCE -pthread -I/usr/include/httpd  -I/usr/include/apr-1   -I/usr/include/apr-1   -D_LARGEFILE64_SOURCE -g -O2 -Wall -Wextra -Wdeclaration-after-statement -Wformat -Wformat-security -Wmissing-declarations -Wno-unused-parameter -Wpointer-arith -Wstrict-prototypes -fprofile-arcs -ftest-coverage   -c -o cas_saml_attr.lo cas_saml_attr.c && touch cas_saml_attr.slo
/usr/lib64/apr-1/build/libtool --silent --mode=link gcc -o mod_auth_cas.la   -lcurl   -lpcre -lssl -lcrypto   -lpcre -lssl -lcrypto -rpath /usr/lib64/httpd/modules -module -avoid-version    cas_saml_attr.lo mod_auth_cas.lo
make[1]: Leaving directory `/home/jgoguen/Code/mod_auth_cas/src'
Making check in tests
make[1]: Entering directory `/home/jgoguen/Code/mod_auth_cas/tests'
make  check_mac
make[2]: Entering directory `/home/jgoguen/Code/mod_auth_cas/tests'
gcc -DHAVE_CONFIG_H -I. -I..  -D_LARGEFILE64_SOURCE -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Wformat-security -fno-strict-aliasing -I/usr/include/httpd  -DLINUX=2 -D_REENTRANT -D_GNU_SOURCE  -I/usr/include/apr-1 @APACHE_INCLUDES@  -fprofile-arcs -ftest-coverage  -g -O2 -Wall -Wextra -Wdeclaration-after-statement -Wformat -Wformat-security -Wmissing-declarations -Wno-unused-parameter -Wpointer-arith -Wstrict-prototypes -MT check_mac-ap_stubs.o -MD -MP -MF .deps/check_mac-ap_stubs.Tpo -c -o check_mac-ap_stubs.o `test -f 'ap_stubs.c' || echo './'`ap_stubs.c
gcc: @APACHE_INCLUDES@: No such file or directory
ap_stubs.c:211: warning: no previous declaration for ‘ap_note_auth_failure’
make[2]: *** [check_mac-ap_stubs.o] Error 1
make[2]: Leaving directory `/home/jgoguen/Code/mod_auth_cas/tests'
make[1]: *** [check-am] Error 2
make[1]: Leaving directory `/home/jgoguen/Code/mod_auth_cas/tests'
make: *** [check-recursive] Error 1

Any ideas what I'm missing? I'm not familiar with GNU autotools so I'm hoping there's some flag to autoreconf or configure I missed. It's the same output if I don't call autoreconf first.

CentOS 6 x64

pames · 2014-04-04T14:15:47Z

I had the same issue -- it works if you remove the @APACHE_INCLUDES@ line (and the '' on the line before) from that makefile. Maybe @forsetti knows why it doesn't get converted to a path.

pames · 2014-04-04T14:20:33Z

src/mod_auth_cas.c

@@ -1968,7 +1992,7 @@ int cas_authenticate(request_rec *r)
 		if(cookieString == NULL) { /* they have not made a gateway trip yet */
 			if(c->CASDebug)
 				ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, "Gateway initial access (%s)", r->parsed_uri.path);
-			setCASCookie(r, d->CASGatewayCookie, "TRUE", ssl);
+			setCASCookie(r, d->CASGatewayCookie, "TRUE", ssl, -1);


this should also be CAS_SESSION_EXPIRE_TIMEOUT right? (and below, in cas_authenticate)

pames · 2014-04-04T14:21:08Z

LGTM modulo those last few nits (domainString, use of the #defined value). Also, there's 1 broken test (that isn't a hard fail()), but it's also broken in master so it's not your changes, I'll have to look into that later.

bnoordhuis · 2014-04-04T15:27:24Z

Apropos APACHE_INCLUDES, that looks like a bad/copy paste job. It's supposed to be accompanied by a rule in configure.ac that replaces it with something that evaluates to apr-1-config --includes` -I`apxs -q INCLUDEDIR but that's missing.

Use apr_time_t instead of a pre-defined string Address comments for changes and fix up tests

jgoguen · 2014-04-04T17:53:40Z

OK, I think I got all the instances of '-1' and '0' being used as arguments to setCASCookie(), replaced '2048' with CAS_MAX_ERROR_SIZE, and the tests for expiring cookie strings works.

pames · 2014-04-04T18:22:12Z

src/mod_auth_cas.c

 {
-	char *headerString, *currentCookies, *pathPrefix = "";
+	char *headerString, *currentCookies, *pathPrefix = "", *expireTimeString = NULL, *errString;
+	char *domainString = "";


Why not one line?
char *headerString, *currentCookies, *errString, *domainString = "", *pathPrefix = "", *expireTimeString = NULL;

pames · 2014-04-04T18:26:11Z

Still LGTM, just bikeshedding at this point.

dotmjs · 2014-04-07T02:36:49Z

Actually, given that APR_INCLUDES and APXS_INCLUDES are already listed,
APACHE_INCLUDES is redundant. I'll yank it.

On Fri, Apr 4, 2014 at 11:27 AM, Ben Noordhuis [email protected]:

Apropos APACHE_INCLUDES, that looks like a bad/copy paste job. It's
supposed to be accompanied by a rule in configure.ac that replaces it
with something that evaluates to apr-1-config --includes -Iapxs -q INCLUDEDIR but that's missing.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/63#issuecomment-39577452
.

[email protected]
PGP: E2144AD8

… by Joel Goguen in apereo#63

If the cookie is invalid, send an expired cookie value

Remove extraneous APACHE_INCLUDES from tests/Makefile.am, as reported by Joel Goguen in apereo#63

pames reviewed Apr 3, 2014
View reviewed changes

pames reviewed Apr 4, 2014
View reviewed changes

If the cookie is invalid, send an expired cookie value

7a14fc0

Use apr_time_t instead of a pre-defined string Address comments for changes and fix up tests

pames reviewed Apr 4, 2014
View reviewed changes

Rearranging bikes in the sheds :)

0f6b986

dotmjs pushed a commit to dotmjs/mod_auth_cas that referenced this pull request Apr 7, 2014

Remove extraneous APACHE_INCLUDES from tests/Makefile.am, as reported…

43a3678

… by Joel Goguen in apereo#63

pames added a commit that referenced this pull request Apr 9, 2014

Merge pull request #63 from jgoguen/cookie-expire

81454a6

If the cookie is invalid, send an expired cookie value

pames merged commit 81454a6 into apereo:master Apr 9, 2014

jgoguen deleted the cookie-expire branch April 9, 2014 15:43

dotmjs pushed a commit to dotmjs/mod_auth_cas that referenced this pull request Apr 13, 2014

Fix apereo#63 APACHE_INCLUDES in tests/Makefile.am

aca4c45

Remove extraneous APACHE_INCLUDES from tests/Makefile.am, as reported by Joel Goguen in apereo#63

If the cookie is invalid, send an expired cookie value #63

If the cookie is invalid, send an expired cookie value #63

Uh oh!

Conversation

jgoguen commented Jan 21, 2014

Uh oh!

bnoordhuis commented Jan 24, 2014

Uh oh!

jgoguen commented Jan 24, 2014

Uh oh!

bnoordhuis commented Jan 24, 2014

Uh oh!

pames commented Jan 24, 2014

Uh oh!

jgoguen commented Feb 24, 2014

Uh oh!

pames commented Feb 24, 2014

Uh oh!

jgoguen commented Feb 24, 2014

Uh oh!

dotmjs commented Feb 24, 2014

Uh oh!

jgoguen commented Mar 9, 2014

Uh oh!

jgoguen commented Apr 3, 2014

Uh oh!

pames Apr 3, 2014

Choose a reason for hiding this comment

Uh oh!

jgoguen commented Apr 4, 2014

Uh oh!

pames commented Apr 4, 2014

Uh oh!

pames Apr 4, 2014

Choose a reason for hiding this comment

Uh oh!

pames commented Apr 4, 2014

Uh oh!

bnoordhuis commented Apr 4, 2014

Uh oh!

jgoguen commented Apr 4, 2014

Uh oh!

pames Apr 4, 2014

Choose a reason for hiding this comment

Uh oh!

pames commented Apr 4, 2014

Uh oh!

dotmjs commented Apr 7, 2014

Uh oh!

Uh oh!