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

IdP Initiate SLO Validate SessionIndex #141

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

IdP Initiate SLO Validate SessionIndex #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 15 additions & 3 deletions auth_mellon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,8 @@ typedef struct am_cache_entry_t {
typedef enum {
AM_CACHE_SESSION,
AM_CACHE_NAMEID,
AM_CACHE_ASSERTIONID
AM_CACHE_ASSERTIONID,
AM_CACHE_SESSIONINDEX
} am_cache_key_t;

/* Type for configuring environment variable names */
Expand Down Expand Up @@ -472,6 +473,8 @@ am_cache_entry_t *am_cache_new(request_rec *r,
const char *key,
const char *cookie_token);
void am_cache_unlock(request_rec *r, am_cache_entry_t *entry);
bool am_cache_mutex_lock(request_rec *r);
void am_cache_mutex_unlock(request_rec *r);

void am_cache_update_expires(request_rec *r, am_cache_entry_t *t, apr_time_t expires);
void am_cache_update_idle_timeout(request_rec *r, am_cache_entry_t *t, int session_idle_timeout);
Expand All @@ -482,24 +485,33 @@ int am_cache_env_append(am_cache_entry_t *session,
const char *am_cache_env_fetch_first(am_cache_entry_t *t,
const char *var);
void am_cache_delete(request_rec *r, am_cache_entry_t *session);
void am_cache_delete_sessions(request_rec *r, apr_array_header_t *sessions);

int am_cache_set_lasso_state(am_cache_entry_t *session,
const char *lasso_identity,
const char *lasso_session,
const char *lasso_saml_response);
const char *am_cache_get_lasso_identity(am_cache_entry_t *session);
const char *am_cache_get_lasso_session(am_cache_entry_t *session);

am_cache_entry_t *am_cache_get_session(request_rec *r,
am_cache_key_t type,
const char *key);
apr_array_header_t *am_cache_get_sessions(request_rec *r,
am_cache_key_t type,
const char *key);

am_cache_entry_t *am_get_request_session(request_rec *r);
am_cache_entry_t *am_get_request_session_by_nameid(request_rec *r,
char *nameid);
am_cache_entry_t *am_get_request_session_by_assertionid(request_rec *r,
char *assertionid);
apr_array_header_t *am_get_request_sessions_by_sessionindex(request_rec *r, GList *sessionindex);
apr_array_header_t *am_get_request_sessions_by_nameid(request_rec *r, char *nameid);
am_cache_entry_t *am_new_request_session(request_rec *r);
void am_release_request_session(request_rec *r, am_cache_entry_t *session);
void am_delete_request_session(request_rec *r, am_cache_entry_t *session);

void am_delete_request_sessions(request_rec *r, apr_array_header_t *sessions);
bool am_validate_session_cookie_token(request_rec *r, am_cache_entry_t *session);

char *am_reconstruct_url(request_rec *r);
int am_validate_redirect_url(request_rec *r, const char *url);
Expand Down
212 changes: 189 additions & 23 deletions auth_mellon_cache.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,38 @@ void am_cache_init(am_mod_cfg_rec *mod_cfg)
am_cache_entry_t *am_cache_lock(request_rec *r,
am_cache_key_t type,
const char *key)
{
/* Lock the table. */
if (!am_cache_mutex_lock(r)) {
return NULL;
}

am_cache_entry_t *e = am_cache_get_session(r, type, key);
if (e == NULL) {
am_cache_mutex_unlock(r);
}
return e;
}

/* This function locates a session entry.
* The table must be locked before calling this function.
*
* Parameters:
* request_rec *r The request we are processing.
* am_cache_key_t type AM_CACHE_SESSION, AM_CACHE_NAMEID or AM_CACHE_ASSERTIONID
* or AM_CACHE_SESSIONINDEX
* const char *key The session key or user
*
* Returns:
* The session entry on success or NULL on failure.
*/
am_cache_entry_t *am_cache_get_session(request_rec *r,
am_cache_key_t type,
const char *key)
{
am_mod_cfg_rec *mod_cfg;
void *table;
apr_size_t i;
int rv;
char buffer[512];


/* Check if we have a valid session key. We abort if we don't. */
Expand All @@ -99,23 +125,111 @@ am_cache_entry_t *am_cache_lock(request_rec *r,
break;
case AM_CACHE_NAMEID:
case AM_CACHE_ASSERTIONID:
case AM_CACHE_SESSIONINDEX:
break;
default:
return NULL;
break;
}

mod_cfg = am_get_mod_cfg(r->server);
table = apr_shm_baseaddr_get(mod_cfg->cache);


/* Lock the table. */
if((rv = apr_global_mutex_lock(mod_cfg->lock)) != APR_SUCCESS) {
AM_LOG_RERROR(APLOG_MARK, APLOG_ERR, 0, r,
"apr_global_mutex_lock() failed [%d]: %s",
rv, apr_strerror(rv, buffer, sizeof(buffer)));
for(i = 0; i < mod_cfg->init_cache_size; i++) {
am_cache_entry_t *e = am_cache_entry_ptr(mod_cfg, table, i);
const char *tablekey;

if (e->key[0] == '\0') {
/* This entry is empty. Skip it. */
continue;
}

switch (type) {
case AM_CACHE_SESSION:
tablekey = e->key;
break;
case AM_CACHE_NAMEID:
/* tablekey may be NULL */
tablekey = am_cache_env_fetch_first(e, "NAME_ID");
break;
case AM_CACHE_ASSERTIONID:
/* tablekey may be NULL */
tablekey = am_cache_env_fetch_first(e, "ASSERTION_ID");
break;
case AM_CACHE_SESSIONINDEX:
/* tablekey may be NULL */
tablekey = am_cache_env_fetch_first(e, "SESSIONINDEX");
break;
default:
tablekey = NULL;
break;
}

if (tablekey == NULL)
continue;

if(strcmp(tablekey, key) == 0) {
apr_time_t now = apr_time_now();
/* We found the entry. */
if ((e->expires > now) &&
((e->idle_timeout == -1) ||
(e->idle_timeout > now))) {
/* And it hasn't expired. */
return e;
}
else {
am_diag_log_cache_entry(r, 0, e,
"found expired session, now %s\n",
am_diag_time_t_to_8601(r, now));
}
}
}


return NULL;
}

/* This function locates a session entries.
* The table must be locked before calling this function.
*
* Parameters:
* request_rec *r The request we are processing.
* am_cache_key_t type AM_CACHE_SESSION, AM_CACHE_NAMEID or AM_CACHE_ASSERTIONID
* or AM_CACHE_SESSIONINDEX
* const char *key The session key or user
*
* Returns:
* The session entries on success or NULL on failure.
*/
apr_array_header_t *am_cache_get_sessions(request_rec *r,
am_cache_key_t type,
const char *key)
{
am_mod_cfg_rec *mod_cfg;
void *table;
apr_size_t i;
apr_array_header_t *sessions = apr_array_make(r->pool, 0, sizeof(am_cache_entry_t *));

/* Check if we have a valid session key. We abort if we don't. */
if (key == NULL)
return NULL;

switch (type) {
case AM_CACHE_SESSION:
if (strlen(key) != AM_ID_LENGTH)
return NULL;
break;
case AM_CACHE_NAMEID:
case AM_CACHE_ASSERTIONID:
case AM_CACHE_SESSIONINDEX:
break;
default:
return NULL;
break;
}

mod_cfg = am_get_mod_cfg(r->server);
table = apr_shm_baseaddr_get(mod_cfg->cache);


Expand All @@ -140,6 +254,10 @@ am_cache_entry_t *am_cache_lock(request_rec *r,
/* tablekey may be NULL */
tablekey = am_cache_env_fetch_first(e, "ASSERTION_ID");
break;
case AM_CACHE_SESSIONINDEX:
/* tablekey may be NULL */
tablekey = am_cache_env_fetch_first(e, "SESSIONINDEX");
break;
default:
tablekey = NULL;
break;
Expand All @@ -155,7 +273,7 @@ am_cache_entry_t *am_cache_lock(request_rec *r,
((e->idle_timeout == -1) ||
(e->idle_timeout > now))) {
/* And it hasn't expired. */
return e;
APR_ARRAY_PUSH(sessions, am_cache_entry_t *) = e;
}
else {
am_diag_log_cache_entry(r, 0, e,
Expand All @@ -164,15 +282,10 @@ am_cache_entry_t *am_cache_lock(request_rec *r,
}
}
}


/* We didn't find a entry matching the key. Unlock the table and
* return NULL;
*/
apr_global_mutex_unlock(mod_cfg->lock);
return NULL;
return sessions;
}


static inline bool am_cache_entry_slot_is_empty(am_cache_storage_t *slot)
{
return (slot->ptr == 0);
Expand Down Expand Up @@ -304,7 +417,6 @@ am_cache_entry_t *am_cache_new(request_rec *r,
int i;
apr_time_t age;
int rv;
char buffer[512];

/* Check if we have a valid session key. We abort if we don't. */
if(key == NULL || strlen(key) != AM_ID_LENGTH) {
Expand All @@ -316,10 +428,7 @@ am_cache_entry_t *am_cache_new(request_rec *r,


/* Lock the table. */
if((rv = apr_global_mutex_lock(mod_cfg->lock)) != APR_SUCCESS) {
AM_LOG_RERROR(APLOG_MARK, APLOG_ERR, 0, r,
"apr_global_mutex_lock() failed [%d]: %s",
rv, apr_strerror(rv, buffer, sizeof(buffer)));
if (!am_cache_mutex_lock(r)) {
return NULL;
}

Expand Down Expand Up @@ -415,7 +524,7 @@ am_cache_entry_t *am_cache_new(request_rec *r,
AM_LOG_RERROR(APLOG_MARK, APLOG_ERR, 0, r,
"Unable to store cookie token in new session.");
t->key[0] = '\0'; /* Mark the entry as free. */
apr_global_mutex_unlock(mod_cfg->lock);
am_cache_mutex_unlock(r);
return NULL;
}

Expand All @@ -439,11 +548,45 @@ am_cache_entry_t *am_cache_new(request_rec *r,
*/
void am_cache_unlock(request_rec *r, am_cache_entry_t *entry)
{
am_mod_cfg_rec *mod_cfg;

/* Update access time. */
entry->access = apr_time_now();

am_cache_mutex_unlock(r);
}

/* This function mutex lock
*
* Parameters:
* request_rec *r The request we are processing.
*
* Returns:
* true on success or false on failure.
*/
bool am_cache_mutex_lock(request_rec *r)
{
am_mod_cfg_rec *mod_cfg = am_get_mod_cfg(r->server);
int rv;
char buffer[512];
if((rv = apr_global_mutex_lock(mod_cfg->lock)) != APR_SUCCESS) {
AM_LOG_RERROR(APLOG_MARK, APLOG_ERR, 0, r,
"apr_global_mutex_lock() failed [%d]: %s",
rv, apr_strerror(rv, buffer, sizeof(buffer)));
return false;
}
return true;
}

/* This function mutext unlocks
*
* Parameters:
* request_rec *r The request we are processing.
*
* Returns:
* Nothing.
*/
void am_cache_mutex_unlock(request_rec *r)
{
am_mod_cfg_rec *mod_cfg;
mod_cfg = am_get_mod_cfg(r->server);
apr_global_mutex_unlock(mod_cfg->lock);
}
Expand Down Expand Up @@ -758,6 +901,29 @@ void am_cache_delete(request_rec *r, am_cache_entry_t *cache)
am_cache_unlock(r, cache);
}

/* This function deletes the specified keys from the session store
* and releases the locks.
* Equivalent to performing am_cache_delete on multiple sessions.
*
* Parameters:
* request_rec *r The request we are processing.
* apr_array_header_t *sessions The entry we are deleting.
*
* Returns:
* Nothing.
*/
void am_cache_delete_sessions(request_rec *r, apr_array_header_t *sessions)
{
am_cache_entry_t *cache;
int i;
for (i = 0; i < sessions->nelts; i++) {
cache = APR_ARRAY_IDX(sessions, i, am_cache_entry_t *);
am_diag_log_cache_entry(r, 0, cache, "delete session");
cache->key[0] = '\0';
cache->access = apr_time_now();
}
am_cache_mutex_unlock(r);
Copy link
Member

Choose a reason for hiding this comment

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

this is bad practice ... hiding a lock this deep (two nested) into a utility function is a sure way to get bugs later as it will be very difficult to remember and figure out.

As a general rule locking and unlocking should happen in the same calling function, so that it is very explicit what is the context of the locked code, and all error conditions can be properly checked.

Please rework this code to get the unlock out of this call and up a few levels.

Copy link
Author

Choose a reason for hiding this comment

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

I agree. However there is a reason why I decided on this code.
The code before modification was to lock with am_get_request_session_nameid() and then unlock with am_delete_request_session().
I tried to make it consistent with the existing code.

I couldn't come up with a good fix, so I need someone else to make a better fix.

}

/* This function stores a lasso identity dump and a lasso session dump in
* the given session object.
Expand Down
6 changes: 6 additions & 0 deletions auth_mellon_diagnostics.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,7 @@ am_diag_cache_key_type_str(am_cache_key_t key_type)
case AM_CACHE_SESSION: return "session";
case AM_CACHE_NAMEID: return "name id";
case AM_CACHE_ASSERTIONID: return "assertion id";
case AM_CACHE_SESSIONINDEX: return "SessionIndex";
default: return "unknown";
}
}
Expand Down Expand Up @@ -1111,6 +1112,7 @@ am_diag_log_cache_entry(request_rec *r, int level, am_cache_entry_t *entry,

const char *name_id = NULL;
const char *assertion_id = NULL;
const char *sessionindex = NULL;

if (!AM_DIAG_ENABLED(diag_cfg)) return;
if (!am_diag_initialize_req(r, diag_cfg, req_cfg)) return;
Expand All @@ -1122,6 +1124,7 @@ am_diag_log_cache_entry(request_rec *r, int level, am_cache_entry_t *entry,
if (entry) {
name_id = am_cache_env_fetch_first(entry, "NAME_ID");
assertion_id = am_cache_env_fetch_first(entry, "ASSERTION_ID");
sessionindex = am_cache_env_fetch_first(entry, "SESSIONINDEX");

apr_file_printf(diag_cfg->fd,
"%skey: %s\n",
Expand All @@ -1132,6 +1135,9 @@ am_diag_log_cache_entry(request_rec *r, int level, am_cache_entry_t *entry,
apr_file_printf(diag_cfg->fd,
"%sassertion_id: %s\n",
indent(level+1), assertion_id);
apr_file_printf(diag_cfg->fd,
"%ssessionindex: %s\n",
indent(level+1), sessionindex);
apr_file_printf(diag_cfg->fd,
"%sexpires: %s\n",
indent(level+1),
Expand Down
Loading