I've got one last complaint about the backend GSS code: we are doing things randomly differently in the two places that install krb_server_keyfile as the active KRB5_KTNAME value. secure_open_gssapi() sets KRB5_KTNAME unconditionally (and doesn't bother to check for error, either, not a good thing in a security-critical operation). But the older code in pg_GSS_recvauth() is written to not override KRB5_KTNAME if it's already set. This of-course-totally-undocumented behavior seems like a fairly bad idea to me: as things stand, the client-side choice of whether to initiate GSS encryption or not could result in two different server keytabs being used. I think we'd be best off to always override KRB5_KTNAME if we have a nonempty krb_server_keyfile setting, so the attached proposed patch makes both functions do it the same way. (I did not make an effort to remove the dependency on setenv, given the nearby thread to standardize on that.) I'm not sure whether there's any documentation change that needs to be made. The docs don't suggest that you're allowed to set krb_server_keyfile to an empty string in the first place, so maybe we needn't explain what happens if you do. regards, tom lane
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index 515ae95fe1..681448da2c 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -1054,29 +1054,18 @@ pg_GSS_recvauth(Port *port) (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("GSSAPI is not supported in protocol version 2"))); - if (pg_krb_server_keyfile && strlen(pg_krb_server_keyfile) > 0) + /* + * Use the configured keytab, if there is one. Unfortunately, Heimdal + * doesn't support the cred store extensions, so use the env var. + */ + if (pg_krb_server_keyfile != NULL && pg_krb_server_keyfile[0] != '\0') { - /* - * Set default Kerberos keytab file for the Krb5 mechanism. - * - * setenv("KRB5_KTNAME", pg_krb_server_keyfile, 0); except setenv() - * not always available. - */ - if (getenv("KRB5_KTNAME") == NULL) + if (setenv("KRB5_KTNAME", pg_krb_server_keyfile, 1) != 0) { - size_t kt_len = strlen(pg_krb_server_keyfile) + 14; - char *kt_path = malloc(kt_len); - - if (!kt_path || - snprintf(kt_path, kt_len, "KRB5_KTNAME=%s", - pg_krb_server_keyfile) != kt_len - 2 || - putenv(kt_path) != 0) - { - ereport(LOG, - (errcode(ERRCODE_OUT_OF_MEMORY), - errmsg("out of memory"))); - return STATUS_ERROR; - } + /* The only likely failure cause is OOM, so report it that way */ + ereport(FATAL, + (errcode(ERRCODE_OUT_OF_MEMORY), + errmsg("out of memory"))); } } diff --git a/src/backend/libpq/be-secure-gssapi.c b/src/backend/libpq/be-secure-gssapi.c index 1747fccb14..e139870c27 100644 --- a/src/backend/libpq/be-secure-gssapi.c +++ b/src/backend/libpq/be-secure-gssapi.c @@ -525,8 +525,16 @@ secure_open_gssapi(Port *port) * Use the configured keytab, if there is one. Unfortunately, Heimdal * doesn't support the cred store extensions, so use the env var. */ - if (pg_krb_server_keyfile != NULL && strlen(pg_krb_server_keyfile) > 0) - setenv("KRB5_KTNAME", pg_krb_server_keyfile, 1); + if (pg_krb_server_keyfile != NULL && pg_krb_server_keyfile[0] != '\0') + { + if (setenv("KRB5_KTNAME", pg_krb_server_keyfile, 1) != 0) + { + /* The only likely failure cause is OOM, so report it that way */ + ereport(FATAL, + (errcode(ERRCODE_OUT_OF_MEMORY), + errmsg("out of memory"))); + } + } while (true) {