Re: Kerberos delegation support in libpq and postgres_fdw

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested

Did a code review pass here; here is some feedback.

+ /* If password was used to connect, make sure it was one provided */
+ if (PQconnectionUsedPassword(conn) && dblink_connstr_has_pw(connstr))
+ return;

Do we need to consider whether these passwords are the same? Is there a different vector where a different password could be acquired from a different source (PGPASSWORD, say) while both of these criteria are true? Seems like it probably doesn't matter that much considering we only checked Password alone in previous version of this code.

---

Looks like the pg_gssinfo struct hides the `proxy_creds` def behind:

#if defined(ENABLE_GSS) | defined(ENABLE_SSPI)
typedef struct
{
gss_buffer_desc outbuf; /* GSSAPI output token buffer */
#ifdef ENABLE_GSS
...
bool proxy_creds; /* GSSAPI Delegated/proxy credentials */
#endif
} pg_gssinfo;
#endif

Which means that the later check in `be_gssapi_get_proxy()` we have:

/*
* Return if GSSAPI delegated/proxy credentials were included on this
* connection.
*/
bool
be_gssapi_get_proxy(Port *port)
{
if (!port || !port->gss)
return NULL;

return port->gss->proxy_creds;
}

So in theory it'd be possible to have SSPI enabled but GSS disabled and we'd fail to compile in that case. (It may be that this routine is never *actually* called in that case, just noting compile-time considerations.) I'm not seeing guards in the actual PQ* routines, but don't think I've done an exhaustive search.

---

gss_accept_deleg

+ <para>
+ Forward (delegate) GSS credentials to the server. The default is
+ <literal>disable</literal> which means credentials will not be forwarded
+ to the server. Set this to <literal>enable</literal> to have
+ credentials forwarded when possible.

When is this not possible? Server policy? External factors?

---

</para>
<para>
Only superusers may connect to foreign servers without password
- authentication, so always specify the <literal>password</literal> option
- for user mappings belonging to non-superusers.
+ authentication or using gssapi proxied credentials, so specify the
+ <literal>password</literal> option for user mappings belonging to
+ non-superusers who are not able to proxy GSSAPI credentials.
</para>
<para>

s/gssapi/GSSAPI/; this is kind of confusing, as this makes it sound like only superuser may use GSSAPI proxied credentials, which I disbelieve to be true. Additionally, it sounds like you're wanting to explicitly maintain a denylist for users to not be allowed proxying; is that correct?

---

libpq/auth.c:

if (proxy != NULL)
{
pg_store_proxy_credential(proxy);
port->gss->proxy_creds = true;
}

Per GSS docs, seems like we should be comparing to GSS_C_NO_CREDENTIAL and validating that the gflags has the `deleg_flag` bit set before considering whether there are valid credentials; in practice this might be the same effect (haven't looked at what that symbol actually resolves to, but NULL would be sensible).

Are there other cases we might need to consider here, like valid credentials, but they are expired? (GSS_S_CREDENTIALS_EXPIRED)

---

+ /*
+ * Set KRB5CCNAME for this backend, so that later calls to gss_acquire_cred
+ * will find the proxied credentials we stored.
+ */

So I'm not seeing this in other use in the code; I assume this is just used by the krb5 libs?

Similar q's for the other places the pg_gss_accept_deleg are used.

---

+int
+PQconnectionUsedGSSAPI(const PGconn *conn)
+{
+ if (!conn)
+ return false;
+ if (conn->gssapi_used)
+ return true;
+ else
+ return false;
+}

Micro-gripe: this routine seems like could be simpler, though the compiler probably has the same thing to say for either, so maybe code clarity is better as written:

int
PQconnectionUsedGSSAPI(const PGconn *conn)
{
return conn && conn->gssapi_used;
}

---

Anything required for adding meson support? I notice src/test/kerberos has Makefile updated, but no meson.build files are changed.

---

Two tests in src/test/kerberos/t/001_auth.pl at :535 and :545 have the same test description:

+ 'succeeds with GSS-encrypted access required and hostgssenc hba and credentials not forwarded',

Since the first test has only `gssencmode` defined (so implicit `gssdeleg` value) and the second has `gssdeleg=disable` I'd suggest that the test on :545 should have its description updated to add the word "explicitly":

'succeeds with GSS-encrypted access required and hostgssenc hba and credentials explicitly not forwarded',

---

In the dblink test, this seems like debugging junk:

+print ("$psql_out");
+print ("$psql_stderr");

Whacking those lines and reviewing the surrounding code block: so this is testing that dblink won't use `.pgpass`; so is this a behavior change, and dblink could be previously used w/postgres user's .pgpass file? I assume since this patch is forbidding this, we've decided that that is a bad idea--was this updated in the docs to note that this is now forbidden, or is this something that should only apply in some cases (i.e., this is now config-specific)? If config-specific, should we have a test in the non-forwarded version of these tests that exercises that behavior?

+$psql_rc = $node->psql(
+ 'postgres',
+ "SELECT * FROM dblink('user=test2 dbname=$dbname port=$port passfile=$pgpass','select 1') as t1(c1 int);",
+ connstr => "user=test1 host=$host hostaddr=$hostaddr gssencmode=require gssdeleg=disable",
+ stdout => \$psql_out,

+is($psql_rc,'3','dblink does not work without proxied credentials and with passfile');
+like($psql_stderr, qr/password or gssapi proxied credentials required/,'dblink does not work without proxied credentials and with passfile');

Same Q's apply to the postgres_fdw version of these tests.

---

:659 and :667, the test description says non-encrypted and the gssencmode=prefer implies encrypted; seems like those descriptions might need to be updated, since it seems like what it's really testing is dblink/postgres_fdw against gssdeleg=enabled. Also looks like later tests are explicitly testing w/gssencmode=require so making me think this is mislabeled further.

---

This is what I noticed on an initial pass-through.

Best,

David

The new status of this patch is: Waiting on Author