Thread: Kerberos delegation support in libpq and postgres_fdw

On 22.07.21 10:39, Peifeng Qiu wrote:
> I've slightly modified the patch to support "gssencmode" and added TAP 
> tests.

For the TAP tests, please put then under src/test/kerberos/, instead of 
copying the whole infrastructure to contrib/postgres_fdw/.  Just make a 
new file, for example t/002_postgres_fdw_proxy.pl, and put your tests there.

Also, you can put code and tests in one patch, no need to separate.

I wonder if this feature would also work in dblink.  Since there is no 
substantial code changes in postgres_fdw itself as part of this patch, I 
would suspect yes.  Can you check?

This patch no longer applies following the Perl namespace changes, can you
please submit a rebased version? Marking the patch "Waiting on Author".

--
Daniel Gustafsson        https://vmware.com/

> On 3 Nov 2021, at 13:41, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
> This patch no longer applies following the Perl namespace changes, can you
> please submit a rebased version? Marking the patch "Waiting on Author".

As the thread has stalled, and the OP email bounces, I'm marking this patch
Returned with Feedback.  Please feel free to resubmit a new entry in case
anyone picks this up.

--
Daniel Gustafsson        https://vmware.com/

Greetings,

(Dropping the original poster as their email address apparently no
longer works)

* Peter Eisentraut (peter.eisentraut@enterprisedb.com) wrote:
> On 22.07.21 10:39, Peifeng Qiu wrote:
> >I've slightly modified the patch to support "gssencmode" and added TAP
> >tests.
>
> For the TAP tests, please put then under src/test/kerberos/, instead of
> copying the whole infrastructure to contrib/postgres_fdw/.  Just make a new
> file, for example t/002_postgres_fdw_proxy.pl, and put your tests there.

I've incorporated the tests into the existing kerberos/001_auth.pl as
there didn't seem any need to create another file.

> Also, you can put code and tests in one patch, no need to separate.

Done.  Also rebased and updated for the changes in the TAP testing
infrastructure and other changes.  Also added code to track if
credentials were forwarded or not and to log that information.

> I wonder if this feature would also work in dblink.  Since there is no
> substantial code changes in postgres_fdw itself as part of this patch, I
> would suspect yes.  Can you check?

Yup, this should work fine.  I didn't include any explicit testing of
postgres_fdw or dblink in this, yet.  Instead, for the moment at least,
I've added to the connection log message an indiciation of if
credentials were passed along with the connection along with tests of
both the negative case and the positive case.  Not sure if that's useful
information to have in pg_stat_gssapi, but if so, then we could add it
there pretty easily.

I'm happy to try and get testing with postgres_fdw and dblink working
soon though, assuming there aren't any particular objections to moving
this forward.

Will add to the CF for consideration.

Thanks,

Stephen

On Mon, 2022-02-28 at 20:28 -0500, Stephen Frost wrote:
> Will add to the CF for consideration.

GSSAPI newbie here, so caveat lector.

> diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
> index efc53f3135..6f820a34f1 100644
> --- a/src/backend/libpq/auth.c
> +++ b/src/backend/libpq/auth.c
> @@ -920,6 +920,7 @@ pg_GSS_recvauth(Port *port)
>      int            mtype;
>      StringInfoData buf;
>      gss_buffer_desc gbuf;
> +    gss_cred_id_t proxy;
>  
>      /*
>       * Use the configured keytab, if there is one.  Unfortunately, Heimdal
> @@ -949,6 +950,9 @@ pg_GSS_recvauth(Port *port)
>       */
>      port->gss->ctx = GSS_C_NO_CONTEXT;
>  
> +    proxy = NULL;
> +    port->gss->proxy_creds = false;
> +
>      /*
>       * Loop through GSSAPI message exchange. This exchange can consist of
>       * multiple messages sent in both directions. First message is always from
> @@ -999,7 +1003,7 @@ pg_GSS_recvauth(Port *port)
>                                            &port->gss->outbuf,
>                                            &gflags,
>                                            NULL,
> -                                          NULL);
> +                                          &proxy);
>  
>          /* gbuf no longer used */
>          pfree(buf.data);
> @@ -1011,6 +1015,12 @@ pg_GSS_recvauth(Port *port)
>  
>          CHECK_FOR_INTERRUPTS();
>  
> +        if (proxy != NULL)
> +        {
> +            pg_store_proxy_credential(proxy);
> +            port->gss->proxy_creds = true;
> +        }
> +

Some implementation docs [1] imply that a delegated_cred_handle is only
valid if the ret_flags include GSS_C_DELEG_FLAG. The C-binding RFC [2],
though, says that we can rely on it being set to GSS_C_NO_CREDENTIAL if
no handle was sent...

I don't know if there are any implementation differences here, but in
any case I think it'd be more clear to use the GSS_C_NO_CREDENTIAL
spelling (instead of NULL) here, if we do decide not to check
ret_flags.

[5] says we have to free the proxy credential with GSS_Release_cred();
I don't see that happening anywhere, but I may have missed it.

