Re: Kerberos delegation support in libpq and postgres_fdw - Mailing list pgsql-hackers

Greetings,

* David Christensen (david+pg@pgguru.net) wrote:
> Did a code review pass here; here is some feedback.

Thanks!

> +     /* 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.

Note that this patch isn't really changing how these checks are being
done but more moving them around and allowing a GSSAPI-based approach
with credential delegation to also be allowed.

That said, as noted in the comments above dblink_connstr_check():

 * For non-superusers, insist that the connstr specify a password, except
 * if GSSAPI credentials have been proxied (and we check that they are used
 * for the connection in dblink_security_check later).  This prevents a
 * password or GSSAPI credentials from being picked up from .pgpass, a
 * service file, the environment, etc.  We don't want the postgres user's
 * passwords or Kerberos credentials to be accessible to non-superusers.

The point of these checks is, indeed, to ensure that environmental
values such as a .pgpass or variable don't end up getting picked up and
used (or, if they do, we realize it post-connection and then throw away
the connection).

libpq does explicitly prefer to use the password passed in as part of
the connection string and won't attempt to look up passwords in a
.pgpass file or similar if a password has been included in the
connection string.

> 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

... right, proxy_creds only exists (today anyway) if ENABLE_GSS is set.

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

Fairly confident the analysis here is wrong, further, the cfbot seems to
agree that there isn't a compile failure here:

https://cirrus-ci.com/task/6589717672624128

[20:19:15.985]     gss                    : NO

(we always build with SSPI on Windows, per
src/include/port/win32_port.h).

>     </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?

Updated to GSSAPI and reworded in the updated patch (attached).
Certainly open to suggestions on how to improve the documentation here.
There is no 'denylist' for users when it comes to GSSAPI proxied
credentials.  If there's a use-case for that then it could be added in
the future.

> ---
>
> 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).

GSS_C_NO_CREDENTIAL is indeed NULL, but updated to that anyway to be a
bit cleaner and also added an explicit check that GSS_C_DELEG_FLAG was
set in gflags.

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

Short answer is no, I don't believe we need to.  We shouldn't actually
get any expired credentials but even if we did, worst is that we'd end
up storing them and they wouldn't be able to be used because they're
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?

Not sure I'm following.  gss_acquire_cred() is called in
src/interfaces/libpq/fe-gssapi-common.c.

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

pg_gss_accept_deleg is checked in the two paths where we could have
credentials delegated to us- either through the encrypted-GSSAPI
connection path in libpq/be-secure-gssapi.c, or the
not-using-GSSAPI-encryption path in libpq/auth.c.

> ---
>
> +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;
>     }

I tend to disagree- explicitly returning true/false seems a bit clearer
to me and is also in-line with what other functions in
libpq/fe-connect.c are doing.  Having this function be different from,
eg, PQconnectionUsedPassword, would probably end up having more
questions about why they're different.  Either way, I'd say we change
both or neither and that doesn't really need to be part of this patch.

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

Short answer is- I don't think so (happy to be told I'm wrong though, if
someone wants to tell me what's wrong).  The other src/test modules that
have EXTRA_INSTALL lines don't have anything for those in the
meson.build, so I'm guessing the assumption is that everything is built
when using meson.

> ---
>
> 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',

Sure, updated.

> ---
>
> In the dblink test, this seems like debugging junk:
>
> +print ("$psql_out");
> +print ("$psql_stderr");

Ah, yeah, removed.

> 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?

Yes, that's what is being tested, but non-superuser dblink already won't
use a .pgpass file if it exists, so it's not a behavior change.  I added
explicit tests here though to make sure that even a dblink connection
created without a password being used in the connection string (because
GSSAPI credentials were proxied) won't end up using the .pgpass file.

Additional tests could perhaps be added to dblink itself (don't know
that we really need to hide those tests under src/test/kerberos) to make
sure that it's not going to use the .pgpass file; I'm not sure why that
wasn't done previously (it was done for postgres_fdw though and the
approach in each is basically the same...).