Re: weird libpq GSSAPI comment

Stephen Frost <sfrost(at)snowman(dot)net> writes:

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

Hi, appreciate the CC since I'm not subscribed anymore. Thanks for your
patience while I was PTO.

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

The comment refers to the first `if`, mostly. The idea is that we want
to check whether we *can* perform GSSAPI negotiation, and skip it
otherwise - which is determined by attempting to acquire credentials.
There will be false positives for this check, but no false negatives,
and it's a step that GSSAPI performs as part of negotiation anyway so it
costs us basically nothing since we cache the result.

The "startup packet" the comment refers to is that just below on 2867 -
the pqBuildStartupPacket one. The flow is:

1. Set up GSSAPI, if possible.
2. Set up TLS, if possible.
3. Send startup packet.

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

Right.

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

Same. I tried to model after the TLS code for this. That has the
following comment:

If SSL is enabled and we haven't already got it running, request it
instead of sending the startup message.

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

This is true, though we tend to play fast and loose with that
distinction and I'm guilty of doing so here.

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

How do you feel about something like this:

If GSSAPI encryption is enabled and we can acquire GSSAPI
credentials, try to set up GSSAPI encryption instead of sending the
startup message. If we succeed, don't set up SSL.

Thanks,
--Robbie