>      maj_stat = gss_init_sec_context(&min_stat,
> -                                    GSS_C_NO_CREDENTIAL,
> +                                    proxy,
>                                      &conn->gctx,
>                                      conn->gtarg_nam,
>                                      GSS_C_NO_OID,
> -                                    GSS_C_MUTUAL_FLAG,
> +                                    GSS_C_MUTUAL_FLAG | GSS_C_DELEG_FLAG,
>                                      0,
>                                      GSS_C_NO_CHANNEL_BINDINGS,
>                                      (ginbuf.value == NULL) ? GSS_C_NO_BUFFER : &ginbuf,
> diff --git a/src/interfaces/libpq/fe-secure-gssapi.c b/src/interfaces/libpq/fe-secure-gssapi.c
> index 6ea52ed866..566c89f52f 100644
> --- a/src/interfaces/libpq/fe-secure-gssapi.c
> +++ b/src/interfaces/libpq/fe-secure-gssapi.c
> @@ -631,7 +631,7 @@ pqsecure_open_gss(PGconn *conn)
>       */
>      major = gss_init_sec_context(&minor, conn->gcred, &conn->gctx,
>                                   conn->gtarg_nam, GSS_C_NO_OID,
> -                                 GSS_REQUIRED_FLAGS, 0, 0, &input, NULL,
> +                                 GSS_REQUIRED_FLAGS | GSS_C_DELEG_FLAG, 0, 0, &input, NULL,

It seems like there should be significant security implications to
allowing delegation across the board. Especially since one postgres_fdw
might delegate to another server, and then another... Should this be
opt-in, maybe via a connection parameter?

(It also looks like there are some mechanisms for further constraining
delegation scope, either by administrator policy or otherwise [3, 4].
Might be a good thing for a v2 of this feature to have.)

Similarly, it feels a little strange that the server would allow the
client to unilaterally force the use of a delegated credential. I think
that should be opt-in on the server side too, unless there's some
context I'm missing around why that's safe.

> +    /* Make the proxy credential only available to current process */
> +    major = gss_store_cred_into(&minor,
> +        cred,
> +        GSS_C_INITIATE, /* credential only used for starting libpq connection */
> +        GSS_C_NULL_OID, /* store all */
> +        true, /* overwrite */
> +        true, /* make default */
> +        &ccset,
> +        &mech,
> +        &usage);
> +
> +
> +    if (major != GSS_S_COMPLETE)
> +    {
> +        pg_GSS_error("gss_store_cred", major, minor);
> +    }
> +
> +    /* quite strange that gss_store_cred doesn't work with "KRB5CCNAME=MEMORY:",
> +     * we have to use gss_store_cred_into instead and set the env for later
> +     * gss_acquire_cred calls. */
> +    setenv("KRB5CCNAME", GSS_MEMORY_CACHE, 1);

If I'm reading it right, we're resetting the default credential in the
MEMORY cache, so if you're a libpq client doing your own GSSAPI work,
I'm guessing you might not be happy with this behavior. Also, we're
globally ignoring whatever ccache was set by an administrator. Can't
two postgres_fdw connections from the same backend process require
different settings?

I notice that gss_store_cred_into() has a companion,
gss_acquire_cred_from(). Is it possible to use that to pull out our
delegated credential explicitly by name, instead of stomping on the
global setup?

Thanks,
--Jacob

[1] https://docs.oracle.com/cd/E36784_01/html/E36875/gss-accept-sec-context-3gss.html
[2] https://datatracker.ietf.org/doc/html/rfc2744#section-5.1
[3] https://datatracker.ietf.org/doc/html/rfc5896
[4] https://web.mit.edu/kerberos/krb5-latest/doc/appdev/gssapi.html#constrained-delegation-s4u
[5] https://datatracker.ietf.org/doc/html/rfc2743#page-50

On Fri, 2022-03-11 at 19:39 -0500, Stephen Frost wrote:
> 
> On Fri, Mar 11, 2022 at 18:55 Jacob Champion <pchampion@vmware.com> wrote:
> > 
> > [5] says we have to free the proxy credential with GSS_Release_cred();
> > I don't see that happening anywhere, but I may have missed it.
> 
> I’m not sure that it’s really necessary or worthwhile to do that at
> process end since … the process is about to end. I suppose we could
> provide a function that a user could call to ask for it to be
> released sooner if we really wanted..?

Do we have to keep the credential handle around once we've stored it
into the MEMORY: cache, though? Just seems like a leak that someone
will have to plug eventually, even if it doesn't really impact things
now.

> > It seems like there should be significant security implications to
> > allowing delegation across the board. Especially since one postgres_fdw
> > might delegate to another server, and then another... Should this be
> > opt-in, maybe via a connection parameter?
> 
> This is already opt-in- at kinit time a user can decide if they’d
> like a proxy-able ticket or not.  I don’t know that we really need to
> have our own option for it … tho I’m not really against adding such
> an option either.

I don't really have experience with the use case. Is it normal for
kinit users to have to decide once, globally, whether they want
everything they interact with to be able to proxy their credentials? It
just seems like you'd want more fine-grained control over who gets to
masquerade as you.

> > Similarly, it feels a little strange that the server would allow the
> > client to unilaterally force the use of a delegated credential. I think
> > that should be opt-in on the server side too, unless there's some
> > context I'm missing around why that's safe.
> 
> Perhaps you could explain what isn’t safe about accepting a delegated
> credential from a client..?  I am not away of a risk to accepting
> such a delegated credential.

My initial impression is that this is effectively modifying the USER
MAPPING that the admin has set up. I'd be worried about an open
credential proxy being used to bypass firewall or HBA restrictions, for
instance -- you might not be able to connect as an admin from your
machine, but you might be able to connect by bouncing through a proxy.
(What damage you can do is going to be limited by what the server
extensions can do, of course.)

Another danger might be disclosure/compromise of middlebox secrets? Is
it possible for someone who has one half of the credentials to snoop on
a gssenc connection between the proxy Postgres and the backend
Postgres?

> Even so, I’m not against adding an option… but exactly how would that
> option be configured?  Server level?  On the HBA line?  role level..?

In the OPTIONS for CREATE SERVER, maybe? At least for the FDW case.

> > If I'm reading it right, we're resetting the default credential in the
> > MEMORY cache, so if you're a libpq client doing your own GSSAPI work,
> > I'm guessing you might not be happy with this behavior.
> 
> This is just done on the server side and not the client side..?

Yeah, I misread the patch, sorry.

> > Also, we're
> > globally ignoring whatever ccache was set by an administrator. Can't
> > two postgres_fdw connections from the same backend process require
> > different settings?
> 
> Settings..? Perhaps, but delegated credentials aren’t really 
> settings, so not really sure what you’re suggesting here.

I mean that one backend server might require delegated credentials, and
another might require whatever the admin has already set up in the
ccache, and the user might want to use tables from both servers in the
same session.

> > I notice that gss_store_cred_into() has a companion,
> > gss_acquire_cred_from(). Is it possible to use that to pull out our
> > delegated credential explicitly by name, instead of stomping on the
> > global setup?
> 
> Not really sure what is meant here by global setup..?  Feeling like
> this is a follow on confusion from maybe mixing server vs client
> libpq?

By my reading, the gss_store_cred_into() call followed by
the setenv("KRB5CCNAME", ...) is effectively performing global
configuration for the process. Any KRB5CCNAME already set up by the
server admin is going to be ignored from that point onward. Is that
accurate?

Thanks,
--Jacob

Greetinsg,

* Jacob Champion (pchampion@vmware.com) wrote:
> On Fri, 2022-03-11 at 19:39 -0500, Stephen Frost wrote:
> > On Fri, Mar 11, 2022 at 18:55 Jacob Champion <pchampion@vmware.com> wrote:
> > > [5] says we have to free the proxy credential with GSS_Release_cred();
> > > I don't see that happening anywhere, but I may have missed it.
> >
> > I’m not sure that it’s really necessary or worthwhile to do that at
> > process end since … the process is about to end. I suppose we could
> > provide a function that a user could call to ask for it to be
> > released sooner if we really wanted..?
>
> Do we have to keep the credential handle around once we've stored it
> into the MEMORY: cache, though? Just seems like a leak that someone
> will have to plug eventually, even if it doesn't really impact things
> now.

We don't, so I've fixed that in the attached.  Not sure it's that big a
deal but I don't think it hurts anything either.

> > > It seems like there should be significant security implications to
> > > allowing delegation across the board. Especially since one postgres_fdw
> > > might delegate to another server, and then another... Should this be
> > > opt-in, maybe via a connection parameter?
> >
> > This is already opt-in- at kinit time a user can decide if they’d
> > like a proxy-able ticket or not.  I don’t know that we really need to
> > have our own option for it … tho I’m not really against adding such
> > an option either.
>
> I don't really have experience with the use case. Is it normal for
> kinit users to have to decide once, globally, whether they want
> everything they interact with to be able to proxy their credentials? It
> just seems like you'd want more fine-grained control over who gets to
> masquerade as you.

Yes, that's pretty typical for kinit users- they usually go with
whatever the org policy is.  Now, you're not wrong about wanting more
fine-grained control, which is what's known as 'constrained delegation'.
That's something that Kerberos in general supports these days though
it's more complicated and requires additional code to do.  That's
something that I think we could certainly add later on.

> > > Similarly, it feels a little strange that the server would allow the
> > > client to unilaterally force the use of a delegated credential. I think
> > > that should be opt-in on the server side too, unless there's some
> > > context I'm missing around why that's safe.
> >
> > Perhaps you could explain what isn’t safe about accepting a delegated
> > credential from a client..?  I am not away of a risk to accepting
> > such a delegated credential.
>
> My initial impression is that this is effectively modifying the USER
> MAPPING that the admin has set up. I'd be worried about an open
> credential proxy being used to bypass firewall or HBA restrictions, for
> instance -- you might not be able to connect as an admin from your
> machine, but you might be able to connect by bouncing through a proxy.
> (What damage you can do is going to be limited by what the server
> extensions can do, of course.)

I'm not sure that I really see the concern here.  Also, in order for
this to work, the user mapping would have to be created with "password
required = false".  Maybe that's something we revisit later, but it
seems like a good way to allow an admin to have control over this.

> Another danger might be disclosure/compromise of middlebox secrets? Is
> it possible for someone who has one half of the credentials to snoop on
> a gssenc connection between the proxy Postgres and the backend
> Postgres?

A compromised middlebox would, of course, be an issue- for any kind of
delegated credentials (which certainly goes for cleartext passwords
being passed along, and that's currently the only thing we support..).
One nice thing about GSSAPI is that the client and the server validate
each other, so it wouldn't just be 'any' middle-box but would have to be
one that was actually a trusted system in the infrastructure which has
somehow been compromised and was still trusted.

> > Even so, I’m not against adding an option… but exactly how would that
> > option be configured?  Server level?  On the HBA line?  role level..?
>
> In the OPTIONS for CREATE SERVER, maybe? At least for the FDW case.

I'm a bit confused on this.  The option to allow or not allow delegated
credentials couldn't be something that's in the CREATE SERVER for FDWs
as it applies to more than just FDWs but also dblink and anything else
where we reach out from PG to contact some other system.

> > > If I'm reading it right, we're resetting the default credential in the
> > > MEMORY cache, so if you're a libpq client doing your own GSSAPI work,
> > > I'm guessing you might not be happy with this behavior.
> >
> > This is just done on the server side and not the client side..?
>
> Yeah, I misread the patch, sorry.

No worries.

> > > Also, we're
> > > globally ignoring whatever ccache was set by an administrator. Can't
> > > two postgres_fdw connections from the same backend process require
> > > different settings?
> >
> > Settings..? Perhaps, but delegated credentials aren’t really
> > settings, so not really sure what you’re suggesting here.
>
> I mean that one backend server might require delegated credentials, and
> another might require whatever the admin has already set up in the
> ccache, and the user might want to use tables from both servers in the
> same session.

That an admin might have a credential cache that's picked up and used
for connections from a regular user backend to another system strikes me
as an altogether concerning idea.  Even so, in such a case, the admin
would have had to set up the user mapping with 'password required =
false' or it wouldn't have worked for a non-superuser anyway, so I'm not
sure that I'm too worried about this case.

> > > I notice that gss_store_cred_into() has a companion,
> > > gss_acquire_cred_from(). Is it possible to use that to pull out our
> > > delegated credential explicitly by name, instead of stomping on the
> > > global setup?
> >
> > Not really sure what is meant here by global setup..?  Feeling like
> > this is a follow on confusion from maybe mixing server vs client
> > libpq?
>
> By my reading, the gss_store_cred_into() call followed by
> the setenv("KRB5CCNAME", ...) is effectively performing global
> configuration for the process. Any KRB5CCNAME already set up by the
> server admin is going to be ignored from that point onward. Is that
> accurate?

The process, yes, but I guess I disagree on that being 'global'- it's
just for that PG backend process.

Attached is an updated patch which adds the gss_release_creds call, a
function in libpq to allow checking if the libpq connection was made
using GSS, changes to dblink to have it check for password-or-gss when
connecting to a remote system, and tests for dblink and postgres_fdw to
make sure that this all works correctly.

Thoughts?

Thanks!

Stephen

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:
> * Jacob Champion (pchampion@vmware.com) wrote:
> > On Fri, 2022-03-11 at 19:39 -0500, Stephen Frost wrote:
> > > Even so, I’m not against adding an option… but exactly how would that
> > > option be configured?  Server level?  On the HBA line?  role level..?
> >
> > In the OPTIONS for CREATE SERVER, maybe? At least for the FDW case.
>
> I'm a bit confused on this.  The option to allow or not allow delegated
> credentials couldn't be something that's in the CREATE SERVER for FDWs
> as it applies to more than just FDWs but also dblink and anything else
> where we reach out from PG to contact some other system.

Thinking it through further, it seems like the right place to allow an
administrator to control if credentials are allowed to be delegated is
through a pg_hba option.  Attached patch adds such an option.

> > > > Also, we're
> > > > globally ignoring whatever ccache was set by an administrator. Can't
> > > > two postgres_fdw connections from the same backend process require
> > > > different settings?
> > >
> > > Settings..? Perhaps, but delegated credentials aren’t really
> > > settings, so not really sure what you’re suggesting here.
> >
> > I mean that one backend server might require delegated credentials, and
> > another might require whatever the admin has already set up in the
> > ccache, and the user might want to use tables from both servers in the
> > same session.
>
> That an admin might have a credential cache that's picked up and used
> for connections from a regular user backend to another system strikes me
> as an altogether concerning idea.  Even so, in such a case, the admin
> would have had to set up the user mapping with 'password required =
> false' or it wouldn't have worked for a non-superuser anyway, so I'm not
> sure that I'm too worried about this case.

To address this, I also added a new GUC which allows an administrator to
control what the credential cache is set to for user-authenticated
backends, with a default of MEMORY:, which should generally be safe and
won't cause a user backend to pick up on a file-based credential cache
which might exist on the server somewhere.  This gives the administrator
the option to set it to more-or-less whatever they'd like though, so if
they want to set it to a file-based credential cache, then they can do
so (I did put some caveats about doing that into the documentation as I
don't think it's generally a good idea to do...).

> > > > I notice that gss_store_cred_into() has a companion,
> > > > gss_acquire_cred_from(). Is it possible to use that to pull out our
> > > > delegated credential explicitly by name, instead of stomping on the
> > > > global setup?
> > >
> > > Not really sure what is meant here by global setup..?  Feeling like
> > > this is a follow on confusion from maybe mixing server vs client
> > > libpq?
> >
> > By my reading, the gss_store_cred_into() call followed by
> > the setenv("KRB5CCNAME", ...) is effectively performing global
> > configuration for the process. Any KRB5CCNAME already set up by the
> > server admin is going to be ignored from that point onward. Is that
> > accurate?
>
> The process, yes, but I guess I disagree on that being 'global'- it's
> just for that PG backend process.

The new krb_user_ccache is a lot closer to 'global', though it's
specifically for user-authenticated backends (allowing the postmaster
and other things like replication connections to use whatever the
credential cache is set to by the administrator on startup), but that
seems like it makes sense to me- generally you're not going to want
regular user backends to be accessing the credential cache of the
'postgres' unix account on the server.

> Attached is an updated patch which adds the gss_release_creds call, a
> function in libpq to allow checking if the libpq connection was made
> using GSS, changes to dblink to have it check for password-or-gss when
> connecting to a remote system, and tests for dblink and postgres_fdw to
> make sure that this all works correctly.

I've added a couple more tests to address the new options too, along
with documentation for them.  This is starting to feel reasonably decent
to me, at least as a first pass at supporting kerberos credential
delegation, which is definitely a feature I've been hoping we would get
into PG for quite a while.  Would certainly appreciate some feedback on
this (from anyone who'd like to comment), though I know we're getting
into the last few hours before feature freeze ends.

Updated patch attached.

Thanks!

Stephen

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:
> The new krb_user_ccache is a lot closer to 'global', though it's
> specifically for user-authenticated backends (allowing the postmaster
> and other things like replication connections to use whatever the
> credential cache is set to by the administrator on startup), but that
> seems like it makes sense to me- generally you're not going to want
> regular user backends to be accessing the credential cache of the
> 'postgres' unix account on the server.

Added an explicit 'environment' option to allow for, basically, existing
behavior, where we don't mess with the environment variable at all,
though I kept the default as MEMORY since I don't think it's really
typical that folks actually want regular user backends to inherit the
credential cache of the server.

Added a few more tests and updated the documentation too.  Sadly, seems
we've missed the deadline for v15 though for lack of feedback on these.
Would really like to get some other folks commenting as these are new
pg_hba and postgresql.conf options being added.

Thanks!

Stephen

On Fri, Apr 8, 2022 at 8:21 AM Stephen Frost <sfrost@snowman.net> wrote:
> Added an explicit 'environment' option to allow for, basically, existing
> behavior, where we don't mess with the environment variable at all,
> though I kept the default as MEMORY since I don't think it's really
> typical that folks actually want regular user backends to inherit the
> credential cache of the server.
>
> Added a few more tests and updated the documentation too.  Sadly, seems
> we've missed the deadline for v15 though for lack of feedback on these.
> Would really like to get some other folks commenting as these are new
> pg_hba and postgresql.conf options being added.

Hi,

I don't think this patch is quite baked enough to go in even if the
deadline hadn't formally passed, but I'm happy to offer a few opinions
... especially if we can also try to sort out a plan for getting that
wider-checksums thing you mentioned done for v16.

 + /* gssencmode is also libpq option, same to above. */
+ {"gssencmode", UserMappingRelationId, true},

I really hate names like this that are just a bunch of stuff strung
together with no punctuation and some arbitrary abbreviations thrown
in for good measure. But since the libpq parameter already exists it's
hard to argue we should do anything else here.

+      <term><literal>allow_cred_delegation</literal></term>

First, I again recommend not choosing words at random to abbreviate.
"delegate_credentials" would be shorter and clearer. Second, I think
we need to decide whether we envision just having one parameter here
for every kind of credential delegation that libpq might ever support,
or whether this is really something specific to GSS. If the latter,
the name should mention GSS.

I also suggest that the default value of this option should be false,
rather than true. I would be unhappy if ssh started defaulting to
ForwardAgent=yes, because that's less secure and I don't want my
credentials shared with random servers without me making a choice to
do that. Similarly here I think we should default to the more secure
option.

+      <listitem>
+       <para>
+        Sets the location of the Kerberos credential cache to be used for
+        regular user backends which go through authentication.  The default is
+        <filename>MEMORY:</filename>, which is where delegated credentials
+        are stored (and is otherwise empty).  Care should be used when changing
+        this value- setting it to a file-based credential cache will mean that
+        user backends could potentially use any credentials stored to access
+        other systems.
+        If this parameter is set to an empty string, then the variable will be
+ explicit un-set and the system-dependent default is used, which may be a
+ file-based credential cache with the same caveats as previously
+ mentioned.  If the special value 'environment' is used, then the variable
+ is left untouched and will be whatever was set in the environment at
+ startup time.

"MEMORY:" seems like a pretty weird choice of arbitrary string. Is it
supposed to look like a Windows drive letter or pseudo-device, or
what? I'm not sure exactly what's better here, but I just think this
doesn't look like anything else we've got today. And then we've got a
second special environment, "environment", which looks completely
different: now it's lower-case and without the colon. And then empty
string is special too.

I wonder whether we really quite this many cases. But if we do they
probably need better and more consistent naming.

The formatting here also looks weird.

+#ifndef PG_KRB_USER_CCACHE
+#define PG_KRB_USER_CCACHE "MEMORY:"
+#endif

At the risk of stating the obvious, the general idea of a #define is
that you define things in one place and then use the defined symbol
rather than the original value everywhere. This patch takes the
less-useful approach of defining two different symbols for the same
string in different files. This one has this #ifndef/#endif guard here
which I think it probably shouldn't, since the choice of string
probably shouldn't be compile-time configurable, but it also won't
work, because there's no similar guard in the other file.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Apr 8, 2022 at 8:21 AM Stephen Frost <sfrost@snowman.net> wrote:
> > Added an explicit 'environment' option to allow for, basically, existing
> > behavior, where we don't mess with the environment variable at all,
> > though I kept the default as MEMORY since I don't think it's really
> > typical that folks actually want regular user backends to inherit the
> > credential cache of the server.
> >
> > Added a few more tests and updated the documentation too.  Sadly, seems
> > we've missed the deadline for v15 though for lack of feedback on these.
> > Would really like to get some other folks commenting as these are new
> > pg_hba and postgresql.conf options being added.
>
> I don't think this patch is quite baked enough to go in even if the
> deadline hadn't formally passed, but I'm happy to offer a few opinions
> ... especially if we can also try to sort out a plan for getting that
> wider-checksums thing you mentioned done for v16.

Sure.

>  + /* gssencmode is also libpq option, same to above. */
> + {"gssencmode", UserMappingRelationId, true},
>
> I really hate names like this that are just a bunch of stuff strung
> together with no punctuation and some arbitrary abbreviations thrown
> in for good measure. But since the libpq parameter already exists it's
> hard to argue we should do anything else here.

Well, yeah.

> +      <term><literal>allow_cred_delegation</literal></term>
>
> First, I again recommend not choosing words at random to abbreviate.
> "delegate_credentials" would be shorter and clearer. Second, I think
> we need to decide whether we envision just having one parameter here
> for every kind of credential delegation that libpq might ever support,
> or whether this is really something specific to GSS. If the latter,
> the name should mention GSS.

delegate_credentials seems to imply that the server has some kind of
control over the act of delegating credentials, which isn't really the
case.  The client has to decide to delegate credentials and it does that
independent of the server- the server side just gets to either accept
those delegated credentials, or ignore them.

In terms of having a prefix, this is certainly something that I'd like
to see SSPI support added for as well (perhaps that can be in v16 too)
and so it's definitely not GSS-exclusive among the authentication
methods that we have today.  In that sense, this option falls into the
same category as 'include_realm' and 'krb_realm' in that it applies to
more than one, but not all, of our authentication methods.

> I also suggest that the default value of this option should be false,
> rather than true. I would be unhappy if ssh started defaulting to
> ForwardAgent=yes, because that's less secure and I don't want my
> credentials shared with random servers without me making a choice to
> do that. Similarly here I think we should default to the more secure
> option.

This is a bit backwards from how it works though- this option is about
if the server will accept delegated credentials, not if the client sends
them.  If your client was set to ForwardAgent=yes, would you be happy if
the server's default was AllowAgentForwarding=no?  (At least on the
system I'm looking at, the current default is AllowAgentForwarding=yes
in sshd_config).

Regarding the client side, it is the case that GSSAPIDelegateCredentials
in ssh defaults to no, so it seems like the next iteration of the patch
should probably include a libpq option similar to that ssh_config
option.  As I mentioned before, users already can decide if they'd like
proxyable credentials or not when they kinit, though more generally this
is set as a environment-wide policy, but we can add an option and
disable it by default.

> +      <listitem>
> +       <para>
> +        Sets the location of the Kerberos credential cache to be used for
> +        regular user backends which go through authentication.  The default is
> +        <filename>MEMORY:</filename>, which is where delegated credentials
> +        are stored (and is otherwise empty).  Care should be used when changing
> +        this value- setting it to a file-based credential cache will mean that
> +        user backends could potentially use any credentials stored to access
> +        other systems.
> +        If this parameter is set to an empty string, then the variable will be
> + explicit un-set and the system-dependent default is used, which may be a
> + file-based credential cache with the same caveats as previously
> + mentioned.  If the special value 'environment' is used, then the variable
> + is left untouched and will be whatever was set in the environment at
> + startup time.
>
> "MEMORY:" seems like a pretty weird choice of arbitrary string. Is it
> supposed to look like a Windows drive letter or pseudo-device, or
> what? I'm not sure exactly what's better here, but I just think this
> doesn't look like anything else we've got today. And then we've got a
> second special environment, "environment", which looks completely
> different: now it's lower-case and without the colon. And then empty
> string is special too.

This isn't actually something we have a choice in, really, it's from the
Kerberos library.  MEMORY is the library's in-memory credential cache.
Other possible values are FILE:/some/file, DIR:/some/dir, API:, and
others.  Documentaton is available here:
https://web.mit.edu/kerberos/krb5-1.12/doc/basic/ccache_def.html

> I wonder whether we really quite this many cases. But if we do they
> probably need better and more consistent naming.

I wouldn't want to end up with values that could end up conflicting with
real values that a user might want to specify, so the choice of
'environment' and empty-value were specifically chosen to avoid that
risk.  If we're worried that doing so isn't sufficient or is too
confusing, the better option would likely be to have another GUC that
controls if we unset, ignore, or set the value to what the other GUC
says to set it to.  I'm fine with that if you agree.

> The formatting here also looks weird.
>
> +#ifndef PG_KRB_USER_CCACHE
> +#define PG_KRB_USER_CCACHE "MEMORY:"
> +#endif
>
> At the risk of stating the obvious, the general idea of a #define is
> that you define things in one place and then use the defined symbol
> rather than the original value everywhere. This patch takes the
> less-useful approach of defining two different symbols for the same
> string in different files. This one has this #ifndef/#endif guard here
> which I think it probably shouldn't, since the choice of string
> probably shouldn't be compile-time configurable, but it also won't
> work, because there's no similar guard in the other file.

Yeah, the other #define should have gone away and been changed to use
the above.  That should be easy enough to fix.

Thanks!

Stephen

On Fri, Apr 8, 2022 at 11:29 AM Stephen Frost <sfrost@snowman.net> wrote:
> > +      <term><literal>allow_cred_delegation</literal></term>
> >
> > First, I again recommend not choosing words at random to abbreviate.
> > "delegate_credentials" would be shorter and clearer. Second, I think
> > we need to decide whether we envision just having one parameter here
> > for every kind of credential delegation that libpq might ever support,
> > or whether this is really something specific to GSS. If the latter,
> > the name should mention GSS.
>
> delegate_credentials seems to imply that the server has some kind of
> control over the act of delegating credentials, which isn't really the
> case.  The client has to decide to delegate credentials and it does that
> independent of the server- the server side just gets to either accept
> those delegated credentials, or ignore them.

Oh ... I thought this was a libpq parameter to control the client
behavior. I guess I didn't read it carefully enough.

> Regarding the client side, it is the case that GSSAPIDelegateCredentials
> in ssh defaults to no, so it seems like the next iteration of the patch
> should probably include a libpq option similar to that ssh_config
> option.  As I mentioned before, users already can decide if they'd like
> proxyable credentials or not when they kinit, though more generally this
> is set as a environment-wide policy, but we can add an option and
> disable it by default.

+1.

> This isn't actually something we have a choice in, really, it's from the
> Kerberos library.  MEMORY is the library's in-memory credential cache.
> Other possible values are FILE:/some/file, DIR:/some/dir, API:, and
> others.  Documentaton is available here:
> https://web.mit.edu/kerberos/krb5-1.12/doc/basic/ccache_def.html

Well, I was just going by the fact that this string ("MEMORY:") seems
to be being interpreted in our code, not the library.

> > I wonder whether we really quite this many cases. But if we do they
> > probably need better and more consistent naming.
>
> I wouldn't want to end up with values that could end up conflicting with
> real values that a user might want to specify, so the choice of
> 'environment' and empty-value were specifically chosen to avoid that
> risk.  If we're worried that doing so isn't sufficient or is too
> confusing, the better option would likely be to have another GUC that
> controls if we unset, ignore, or set the value to what the other GUC
> says to set it to.  I'm fine with that if you agree.

Yeah, I thought of that, and it might be the way to go. I wasn't too
sure we needed the explicit-unset behavior as an option, but I defer
to you on that.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On 4/8/22 05:21, Stephen Frost wrote:
> Added a few more tests and updated the documentation too.  Sadly, seems
> we've missed the deadline for v15 though for lack of feedback on these.
> Would really like to get some other folks commenting as these are new
> pg_hba and postgresql.conf options being added.

Sorry for the incredibly long delay; I lost track of this thread during
the email switch. I'm testing the patch with various corner cases to try
to figure out how it behaves, so this isn't a full review, but I wanted
to jump through some of the emails I missed and at least give you some
responses.

As an overall note, I think the patch progression, and adding more
explicit control over when credentials may be delegated, is very
positive, and +1 for the proposed libpq connection option elsewhere in
the thread.

On 4/7/22 21:21, Stephen Frost wrote:
>> That an admin might have a credential cache that's picked up and used
>> for connections from a regular user backend to another system strikes me
>> as an altogether concerning idea.  Even so, in such a case, the admin
>> would have had to set up the user mapping with 'password required =
>> false' or it wouldn't have worked for a non-superuser anyway, so I'm not
>> sure that I'm too worried about this case.
>
> To address this, I also added a new GUC which allows an administrator to
> control what the credential cache is set to for user-authenticated
> backends, with a default of MEMORY:, which should generally be safe and
> won't cause a user backend to pick up on a file-based credential cache
> which might exist on the server somewhere.  This gives the administrator
> the option to set it to more-or-less whatever they'd like though, so if
> they want to set it to a file-based credential cache, then they can do
> so (I did put some caveats about doing that into the documentation as I
> don't think it's generally a good idea to do...).

I'm not clear on how this handles the collision case. My concern was
with a case where you have more than one foreign table/server, and they
need to use separate credentials. It's not obvious to me how changing
the location of a (single, backend-global) cache mitigates that problem.

I'm also missing something about why password_required=false is
necessary (as opposed to simply setting a password in the USER MAPPING).
My current test case doesn't make use of password_required=false and it
appears to work just fine.

On 4/6/22 12:27, Stephen Frost wrote:
>> Another danger might be disclosure/compromise of middlebox secrets? Is
>> it possible for someone who has one half of the credentials to snoop on
>> a gssenc connection between the proxy Postgres and the backend
>> Postgres?
>
> A compromised middlebox would, of course, be an issue- for any kind of
> delegated credentials (which certainly goes for cleartext passwords
> being passed along, and that's currently the only thing we support..).
> One nice thing about GSSAPI is that the client and the server validate
> each other, so it wouldn't just be 'any' middle-box but would have to be
> one that was actually a trusted system in the infrastructure which has
> somehow been compromised and was still trusted.

I wasn't clear enough, sorry -- I mean that we have to prove that
defaulting allow_cred_delegation to true doesn't cause the compromise of
existing deployments.

As an example, right now I'm trying to characterize behavior with the
following pg_hba setup on the foreign server:

    hostgssenc all all ... password

So in other words we're using GSS as transport encryption only, not as
an authentication provider. On the middlebox, we create a FOREIGN
SERVER/TABLE that points to this, and set up a USER MAPPING (with no
USAGE rights) that contains the necessary password. (I'm using a
plaintext password to make it more obvious what the danger is, not
suggesting that this would be good practice.)

As far as I can tell, to make this work today, a server admin has to set
up a local credential cache with the keys for some one-off principal. It
doesn't have to be an admin principal, because the point is just to
provide transport protection for the password, so it's not really
particularly scary to make it available to user backends. But this new
proposed feature lets the client override that credential cache,
substituting their own credentials, for which they have all the Kerberos
symmetric key material.

So my question is this: does substituting my credentials for the admin's
credentials let me weaken or break the transport encryption on the
backend connection, and grab the password that I'm not supposed to have
access to as a front-end client?

I honestly don't know the answer; GSSAPI is a black box that defers to
Kerberos and there's a huge number of specs that I've been slowly making
my way through. But in my tests, if I turn on credential forwarding,
Wireshark is suddenly able to use the *client's* keys to decrypt pieces
of the TGS's conversations with the *middlebox*, including session keys,
and that doesn't make me feel very good about the strength of the crypto
when the middlebox starts talking to the backend foreign server.

Maybe there's some ephemeral exchange going on that makes it too hard to
attack in practice, or some other mitigations. But Wireshark doesn't
understand how to dissect the libpq gssenc exchange, and I don't know
the specs well enough yet, so I can't really prove it either way. Do you
know more about the underlying GSS exchange, or else which specs cover
the low-level details? I'm trying to avoid writing Wireshark dissector
code, but maybe that'd be useful either way...

--Jacob

On Thu, Jul 7, 2022 at 4:24 PM Jacob Champion <jchampion@timescale.com> wrote:
> So my question is this: does substituting my credentials for the admin's
> credentials let me weaken or break the transport encryption on the
> backend connection, and grab the password that I'm not supposed to have
> access to as a front-end client?

With some further research: yes, it does.

If a DBA is using a GSS encrypted tunnel to communicate to a foreign
server, accepting delegation by default means that clients will be
able to break that backend encryption at will, because the keys in use
will be under their control.

> Maybe there's some ephemeral exchange going on that makes it too hard to
> attack in practice, or some other mitigations.

There is no forward secrecy, ephemeral exchange, etc. to mitigate this [1]:

   The Kerberos protocol in its basic form does not provide perfect
   forward secrecy for communications.  If traffic has been recorded by
   an eavesdropper, then messages encrypted using the KRB_PRIV message,
   or messages encrypted using application-specific encryption under
   keys exchanged using Kerberos can be decrypted if the user's,
   application server's, or KDC's key is subsequently discovered.

So the client can decrypt backend communications that make use of its
delegated key material. (This also means that gssencmode is a lot
weaker than I expected.)

> I'm trying to avoid writing Wireshark dissector
> code, but maybe that'd be useful either way...

I did end up filling out the existing PGSQL dissector so that it could
decrypt GSSAPI exchanges (with the use of a keytab, that is). If you'd
like to give it a try, the patch, based on Wireshark 3.7.1, is
attached. Note the GPLv2 license. It isn't correct code yet, because I
didn't understand how packet reassembly worked in Wireshark when I
started writing the code, so really large GSSAPI messages that are
split across multiple TCP packets will confuse the dissector. But it's
enough to prove the concept.

To see this in action, set up an FDW connection that uses gssencmode
(so the server in the middle will need its own Kerberos credentials).
Capture traffic starting from the kinit through the query on the
foreign table. Export the client's key material into a keytab, and set
up Wireshark to use that keytab for decryption. When credential
forwarding is *not* in use, Wireshark will be able to decrypt the
initial client connection, but it won't be able to see anything inside
the foreign server connection. When credential forwarding is in use,
Wireshark will be able to decrypt both connections.

Thanks,
--Jacob

[1] https://www.rfc-editor.org/rfc/rfc4120

Greetings,

* Jacob Champion (jchampion@timescale.com) wrote:
> On Thu, Jul 7, 2022 at 4:24 PM Jacob Champion <jchampion@timescale.com> wrote:
> > So my question is this: does substituting my credentials for the admin's
> > credentials let me weaken or break the transport encryption on the
> > backend connection, and grab the password that I'm not supposed to have
> > access to as a front-end client?
>
> With some further research: yes, it does.
>
> If a DBA is using a GSS encrypted tunnel to communicate to a foreign
> server, accepting delegation by default means that clients will be
> able to break that backend encryption at will, because the keys in use
> will be under their control.

This is coming across as if it's a surprise of some kind when it
certainly isn't..  If the delegated credentials are being used to
authenticate and establish the connection from that backend to another
system then, yes, naturally that means that the keys provided are coming
from the client and the client knows them.  The idea of arranging to
have an admin's credentials used to authenticate to another system where
the backend is actually controlled by a non-admin user is, in fact, the
issue in what is being outlined above as that's clearly a situation
where the user's connection is being elevated to an admin level.  That's
also something that we try to avoid having happen because it's not
really a good idea, which is why we require a password today for the
connection to be established (postgres_fdw/connection.c:

Non-superuser cannot connect if the server does not request a password.

).

Consider that, in general, the user could also simply directly connect
to the other system themselves instead of having a PG backend make that
connection for them- the point in doing it from PG would be to avoid
having to pass all the data back through the client's system.

Consider SSH instead of PG.  What you're pointing out, accurately, is
that if an admin were to install their keys into a user's .ssh directory
unencrypted and then the user logged into the system, they'd then be
able to SSH to another system with the admin's credentials and then
they'd need the admin's credentials to decrypt the traffic, but that if,
instead, the user brings their own credentials then they could
potentially decrypt the connection between the systems.  Is that really
the issue here?  Doesn't seem like that's where the concern should be in
this scenario.

> > Maybe there's some ephemeral exchange going on that makes it too hard to
> > attack in practice, or some other mitigations.
>
> There is no forward secrecy, ephemeral exchange, etc. to mitigate this [1]:
>
>    The Kerberos protocol in its basic form does not provide perfect
>    forward secrecy for communications.  If traffic has been recorded by
>    an eavesdropper, then messages encrypted using the KRB_PRIV message,
>    or messages encrypted using application-specific encryption under
>    keys exchanged using Kerberos can be decrypted if the user's,
>    application server's, or KDC's key is subsequently discovered.
>
> So the client can decrypt backend communications that make use of its
> delegated key material. (This also means that gssencmode is a lot
> weaker than I expected.)

The backend wouldn't be able to establish the connection in the first
place without those delegated credentials.

> > I'm trying to avoid writing Wireshark dissector
> > code, but maybe that'd be useful either way...
>
> I did end up filling out the existing PGSQL dissector so that it could
> decrypt GSSAPI exchanges (with the use of a keytab, that is). If you'd
> like to give it a try, the patch, based on Wireshark 3.7.1, is
> attached. Note the GPLv2 license. It isn't correct code yet, because I
> didn't understand how packet reassembly worked in Wireshark when I
> started writing the code, so really large GSSAPI messages that are
> split across multiple TCP packets will confuse the dissector. But it's
> enough to prove the concept.
>
> To see this in action, set up an FDW connection that uses gssencmode
> (so the server in the middle will need its own Kerberos credentials).

The server in the middle should *not* be using its own Kerberos
credentials to establish the connection to the other system- that's
elevating the credentials used for that connection and is something that
should be prevented for non-superusers already (see above).  We do allow
that when a superuser is involved because they are considered to
essentially have OS-level privileges and therefore could see those
credentials anyway, but that's not the case for non-superusers.

Thanks,

Stephen

On 9/19/22 10:05, Stephen Frost wrote:
> This is coming across as if it's a surprise of some kind when it
> certainly isn't..  If the delegated credentials are being used to
> authenticate and establish the connection from that backend to another
> system then, yes, naturally that means that the keys provided are coming
> from the client and the client knows them.

I think it may be surprising to end users that credential delegation
lets them trivially break transport encryption. Like I said before, it
was a surprise to me, because the cryptosystems I'm familiar with
prevent that.

If it wasn't surprising to you, I could really have used a heads up back
when I asked you about it.

> The idea of arranging to
> have an admin's credentials used to authenticate to another system where
> the backend is actually controlled by a non-admin user is, in fact, the
> issue in what is being outlined above as that's clearly a situation
> where the user's connection is being elevated to an admin level.

Yes, controlled elevation is the goal in the scenario I'm describing.

> That's
> also something that we try to avoid having happen because it's not
> really a good idea, which is why we require a password today for the
> connection to be established (postgres_fdw/connection.c:
> 
> Non-superuser cannot connect if the server does not request a password.

A password is being used in this scenario. The password is the secret
being stolen.

The rest of your email describes a scenario different from what I'm
attacking here. Here's my sample HBA line for the backend again:

    hostgssenc all all ... password

I'm using password authentication with a Kerberos-encrypted channel.
It's similar to protecting password authentication with TLS and a client
cert:

    hostssl all all ... password clientcert=verify-*

> Consider that, in general, the user could also simply directly connect
> to the other system themselves

No, because they don't have the password. They don't have USAGE on the
foreign table, so they can't see the password in the USER MAPPING.

With the new default introduced in this patch, they can now steal the
password by delegating their credentials and cracking the transport
encryption. This bypasses the protections that are documented for the
pg_user_mappings view.

> Consider SSH instead of PG.  What you're pointing out, accurately, is
> that if an admin were to install their keys into a user's .ssh directory
> unencrypted and then the user logged into the system, they'd then be
> able to SSH to another system with the admin's credentials and then
> they'd need the admin's credentials to decrypt the traffic, but that if,
> instead, the user brings their own credentials then they could
> potentially decrypt the connection between the systems.  Is that really
> the issue here?

No, it's not the issue here. This is more like setting up a restricted
shell that provides limited access to a resource on another machine
(analogous to the foreign table). The user SSHs into this restricted
shell, and then invokes an admin-blessed command whose implementation
uses some credentials (which they cannot read, analogous to the USER
MAPPING) over an encrypted channel to access the backend resource. In
this situation an admin would want to ensure that the encrypted tunnel
couldn't be weakened by the client, so that they can't learn how to
bypass the blessed command and connect to the backend directly.

Unlike SSH, we've never supported credential delegation, and now we're
introducing it. So my claim is, it's possible for someone who was
previously in a secure situation to be broken by the new default.

>> So the client can decrypt backend communications that make use of its
>> delegated key material. (This also means that gssencmode is a lot
>> weaker than I expected.)
> 
> The backend wouldn't be able to establish the connection in the first
> place without those delegated credentials.

That's not true in the situation I'm describing; hopefully my comments
above help clarify.

> The server in the middle should *not* be using its own Kerberos
> credentials to establish the connection to the other system- that's
> elevating the credentials used for that connection and is something that
> should be prevented for non-superusers already (see above).

It's not prevented, because a password is being used. In my tests I'm
connecting as an unprivileged user.

You're claiming that the middlebox shouldn't be doing this. If this new
default behavior were the historical behavior, then I would have agreed.
But the cat's already out of the bag on that, right? It's safe today.
And if it's not safe today for some other reason, please share why, and
maybe I can work on a patch to try to prevent people from doing it.

Thanks,
--Jacob

On Mon, Sep 19, 2022 at 02:05:39PM -0700, Jacob Champion wrote:
> It's not prevented, because a password is being used. In my tests I'm
> connecting as an unprivileged user.
>
> You're claiming that the middlebox shouldn't be doing this. If this new
> default behavior were the historical behavior, then I would have agreed.
> But the cat's already out of the bag on that, right? It's safe today.
> And if it's not safe today for some other reason, please share why, and
> maybe I can work on a patch to try to prevent people from doing it.

Please note that this has been marked as returned with feedback in the
current CF, as this has remained unanswered for a bit more than three
weeks.
--
Michael

Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Mon, Sep 19, 2022 at 02:05:39PM -0700, Jacob Champion wrote:
> > It's not prevented, because a password is being used. In my tests I'm
> > connecting as an unprivileged user.
> >
> > You're claiming that the middlebox shouldn't be doing this. If this new
> > default behavior were the historical behavior, then I would have agreed.
> > But the cat's already out of the bag on that, right? It's safe today.
> > And if it's not safe today for some other reason, please share why, and
> > maybe I can work on a patch to try to prevent people from doing it.
>
> Please note that this has been marked as returned with feedback in the
> current CF, as this has remained unanswered for a bit more than three
> weeks.

There's some ongoing discussion about how to handle outbound connections
from the server ending up picking up credentials from the server's
environment (that really shouldn't be allowed unless specifically asked
for..), that's ultimately an independent change from what this patch is
doing.

Here's an updated version which does address Robert's concerns around
having this disabled by default and having options on both the server
and client side saying if it is to be enabled or not.  Also added to
pg_stat_gssapi a field that indicates if credentials were proxied or not
and made some other improvements and added additional regression tests
to test out various combinations.

Thanks,

Stephen

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:
> * Michael Paquier (michael@paquier.xyz) wrote:
> > On Mon, Sep 19, 2022 at 02:05:39PM -0700, Jacob Champion wrote:
> > > It's not prevented, because a password is being used. In my tests I'm
> > > connecting as an unprivileged user.
> > >
> > > You're claiming that the middlebox shouldn't be doing this. If this new
> > > default behavior were the historical behavior, then I would have agreed.
> > > But the cat's already out of the bag on that, right? It's safe today.
> > > And if it's not safe today for some other reason, please share why, and
> > > maybe I can work on a patch to try to prevent people from doing it.
> >
> > Please note that this has been marked as returned with feedback in the
> > current CF, as this has remained unanswered for a bit more than three
> > weeks.
>
> There's some ongoing discussion about how to handle outbound connections
> from the server ending up picking up credentials from the server's
> environment (that really shouldn't be allowed unless specifically asked
> for..), that's ultimately an independent change from what this patch is
> doing.

That got committed, which is great, though it didn't go quite as far as
I had been hoping regarding dealing with outbound connections from the
server- perhaps we should make it clear at least for postgres_fdw that
it might be good for administrators to explicitly say which options are
allowed for a given user-map when it comes to how authentication is
done to the remote server?  Seems like mostly a documentation
improvement, I think?  Or should we have some special handling around
that option for postgres_fdw/dblink?

> Here's an updated version which does address Robert's concerns around
> having this disabled by default and having options on both the server
> and client side saying if it is to be enabled or not.  Also added to
> pg_stat_gssapi a field that indicates if credentials were proxied or not
> and made some other improvements and added additional regression tests
> to test out various combinations.

I've done some self-review and also reworked how the security checks are
done to be sure that we're not ending up pulling credentials from the
environment (with added regression tests to check for it too).  If
there's remaining concerns around that, please let me know.  Of course,
other review would be great also.  Presently though:

- Rebased up to today
- Requires explicitly being enabled on client and server
- Authentication to a remote server via dblink or postgres_fdw with
  GSSAPI requires that credentials were proxied by the client to the
  server, except if the superuser set 'password_required' to false on
  the postgres_fdw (which has lots of caveats around it in the
  documentation because it's inherently un-safe to do).
- Includes updated documentation
- Quite a few additional regression tests to check for unrelated
  credentials coming from the environment in either cases where
  credentials have been proxied and in cases where they haven't.
- Only changes to existing regression tests for dblink/postgres_fdw are
  in the error message wording updates.

Thanks!

Stephen

The CFBot says there's a function be_gssapi_get_proxy() which is
undefined. Presumably this is a missing #ifdef or a definition that
should be outside an #ifdef.

[14:05:21.532] dblink.c: In function ‘dblink_security_check’:
[14:05:21.532] dblink.c:2606:38: error: implicit declaration of
function ‘be_gssapi_get_proxy’ [-Werror=implicit-function-declaration]
[14:05:21.532] 2606 | if (PQconnectionUsedGSSAPI(conn) &&
be_gssapi_get_proxy(MyProcPort))
[14:05:21.532] | ^~~~~~~~~~~~~~~~~~~
[14:05:21.532] cc1: all warnings being treated as errors

[13:56:28.789] dblink.c.obj : error LNK2019: unresolved external
symbol be_gssapi_get_proxy referenced in function dblink_connstr_check
[13:56:29.040] contrib\dblink\dblink.dll : fatal error LNK1120: 1
unresolved externals

On Mon, 20 Mar 2023 at 09:30, Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> * Stephen Frost (sfrost@snowman.net) wrote:
> > * Michael Paquier (michael@paquier.xyz) wrote:
> > > On Mon, Sep 19, 2022 at 02:05:39PM -0700, Jacob Champion wrote:
> > > > It's not prevented, because a password is being used. In my tests I'm
> > > > connecting as an unprivileged user.
> > > >
> > > > You're claiming that the middlebox shouldn't be doing this. If this new
> > > > default behavior were the historical behavior, then I would have agreed.
> > > > But the cat's already out of the bag on that, right? It's safe today.
> > > > And if it's not safe today for some other reason, please share why, and
> > > > maybe I can work on a patch to try to prevent people from doing it.
> > >
> > > Please note that this has been marked as returned with feedback in the
> > > current CF, as this has remained unanswered for a bit more than three
> > > weeks.
> >
> > There's some ongoing discussion about how to handle outbound connections
> > from the server ending up picking up credentials from the server's
> > environment (that really shouldn't be allowed unless specifically asked
> > for..), that's ultimately an independent change from what this patch is
> > doing.
>
> That got committed, which is great, though it didn't go quite as far as
> I had been hoping regarding dealing with outbound connections from the
> server- perhaps we should make it clear at least for postgres_fdw that
> it might be good for administrators to explicitly say which options are
> allowed for a given user-map when it comes to how authentication is
> done to the remote server?  Seems like mostly a documentation
> improvement, I think?  Or should we have some special handling around
> that option for postgres_fdw/dblink?
>
> > Here's an updated version which does address Robert's concerns around
> > having this disabled by default and having options on both the server
> > and client side saying if it is to be enabled or not.  Also added to
> > pg_stat_gssapi a field that indicates if credentials were proxied or not
> > and made some other improvements and added additional regression tests
> > to test out various combinations.
>
> I've done some self-review and also reworked how the security checks are
> done to be sure that we're not ending up pulling credentials from the
> environment (with added regression tests to check for it too).  If
> there's remaining concerns around that, please let me know.  Of course,
> other review would be great also.  Presently though:
>
> - Rebased up to today
> - Requires explicitly being enabled on client and server
> - Authentication to a remote server via dblink or postgres_fdw with
>   GSSAPI requires that credentials were proxied by the client to the
>   server, except if the superuser set 'password_required' to false on
>   the postgres_fdw (which has lots of caveats around it in the
>   documentation because it's inherently un-safe to do).
> - Includes updated documentation
> - Quite a few additional regression tests to check for unrelated
>   credentials coming from the environment in either cases where
>   credentials have been proxied and in cases where they haven't.
> - Only changes to existing regression tests for dblink/postgres_fdw are
>   in the error message wording updates.
>
> Thanks!
>
> Stephen



--
greg

Greetings,

* Greg Stark (stark@mit.edu) wrote:
> The CFBot says there's a function be_gssapi_get_proxy() which is
> undefined. Presumably this is a missing #ifdef or a definition that
> should be outside an #ifdef.

Yup, just a couple of missing #ifdef's.

Updated and rebased patch attached.

Thanks!

Stephen

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:
> * Greg Stark (stark@mit.edu) wrote:
> > The CFBot says there's a function be_gssapi_get_proxy() which is
> > undefined. Presumably this is a missing #ifdef or a definition that
> > should be outside an #ifdef.
>
> Yup, just a couple of missing #ifdef's.
>
> Updated and rebased patch attached.

... and a few more.  Apparently hacking on a plane without enough sleep
leads to changing ... and unchanging configure flags before testing.

Thanks,

Stephen

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
couldbe acquired from a different source (PGPASSWORD, say) while both of these criteria are true?  Seems like it
probablydoesn'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
thatthis routine is never *actually* called in that case, just noting compile-time considerations.)  I'm not seeing
guardsin 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
denylistfor 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'tlooked 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
arechanged.
 

---

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'dsuggest 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
isthis a behavior change, and dblink could be previously used w/postgres user's .pgpass file?  I assume since this
patchis 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
credentialsand 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
descriptionsmight 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
mislabeledfurther.
 

---

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

Best,

David

The new status of this patch is: Waiting on Author

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
passwordcould be acquired from a different source (PGPASSWORD, say) while both of these criteria are true?  Seems like
itprobably 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:
>
>     /*
>      * 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;
>     }

but we don't build be-secure-gssapi.c, where this function is added,
unless --with-gssapi is included, from src/backend/libpq/Makefile:

ifeq ($(with_gssapi),yes)
OBJS += be-gssapi-common.o be-secure-gssapi.o
endif

Further, src/include/libpq/libpq-be.h has a matching #ifdef ENABLE_GSS
for the function declarations:

#ifdef ENABLE_GSS
/*
 * Return information about the GSSAPI authenticated connection
 */
extern bool be_gssapi_get_auth(Port *port);
extern bool be_gssapi_get_enc(Port *port);
extern const char *be_gssapi_get_princ(Port *port);
extern bool be_gssapi_get_proxy(Port *port);

> 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
bethat this routine is never *actually* called in that case, just noting compile-time considerations.)  I'm not seeing
guardsin the actual PQ* routines, but don't think I've done an exhaustive search. 

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

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

The Kerberos credentials have to be forwardable for them to be allowed
to be forwarded and the server has to be configured to accept them.

>     </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
denylistfor 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'tlooked 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
arechanged. 

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
isthis a behavior change, and dblink could be previously used w/postgres user's .pgpass file?  I assume since this
patchis 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...).

> +$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
credentialsand with passfile'); 
>
> Same Q's apply to the postgres_fdw version of these tests.

Regarding postgres_fdw, there are already tests in contrib/postgres_fdw
to make sure that when password_required=true that .pgpass and such
don't end up getting used, but when password_required=false (which can
only be set by a superuser) then it's allowed to use environmental
authentication options such as a .pgpass file.

> ---
>
> :659 and :667, the test description says non-encrypted and the gssencmode=prefer implies encrypted; seems like those
descriptionsmight need to be updated, since it seems like what it's really testing is dblink/postgres_fdw against
gssdeleg=enabled.

The server is configured at this point to not accept encrypted
connections (the pg_hba.conf has only:

local all test2 scram-sha-256
hostnogssenc all all $hostaddr/32 gss map=mymap

in it).

Updated the test descriptions.

> Also looks like later tests are explicitly testing w/gssencmode=require so making me think this is mislabeled
further.

Those are after the pg_hba.conf has been adjusted again to allow
encrypted connections.

> This is what I noticed on an initial pass-through.
> The new status of this patch is: Waiting on Author

Changed back to Needs Review.

Thanks again!

Stephen

Greetings,

* David Christensen (david@pgguru.net) wrote:
> On Wed, Apr 5, 2023 at 3:30 PM Stephen Frost <sfrost@snowman.net> wrote:
> > 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?

Sure, done, and updated for both auth.c and be-secure-gssapi.c

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

Ah, yes, that's correct.

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

That's a fair point and so I've updated the patch to consistently use
'delegated credentials' and similar to match the Kerberos documentation.
In Kerberos there is *also* the concept of proxied credentials which are
very very similar to delegated credentials (they're actually
"constrainted delegations") but they're not exactly the same and that
isn't what we're doing with this particular patch (though I hope that
once we get support for unconstrained delegation, which is what this
patch is doing, we can then go add support for constrainted
delegations).

Updated patch attached.

Thanks!

Stephen

Greetings,

* David Christensen (david@pgguru.net) wrote:
> Reviewed v8; largely looking good, though I notice this hunk, which may
> arguably be a bug fix, but doesn't appear to be relevant to this specific
> patch, so could probably be debated independently (and if a bug, should
> probably be backpatched):
>
> diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
> index 4229d2048c..11d41979c6 100644
> --- a/contrib/postgres_fdw/option.c
> +++ b/contrib/postgres_fdw/option.c
> @@ -288,6 +288,9 @@ InitPgFdwOptions(void)
>   {"sslcert", UserMappingRelationId, true},
>   {"sslkey", UserMappingRelationId, true},
>
> + /* gssencmode is also libpq option, same to above. */
> + {"gssencmode", UserMappingRelationId, true},
> +
>   {NULL, InvalidOid, false}
>   };

Hmm, yeah, hard to say if that makes sense at a user-mapping level or
not.  Agreed that we could have an independent discussion regarding
that and if it should be back-patched, so removed it from this patch.

> That said, should "gssdeleg" be exposed as a user mapping?  (This shows up
> in postgresql_fdw; not sure if there are other places that would be
> relevant, like in dblink somewhere as well, just a thought.)

Ah, yeah, that certainly makes sense to have as optional for a user
mapping.  dblink doesn't have the distinction between server-level
options and user mapping options (as it doesn't have user mappings at
all really) so it doesn't have something similar.

Updated patch attached.

Thanks!

Stephen

Greetings,

* David Christensen (david@pgguru.net) wrote:
> Ok, based on the interdiff there, I'm happy with that last change.  Marking
> as Ready For Committer.

Great, thanks!

I'm going to go through it again myself but I feel reasonably good about
it and if nothing else pops and there aren't objections, I'll push it
before feature freeze.

Thanks!

Stephen

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:
> * David Christensen (david@pgguru.net) wrote:
> > Ok, based on the interdiff there, I'm happy with that last change.  Marking
> > as Ready For Committer.
>
> Great, thanks!
>
> I'm going to go through it again myself but I feel reasonably good about
> it and if nothing else pops and there aren't objections, I'll push it
> before feature freeze.

Alright, I've cleaned up a few of the error messages to consistently use
GSSAPI (instead of a mix of GSSAPI and gssapi) and run the changes through
pgindent.  Updated patch attached.  cfbot looks happy.  Feeling like
this is looking just about ready to go.

Thanks!

Stephen

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:
> * Stephen Frost (sfrost@snowman.net) wrote:
> > * David Christensen (david@pgguru.net) wrote:
> > > Ok, based on the interdiff there, I'm happy with that last change.  Marking
> > > as Ready For Committer.
> >
> > Great, thanks!
> >
> > I'm going to go through it again myself but I feel reasonably good about
> > it and if nothing else pops and there aren't objections, I'll push it
> > before feature freeze.
>
> Alright, I've cleaned up a few of the error messages to consistently use
> GSSAPI (instead of a mix of GSSAPI and gssapi) and run the changes through
> pgindent.  Updated patch attached.  cfbot looks happy.  Feeling like
> this is looking just about ready to go.

And pushed, thanks again!  Time to watch the buildfarm.. :)

Stephen