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

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