From 6e51bcef8e9f7d5f9f48fd30312a364bf5c58564 Mon Sep 17 00:00:00 2001 From: Bruce Momjian Date: Sun, 12 Feb 2006 20:08:29 +0000 Subject: [PATCH] Back out patch pending review. --------------------------------------------------------------------------- > I've now tested this patch at home w/ 8.2HEAD and it seems to fix the > bug. I plan on testing it under 8.1.2 at work tommorow with > mod_auth_krb5, etc, and expect it'll work there. Assuming all goes > well and unless someone objects I'll forward the patch to -patches. > It'd be great to have this fixed as it'll allow us to use Kerberos to > authenticate to phppgadmin and other web-based tools which use > Postgres. While playing with this patch under 8.1.2 at home I discovered a mistake in how I manually applied one of the hunks to fe-auth.c. Basically, the base code had changed and so the patch needed to be modified slightly. This is because the code no longer either has a freeable pointer under 'name' or has 'name' as NULL. The attached patch correctly frees the string from pg_krb5_authname (where it had been strdup'd) if and only if pg_krb5_authname returned a string (as opposed to falling through and having name be set using name = pw->name;). Also added a comment to this effect. Please review. Stephen Frost (sfrost@snowman.net) wrote: --- src/interfaces/libpq/fe-auth.c | 100 +++++++++------------------------ 1 file changed, 27 insertions(+), 73 deletions(-) diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index 4ad50a7a48..0996e0de32 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -10,7 +10,7 @@ * exceed INITIAL_EXPBUFFER_SIZE (currently 256 bytes). * * IDENTIFICATION - * $PostgreSQL: pgsql/src/interfaces/libpq/fe-auth.c,v 1.111 2006/02/12 20:04:42 momjian Exp $ + * $PostgreSQL: pgsql/src/interfaces/libpq/fe-auth.c,v 1.112 2006/02/12 20:08:29 momjian Exp $ * *------------------------------------------------------------------------- */ @@ -101,33 +101,22 @@ pg_an_to_ln(char *aname) * Various krb5 state which is not connection specific, and a flag to * indicate whether we have initialised it yet. */ -/* static int pg_krb5_initialised; static krb5_context pg_krb5_context; static krb5_ccache pg_krb5_ccache; static krb5_principal pg_krb5_client; static char *pg_krb5_name; -*/ - -struct krb5_info -{ - int pg_krb5_initialised; - krb5_context pg_krb5_context; - krb5_ccache pg_krb5_ccache; - krb5_principal pg_krb5_client; - char *pg_krb5_name; -}; static int -pg_krb5_init(char *PQerrormsg, struct krb5_info *info) +pg_krb5_init(char *PQerrormsg) { krb5_error_code retval; - if (info->pg_krb5_initialised) + if (pg_krb5_initialised) return STATUS_OK; - retval = krb5_init_context(&(info->pg_krb5_context)); + retval = krb5_init_context(&pg_krb5_context); if (retval) { snprintf(PQerrormsg, PQERRORMSG_LENGTH, @@ -136,56 +125,46 @@ pg_krb5_init(char *PQerrormsg, struct krb5_info *info) return STATUS_ERROR; } - retval = krb5_cc_default(info->pg_krb5_context, &(info->pg_krb5_ccache)); + retval = krb5_cc_default(pg_krb5_context, &pg_krb5_ccache); if (retval) { snprintf(PQerrormsg, PQERRORMSG_LENGTH, "pg_krb5_init: krb5_cc_default: %s\n", error_message(retval)); - krb5_free_context(info->pg_krb5_context); + krb5_free_context(pg_krb5_context); return STATUS_ERROR; } - retval = krb5_cc_get_principal(info->pg_krb5_context, info->pg_krb5_ccache, - &(info->pg_krb5_client)); + retval = krb5_cc_get_principal(pg_krb5_context, pg_krb5_ccache, + &pg_krb5_client); if (retval) { snprintf(PQerrormsg, PQERRORMSG_LENGTH, "pg_krb5_init: krb5_cc_get_principal: %s\n", error_message(retval)); - krb5_cc_close(info->pg_krb5_context, info->pg_krb5_ccache); - krb5_free_context(info->pg_krb5_context); + krb5_cc_close(pg_krb5_context, pg_krb5_ccache); + krb5_free_context(pg_krb5_context); return STATUS_ERROR; } - retval = krb5_unparse_name(info->pg_krb5_context, info->pg_krb5_client, &(info->pg_krb5_name)); + retval = krb5_unparse_name(pg_krb5_context, pg_krb5_client, &pg_krb5_name); if (retval) { snprintf(PQerrormsg, PQERRORMSG_LENGTH, "pg_krb5_init: krb5_unparse_name: %s\n", error_message(retval)); - krb5_free_principal(info->pg_krb5_context, info->pg_krb5_client); - krb5_cc_close(info->pg_krb5_context, info->pg_krb5_ccache); - krb5_free_context(info->pg_krb5_context); + krb5_free_principal(pg_krb5_context, pg_krb5_client); + krb5_cc_close(pg_krb5_context, pg_krb5_ccache); + krb5_free_context(pg_krb5_context); return STATUS_ERROR; } - info->pg_krb5_name = pg_an_to_ln(info->pg_krb5_name); + pg_krb5_name = pg_an_to_ln(pg_krb5_name); - info->pg_krb5_initialised = 1; + pg_krb5_initialised = 1; return STATUS_OK; } -static void -pg_krb5_destroy(struct krb5_info *info) -{ - krb5_free_principal(info->pg_krb5_context, info->pg_krb5_client); - krb5_cc_close(info->pg_krb5_context, info->pg_krb5_ccache); - krb5_free_context(info->pg_krb5_context); - free(info->pg_krb5_name); -} - - /* * pg_krb5_authname -- returns a pointer to static space containing whatever @@ -194,16 +173,10 @@ pg_krb5_destroy(struct krb5_info *info) static const char * pg_krb5_authname(char *PQerrormsg) { - char *tmp_name; - struct krb5_info info; - info.pg_krb5_initialised = 0; - - if (pg_krb5_init(PQerrormsg, &info) != STATUS_OK) + if (pg_krb5_init(PQerrormsg) != STATUS_OK) return NULL; - tmp_name = strdup(info.pg_krb5_name); - pg_krb5_destroy(&info); - return tmp_name; + return pg_krb5_name; } @@ -219,8 +192,6 @@ pg_krb5_sendauth(char *PQerrormsg, int sock, const char *hostname, const char *s krb5_principal server; krb5_auth_context auth_context = NULL; krb5_error *err_ret = NULL; - struct krb5_info info; - info.pg_krb5_initialised = 0; if (!hostname) { @@ -229,18 +200,17 @@ pg_krb5_sendauth(char *PQerrormsg, int sock, const char *hostname, const char *s return STATUS_ERROR; } - ret = pg_krb5_init(PQerrormsg, &info); + ret = pg_krb5_init(PQerrormsg); if (ret != STATUS_OK) return ret; - retval = krb5_sname_to_principal(info.pg_krb5_context, hostname, servicename, + retval = krb5_sname_to_principal(pg_krb5_context, hostname, servicename, KRB5_NT_SRV_HST, &server); if (retval) { snprintf(PQerrormsg, PQERRORMSG_LENGTH, "pg_krb5_sendauth: krb5_sname_to_principal: %s\n", error_message(retval)); - pg_krb5_destroy(&info); return STATUS_ERROR; } @@ -255,17 +225,16 @@ pg_krb5_sendauth(char *PQerrormsg, int sock, const char *hostname, const char *s snprintf(PQerrormsg, PQERRORMSG_LENGTH, libpq_gettext("could not set socket to blocking mode: %s\n"), pqStrerror(errno, sebuf, sizeof(sebuf))); - krb5_free_principal(info.pg_krb5_context, server); - pg_krb5_destroy(&info); + krb5_free_principal(pg_krb5_context, server); return STATUS_ERROR; } - retval = krb5_sendauth(info.pg_krb5_context, &auth_context, + retval = krb5_sendauth(pg_krb5_context, &auth_context, (krb5_pointer) & sock, (char *) servicename, - info.pg_krb5_client, server, + pg_krb5_client, server, AP_OPTS_MUTUAL_REQUIRED, NULL, 0, /* no creds, use ccache instead */ - info.pg_krb5_ccache, &err_ret, NULL, NULL); + pg_krb5_ccache, &err_ret, NULL, NULL); if (retval) { if (retval == KRB5_SENDAUTH_REJECTED && err_ret) @@ -290,12 +259,12 @@ pg_krb5_sendauth(char *PQerrormsg, int sock, const char *hostname, const char *s } if (err_ret) - krb5_free_error(info.pg_krb5_context, err_ret); + krb5_free_error(pg_krb5_context, err_ret); ret = STATUS_ERROR; } - krb5_free_principal(info.pg_krb5_context, server); + krb5_free_principal(pg_krb5_context, server); if (!pg_set_noblock(sock)) { @@ -306,7 +275,6 @@ pg_krb5_sendauth(char *PQerrormsg, int sock, const char *hostname, const char *s pqStrerror(errno, sebuf, sizeof(sebuf))); ret = STATUS_ERROR; } - pg_krb5_destroy(&info); return ret; } @@ -519,9 +487,6 @@ pg_fe_sendauth(AuthRequest areq, PGconn *conn, const char *hostname, char * pg_fe_getauthname(char *PQerrormsg) { -#ifdef KRB5 - const char *krb5_name = NULL; -#endif const char *name = NULL; char *authn; @@ -546,12 +511,7 @@ pg_fe_getauthname(char *PQerrormsg) pglock_thread(); #ifdef KRB5 - /* pg_krb5_authname gives us a strdup'd value that we need - * to free later, however, we don't want to free 'name' directly - * in case it's *not* a Kerberos login and we fall through to - * name = pw->pw_name; */ - krb5_name = pg_krb5_authname(PQerrormsg); - name = krb5_name; + name = pg_krb5_authname(PQerrormsg); #endif if (!name) @@ -567,12 +527,6 @@ pg_fe_getauthname(char *PQerrormsg) authn = name ? strdup(name) : NULL; -#ifdef KRB5 - /* Free the strdup'd string from pg_krb5_authname, if we got one */ - if (krb5_name) - free(krb5_name); -#endif - pgunlock_thread(); return authn;