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
Attachments:
gssapi_encryption_startup_comment_fix_v1.patchtext/x-diff; charset=us-asciiDownload
From 49a57d5040c487c65cd9968504e978d11b4aefca Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@snowman.net>
Date: Mon, 6 Jan 2020 16:49:02 -0500
Subject: [PATCH] Improve GSSAPI Encryption startup comment in libpq
The original comment was a bit confusing, pointed out by Alvaro Herrera.
Thread: https://postgr.es/m/20191224151520.GA16435%40alvherre.pgsql
---
src/interfaces/libpq/fe-connect.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 3bd30482ec..89b134665b 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2800,10 +2800,12 @@ keep_going: /* We will come back to here until there is
#ifdef ENABLE_GSS
/*
- * 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.
+ * 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).
*/
if (conn->try_gss && !conn->gctx)
conn->try_gss = pg_GSS_have_cred_cache(&conn->gcred);
--
2.20.1
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