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.

Sign up for GitHub

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

Jump to bottom

add CASRedirectAfterValidation parameter to mimic java-cas-client's parameter #136

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

add CASRedirectAfterValidation parameter to mimic java-cas-client's parameter #136

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
eba7841
add CASRedirectAfterValidation parameter to mimic java-cas-client's r…
ruckc Nov 3, 2017
512f34e
adjust based on PR-136 comments
ruckc Nov 28, 2017
2fda608
adjust README verbiage for CASDisableRedirectAfterValidation
ruckc Nov 28, 2017
70b6112
add additional return OK
ruckc Nov 29, 2017
9675e4f
Merge branch 'redirectAfterValidation' of https://github.com/ruckc/mo…
ruckc Nov 29, 2017
c2e865e
try fixing mod_auth_cas too many redirects
ruckc Dec 14, 2017
71905b4
revert last commit
ruckc Dec 14, 2017
01c2221
Merge branch 'master' into redirectAfterValidation
ruckc Jan 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions README
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,11 @@ Default: NULL
Description: The URL to use when performing a proxy validation. This is currently
an unimplemented feature, so setting this will have no effect.

Directive: CASDisableRedirectAfterValidation
Default: On
Description: Whether to redirect to the same URL after ticket validation, but without the
ticket in the parameter. Defaults to on
dhawes marked this conversation as resolved.
Show resolved Hide resolved

Directive: CASRootProxiedAs
Default: NULL
Description: This URL represents the URL that end users may see in the event that
Expand Down
61 changes: 40 additions & 21 deletions src/mod_auth_cas.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ void *cas_create_dir_config(apr_pool_t *pool, char *path)
c->CASGatewayCookie = CAS_DEFAULT_GATEWAY_COOKIE;
c->CASAuthNHeader = CAS_DEFAULT_AUTHN_HEADER;
c->CASScrubRequestHeaders = CAS_DEFAULT_SCRUB_REQUEST_HEADERS;
c->CASDisableRedirectAfterValidation = CAS_DEFAULT_REDIRECT_AFTER_VALIDATION;
return(c);
}

Expand Down Expand Up @@ -237,6 +238,12 @@ void *cas_merge_dir_config(apr_pool_t *pool, void *BASE, void *ADD)
if(add->CASScrubRequestHeaders != NULL && apr_strnatcasecmp(add->CASScrubRequestHeaders, "Off") == 0)
c->CASScrubRequestHeaders = NULL;

c->CASDisableRedirectAfterValidation = (add->CASDisableRedirectAfterValidation != CAS_DEFAULT_REDIRECT_AFTER_VALIDATION ?
add->CASDisableRedirectAfterValidation :
base->CASDisableRedirectAfterValidation);
if(add->CASDisableRedirectAfterValidation != NULL && apr_strnatcasecmp(add->CASDisableRedirectAfterValidation, "Off") == 0)
c->CASDisableRedirectAfterValidation = NULL;

return(c);
}

