#48 already_succeeded prevents KrbSaveCredentials to work

Version 5.*
open
nobody
5
2009-07-07
2009-07-07
Anonymous
No

This bug is pretty obvious and I can't suggest any work around other than let the use of already_succeeded be optional or to keep the ticket after the request.

The use of Negotiate should solve this issue on kerberized browsers.

Discussion

  • pj101
    pj101
    2010-01-07

    In order to resolve the problem there are two possibilities:
    1. Disable the already_succeed optimisation in the case when " KrbSaveCredentials On"
    2. Find a way to save the credentials cache in case already_succeed is valid and re-populate the new credentials cache from this backup.

    The first solution is very simple :
    --- mod_auth_kerb.c 2008-12-04 11:14:03.000000000 +0100
    +++ ../../mod_auth_kerb-5.4-patched.c 2010-01-07 10:25:28.000000000 +0100
    @@ -1670,7 +1670,7 @@
    (strcasecmp(auth_type, "Basic") == 0))
    return DECLINED;

    - if ( (prevauth = already_succeeded(r, auth_line)) == NULL) {
    + if ( ((prevauth = already_succeeded(r, auth_line)) == NULL) || (conf->krb_save_credentials)) {
    ret = HTTP_UNAUTHORIZED;

    #ifdef KRB5

     
  • Benjamin Kahn
    Benjamin Kahn
    2011-03-16

    The second solution sounds more correct. I have a patch which solves the problem. It is here:
    https://sourceforge.net/tracker/?func=detail&aid=3216656&group_id=51775&atid=464526

    --- mod_auth_kerb-5.4/src/mod_auth_kerb.c 2008-12-04 05:14:03.000000000 -0500
    +++ mod_auth_kerb-5.4-new/src/mod_auth_kerb.c 2011-03-15 16:55:07.067521720 -0400
    @@ -176,6 +176,7 @@
    char *authline;
    char *user;
    char *mech;
    + char *ccname;
    int last_return;
    } krb5_conn_data;

    @@ -869,7 +870,7 @@
    }

    apr_table_setn(r->subprocess_env, "KRB5CCNAME", ccname);
    - apr_pool_cleanup_register(r->pool, ccname, krb5_cache_cleanup,
    + apr_pool_cleanup_register(r->connection->pool, ccname, krb5_cache_cleanup,
    apr_pool_cleanup_null);

    *ccache = tmp_ccache;
    @@ -1696,6 +1702,8 @@
    ret = prevauth->last_return;
    MK_USER = prevauth->user;
    MK_AUTH_TYPE = prevauth->mech;
    + if (prevauth->ccname)
    + apr_table_setn(r->subprocess_env, "KRB5CCNAME", prevauth->ccname);
    }

    /*
    @@ -1706,7 +1714,8 @@
    prevauth->user = apr_pstrdup(r->connection->pool, MK_USER);
    prevauth->authline = apr_pstrdup(r->connection->pool, auth_line);
    prevauth->mech = apr_pstrdup(r->connection->pool, auth_type);
    + prevauth->ccname = apr_pstrdup(r->connection->pool, apr_table_get(r->subprocess_env, "KRB5CCNAME"));
    prevauth->last_return = ret;
    snprintf(keyname, sizeof(keyname) - 1,
    "mod_auth_kerb::connection::%s::%ld",
    r->connection->remote_ip, r->connection->id);

     
  • mgbowman
    mgbowman
    2011-09-09

    I agree that the second solution is more correct. Considering that apache and modern browsers are configured by default to support Keep-Alive, we should see a performance gain if we keep the cached credentials around for the lifetime of the connection instead of only the request.

    @xkahn: I was affected by this bug yesterday and started playing with your patch but there were 2 minor issues.

    1) The ccname was being allocated in the request pool instead of the connection pool which was leading to intermittent cleanup issues. Sometimes the cc file was cleaned up - other times is was not.

    2) The check is already_succeeded(...) was failing everytime with Negotiate auth due to the fact the Authorization head always changes per request even over the same connection. I changed the check to test for readability of the cc file.

    I fixed these two issues and preliminary testing shows proper handling / cleanup of both Basic and Negotiate authentication.

    Index: libapache-mod-auth-kerb-5.4/src/mod_auth_kerb.c

    --- libapache-mod-auth-kerb-5.4.orig/src/mod_auth_kerb.c 2011-09-08 13:41:49.000000000 +0300
    +++ libapache-mod-auth-kerb-5.4/src/mod_auth_kerb.c 2011-09-08 13:53:07.000000000 +0300
    @@ -176,6 +176,7 @@
    char *authline;
    char *user;
    char *mech;
    + char *ccname;
    int last_return;
    } krb5_conn_data;

    @@ -839,7 +840,7 @@
    int ret;
    krb5_ccache tmp_ccache = NULL;

    - ccname = apr_psprintf(r->pool, "FILE:%s/krb5cc_apache_XXXXXX", P_tmpdir);
    + ccname = apr_psprintf(r->connection->pool, "FILE:%s/krb5cc_apache_XXXXXX", P_tmpdir);
    fd = mkstemp(ccname + strlen("FILE:"));
    if (fd < 0) {
    log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
    @@ -869,7 +870,7 @@
    }

    apr_table_setn(r->subprocess_env, "KRB5CCNAME", ccname);
    - apr_pool_cleanup_register(r->pool, ccname, krb5_cache_cleanup,
    + apr_pool_cleanup_register(r->connection->pool, ccname, krb5_cache_cleanup,
    apr_pool_cleanup_null);

    *ccache = tmp_ccache;
    @@ -1553,6 +1554,7 @@
    {
    krb5_conn_data *conn_data;
    char keyname[1024];
    + FILE* fd = NULL;

    snprintf(keyname, sizeof(keyname) - 1,
    "mod_auth_kerb::connection::%s::%ld", r->connection->remote_ip,
    @@ -1561,9 +1563,11 @@
    if (apr_pool_userdata_get((void**)&conn_data, keyname, r->connection->pool) != 0)
    return NULL;

    - if(conn_data) {
    - if(strcmp(conn_data->authline, auth_line) == 0) {
    + if(conn_data && conn_data->ccname != NULL) {
    + fd = fopen(conn_data->ccname + strlen("FILE:"), "r");
    + if (fd != NULL) {
    log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, "matched previous auth request");
    + fclose(fd);
    return conn_data;
    }
    }
    @@ -1696,6 +1700,8 @@
    ret = prevauth->last_return;
    MK_USER = prevauth->user;
    MK_AUTH_TYPE = prevauth->mech;
    + if (prevauth->ccname)
    + apr_table_setn(r->subprocess_env, "KRB5CCNAME", prevauth->ccname);
    }

    /*
    @@ -1706,6 +1712,7 @@
    prevauth->user = apr_pstrdup(r->connection->pool, MK_USER);
    prevauth->authline = apr_pstrdup(r->connection->pool, auth_line);
    prevauth->mech = apr_pstrdup(r->connection->pool, auth_type);
    + prevauth->ccname = apr_pstrdup(r->connection->pool, apr_table_get(r->subprocess_env, "KRB5CCNAME"));
    prevauth->last_return = ret;
    snprintf(keyname, sizeof(keyname) - 1,
    "mod_auth_kerb::connection::%s::%ld",