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
