Re: weird libpq GSSAPI comment

Greetings,

(I've added Robbie to this thread, so he can correct me if/when I go
wrong in my descriptions regarding the depths of GSSAPI ;)

* Alvaro Herrera (alvherre(at)2ndquadrant(dot)com) wrote:
> I found this comment in fe-connect.c:
>
> /*
> * If GSSAPI is enabled and we have a credential cache, try to
> * set it up before sending startup messages. If it's already
> * operating, don't try SSL and instead just build the startup
> * packet.
> */
>
> I'm not sure I understand this correctly. Why does it say "just build
> the startup" packet about the SSL thing, when in reality the SSL block
> below is unrelated to the GSS logic? If I consider that SSL is just a
> typo for GSS, then the comment doesn't seem to describe the logic
> either, because what it does is go to CONNECTION_GSS_STARTUP state which
> *doesn't* "build the startup packet" in the sense of pqBuildStartupPacket2/3,
> but instead it just does pqPacketSend (which is what the SSL block below
> calls "request SSL instead of sending the startup packet").

SSL there isn't a typo for GSS. The "startup packet" being referred to
in that comment is specifically the "request GSS" that's sent via the
following pqPacketSend, not the pqBuildStartupPacket one. I agree
that's a bit confusing and we should probably reword that, since
"Startup Packet" has a specific meaning in this area of the code.

> Also, it says "... and we have a credential cache, try to set it up..." but I
> think it should say "if we *don't* have a credential cache".

No, we call pg_GSS_have_cred_cache() here, which goes on to call
gss_acquire_cred(), which, as the comment above that function says,
checks to see if we can acquire credentials by making sure that we *do*
have a credential cache. If we *don't* have a credential cache, then we
fall back to SSL (and then to non-SSL).

> Now that I've read this code half a dozen times, I think I'm starting to
> vaguely understand how it works, but I would have expected the comment
> to explain it so that I didn't have to do that.

I'm concerned that you don't quite understand it though, I'm afraid.

> Can we discuss a better wording for this comment? I wrote this, but I
> don't think it captures all the nuances in this code:
>
> /*
> * If GSSAPI is enabled, we need a credential cache; we may
> * already have it, or set it up if not. Then, if we don't
> * have a GSS context, request it and switch to
> * CONNECTION_GSS_STARTUP to wait for the response.
> *
> * Fail altogether if GSS is required but cannot be had.
> */

We don't set up a credential cache at any point in this code, we only
check to see if one exists, and only in that case do we send a request
to start GSSAPI encryption (if it's allowed for us to do so).

Maybe part of the confusion here is that there's two different things- a
credential cache, and then a credential *handle*. Calling
gss_acquire_cred() will, if a credential *cache* exists, return to us a
credential *handle* (in the form of conn->gcred) that we then pass to
gss_init_sec_context().

There's then also a GSS *context* (conn->gctx), which gets set up when
we first call gss_init_sec_context(), and is then used throughout a
connection.

Typically, the credential cache is actually created when you log into a
kerberized system, but if not, you can create one by using 'kinit'
manually.

Hopefully that helps. I'm certainly happy to work with you to reword
the comment, of course, but let's make sure there's agreement and
understanding of what the code does first.

Thanks,

Stephen