Обсуждение: weird libpq GSSAPI comment

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

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

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.

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

Thanks!

-- 
Álvaro Herrera                                http://www.twitter.com/alvherre

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@2ndquadrant.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

On 2019-Dec-27, Stephen Frost wrote:

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

Hmm, ok, yeah I certainly didn't understand that -- I was thinking that
the call was creating the credential cache itself, not a *handle* to
access it (I suppose that terminology must be clear to somebody familiar
with GSS).

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

                 * If GSSAPI is enabled and we can reach a credential cache,
                 * set up a handle for it; if it's operating, just send a
                 * GSS startup message, instead of the SSL negotiation and
                 * regular startup message below.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Stephen Frost <sfrost@snowman.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@2ndquadrant.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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

> How about this?
>
>                  * If GSSAPI is enabled and we can reach a credential cache,
>                  * set up a handle for it; if it's operating, just send a
>                  * GSS startup message, instead of the SSL negotiation and
>                  * regular startup message below.

Due to the way postgres handled this historically, there are two ways
GSSAPI can be used: for connection encryption, and for authentication
only.  We perform the same dance of sending a "request packet" for
GSSAPI encryption as we do for TLS encryption.  So I'd like us to be
precise about which one we're talking about here (encryption).

The GSSAPI idiom I should have used is "can acquire credentials" (i.e.,
instead of "can reach a credential cache" in your proposal).

There's no such thing as a "GSS startup message".  After negotiating
GSSAPI/TLS encryption (or failing to do so), we send the same things in
all cases, which includes negotiation of authentication mechanism if
any.  (Negotiating GSSAPI for authentication after negotiating GSSAPI
for encryption will short-circuit rather than establishing a second
context, if I remember right.)

I wonder if part of the confusion might be due to the synonyms we're
using here for "in use".  Things seem to be "got running", "set up",
"operating", "negotiated", ... - maybe that's part of the barrier to
understanding?

Thanks,
--Robbie

Greetings,

* Robbie Harwood (rharwood@redhat.com) wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>
> > How about this?
> >
> >                  * If GSSAPI is enabled and we can reach a credential cache,
> >                  * set up a handle for it; if it's operating, just send a
> >                  * GSS startup message, instead of the SSL negotiation and
> >                  * regular startup message below.
>
> Due to the way postgres handled this historically, there are two ways
> GSSAPI can be used: for connection encryption, and for authentication
> only.  We perform the same dance of sending a "request packet" for
> GSSAPI encryption as we do for TLS encryption.  So I'd like us to be
> precise about which one we're talking about here (encryption).

Alright, that's fair.

> The GSSAPI idiom I should have used is "can acquire credentials" (i.e.,
> instead of "can reach a credential cache" in your proposal).

Ok.

> There's no such thing as a "GSS startup message".  After negotiating
> GSSAPI/TLS encryption (or failing to do so), we send the same things in
> all cases, which includes negotiation of authentication mechanism if
> any.  (Negotiating GSSAPI for authentication after negotiating GSSAPI
> for encryption will short-circuit rather than establishing a second
> context, if I remember right.)

Yes, you can see that around src/backend/libpq/auth.c:538 where we skip
straight to pg_GSS_checkauth() if we already have encryption up and
running, and if we don't then we go through pg_GSS_recvauth() (which
will eventually call pg_GSS_checkauth() too).

> I wonder if part of the confusion might be due to the synonyms we're
> using here for "in use".  Things seem to be "got running", "set up",
> "operating", "negotiated", ... - maybe that's part of the barrier to
> understanding?

How about something like this?

 * If GSSAPI Encryption is enabled, then call pg_GSS_have_cred_cache()
 * which will return true if we can acquire credentials (and give us a
 * handle to use in conn->gcred), and then send a packet to the server
 * asking for GSSAPI Encryption (and skip past SSL negotiation and
 * regular startup below).

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

>> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>
> How about something like this?
>
>  * If GSSAPI Encryption is enabled, then call pg_GSS_have_cred_cache()
>  * which will return true if we can acquire credentials (and give us a
>  * handle to use in conn->gcred), and then send a packet to the server
>  * asking for GSSAPI Encryption (and skip past SSL negotiation and
>  * regular startup below).

This looks correct to me (and uses plenty of parentheticals, so it feels
in keeping with something I'd write) :)

Thanks,
--Robbie

On 2020-Jan-06, Stephen Frost wrote:

> > I wonder if part of the confusion might be due to the synonyms we're
> > using here for "in use".  Things seem to be "got running", "set up",
> > "operating", "negotiated", ... - maybe that's part of the barrier to
> > understanding?
> 
> How about something like this?
> 
>  * If GSSAPI Encryption is enabled, then call pg_GSS_have_cred_cache()
>  * which will return true if we can acquire credentials (and give us a
>  * handle to use in conn->gcred), and then send a packet to the server
>  * asking for GSSAPI Encryption (and skip past SSL negotiation and
>  * regular startup below).

WFM.  (I'm not sure why you uppercase Encryption, though.)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello,

On 2020-Jan-06, Robbie Harwood wrote:

> This looks correct to me (and uses plenty of parentheticals, so it feels
> in keeping with something I'd write) :)

(You know, long ago I used to write with a lot of parenthicals (even
nested ones).  But I read somewhere that prose is more natural for
normal people without them, so I mostly stopped using them.)

Cheers,

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Greetings,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> On 2020-Jan-06, Stephen Frost wrote:
>
> > > I wonder if part of the confusion might be due to the synonyms we're
> > > using here for "in use".  Things seem to be "got running", "set up",
> > > "operating", "negotiated", ... - maybe that's part of the barrier to
> > > understanding?
> >
> > How about something like this?
> >
> >  * If GSSAPI Encryption is enabled, then call pg_GSS_have_cred_cache()
> >  * which will return true if we can acquire credentials (and give us a
> >  * handle to use in conn->gcred), and then send a packet to the server
> >  * asking for GSSAPI Encryption (and skip past SSL negotiation and
> >  * regular startup below).
>
> WFM.  (I'm not sure why you uppercase Encryption, though.)

Ok, great, attached is an actual patch which I'll push soon if there
aren't any other comments.

Thanks!

Stephen

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:
> * Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> > On 2020-Jan-06, Stephen Frost wrote:
> > > > I wonder if part of the confusion might be due to the synonyms we're
> > > > using here for "in use".  Things seem to be "got running", "set up",
> > > > "operating", "negotiated", ... - maybe that's part of the barrier to
> > > > understanding?
> > >
> > > How about something like this?
> > >
> > >  * If GSSAPI Encryption is enabled, then call pg_GSS_have_cred_cache()
> > >  * which will return true if we can acquire credentials (and give us a
> > >  * handle to use in conn->gcred), and then send a packet to the server
> > >  * asking for GSSAPI Encryption (and skip past SSL negotiation and
> > >  * regular startup below).
> >
> > WFM.  (I'm not sure why you uppercase Encryption, though.)
>
> Ok, great, attached is an actual patch which I'll push soon if there
> aren't any other comments.

Pushed.

Thanks!

Stephen