Expand Down Expand Up @@ -1664,6 +1671,7 @@ apr_byte_t isValidCASTicket(request_rec *r, cas_cfg *c, char *ticket, char **use

apr_byte_t isValidCASCookie(request_rec *r, cas_cfg *c, char *cookie, char **user, cas_saml_attr **attrs)
{
char *path;
dhawes marked this conversation as resolved.
Show resolved Hide resolved
cas_cache_entry cache;
cas_dir_cfg *d = ap_get_module_config(r->per_dir_config, &auth_cas_module);

Expand All @@ -1677,6 +1685,8 @@ apr_byte_t isValidCASCookie(request_rec *r, cas_cfg *c, char *cookie, char **use
return FALSE;
}

path = apr_psprintf(r->pool, "%s%s", c->CASCookiePath, cookie);

/*
* mitigate session hijacking by not allowing cookies transmitted in the clear to be submitted
* for HTTPS URLs and by voiding HTTPS cookies sent in the clear
Expand Down Expand Up @@ -1836,6 +1846,8 @@ char *getResponseFromServer (request_rec *r, cas_cfg *c, char *ticket)
curl_easy_setopt(curl, CURLOPT_HTTPGET, 1L);

memcpy(&validateURL, &c->CASValidateURL, sizeof(apr_uri_t));
if(c->CASDebug)
ap_log_rerror(APLOG_MARK,APLOG_DEBUG,0,r,"MOD_AUTH_CAS: validateUrl: %s", apr_uri_unparse(r->pool, &validateURL, 0));
if(c->CASValidateSAML == FALSE)
validateURL.query = apr_psprintf(r->pool, "service=%s&ticket=%s%s", getCASService(r, c), ticket, getCASRenew(r));
else
Expand All @@ -1844,8 +1856,10 @@ char *getResponseFromServer (request_rec *r, cas_cfg *c, char *ticket)
curl_easy_setopt(curl, CURLOPT_URL, apr_uri_unparse(r->pool, &validateURL, 0));

if(curl_easy_perform(curl) != CURLE_OK) {
if(c->CASDebug)
if(c->CASDebug) {
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, "MOD_AUTH_CAS: query: %s", validateURL.query);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably move the previous log addition to line 1855 after validateURL.query is set so we get the whole validateURL printed out.

Copy link
Author

Choose a reason for hiding this comment

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

This was moved. Good catch.

Copy link
Contributor

Choose a reason for hiding this comment

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

The log at line 1858 is redundant since it's logged at 1852 now.

ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, "MOD_AUTH_CAS: curl_easy_perform() failed (%s)", curlError);
}
goto out;
}

Expand Down Expand Up @@ -2124,31 +2138,35 @@ int cas_authenticate(request_rec *r)
if(d->CASAuthNHeader != NULL)
apr_table_set(r->headers_in, d->CASAuthNHeader, remoteUser);

if(parametersRemoved == TRUE) {
if(ssl == TRUE && port != 443)
printPort = TRUE;
else if(port != 80)
printPort = TRUE;

if(c->CASRootProxiedAs.is_initialized) {
if(d->CASDisableRedirectAfterValidation == NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not:

                       if(d->CASDisableRedirectAfterValidation) {
                               return OK;
                       }

and keep everything in the "if(parametersRemoved == TRUE)" unchanged? The patch and code are much more readable like that.

if(parametersRemoved == TRUE) {
if(ssl == TRUE && port != 443)
printPort = TRUE;
else if(port != 80)
printPort = TRUE;

if(c->CASRootProxiedAs.is_initialized) {
newLocation = apr_psprintf(r->pool, "%s%s%s%s", apr_uri_unparse(r->pool, &c->CASRootProxiedAs, 0), r->uri, ((r->args != NULL) ? "?" : ""), ((r->args != NULL) ? r->args : ""));
} else {
} else {
#ifdef APACHE2_0
if(printPort == TRUE)
newLocation = apr_psprintf(r->pool, "%s://%s:%u%s%s%s", ap_http_method(r), r->server->server_hostname, r->connection->local_addr->port, r->uri, ((r->args != NULL) ? "?" : ""), ((r->args != NULL) ? r->args : ""));
else
newLocation = apr_psprintf(r->pool, "%s://%s%s%s%s", ap_http_method(r), r->server->server_hostname, r->uri, ((r->args != NULL) ? "?" : ""), ((r->args != NULL) ? r->args : ""));
if(printPort == TRUE)
newLocation = apr_psprintf(r->pool, "%s://%s:%u%s%s%s", ap_http_method(r), r->server->server_hostname, r->connection->local_addr->port, r->uri, ((r->args != NULL) ? "?" : ""), ((r->args != NULL) ? r->args : ""));
else
newLocation = apr_psprintf(r->pool, "%s://%s%s%s%s", ap_http_method(r), r->server->server_hostname, r->uri, ((r->args != NULL) ? "?" : ""), ((r->args != NULL) ? r->args : ""));
#else
if(printPort == TRUE)
newLocation = apr_psprintf(r->pool, "%s://%s:%u%s%s%s", ap_http_scheme(r), r->server->server_hostname, r->connection->local_addr->port, r->uri, ((r->args != NULL) ? "?" : ""), ((r->args != NULL) ? r->args : ""));
else
newLocation = apr_psprintf(r->pool, "%s://%s%s%s%s", ap_http_scheme(r), r->server->server_hostname, r->uri, ((r->args != NULL) ? "?" : ""), ((r->args != NULL) ? r->args : ""));
if(printPort == TRUE)
newLocation = apr_psprintf(r->pool, "%s://%s:%u%s%s%s", ap_http_scheme(r), r->server->server_hostname, r->connection->local_addr->port, r->uri, ((r->args != NULL) ? "?" : ""), ((r->args != NULL) ? r->args : ""));
else
newLocation = apr_psprintf(r->pool, "%s://%s%s%s%s", ap_http_scheme(r), r->server->server_hostname, r->uri, ((r->args != NULL) ? "?" : ""), ((r->args != NULL) ? r->args : ""));
#endif
}
if(c->CASDebug)
ap_log_rerror(APLOG_MARK,APLOG_DEBUG,0,r,"Sending 302 redirect (%s)", newLocation);
apr_table_add(r->headers_out, "Location", newLocation);
return HTTP_MOVED_TEMPORARILY;
} else {
return OK;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intended that we go all the way to https://github.com/ruckc/mod_auth_cas/blob/eba78415e60b2d597c5ee03afd5245f53129a15a/src/mod_auth_cas.c#L2231 to return OK? Should it return before that?

Copy link
Author

Choose a reason for hiding this comment

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

The return OK; was unmodified (except additional indention).

Copy link
Contributor

Choose a reason for hiding this comment

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

It's now inside the if block that checks CASDisableRedirectAfterValidation ( eba7841#diff-b823cf0e10152100b941acd0fb5838a8R2141 ), so it never returns if CASDisableRedirectAfterValidation is On.

Copy link
Author

Choose a reason for hiding this comment

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

I added an additional } else { return OK; }. In my office environment I had caught this but forgot to commit it back.

Copy link
Contributor

@dhawes dhawes Nov 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If that's the intention, this can be simplified by changing the check to:

if(parametersRemoved == TRUE && d->CASDisableRedirectAfterValidation == NULL )

I mostly ask because the original merge request would fall through to a later return where cas attributes were populated and headers were set. The side effect of this is that Require cas-attribute statements would work. In this commit that won't happen, so authorization will fail. What is desired here?

}
apr_table_add(r->headers_out, "Location", newLocation);
return HTTP_MOVED_TEMPORARILY;
} else {
return OK;
}
} else {
/* sometimes, pages that automatically refresh will re-send the ticket parameter, so let's check any cookies presented or return an error if none */
Expand Down Expand Up @@ -2827,6 +2845,7 @@ const command_rec cas_cmds [] = {
AP_INIT_TAKE1("CASTimeout", cfg_readCASParameter, (void *) cmd_session_timeout, RSRC_CONF, "Maximum time (in seconds) a session cookie is valid for, regardless of idle time. Set to 0 to allow non-idle sessions to never expire"),
AP_INIT_TAKE1("CASIdleTimeout", cfg_readCASParameter, (void *) cmd_idle_timeout, RSRC_CONF, "Maximum time (in seconds) a session can be idle for"),
AP_INIT_TAKE1("CASCacheCleanInterval", cfg_readCASParameter, (void *) cmd_cache_interval, RSRC_CONF, "Amount of time (in seconds) between cache cleanups. This value is checked when a new local ticket is issued or when a ticket expires."),
AP_INIT_TAKE1("CASDisableRedirectAfterValidation", ap_set_string_slot, (void *) APR_OFFSETOF(cas_dir_cfg, CASDisableRedirectAfterValidation), ACCESS_CONF, "Set 'Off' to not send a 302 redirect to remove the ticket parameter after ticket validation"),
dhawes marked this conversation as resolved.
Show resolved Hide resolved
AP_INIT_TAKE1("CASRootProxiedAs", cfg_readCASParameter, (void *) cmd_root_proxied_as, RSRC_CONF, "URL used to access the root of the virtual server (only needed when the server is proxied)"),
AP_INIT_TAKE1("CASScrubRequestHeaders", ap_set_string_slot, (void *) APR_OFFSETOF(cas_dir_cfg, CASScrubRequestHeaders), ACCESS_CONF, "Scrub CAS user name and SAML attribute headers from the user's request."),
/* authorization options */
Expand Down
4 changes: 3 additions & 1 deletion src/mod_auth_cas.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
#define CAS_DEFAULT_VALIDATE_V2_URL NULL
#define CAS_DEFAULT_VALIDATE_URL CAS_DEFAULT_VALIDATE_V2_URL
#define CAS_DEFAULT_PROXY_VALIDATE_URL NULL
#define CAS_DEFAULT_REDIRECT_AFTER_VALIDATION NULL
dhawes marked this conversation as resolved.
Show resolved Hide resolved
#define CAS_DEFAULT_ROOT_PROXIED_AS_URL NULL
#define CAS_DEFAULT_COOKIE_ENTROPY 32
#define CAS_DEFAULT_COOKIE_DOMAIN NULL
Expand Down Expand Up @@ -144,6 +145,7 @@ typedef struct cas_dir_cfg {
char *CASGatewayCookie;
char *CASAuthNHeader;
char *CASScrubRequestHeaders;
char *CASDisableRedirectAfterValidation;
} cas_dir_cfg;

typedef struct cas_cache_entry {
Expand All @@ -167,7 +169,7 @@ typedef enum {
cmd_loginurl, cmd_validateurl, cmd_proxyurl, cmd_cookie_entropy, cmd_session_timeout,
cmd_idle_timeout, cmd_cache_interval, cmd_cookie_domain, cmd_cookie_httponly,
cmd_sso, cmd_validate_saml, cmd_attribute_delimiter, cmd_attribute_prefix,
cmd_root_proxied_as, cmd_authoritative
cmd_root_proxied_as, cmd_authoritative, cmd_validate_after_redirect
dhawes marked this conversation as resolved.
Show resolved Hide resolved
} valid_cmds;

module AP_MODULE_DECLARE_DATA auth_cas_module;
Expand Down