Re: Kerberos delegation support in libpq and postgres_fdw

On Wed, Apr 5, 2023 at 3:30 PM Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> Greetings,
>
> * David Christensen (david+pg(at)pgguru(dot)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.
>

The case I think I was thinking of was (manufactured) when we connected to
a backend with one password but the dblink or postgresql_fdw includes an
explicit password to a different server. But now I'm thinking that this
PQconnectionUsedPassword() is checking the outgoing connection for dblink
itself, not the connection of the backend that connected to the main
server, so I think this objection is moot, like you say.

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

[analysis snipped]

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

Cool; since we have coverage for that case seems like my concern was
unwarranted.

[snip]

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

Okay, I think your revisions here seem more clear, thanks.

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

+ proxy = NULL;
[...]
+ if (proxy != GSS_C_NO_CREDENTIAL && gflags & GSS_C_DELEG_FLAG)

We should probably also initialize "proxy" to GSS_C_NO_CREDENTIAL as well,
yes?

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

Okay.

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

I just meant the KRB5CCNAME envvar itself; looks like my assumption was
right.

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

Sounds good.

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

Fair points; we should presumably optimize for comprehension.

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

Okay, just validating.

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

Thanks.

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

I'm fine with that being out-of-scope for this patch and agreed there are
more appropriate places for this one than the kerberos tests.

[snip]

So on a re-read of the v7 patch, there seems to be a bit of inconsistent
usage between delegation and proxying; i.e., the field itself is called
gss_proxy in the gssstatus struct, authentication messages, etc, but the
setting and docs refer to GSS delegation. Are there subtle distinctions
between these? It seems like this patch is using them interchangeably, so
it might be good to settle on one terminology here unless there are already
well-defined categories for where to use one and where to use the other.

Thanks,

David