Re: [PATCH v20] GSSAPI encryption support

[Michael Paquier]

On Fri, Apr 19, 2019 at 09:25:14PM -0400, Stephen Frost wrote:
> Great, glad to hear it.

What you have committed looks fine seen from here.  Thanks for taking
care of the issue, Stephen.
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Michael Paquier (mich...@paquier.xyz) wrote:
> On Mon, Apr 15, 2019 at 08:24:52AM -0400, Stephen Frost wrote:
> > The tests are really fast enough with one KDC that I don't think it
> > makes sense to have two independent tests.
> 
> Perhaps you should add a comment about the need of unicity at the top
> of 001_auth.pl with a short description of the test?

I added some comments there that I think explain why it makes sense to
have just one test file there.

> > Please find attached a patch which updates the protocol.sgml docs that
> > Michael mentioned before, and merges the tests into one test file (while
> > adding in some additional tests to make sure that the server also agrees
> > with what our expectations are, using the pg_stat_gssapi view).
> 
> Thanks for addressing all that feedback.  Parallel runs look more
> stable on my side.  At least it seems that I can re-enable it safely.

Great, glad to hear it.

> > I'll push this soon unless there are concerns.  If you get a chance to
> > test the patch out, that would be great.  It's working happily for me
> > locally.
> 
> +calling gss_init_sec_context() in a loop and sending the result to the
> Some markups should be added here for all function names.  Not all the
> clients use C either, so you may want to say "or equivalent"?

I added the markups for function names along with a sentence fragment
saying that the functions referenced are the C GSSAPI bindings, and that
equivilants can be used.

> +test_access($node, 'test1', 'SELECT gss_authenticated AND encrypted
> from pg_stat_gssapi where pid = pg_backend_pid();', 0, '', 'succeeds
> with mapping with default gssencmode and host hba');
> +test_access($node, "test1", 'SELECT gss_authenticated AND encrypted
> from pg_stat_gssapi where pid = pg_backend_pid();', 0,
> "gssencmode=prefer", "succeeds with GSS-encrypted access preferred
> with host hba");
> +test_access($node, "test1", 'SELECT gss_authenticated AND encrypted
> from pg_stat_gssapi where pid = pg_backend_pid();', 0,
> "gssencmode=require", "succeeds with GSS-encrypted access required
> with host hba");
> If you could rework a bit the indentation of the new code added in
> kerberos/t/001_auth.pl that would be nice.  I am afraid that the
> current format makes debugging harder than necessary.

I ran perltidy on it, sorry, should have done that before.

> +$node->append_conf('pg_hba.conf',
> +   qq{hostgssenc all all $hostaddr/32 gss map=mymap});
> +$node->restart;
> A reload should be enough but not race-condition free, which is why a
> set of restarts is done in this test right?  (I have noticed that it
> is done this way since the beginning.)

Right, we want this to be a restart as Peter mentions downthread.

I've now pushed these changes and will mark the open item as addressed.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Peter Eisentraut]

On 2019-04-16 06:36, Michael Paquier wrote:
> +$node->append_conf('pg_hba.conf',
> +   qq{hostgssenc all all $hostaddr/32 gss map=mymap});
> +$node->restart;
> A reload should be enough but not race-condition free, which is why a
> set of restarts is done in this test right?  (I have noticed that it
> is done this way since the beginning.)

We got rid of all the reloads in these kinds of tests because they have
the effect that if the configuration has an error, the reload just
ignores it.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH v20] GSSAPI encryption support

[Michael Paquier]

On Mon, Apr 15, 2019 at 08:24:52AM -0400, Stephen Frost wrote:
> The tests are really fast enough with one KDC that I don't think it
> makes sense to have two independent tests.

Perhaps you should add a comment about the need of unicity at the top
of 001_auth.pl with a short description of the test?

> Please find attached a patch which updates the protocol.sgml docs that
> Michael mentioned before, and merges the tests into one test file (while
> adding in some additional tests to make sure that the server also agrees
> with what our expectations are, using the pg_stat_gssapi view).

Thanks for addressing all that feedback.  Parallel runs look more
stable on my side.  At least it seems that I can re-enable it safely.

> I'll push this soon unless there are concerns.  If you get a chance to
> test the patch out, that would be great.  It's working happily for me
> locally.

+calling gss_init_sec_context() in a loop and sending the result to the
Some markups should be added here for all function names.  Not all the
clients use C either, so you may want to say "or equivalent"?

+test_access($node, 'test1', 'SELECT gss_authenticated AND encrypted
from pg_stat_gssapi where pid = pg_backend_pid();', 0, '', 'succeeds
with mapping with default gssencmode and host hba');
+test_access($node, "test1", 'SELECT gss_authenticated AND encrypted
from pg_stat_gssapi where pid = pg_backend_pid();', 0,
"gssencmode=prefer", "succeeds with GSS-encrypted access preferred
with host hba");
+test_access($node, "test1", 'SELECT gss_authenticated AND encrypted
from pg_stat_gssapi where pid = pg_backend_pid();', 0,
"gssencmode=require", "succeeds with GSS-encrypted access required
with host hba");
If you could rework a bit the indentation of the new code added in
kerberos/t/001_auth.pl that would be nice.  I am afraid that the
current format makes debugging harder than necessary.

+$node->append_conf('pg_hba.conf',
+   qq{hostgssenc all all $hostaddr/32 gss map=mymap});
+$node->restart;
A reload should be enough but not race-condition free, which is why a
set of restarts is done in this test right?  (I have noticed that it
is done this way since the beginning.)
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Robbie Harwood]

Stephen Frost  writes:

> Please find attached a patch which updates the protocol.sgml docs that
> Michael mentioned before, and merges the tests into one test file
> (while adding in some additional tests to make sure that the server
> also agrees with what our expectations are, using the pg_stat_gssapi
> view).

I can't speak to the Perl, but the documentation matches what I think
the code does :)

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 2019-04-09 09:32, Peter Eisentraut wrote:
> > On 2019-04-09 04:51, Stephen Frost wrote:
> >>> Running just 002_enc.pl by itself passes the tests!
> >> Great!  I think what I'll do is work to incorporate the two tests back
> >> into one script, to avoid whatever the race condition or other confusion
> >> is happening on macOS here.
> > 
> > That seems reasonable.  It also avoids the large amount of duplicate
> > setup code.
> 
> Another problem is that the two test files cannot be run in parallel
> because they use the same hardcoded data directories.  That would have
> to be replaced by temporary directories.

The tests are really fast enough with one KDC that I don't think it
makes sense to have two independent tests.

> The race condition alluded to above appears to be simply that at the end
> of the test the kdc is shut down by kill -INT but nothing waits for that
> to finish before a new one is started by the next test.  So there is
> potential for all kinds of confusion.

Ah, good to know that's what was happening.

> Putting it all in one test file seems to be the easiest way out.

Please find attached a patch which updates the protocol.sgml docs that
Michael mentioned before, and merges the tests into one test file (while
adding in some additional tests to make sure that the server also agrees
with what our expectations are, using the pg_stat_gssapi view).

I'll push this soon unless there are concerns.  If you get a chance to
test the patch out, that would be great.  It's working happily for me
locally.

Thanks!

Stephen
From e0745810ba1582601f2c71db3b67a8cfc2480546 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Sun, 14 Apr 2019 21:14:42 -0400
Subject: [PATCH] GSSAPI: Improve documentation and tests

The GSSAPI encryption patch neglected to update the protocol
documentation to describe how to set up a GSSAPI encrypted connection
from a client to the server, so fix that by adding the appropriate
documentation to protocol.sgml.

The tests added for encryption support were overly long and couldn't be
run in parallel due to race conditions; this was largely because each
test was setting up its own KDC to perform the tests.  Instead, merge
the authentication tests and the encryption tests into one test file,
where we only create one KDC to run the tests with.  Also, have the
tests check what the server's opinion is of the connection and if it was
GSS authenticated or encrypted using the previously added pg_stat_gssapi
view.

In passing, fix the libpq label for GSSENC-Mode to be consistent with
the "PGGSSENCMODE" environment variable.

Missing protocol documentation pointed out by Michael Paquier.
Issues with the tests pointed out by Tom Lane and Peter Eisentraut.

Refactored tests and added documentation by me.
---
 doc/src/sgml/protocol.sgml| 103 
 src/interfaces/libpq/fe-connect.c |   2 +-
 src/test/kerberos/t/001_auth.pl   |  48 ++--
 src/test/kerberos/t/002_enc.pl| 197 --
 4 files changed, 141 insertions(+), 209 deletions(-)
 delete mode 100644 src/test/kerberos/t/002_enc.pl

diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 09893d96b5..204dca7c33 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -1479,6 +1479,72 @@ SELCT 1/0;
 of authentication checking.

   
+
+  
+   GSSAPI Session Encryption
+
+   
+If PostgreSQL was built with
+GSSAPI support, frontend/backend communications
+can be encrypted using GSSAPI.  This provides
+communication security in environments where attackers might be
+able to capture the session traffic. For more information on
+encrypting PostgreSQL sessions with
+GSSAPI, see .
+   
+
+   
+To initiate a GSSAPI-encrypted connection, the
+frontend initially sends a GSSENCRequest message rather than a
+StartupMessage.  The server then responds with a single byte
+containing G or N, indicating that it
+is willing or unwilling to perform GSSAPI encryption,
+respectively.  The frontend might close the connection at this point
+if it is dissatisfied with the response.  To continue after
+G, perform a GSSAPI initialization by
+calling gss_init_sec_context() in a loop and sending the result to the
+server, starting with an empty input and then with each result from the
+server, until it returns no output.  When sending the results of
+gss_init_sec_context() to the server, prepend the length of the message as a
+four byte integer in network byte order.  If this is successful, then use
+gss_wrap() to encrypt the usual StartupMessage and all subsequent data,
+prepending the length of the result from gss_wrap() as a four byte integer
+in network byte order to the actual encrypted payload.  Note that the server
+will only accept encrypted packets from the client which are less than 16KB;
+

Re: [PATCH v20] GSSAPI encryption support

[Michael Paquier]

On Fri, Apr 12, 2019 at 10:22:03AM +0200, Peter Eisentraut wrote:
> Another problem is that the two test files cannot be run in parallel
> because they use the same hardcoded data directories.  That would have
> to be replaced by temporary directories.

Please note that I have added an open item about the instability of
these tests.

If ones's PG_TEST_EXTRA uses kerberos, just using PROVE_FLAGS="-j4" or
such to run multiple scripts in parallel makes the test failure very
easy to reproduce.  That's my case, and I had to disable the test for
now...
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Peter Eisentraut]

On 2019-04-09 09:32, Peter Eisentraut wrote:
> On 2019-04-09 04:51, Stephen Frost wrote:
>>> Running just 002_enc.pl by itself passes the tests!
>> Great!  I think what I'll do is work to incorporate the two tests back
>> into one script, to avoid whatever the race condition or other confusion
>> is happening on macOS here.
> 
> That seems reasonable.  It also avoids the large amount of duplicate
> setup code.

Another problem is that the two test files cannot be run in parallel
because they use the same hardcoded data directories.  That would have
to be replaced by temporary directories.

The race condition alluded to above appears to be simply that at the end
of the test the kdc is shut down by kill -INT but nothing waits for that
to finish before a new one is started by the next test.  So there is
potential for all kinds of confusion.

Putting it all in one test file seems to be the easiest way out.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH v20] GSSAPI encryption support

[Robbie Harwood]

Stephen Frost  writes:

> Robbie Harwood (rharw...@redhat.com) wrote:
>> Bruce Momjian  writes:
>>> Magnus Hagander wrote:
 Joe Conway  wrote:

 If it was on the table it might have been better to keep hostgss
 and change the authentication method to gssauth or something, but
 that ship sailed *years* ago.
>>>
>>> Uh, did we consider keeping hostgss and changing the auth part at
>>> the end to "gssauth"?
>> 
>> I think that was implicitly rejected because we'd have to keep the
>> capability to configure "gss" there else break compatibility.
>
> Right, if we changed the name of the auth method then everyone who is
> using the "gss" auth method would have to update their pg_hba.conf
> files...  That would be very ugly.  Also, it wasn't implicitly
> rejected, it was discussed up-thread (see the comments between Magnus
> and I, specifically, quoted above- "that ship sailed *years* ago") and
> explicitly rejected.

Apologies, you're right of course.  I intended to say why *I* had
rejected it but got bit by the passive voice.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Magnus Hagander]

On Thu, Apr 11, 2019 at 3:56 PM Robert Haas  wrote:

> On Wed, Apr 10, 2019 at 9:47 PM Stephen Frost  wrote:
> > Right, if we changed the name of the auth method then everyone who is
> > using the "gss" auth method would have to update their pg_hba.conf
> > files...  That would be very ugly.  Also, it wasn't implicitly rejected,
> > it was discussed up-thread (see the comments between Magnus and I,
> > specifically, quoted above- "that ship sailed *years* ago") and
> > explicitly rejected.
>
> Slightly off-topic, but I am not familiar with GSSAPI and don't quite
> understand what the benefits of GSSAPI encryption are as compared with
> OpenSSL encryption.  I am sure there must be some; otherwise, nobody
> would have bothered writing, reviewing, and committing this patch.
> Can somebody enlighten me?
>

You don't need to set up an SSL PKI.

Yes you need the similar keys and stuff set up for GSSAPI, but if you
already *have* those (which you do if you are using gss authentication for
example) then it's a lot less extra overhead.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: [PATCH v20] GSSAPI encryption support

[Robert Haas]

On Wed, Apr 10, 2019 at 9:47 PM Stephen Frost  wrote:
> Right, if we changed the name of the auth method then everyone who is
> using the "gss" auth method would have to update their pg_hba.conf
> files...  That would be very ugly.  Also, it wasn't implicitly rejected,
> it was discussed up-thread (see the comments between Magnus and I,
> specifically, quoted above- "that ship sailed *years* ago") and
> explicitly rejected.

Slightly off-topic, but I am not familiar with GSSAPI and don't quite
understand what the benefits of GSSAPI encryption are as compared with
OpenSSL encryption.  I am sure there must be some; otherwise, nobody
would have bothered writing, reviewing, and committing this patch.
Can somebody enlighten me?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Robbie Harwood (rharw...@redhat.com) wrote:
> Bruce Momjian  writes:
> > On Wed, Apr  3, 2019 at 08:49:25AM +0200, Magnus Hagander wrote:
> >> On Wed, Apr 3, 2019 at 12:22 AM Joe Conway  wrote:
> >>
> >> Personally I don't find it as confusing as is either, and I find
> >> hostgss to be a good analog of hostssl. On the other hand hostgssenc
> >> is long and unintuitive. So +1 for leaving as is and -1 one for
> >> changing it IMHO.
> >> 
> >> I think for those who are well versed in pg_hba (and maybe gss as
> >> well), it's not confusing. That includes me.
> >> 
> >> However, for a new user, I can definitely see how it can be
> >> considered confusing. And confusion in *security configuration* is
> >> always a bad idea, even if it's just potential.
> >> 
> >> Thus +1 on changing it.
> >> 
> >> If it was on the table it might have been better to keep hostgss and
> >> change the authentication method to gssauth or something, but that
> >> ship sailed *years* ago.
> >
> > Uh, did we consider keeping hostgss and changing the auth part at the
> > end to "gssauth"?
> 
> I think that was implicitly rejected because we'd have to keep the
> capability to configure "gss" there else break compatibility.

Right, if we changed the name of the auth method then everyone who is
using the "gss" auth method would have to update their pg_hba.conf
files...  That would be very ugly.  Also, it wasn't implicitly rejected,
it was discussed up-thread (see the comments between Magnus and I,
specifically, quoted above- "that ship sailed *years* ago") and
explicitly rejected.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Robbie Harwood]

Bruce Momjian  writes:

> On Wed, Apr  3, 2019 at 08:49:25AM +0200, Magnus Hagander wrote:
>> On Wed, Apr 3, 2019 at 12:22 AM Joe Conway  wrote:
>>
>> Personally I don't find it as confusing as is either, and I find
>> hostgss to be a good analog of hostssl. On the other hand hostgssenc
>> is long and unintuitive. So +1 for leaving as is and -1 one for
>> changing it IMHO.
>> 
>> I think for those who are well versed in pg_hba (and maybe gss as
>> well), it's not confusing. That includes me.
>> 
>> However, for a new user, I can definitely see how it can be
>> considered confusing. And confusion in *security configuration* is
>> always a bad idea, even if it's just potential.
>> 
>> Thus +1 on changing it.
>> 
>> If it was on the table it might have been better to keep hostgss and
>> change the authentication method to gssauth or something, but that
>> ship sailed *years* ago.
>
> Uh, did we consider keeping hostgss and changing the auth part at the
> end to "gssauth"?

I think that was implicitly rejected because we'd have to keep the
capability to configure "gss" there else break compatibility.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Bruce Momjian]

On Wed, Apr  3, 2019 at 08:49:25AM +0200, Magnus Hagander wrote:
> On Wed, Apr 3, 2019 at 12:22 AM Joe Conway  wrote:
> Personally I don't find it as confusing as is either, and I find hostgss
> to be a good analog of hostssl. On the other hand hostgssenc is long and
> unintuitive. So +1 for leaving as is and -1 one for changing it IMHO.
> 
> 
> I think for those who are well versed in pg_hba (and maybe gss as well), it's
> not confusing. That includes me.
> 
> However, for a new user, I can definitely see how it can be considered
> confusing. And confusion in *security configuration* is always a bad idea, 
> even
> if it's just potential.
> 
> Thus +1 on changing it.
> 
> If it was on the table it might have been better to keep hostgss and change 
> the
> authentication method to gssauth or something, but that ship sailed *years*
> ago.

Uh, did we consider keeping hostgss and changing the auth part at the
end to "gssauth"?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [PATCH v20] GSSAPI encryption support

[Peter Eisentraut]

On 2019-04-09 06:11, Tom Lane wrote:
> I tried to replicate this on my own laptop (macOS 10.14.4 ... I do not
> think there is or ever will be a 10.14.14).

right

> kerberos test fails immediately:
> 
> 1..4
> # setting up Kerberos
> # Running: krb5-config --version
> # Running: kdb5_util create -s -P secret0
> Can't exec "kdb5_util": No such file or directory at 
> /Users/tgl/pgsql/src/test/kerberos/../../../src/test/perl/TestLib.pm line 190.
> Bail out!  system kdb5_util failed
> 
> and indeed, there's no kdb5_util in /usr/bin/ or anywhere else that
> I can find.  So I speculate that Peter is running some weird hodgepodge
> of Apple and Homebrew code, making the question not so much "why does
> it fail" as "how did it ever work".

Yes, you need a krb5 installation from either Homebrew or MacPorts.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH v20] GSSAPI encryption support

[Peter Eisentraut]

On 2019-04-09 04:51, Stephen Frost wrote:
>> Running just 002_enc.pl by itself passes the tests!
> Great!  I think what I'll do is work to incorporate the two tests back
> into one script, to avoid whatever the race condition or other confusion
> is happening on macOS here.

That seems reasonable.  It also avoids the large amount of duplicate
setup code.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH v20] GSSAPI encryption support

[Tom Lane]

Peter Eisentraut  writes:
> On 2019-04-05 23:37, Stephen Frost wrote:
>> I've also reached out to some colleagues about having one of them test
>> with MacOS.  What version are you on..? 

> macOS 10.14.14 it says.

I tried to replicate this on my own laptop (macOS 10.14.4 ... I do not
think there is or ever will be a 10.14.14).  I can't, because the
kerberos test fails immediately:

1..4
# setting up Kerberos
# Running: krb5-config --version
# Running: kdb5_util create -s -P secret0
Can't exec "kdb5_util": No such file or directory at 
/Users/tgl/pgsql/src/test/kerberos/../../../src/test/perl/TestLib.pm line 190.
Bail out!  system kdb5_util failed

and indeed, there's no kdb5_util in /usr/bin/ or anywhere else that
I can find.  So I speculate that Peter is running some weird hodgepodge
of Apple and Homebrew code, making the question not so much "why does
it fail" as "how did it ever work".

I also notice that the build spews out a bunch of deprecation warnings,
because just as with openSSL, Apple has stuck deprecation attributes
on everything in gssapi/gssapi.h.  They want you to use their GSS
"framework" instead.

So I'm not convinced we should spend a lot of effort on fooling with
the test scripts for this.  This platform has got much more fundamental
problems than that.

regards, tom lane

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 2019-04-05 23:37, Stephen Frost wrote:
> > I wonder if somehow the keytab file that the server is using isn't
> > getting destroyed between the two test runs and so you're ending up with
> > the server using the key from the old KDC, while the user is using the
> > new one..?  Or something is equally going wrong in the tests.
> > 
> > Could you try doing something like removing the 001_auth.pl, moving the
> > 002_enc.pl to 001_enc.pl, double-check that everything is cleaned up and
> > that there aren't any old PG servers running, et al, and re-try that
> > way?
> 
> Running just 002_enc.pl by itself passes the tests!

Great!  I think what I'll do is work to incorporate the two tests back
into one script, to avoid whatever the race condition or other confusion
is happening on macOS here.

Thanks so much for testing it!

Stephen


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Peter Eisentraut]

On 2019-04-05 23:37, Stephen Frost wrote:
> I wonder if somehow the keytab file that the server is using isn't
> getting destroyed between the two test runs and so you're ending up with
> the server using the key from the old KDC, while the user is using the
> new one..?  Or something is equally going wrong in the tests.
> 
> Could you try doing something like removing the 001_auth.pl, moving the
> 002_enc.pl to 001_enc.pl, double-check that everything is cleaned up and
> that there aren't any old PG servers running, et al, and re-try that
> way?

Running just 002_enc.pl by itself passes the tests!

> I've also reached out to some colleagues about having one of them test
> with MacOS.  What version are you on..? 

macOS 10.14.14 it says.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 2019-04-05 14:48, Stephen Frost wrote:
> > On a failure to set up an encrypted connection, we'll actually fall back
> > to a non-encrypted one using GSSAPI *just* for authentication, which is> 
> > why I was asking if this worked before the encryption patch went in.
> 
> The tests have always worked before.  I've run them probably hundreds of
> times.

Ok.

> > Also, which of the tests are still failing, exactly?  The authentication
> > ones or the encryption ones or both?
> 
> Only the encryption ones:
> 
> not ok 1 - GSS-encrypted access
> 
> #   Failed test 'GSS-encrypted access'
> #   at t/002_enc.pl line 170.
> #  got: '2'
> # expected: '0'
> ok 2 - GSS encryption disabled
> not ok 3 - GSS encryption without auth
> 
> #   Failed test 'GSS encryption without auth'
> #   at t/002_enc.pl line 170.
> #  got: '2'
> # expected: '0'
> not ok 4 - GSS unencrypted fallback
> 
> #   Failed test 'GSS unencrypted fallback'
> #   at t/002_enc.pl line 170.
> #  got: '2'
> # expected: '0'
> ok 5 - GSS unencrypted fallback prevention

So, looking back at the error message you got:

FATAL:  GSSAPI context error
DETAIL:   Miscellaneous failure (see text): Decrypt integrity check
failed for checksum type hmac-sha1-96-aes256, key type
aes256-cts-hmac-sha1-96

FATAL:  accepting GSS security context failed
DETAIL:   Miscellaneous failure (see text): Decrypt integrity check
failed for checksum type hmac-sha1-96-aes256, key type
aes256-cts-hmac-sha1-96

(assuming that's still what you're getting..?)

What this is saying is basically that the key that the PG server is
using from its keytab and the key used to encrypt the ticket that the
user has (from the KDC) don't match up.

I wonder if somehow the keytab file that the server is using isn't
getting destroyed between the two test runs and so you're ending up with
the server using the key from the old KDC, while the user is using the
new one..?  Or something is equally going wrong in the tests.

Could you try doing something like removing the 001_auth.pl, moving the
002_enc.pl to 001_enc.pl, double-check that everything is cleaned up and
that there aren't any old PG servers running, et al, and re-try that
way?

I've also reached out to some colleagues about having one of them test
with MacOS.  What version are you on..? 

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Peter Eisentraut]

On 2019-04-05 14:48, Stephen Frost wrote:
> All of it was built against the OS-provided Kerberos install, and you
> got the failure..?

right

> On a failure to set up an encrypted connection, we'll actually fall back
> to a non-encrypted one using GSSAPI *just* for authentication, which is> why 
> I was asking if this worked before the encryption patch went in.

The tests have always worked before.  I've run them probably hundreds of
times.

> Also, which of the tests are still failing, exactly?  The authentication
> ones or the encryption ones or both?

Only the encryption ones:

not ok 1 - GSS-encrypted access

#   Failed test 'GSS-encrypted access'
#   at t/002_enc.pl line 170.
#  got: '2'
# expected: '0'
ok 2 - GSS encryption disabled
not ok 3 - GSS encryption without auth

#   Failed test 'GSS encryption without auth'
#   at t/002_enc.pl line 170.
#  got: '2'
# expected: '0'
not ok 4 - GSS unencrypted fallback

#   Failed test 'GSS unencrypted fallback'
#   at t/002_enc.pl line 170.
#  got: '2'
# expected: '0'
ok 5 - GSS unencrypted fallback prevention

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH v20] GSSAPI encryption support

[Robbie Harwood]

Stephen Frost  writes:

> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
>> On 2019-04-05 04:59, Stephen Frost wrote:
>>
>>> Alright, that over-size error was a bug in the error-handling code,
>>> which I've just pushed a fix for.  That said...
>> 
>> Yes, that looks better now.
>
> Great.
>
>>> This looks like it's a real issue and it's unclear what's going on
>>> here.  I wonder- are you certain that you're using all the same
>>> Kerberos libraries for the KDC, the server, and psql?
>> 
>> Right, it was built against the OS-provided Kerberos installation
>> (/usr/bin etc.).  If I build against the Homebrew-provided one then
>> the tests pass.
>
> All of it was built against the OS-provided Kerberos install, and you
> got the failure..?
>
>> So maybe that means that this encryption feature is not supported on
>> that (presumably older) installation?  (krb5-config --version says
>> "Kerberos 5 release 1.7-prerelease") Is that plausible?  Is a gentler
>> failure mode possible?

Heimdal never had a 1.7 release - they went from 1.5.2 to 7.1.0.

MIT did have a 1.7 release - in 2009.

Apple doesn't open source their Kerberos implementation, so I can't
exactly point a debugger at it.  But if it's in fact somehow related to
MIT 1.7-prerelease, I imagine they inherited a bug or two that's been
fixed in the ten years since then.

As for the code: I'm not doing anything complicated.  The interface I'm
using is as specified in RFC2743 and RFC2744, which is from 2000 (though
I think technically I'm mostly backward compatible to RFC1509, from
1993), and Kerberos V5 itself is specified in RFC4120 (from 2005).

> On a failure to set up an encrypted connection, we'll actually fall
> back to a non-encrypted one, using GSSAPI *just* for authentication,
> which is why I was asking if this worked before the encryption patch
> went in.  Also, which of the tests are still failing, exactly?  The
> authentication ones or the encryption ones or both?

Good question.

> If we determine that this is some issue with the MacOS-provided
> Kerberos libraries, then we could try to detect them and disable
> GSSAPI encryption in that case explicitly, I suppose, but I've seen
> odd things with the MacOS-provided Kerberos libraries before on
> released versions of PG (without any encryption support), so I'm not
> yet convinced that this is an issue that's specific to adding support
> for encryption.

If we have to, a version check >1.7 would probably work.  That'll remove
the ability to work on RHEL/CentOS 5, but that's probably fine, and I'm
not aware of any other supported OSs that would be impacted.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 2019-04-05 04:59, Stephen Frost wrote:
> > Alright, that over-size error was a bug in the error-handling code,
> > which I've just pushed a fix for.  That said...
> 
> Yes, that looks better now.

Great.

> > This looks like it's a real issue and it's unclear what's going on here.
> > I wonder- are you certain that you're using all the same Kerberos
> > libraries for the KDC, the server, and psql?
> 
> Right, it was built against the OS-provided Kerberos installation
> (/usr/bin etc.).  If I build against the Homebrew-provided one then the
> tests pass.

All of it was built against the OS-provided Kerberos install, and you
got the failure..?

> So maybe that means that this encryption feature is not supported on
> that (presumably older) installation?  (krb5-config --version says
> "Kerberos 5 release 1.7-prerelease")  Is that plausible?  Is a gentler
> failure mode possible?

On a failure to set up an encrypted connection, we'll actually fall back
to a non-encrypted one, using GSSAPI *just* for authentication, which is
why I was asking if this worked before the encryption patch went in.
Also, which of the tests are still failing, exactly?  The authentication
ones or the encryption ones or both?

If we determine that this is some issue with the MacOS-provided Kerberos
libraries, then we could try to detect them and disable GSSAPI
encryption in that case explicitly, I suppose, but I've seen odd things
with the MacOS-provided Kerberos libraries before on released versions
of PG (without any encryption support), so I'm not yet convinced that
this is an issue that's specific to adding support for encryption.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Peter Eisentraut]

On 2019-04-05 04:59, Stephen Frost wrote:
> Alright, that over-size error was a bug in the error-handling code,
> which I've just pushed a fix for.  That said...

Yes, that looks better now.

> This looks like it's a real issue and it's unclear what's going on here.
> I wonder- are you certain that you're using all the same Kerberos
> libraries for the KDC, the server, and psql?

Right, it was built against the OS-provided Kerberos installation
(/usr/bin etc.).  If I build against the Homebrew-provided one then the
tests pass.

So maybe that means that this encryption feature is not supported on
that (presumably older) installation?  (krb5-config --version says
"Kerberos 5 release 1.7-prerelease")  Is that plausible?  Is a gentler
failure mode possible?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> Kerberos tests are now failing for me (macOS).  I'm seeing
> 
> psql: error: could not connect to server: Over-size error packet sent by
> the server.
> not ok 3 - GSS encryption without auth
> 
> #   Failed test 'GSS encryption without auth'
> #   at t/002_enc.pl line 170.
> #  got: '2'
> # expected: '0'
> 
> (and repeated for several other tests).

Alright, that over-size error was a bug in the error-handling code,
which I've just pushed a fix for.  That said...

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 2019-04-04 17:35, Stephen Frost wrote:
> > Ok, it looks like there's a server-side error happening here, and it
> > would be good to see what that is, so can you send the server logs?
> 
> These errors appear several times in the server logs:
> 
> FATAL:  GSSAPI context error
> DETAIL:   Miscellaneous failure (see text): Decrypt integrity check
> failed for checksum type hmac-sha1-96-aes256, key type
> aes256-cts-hmac-sha1-96
> 
> FATAL:  accepting GSS security context failed
> DETAIL:   Miscellaneous failure (see text): Decrypt integrity check
> failed for checksum type hmac-sha1-96-aes256, key type
> aes256-cts-hmac-sha1-96

This looks like it's a real issue and it's unclear what's going on here.
I wonder- are you certain that you're using all the same Kerberos
libraries for the KDC, the server, and psql?

If you go back to before the GSSAPI encryption patch, does it work..?

I've certainly seen interesting issues on MacOS, in particular, due to
different Kerberos libraries/tools being installed and I wonder if
that's what is going on here.  Maybe you could check klist vs. psql wrt
what libraries are linked in?

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Robbie Harwood]

Tom Lane  writes:

> I wrote:
>> Stephen Frost  writes:
>>> So I'm a bit surprised that it's taking 4 minutes for you.  I wonder if
>>> there might be an issue related to the KDC wanting to get some amount of
>>> random data and the system you're on isn't producing random bytes very
>>> fast..?
>
>> Not sure.  This is my usual development box and it also does mail, DNS,
>> etc for my household, so I'd expect it to have plenty of entropy.
>> But it's running a pretty old kernel, and old Kerberos too, so maybe
>> the explanation is in there somewhere.
>
> Same test on a laptop running Fedora 28 takes a shade under 5 seconds.
> The laptop has a somewhat better geekbench rating than my workstation,
> but certainly not 50x better.  And I really doubt it's got more entropy
> sources than the workstation.  Gotta be something about the kernel.
>
> Watching the test logs, I see that essentially all the time on the RHEL6
> machine is consumed by the two
>
> # Running: /usr/sbin/kdb5_util create -s -P secret0
>
> steps.  Is there a case for merging the two scripts so we only have to
> do that once?  Maybe not, if nobody else sees this.

I think that would be a good idea!  Unfortunately I don't speak perl
well enough to do that, so I'd just copied-and-modified.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Robbie Harwood]

Tom Lane  writes:

> Stephen Frost  writes:
>> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>>> Well, if the caller thinks what is being passed back is an int,
>>> it will do a 32-to-64-bit widening, which is almost certainly
>>> going to result in a corrupted pointer.
>
>> Oh, good point.  Interesting that it still works then.
>
> There must be something about the x86_64 ABI that allows this to
> accidentally work -- maybe integers are presumed to be sign-extended
> to 64 bits by callee not caller?  I added some logging and verified
> that pgstat.c is seeing the correct string value, so it's working
> somehow.
>
>> I've got a fix for the missing prototypes, I hadn't noticed the issue
>> previously due to always building with SSL enabled as well.
>
> Yeah, I'd just come to the conclusion that it's because I didn't
> include --with-openssl, and libpq-be.h's #ifdef nest doesn't expect
> that.
>
> BTW, the kerberos test suite takes nearly 4 minutes for me, is
> it supposed to be so slow?

My guess is entropy problems as well.  If available, configuring
/dev/urandom passthrough from the host is a generally helpful thing to
do.

My (Fedora, Centos/RHEL 7+) krb5 builds use getrandom() for entropy, so
they shouldn't be slow; I believe Debian also has started doing so
recently as well.  I don't know what other distros/OSs do for this.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> Watching the test logs, I see that essentially all the time on the RHEL6
>> machine is consumed by the two
>> # Running: /usr/sbin/kdb5_util create -s -P secret0
>> steps.  Is there a case for merging the two scripts so we only have to
>> do that once?  Maybe not, if nobody else sees this.

> I do think that mergeing them would be a good idea and I can look into
> that, though at least locally that step takes less than a second..  I
> wonder if you might strace (or whatever is appropriate) that kdb5_util
> and see what's taking so long.  I seriously doubt it's the actual
> kdb5_util code and strongly suspect it's some kernel call.

"strace -r" pins the blame pretty firmly on /dev/random:

 0.76 open("/dev/random", O_RDONLY) = 3
 0.000227 fcntl(3, F_SETFD, FD_CLOEXEC) = 0
 0.61 fstat(3, {st_mode=S_IFCHR|0666, st_rdev=makedev(1, 8), ...}) = 0
 0.68 read(3, "\336&\301\310V\344q\217\264-\262\320w-", 64) = 14
 0.91 read(3, "\326\353I\371$\361", 50) = 6
15.328306 read(3, "\214\301\313]I\325", 44) = 6
17.418929 read(3, "z\251\37\275\365\24", 38) = 6
13.366997 read(3, "6\257I\315f\3", 32) = 6
11.457994 read(3, "\370\275\2765\31(", 26) = 6
23.472194 read(3, "\226\r\314\373\2014", 20) = 6
11.746848 read(3, "\335\336BR\30\322", 14) = 6
20.823940 read(3, "\366\214\r\211\0267", 8) = 6
14.429214 read(3, ",g", 2)  = 2
15.494835 close(3)  = 0

There's no other part of the trace that takes more than ~ 0.1s.
So this boils down to the old bugaboo about how much entropy
there really is in /dev/random.

regards, tom lane

Re: [PATCH v20] GSSAPI encryption support

[Tom Lane]

I wrote:
> Stephen Frost  writes:
>> So I'm a bit surprised that it's taking 4 minutes for you.  I wonder if
>> there might be an issue related to the KDC wanting to get some amount of
>> random data and the system you're on isn't producing random bytes very
>> fast..?

> Not sure.  This is my usual development box and it also does mail, DNS,
> etc for my household, so I'd expect it to have plenty of entropy.
> But it's running a pretty old kernel, and old Kerberos too, so maybe
> the explanation is in there somewhere.

Same test on a laptop running Fedora 28 takes a shade under 5 seconds.
The laptop has a somewhat better geekbench rating than my workstation,
but certainly not 50x better.  And I really doubt it's got more entropy
sources than the workstation.  Gotta be something about the kernel.

Watching the test logs, I see that essentially all the time on the RHEL6
machine is consumed by the two

# Running: /usr/sbin/kdb5_util create -s -P secret0

steps.  Is there a case for merging the two scripts so we only have to
do that once?  Maybe not, if nobody else sees this.

regards, tom lane

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> Kerberos tests are now failing for me (macOS).  I'm seeing
> 
> psql: error: could not connect to server: Over-size error packet sent by
> the server.
> not ok 3 - GSS encryption without auth
> 
> #   Failed test 'GSS encryption without auth'
> #   at t/002_enc.pl line 170.
> #  got: '2'
> # expected: '0'
> 
> (and repeated for several other tests).

Ok, it looks like there's a server-side error happening here, and it
would be good to see what that is, so can you send the server logs?

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> There must be something about the x86_64 ABI that allows this to
>> accidentally work -- maybe integers are presumed to be sign-extended
>> to 64 bits by callee not caller?  I added some logging and verified
>> that pgstat.c is seeing the correct string value, so it's working
>> somehow.

> Huh, I'm not sure.  That's certainly interesting though.

Oh, no, it's simpler than that: the pointer values that
be_gssapi_get_princ() is returning just happen to be less than 2^31
on my system.  I'd dismissed that as being unlikely, but it's the truth.

> So I'm a bit surprised that it's taking 4 minutes for you.  I wonder if
> there might be an issue related to the KDC wanting to get some amount of
> random data and the system you're on isn't producing random bytes very
> fast..?

Not sure.  This is my usual development box and it also does mail, DNS,
etc for my household, so I'd expect it to have plenty of entropy.
But it's running a pretty old kernel, and old Kerberos too, so maybe
the explanation is in there somewhere.

regards, tom lane

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> Well, if the caller thinks what is being passed back is an int,
> >> it will do a 32-to-64-bit widening, which is almost certainly
> >> going to result in a corrupted pointer.
> 
> > Oh, good point.  Interesting that it still works then.
> 
> There must be something about the x86_64 ABI that allows this to
> accidentally work -- maybe integers are presumed to be sign-extended
> to 64 bits by callee not caller?  I added some logging and verified
> that pgstat.c is seeing the correct string value, so it's working
> somehow.

Huh, I'm not sure.  That's certainly interesting though.

> > I've got a fix for the missing prototypes, I hadn't noticed the issue
> > previously due to always building with SSL enabled as well.
> 
> Yeah, I'd just come to the conclusion that it's because I didn't
> include --with-openssl, and libpq-be.h's #ifdef nest doesn't expect
> that.

Right, that should be fixed now with the commit I just pushed.

> BTW, the kerberos test suite takes nearly 4 minutes for me, is
> it supposed to be so slow?

Unfortunately, the kerberos test suite requires building a KDC to get
tickets from and that takes a bit of time.  On my laptop it takes about
8s:

make -s check  4.67s user 0.85s system 70% cpu 7.819 total

So I'm a bit surprised that it's taking 4 minutes for you.  I wonder if
there might be an issue related to the KDC wanting to get some amount of
random data and the system you're on isn't producing random bytes very
fast..?

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Peter Eisentraut]

On 2019-04-04 17:16, Tom Lane wrote:
> BTW, the kerberos test suite takes nearly 4 minutes for me, is
> it supposed to be so slow?

I've seen this on some virtualized machines that didn't have a lot of
entropy.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH v20] GSSAPI encryption support

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> Well, if the caller thinks what is being passed back is an int,
>> it will do a 32-to-64-bit widening, which is almost certainly
>> going to result in a corrupted pointer.

> Oh, good point.  Interesting that it still works then.

There must be something about the x86_64 ABI that allows this to
accidentally work -- maybe integers are presumed to be sign-extended
to 64 bits by callee not caller?  I added some logging and verified
that pgstat.c is seeing the correct string value, so it's working
somehow.

> I've got a fix for the missing prototypes, I hadn't noticed the issue
> previously due to always building with SSL enabled as well.

Yeah, I'd just come to the conclusion that it's because I didn't
include --with-openssl, and libpq-be.h's #ifdef nest doesn't expect
that.

BTW, the kerberos test suite takes nearly 4 minutes for me, is
it supposed to be so slow?

regards, tom lane

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> I'm not very sure why the integer/pointer confusion in pgstat_bestart
> >> doesn't cause hard crashes when using gss auth --- or does
> >> this suite not actually test that?
> 
> > Isn't it just saying that because of the implicit declaration..?
> > Once that's fixed, the integer/pointer warning will go away, but
> > it's actually a pointer in either case, hence why it isn't crashing.
> 
> Well, if the caller thinks what is being passed back is an int,
> it will do a 32-to-64-bit widening, which is almost certainly
> going to result in a corrupted pointer.

Oh, good point.  Interesting that it still works then.

I've got a fix for the missing prototypes, I hadn't noticed the issue
previously due to always building with SSL enabled as well.  I'm testing
with a non-SSL build and will push the fix shortly.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> I'm not very sure why the integer/pointer confusion in pgstat_bestart
>> doesn't cause hard crashes when using gss auth --- or does
>> this suite not actually test that?

> Isn't it just saying that because of the implicit declaration..?
> Once that's fixed, the integer/pointer warning will go away, but
> it's actually a pointer in either case, hence why it isn't crashing.

Well, if the caller thinks what is being passed back is an int,
it will do a 32-to-64-bit widening, which is almost certainly
going to result in a corrupted pointer.

> The test suite does test GSS authentication and GSS encryption.

Hm.  I'll poke at this more closely.

regards, tom lane

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > On Thu, Apr 4, 2019 at 05:20 Peter Eisentraut <
> > peter.eisentr...@2ndquadrant.com> wrote:
> >> Kerberos tests are now failing for me (macOS).
> 
> > Interesting, they work locally for me on Ubuntu.  Unfortunately, I don’t
> > have macOS.  This only happens when encryption is being used, presumably?
> > GSS authentication is still working fine?
> 
> The kerberos test suite passes for me on RHEL6 (kerberos 1.10.3),
> but I observe some compiler warnings that need to be dealt with:

Interesting, I don't see those with my build.  I'll have to figure out
why not.  Will fix them in any case.

> $ ./configure --with-gssapi ...
> $ time make -j8 -s
> be-secure-gssapi.c:597: warning: no previous prototype for 
> 'be_gssapi_get_auth'
> be-secure-gssapi.c:609: warning: no previous prototype for 'be_gssapi_get_enc'
> be-secure-gssapi.c:621: warning: no previous prototype for 
> 'be_gssapi_get_princ'
> pgstat.c: In function 'pgstat_bestart':
> pgstat.c:2986: warning: implicit declaration of function 'be_gssapi_get_auth'
> pgstat.c:2987: warning: implicit declaration of function 'be_gssapi_get_enc'
> pgstat.c:2990: warning: implicit declaration of function 'be_gssapi_get_princ'
> pgstat.c:2990: warning: passing argument 2 of 'strlcpy' makes pointer from 
> integer without a cast
> ../../../src/include/port.h:429: note: expected 'const char *' but argument 
> is of type 'int'
> All of PostgreSQL successfully made. Ready to install.
> 
> I'm not very sure why the integer/pointer confusion in pgstat_bestart
> doesn't cause hard crashes when using gss auth --- or does
> this suite not actually test that?

Isn't it just saying that because of the implicit declaration..?
Once that's fixed, the integer/pointer warning will go away, but
it's actually a pointer in either case, hence why it isn't crashing.

The test suite does test GSS authentication and GSS encryption.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Tom Lane]

Stephen Frost  writes:
> On Thu, Apr 4, 2019 at 05:20 Peter Eisentraut <
> peter.eisentr...@2ndquadrant.com> wrote:
>> Kerberos tests are now failing for me (macOS).

> Interesting, they work locally for me on Ubuntu.  Unfortunately, I don’t
> have macOS.  This only happens when encryption is being used, presumably?
> GSS authentication is still working fine?

The kerberos test suite passes for me on RHEL6 (kerberos 1.10.3),
but I observe some compiler warnings that need to be dealt with:

$ ./configure --with-gssapi ...
$ time make -j8 -s
be-secure-gssapi.c:597: warning: no previous prototype for 'be_gssapi_get_auth'
be-secure-gssapi.c:609: warning: no previous prototype for 'be_gssapi_get_enc'
be-secure-gssapi.c:621: warning: no previous prototype for 'be_gssapi_get_princ'
pgstat.c: In function 'pgstat_bestart':
pgstat.c:2986: warning: implicit declaration of function 'be_gssapi_get_auth'
pgstat.c:2987: warning: implicit declaration of function 'be_gssapi_get_enc'
pgstat.c:2990: warning: implicit declaration of function 'be_gssapi_get_princ'
pgstat.c:2990: warning: passing argument 2 of 'strlcpy' makes pointer from 
integer without a cast
../../../src/include/port.h:429: note: expected 'const char *' but argument is 
of type 'int'
All of PostgreSQL successfully made. Ready to install.

I'm not very sure why the integer/pointer confusion in pgstat_bestart
doesn't cause hard crashes when using gss auth --- or does
this suite not actually test that?

regards, tom lane

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

On Thu, Apr 4, 2019 at 05:20 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> Kerberos tests are now failing for me (macOS).  I'm seeing
>
> psql: error: could not connect to server: Over-size error packet sent by
> the server.
> not ok 3 - GSS encryption without auth
>
> #   Failed test 'GSS encryption without auth'
> #   at t/002_enc.pl line 170.
> #  got: '2'
> # expected: '0'
>
> (and repeated for several other tests).


Interesting, they work locally for me on Ubuntu.  Unfortunately, I don’t
have macOS.  This only happens when encryption is being used, presumably?
GSS authentication is still working fine?

Thanks,

Stephen

Re: [PATCH v20] GSSAPI encryption support

[Peter Eisentraut]

Kerberos tests are now failing for me (macOS).  I'm seeing

psql: error: could not connect to server: Over-size error packet sent by
the server.
not ok 3 - GSS encryption without auth

#   Failed test 'GSS encryption without auth'
#   at t/002_enc.pl line 170.
#  got: '2'
# expected: '0'

(and repeated for several other tests).

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH v20] GSSAPI encryption support

[Michael Paquier]

On Wed, Apr 03, 2019 at 10:09:54PM -0400, Stephen Frost wrote:
> Yes, that’s a fair point. I’ll work on adding documentation to
> protocol.sgml for the GSSAPI encrypted setup and message passing.

Thanks.  I have added an open item to track.
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

On Wed, Apr 3, 2019 at 22:02 Michael Paquier  wrote:

> On Wed, Apr 03, 2019 at 05:51:06PM -0400, Stephen Frost wrote:
> > Thanks so much for pushing on it for so long, it’s a great feature to
> have!
>
> Glad to see that the final result is using an API layer in
> be-secure.c and that we have tests.  Now, shouldn't there be some
> documentation in protocol.sgml for the read and write handling of the
> encrypted messages?  Other drivers could need to implement that stuff,
> no?  We have that for SSL, with SSLRequest and such.


Yes, that’s a fair point. I’ll work on adding documentation to
protocol.sgml for the GSSAPI encrypted setup and message passing.

Thanks,

Stephen

>

Re: [PATCH v20] GSSAPI encryption support

[Michael Paquier]

On Wed, Apr 03, 2019 at 05:51:06PM -0400, Stephen Frost wrote:
> Thanks so much for pushing on it for so long, it’s a great feature to have!

Glad to see that the final result is using an API layer in
be-secure.c and that we have tests.  Now, shouldn't there be some
documentation in protocol.sgml for the read and write handling of the
encrypted messages?  Other drivers could need to implement that stuff,
no?  We have that for SSL, with SSLRequest and such.
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings Robbie,

On Wed, Apr 3, 2019 at 17:47 Robbie Harwood  wrote:

> Stephen Frost  writes:
>
> > On Wed, Apr 3, 2019 at 16:01 Andres Freund  wrote:
> >> On 2019-04-03 10:43:33 -0400, Stephen Frost wrote:
> >>
> >>> I'll push this in a few hours unless there's anything else.
> >>
> >> The CF entry for this is still open - is there any work missing? Just
> >> trying to do some triage...
> >>
> >> https://commitfest.postgresql.org/22/1647/
> >
> > No, I was just waiting to make sure the buildfarm was happy, which it
> > seems to be.  I can take care of the entry in 30m or so, or if you’d
> > like to close it out, that would be fine too.
>
> Thanks for merging!  I'll stick around the mailing list/IRC for a while
> on the off-chance that anything comes up, but the project should feel
> free to reach out to me directly with Kerberos-related issues in the
> future.


Thanks so much for pushing on it for so long, it’s a great feature to have!

I would love to see Kerberos/GSSAPI grow a way to find out what the
encryption used on the connection is, as we had discussed before...  Do we
know if that encryption must match the encryption type of the tickets
acquired?  Would it be possible to inspect the ticket in the same way that
klist does, to determine the encryption that must be used?  And similarly
the key tab on the server side?  Though of course there can be more than
one but maybe we can find out which was used?

Just some thoughts for future improvements here.

Thanks!

Stephen

>

Re: [PATCH v20] GSSAPI encryption support

[Robbie Harwood]

Stephen Frost  writes:

> On Wed, Apr 3, 2019 at 16:01 Andres Freund  wrote:
>> On 2019-04-03 10:43:33 -0400, Stephen Frost wrote:
>>
>>> I'll push this in a few hours unless there's anything else.
>>
>> The CF entry for this is still open - is there any work missing? Just
>> trying to do some triage...
>>
>> https://commitfest.postgresql.org/22/1647/
>
> No, I was just waiting to make sure the buildfarm was happy, which it
> seems to be.  I can take care of the entry in 30m or so, or if you’d
> like to close it out, that would be fine too.

Thanks for merging!  I'll stick around the mailing list/IRC for a while
on the off-chance that anything comes up, but the project should feel
free to reach out to me directly with Kerberos-related issues in the
future.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

On Wed, Apr 3, 2019 at 16:01 Andres Freund  wrote:

> Hi,
>
> On 2019-04-03 10:43:33 -0400, Stephen Frost wrote:
> > I'll push this in a few hours unless there's anything else.
>
> The CF entry for this is still open - is there any work missing? Just
> trying to do some triage...
>
> https://commitfest.postgresql.org/22/1647/


No, I was just waiting to make sure the buildfarm was happy, which it seems
to be.  I can take care of the entry in 30m or so, or if you’d like to
close it out, that would be fine too.

Thanks!

Stephen

> 

Re: [PATCH v20] GSSAPI encryption support

[Andres Freund]

Hi,

On 2019-04-03 10:43:33 -0400, Stephen Frost wrote:
> I'll push this in a few hours unless there's anything else.

The CF entry for this is still open - is there any work missing? Just
trying to do some triage...

https://commitfest.postgresql.org/22/1647/

- Andres

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Magnus Hagander (mag...@hagander.net) wrote:
> On Wed, Apr 3, 2019 at 12:22 AM Joe Conway  wrote:
> > On 4/2/19 6:18 PM, Stephen Frost wrote:
> > > On Tue, Apr 2, 2019 at 18:10 Peter Eisentraut
> > >  > > > wrote:
> > >
> > > On 2019-02-23 17:27, Stephen Frost wrote:
> > > >> About pg_hba.conf: The "hostgss" keyword seems a bit confusing.
> > > It only
> > > >> applies to encrypted gss-using connections, not all of them.
> > Maybe
> > > >> "hostgssenc" or "hostgsswrap"?
> > > > Not quite sure what you mean here, but 'hostgss' seems to be quite
> > > well
> > > > in-line with what we do for SSL...  as in, we have 'hostssl', we
> > don't
> > > > say 'hostsslenc'.  I feel like I'm just not understanding what you
> > > mean
> > > > by "not all of them".
> > >
> > > Reading the latest patch, I think this is still a bit confusing.
> > > Consider an entry like
> > >
> > > hostgss all all 0.0.0.0/0
> > >    gss
> > >
> > > The "hostgss" part means, the connection is GSS-*encrypted*.  The
> > "gss"
> > > entry in the last column means use gss for *authentication*.  But
> > didn't
> > > "hostgss" already imply that?  No.  I understand what's going on,
> > but it
> > > seems quite confusing.  They both just say "gss"; you have to know a
> > lot
> > > about the nuances of pg_hba.conf processing to get that.
> > >
> > > If you have line like
> > >
> > > hostgss all all 0.0.0.0/0
> > >    md5
> > >
> > > it is not obvious that this means, if GSS-encrypted, use md5.  It
> > could
> > > just as well mean, if GSS-authenticated, use md5.
> > >
> > > The analogy with SSL is such that we use "hostssl" for connections
> > using
> > > SSL encryption and "cert" for the authentication method.  So there we
> > > use two different words for two different aspects of SSL.
> > >
> > >
> > > I don’t view it as confusing, but I’ll change it to hostgssenc as was
> > > suggested earlier to address that concern.  It’s a bit wordy but if it
> > > helps reduce confusion then that’s a good thing.
> >
> > Personally I don't find it as confusing as is either, and I find hostgss
> > to be a good analog of hostssl. On the other hand hostgssenc is long and
> > unintuitive. So +1 for leaving as is and -1 one for changing it IMHO.
> 
> I think for those who are well versed in pg_hba (and maybe gss as well),
> it's not confusing. That includes me.
> 
> However, for a new user, I can definitely see how it can be considered
> confusing. And confusion in *security configuration* is always a bad idea,
> even if it's just potential.
> 
> Thus +1 on changing it.

Alright, I've made that change, and also changed "gssmode" to be
"gssencmode" to be both consistent and also clearer (that, imv anyway,
is actually a much better reason to go to using 'gssenc' instead of just
'gss' for this, since "gssmode" could be thought of as being related to
GSS authentication rather than being for GSS encryption).

> If it was on the table it might have been better to keep hostgss and change
> the authentication method to gssauth or something, but that ship sailed
> *years* ago.

Agreed, we certainly can't change that now.

Updated patch attached with the host[no]gss -> host[no]gssenc and
gssmode -> gssencmode changes, along with some other minor improvements.
I'll push this in a few hours unless there's anything else.

Thanks!

Stephen
From ac0a6f2d9f7c8a5c3cd65cdd9291681f8600f26a Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Sat, 19 Jan 2019 11:30:20 -0500
Subject: [PATCH] GSSAPI encryption support

On both the frontend and backend, prepare for GSSAPI encryption
support by moving common code for error handling into a separate file.
Fix a TODO for handling multiple status messages in the process.
Eliminate the OIDs, which have not been needed for some time.

Add frontend and backend encryption support functions.  Keep the
context initiation for authentication-only separate on both the
frontend and backend in order to avoid concerns about changing the
requested flags to include encryption support.

In postmaster, pull GSSAPI authorization checking into a shared
function.  Also share the initiator name between the encryption and
non-encryption codepaths.

For HBA, add "hostgssenc" and "hostnogssenc" entries that behave
similarly to their SSL counterparts.  "hostgssenc" requires either
"gss", "trust", or "reject" for its authentication.

Similarly, add a "gssencmode" parameter to libpq.  Supported values are
"disable", "require", and "prefer".  Notably, negotiation will only be
attempted if credentials can be acquired.  Move credential acquisition
into its own function to support this behavior.

Add a simple pg_stat_gssapi view similar to pg_stat_ssl, for monitoring
if GSSAPI authentication 

Re: [PATCH v20] GSSAPI encryption support

[Magnus Hagander]

On Wed, Apr 3, 2019 at 12:22 AM Joe Conway  wrote:

> On 4/2/19 6:18 PM, Stephen Frost wrote:
> > Greetings,
> >
> > On Tue, Apr 2, 2019 at 18:10 Peter Eisentraut
> >  > > wrote:
> >
> > On 2019-02-23 17:27, Stephen Frost wrote:
> > >> About pg_hba.conf: The "hostgss" keyword seems a bit confusing.
> > It only
> > >> applies to encrypted gss-using connections, not all of them.
> Maybe
> > >> "hostgssenc" or "hostgsswrap"?
> > > Not quite sure what you mean here, but 'hostgss' seems to be quite
> > well
> > > in-line with what we do for SSL...  as in, we have 'hostssl', we
> don't
> > > say 'hostsslenc'.  I feel like I'm just not understanding what you
> > mean
> > > by "not all of them".
> >
> > Reading the latest patch, I think this is still a bit confusing.
> > Consider an entry like
> >
> > hostgss all all 0.0.0.0/0
> >    gss
> >
> > The "hostgss" part means, the connection is GSS-*encrypted*.  The
> "gss"
> > entry in the last column means use gss for *authentication*.  But
> didn't
> > "hostgss" already imply that?  No.  I understand what's going on,
> but it
> > seems quite confusing.  They both just say "gss"; you have to know a
> lot
> > about the nuances of pg_hba.conf processing to get that.
> >
> > If you have line like
> >
> > hostgss all all 0.0.0.0/0
> >    md5
> >
> > it is not obvious that this means, if GSS-encrypted, use md5.  It
> could
> > just as well mean, if GSS-authenticated, use md5.
> >
> > The analogy with SSL is such that we use "hostssl" for connections
> using
> > SSL encryption and "cert" for the authentication method.  So there we
> > use two different words for two different aspects of SSL.
> >
> >
> > I don’t view it as confusing, but I’ll change it to hostgssenc as was
> > suggested earlier to address that concern.  It’s a bit wordy but if it
> > helps reduce confusion then that’s a good thing.
>
> Personally I don't find it as confusing as is either, and I find hostgss
> to be a good analog of hostssl. On the other hand hostgssenc is long and
> unintuitive. So +1 for leaving as is and -1 one for changing it IMHO.
>

I think for those who are well versed in pg_hba (and maybe gss as well),
it's not confusing. That includes me.

However, for a new user, I can definitely see how it can be considered
confusing. And confusion in *security configuration* is always a bad idea,
even if it's just potential.

Thus +1 on changing it.

If it was on the table it might have been better to keep hostgss and change
the authentication method to gssauth or something, but that ship sailed
*years* ago.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: [PATCH v20] GSSAPI encryption support

[Joe Conway]

On 4/2/19 6:18 PM, Stephen Frost wrote:
> Greetings,
> 
> On Tue, Apr 2, 2019 at 18:10 Peter Eisentraut
>  > wrote:
> 
> On 2019-02-23 17:27, Stephen Frost wrote:
> >> About pg_hba.conf: The "hostgss" keyword seems a bit confusing. 
> It only
> >> applies to encrypted gss-using connections, not all of them.  Maybe
> >> "hostgssenc" or "hostgsswrap"?
> > Not quite sure what you mean here, but 'hostgss' seems to be quite
> well
> > in-line with what we do for SSL...  as in, we have 'hostssl', we don't
> > say 'hostsslenc'.  I feel like I'm just not understanding what you
> mean
> > by "not all of them".
> 
> Reading the latest patch, I think this is still a bit confusing.
> Consider an entry like
> 
>     hostgss all             all             0.0.0.0/0
>                gss
> 
> The "hostgss" part means, the connection is GSS-*encrypted*.  The "gss"
> entry in the last column means use gss for *authentication*.  But didn't
> "hostgss" already imply that?  No.  I understand what's going on, but it
> seems quite confusing.  They both just say "gss"; you have to know a lot
> about the nuances of pg_hba.conf processing to get that.
> 
> If you have line like
> 
>     hostgss all             all             0.0.0.0/0
>                md5
> 
> it is not obvious that this means, if GSS-encrypted, use md5.  It could
> just as well mean, if GSS-authenticated, use md5.
> 
> The analogy with SSL is such that we use "hostssl" for connections using
> SSL encryption and "cert" for the authentication method.  So there we
> use two different words for two different aspects of SSL.
> 
> 
> I don’t view it as confusing, but I’ll change it to hostgssenc as was
> suggested earlier to address that concern.  It’s a bit wordy but if it
> helps reduce confusion then that’s a good thing.

Personally I don't find it as confusing as is either, and I find hostgss
to be a good analog of hostssl. On the other hand hostgssenc is long and
unintuitive. So +1 for leaving as is and -1 one for changing it IMHO.

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

On Tue, Apr 2, 2019 at 18:10 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 2019-02-23 17:27, Stephen Frost wrote:
> >> About pg_hba.conf: The "hostgss" keyword seems a bit confusing.  It only
> >> applies to encrypted gss-using connections, not all of them.  Maybe
> >> "hostgssenc" or "hostgsswrap"?
> > Not quite sure what you mean here, but 'hostgss' seems to be quite well
> > in-line with what we do for SSL...  as in, we have 'hostssl', we don't
> > say 'hostsslenc'.  I feel like I'm just not understanding what you mean
> > by "not all of them".
>
> Reading the latest patch, I think this is still a bit confusing.
> Consider an entry like
>
> hostgss all all 0.0.0.0/0   gss
>
> The "hostgss" part means, the connection is GSS-*encrypted*.  The "gss"
> entry in the last column means use gss for *authentication*.  But didn't
> "hostgss" already imply that?  No.  I understand what's going on, but it
> seems quite confusing.  They both just say "gss"; you have to know a lot
> about the nuances of pg_hba.conf processing to get that.
>
> If you have line like
>
> hostgss all all 0.0.0.0/0   md5
>
> it is not obvious that this means, if GSS-encrypted, use md5.  It could
> just as well mean, if GSS-authenticated, use md5.
>
> The analogy with SSL is such that we use "hostssl" for connections using
> SSL encryption and "cert" for the authentication method.  So there we
> use two different words for two different aspects of SSL.


I don’t view it as confusing, but I’ll change it to hostgssenc as was
suggested earlier to address that concern.  It’s a bit wordy but if it
helps reduce confusion then that’s a good thing.

Thanks,

Stephen

>

Re: [PATCH v20] GSSAPI encryption support

[Peter Eisentraut]

On 2019-02-23 17:27, Stephen Frost wrote:
>> About pg_hba.conf: The "hostgss" keyword seems a bit confusing.  It only
>> applies to encrypted gss-using connections, not all of them.  Maybe
>> "hostgssenc" or "hostgsswrap"?
> Not quite sure what you mean here, but 'hostgss' seems to be quite well
> in-line with what we do for SSL...  as in, we have 'hostssl', we don't
> say 'hostsslenc'.  I feel like I'm just not understanding what you mean
> by "not all of them".

Reading the latest patch, I think this is still a bit confusing.
Consider an entry like

hostgss all all 0.0.0.0/0   gss

The "hostgss" part means, the connection is GSS-*encrypted*.  The "gss"
entry in the last column means use gss for *authentication*.  But didn't
"hostgss" already imply that?  No.  I understand what's going on, but it
seems quite confusing.  They both just say "gss"; you have to know a lot
about the nuances of pg_hba.conf processing to get that.

If you have line like

hostgss all all 0.0.0.0/0   md5

it is not obvious that this means, if GSS-encrypted, use md5.  It could
just as well mean, if GSS-authenticated, use md5.

The analogy with SSL is such that we use "hostssl" for connections using
SSL encryption and "cert" for the authentication method.  So there we
use two different words for two different aspects of SSL.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH v20] GSSAPI encryption support

[Robbie Harwood]

Stephen Frost  writes:

> One of the things that I really didn't care for in this patch was the
> use of the string buffers, without any real checks (except for "oh,
> you tried to allocated over 1G"...) to make sure that the other side
> of the connection wasn't feeding us ridiculous packets, and with the
> resizing of the buffers, etc, that really shouldn't be necessary.
> After chatting with Robbie about these concerns while reading through
> the code, we agreed that we should be able to use fixed buffer sizes
> and use the quite helpful gss_wrap_size_limit() to figure out how much
> data we can encrypt without going over our fixed buffer size.  As
> Robbie didn't have time to implement those changes this past week, I
> did so, and added a bunch more comments and such too, and have now
> gone through and done more testing.  Robbie has said that he should
> have time this upcoming week to review the changes that I made, and
> I'm planning to go back and review other parts of the patch more
> closely now as well.

In general this looks good - there are a couple minor comments inline,
but it's fine.

I wanted to note a couple things about this approach.  It now uses one
more buffer than before (in contrast to the previous approach, which
reused a buffer for received data that was encrypted and decrypted).
Since these are static fixed size buffers, this increases the total
steady-state memory usage by 16k as opposed to re-using the buffer.
This may be fine; I don't know how tight RAM is here.

> Note that there's an issue with exporting the context to get the
> encryption algorithm used that I've asked Robbie to look into, so
> that's no longer done and instead we just print that the connection is
> encrypted, if it is.  If we can't figure out a way to make that work
> then obviously I'll pull out that code, and if we can get it to work
> then I'll update it to be done through libpq properly, as I had
> suggested earlier.  That's really more of a nice to have in any case
> though, so I may just exclude it for now anyway if it ends up adding
> any complications.

Correct.  Unfortunately I'd overlooked that the lucid interface won't
meet our needs (destroys the context).  So the two options here are:
SASL SSF (and I'll separately push more mechs to add support for that),
or nothing at all.

If you want a patch for that I can make one, but I think there was code
already... just needed a ./configure check program for whether the OID
is defined.

> +ssize_t
> +be_gssapi_write(Port *port, void *ptr, size_t len)
> +{
> + size_t  bytes_to_encrypt = len;
> + size_t  bytes_encrypted = 0;
> +
> + /*
> +  * Loop through encrypting data and sending it out until
> +  * secure_raw_write() complains (which would likely mean that the socket
> +  * is non-blocking and the requested send() would block, or there was 
> some
> +  * kind of actual error) and then return.
> +  */
> + while (bytes_to_encrypt || PqGSSSendPointer)
> + {

I guess it's not a view everyone will share, but I think this block is
too long.  Maybe a helper function around secure_raw_write() would help?
(The check-and-send part at the start.)

I have similar nits about the other functions that don't fit on my
(86-line high) screen, though I guess a lot of this is due to project
style using a lot of vertical space.

> + if (GSS_ERROR(major))
> + pg_GSS_error(FATAL, gettext_noop("GSSAPI context error"),

I'd prefer this to be a different error message than the init/accept
errors - maybe something like "GSSAPI size check error"?

> + if (GSS_ERROR(major))
> + pg_GSS_error(libpq_gettext("GSSAPI context error"), 
> conn,

Same here.

Again, these are nits, and I think I'm okay with the changes.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings!

* Robbie Harwood (rharw...@redhat.com) wrote:
> Stephen Frost  writes:
> > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> >> Or maybe we avoid that, and you rename be-secure-gssapi.c to just
> >> be-gssapi.c and also combine that with the contents of
> >> be-gssapi-common.c.
> >
> > I don't know why we would need to, or want to, combine
> > be-secure-gssapi.c and be-gssapi-common.c, they do have different
> > roles in that be-gssapi-common.c is used even if you aren't doing
> > encryption, while bs-secure-gssapi.c is specifically for handling the
> > encryption side of things.
> >
> > Then again, be-gssapi-common.c does currently only have the one
> > function in it, and it's not like there's an option to compile for
> > *just* GSS authentication (and not encryption), or at least, I don't
> > think that would be a useful option to have..  Robbie?
> 
> Yeah, I think I'm opposed to making that an option.

Yeah, I tend to agree, seems silly.

> Worth pointing out here: up until v6, I had this structured differently,
> with all the GSSAPI code in fe-gssapi.c and be-gssapi.c.  The current
> separation was suggested by Michael Paquier for ease of reading and to
> keep the code churn down.

I'm still a bit on the fence myself regarding the filenames and where
things exist today.  I do agree that it might make sense to move things
around to make the code structure clearer but I also think that doesn't
necessairly have to be done at the same time as this patch.

> >> (And similarly in libpq.)
> >
> > I do agree that we should try to keep the frontend and backend code
> > structures similar in this area, that seems to make sense.
> 
> Well, I don't know if it's an argument in either direction, but: on the
> frontend we have about twice as much shared code in fe-gssapi-common.c
> (pg_GSS_have_ccache() and pg_GSS_load_servicename()).

Yeah, that's an interesting point, and I do wonder if we will actually
end up having code that's shared between the frontend and backend
eventually (if we can figure out how to pull out the encryption
algorithm info, for example).

> >> I don't see any tests in the patch.  We have a Kerberos test suite at
> >> src/test/kerberos/ and an SSL test suite at src/test/ssl/.  You can
> >> get some ideas there.
> >
> > Yeah, I was going to comment on that as well.  We definitely should
> > implement tests around this.
> 
> Attached.  Please note that I don't really speak perl.  There's a pile
> of duplicated code in 002_enc.pl that probably should be shared between
> the two.  (It would also I think be possible for 001_auth.pl to set up
> the KDC and for 002_enc.pl to then use it.)

I don't think the code duplication between the tests is really all that
much of an issue, though I wouldn't complain if someone wanted to work
on improving that situation.  Thanks a lot for adding those test though!

One of the things that I really didn't care for in this patch was the
use of the string buffers, without any real checks (except for "oh, you
tried to allocated over 1G"...) to make sure that the other side of the
connection wasn't feeding us ridiculous packets, and with the resizing
of the buffers, etc, that really shouldn't be necessary.  After chatting
with Robbie about these concerns while reading through the code, we
agreed that we should be able to use fixed buffer sizes and use the
quite helpful gss_wrap_size_limit() to figure out how much data we can
encrypt without going over our fixed buffer size.  As Robbie didn't have
time to implement those changes this past week, I did so, and added a
bunch more comments and such too, and have now gone through and done
more testing.  Robbie has said that he should have time this upcoming
week to review the changes that I made, and I'm planning to go back and
review other parts of the patch more closely now as well.

Note that there's an issue with exporting the context to get the
encryption algorithm used that I've asked Robbie to look into, so that's
no longer done and instead we just print that the connection is
encrypted, if it is.  If we can't figure out a way to make that work
then obviously I'll pull out that code, and if we can get it to work
then I'll update it to be done through libpq properly, as I had
suggested earlier.  That's really more of a nice to have in any case
though, so I may just exclude it for now anyway if it ends up adding any
complications.

So, please find attached a new version, which also includes the tests
and the other bits that Robbie had sent independent patches for.

Thanks!

Stephen
From ae87c57cc1ca6faacd3e7df08223d8000b1616c6 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Sat, 19 Jan 2019 11:30:20 -0500
Subject: [PATCH] GSSAPI encryption support

On both the frontend and backend, prepare for GSSAPI encryption
support by moving common code for error handling into a separate file.
Fix a TODO for handling multiple status messages in the process.
Eliminate the OIDs, which have not been 

Re: [PATCH v20] GSSAPI encryption support

[Robbie Harwood]

Stephen Frost  writes:

> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
>
>> Or maybe we avoid that, and you rename be-secure-gssapi.c to just
>> be-gssapi.c and also combine that with the contents of
>> be-gssapi-common.c.
>
> I don't know why we would need to, or want to, combine
> be-secure-gssapi.c and be-gssapi-common.c, they do have different
> roles in that be-gssapi-common.c is used even if you aren't doing
> encryption, while bs-secure-gssapi.c is specifically for handling the
> encryption side of things.
>
> Then again, be-gssapi-common.c does currently only have the one
> function in it, and it's not like there's an option to compile for
> *just* GSS authentication (and not encryption), or at least, I don't
> think that would be a useful option to have..  Robbie?

Yeah, I think I'm opposed to making that an option.

Worth pointing out here: up until v6, I had this structured differently,
with all the GSSAPI code in fe-gssapi.c and be-gssapi.c.  The current
separation was suggested by Michael Paquier for ease of reading and to
keep the code churn down.

>> (And similarly in libpq.)
>
> I do agree that we should try to keep the frontend and backend code
> structures similar in this area, that seems to make sense.

Well, I don't know if it's an argument in either direction, but: on the
frontend we have about twice as much shared code in fe-gssapi-common.c
(pg_GSS_have_ccache() and pg_GSS_load_servicename()).

>> I don't see any tests in the patch.  We have a Kerberos test suite at
>> src/test/kerberos/ and an SSL test suite at src/test/ssl/.  You can
>> get some ideas there.
>
> Yeah, I was going to comment on that as well.  We definitely should
> implement tests around this.

Attached.  Please note that I don't really speak perl.  There's a pile
of duplicated code in 002_enc.pl that probably should be shared between
the two.  (It would also I think be possible for 001_auth.pl to set up
the KDC and for 002_enc.pl to then use it.)

Thanks,
--Robbie


signature.asc
Description: PGP signature
>From 42ab1ccae8e517934866ee923d80554ef1996709 Mon Sep 17 00:00:00 2001
From: Robbie Harwood 
Date: Tue, 5 Mar 2019 22:54:11 -0500
Subject: [PATCH] Add tests for GSSAPI/krb5 encryption

---
 src/test/kerberos/t/002_enc.pl | 197 +
 1 file changed, 197 insertions(+)
 create mode 100644 src/test/kerberos/t/002_enc.pl

diff --git a/src/test/kerberos/t/002_enc.pl b/src/test/kerberos/t/002_enc.pl
new file mode 100644
index 00..927abe15e4
--- /dev/null
+++ b/src/test/kerberos/t/002_enc.pl
@@ -0,0 +1,197 @@
+use strict;
+use warnings;
+use TestLib;
+use PostgresNode;
+use Test::More;
+use File::Path 'remove_tree';
+
+if ($ENV{with_gssapi} eq 'yes')
+{
+	plan tests => 5;
+}
+else
+{
+	plan skip_all => 'GSSAPI/Kerberos not supported by this build';
+}
+
+my ($krb5_bin_dir, $krb5_sbin_dir);
+
+if ($^O eq 'darwin')
+{
+	$krb5_bin_dir  = '/usr/local/opt/krb5/bin';
+	$krb5_sbin_dir = '/usr/local/opt/krb5/sbin';
+}
+elsif ($^O eq 'freebsd')
+{
+	$krb5_bin_dir  = '/usr/local/bin';
+	$krb5_sbin_dir = '/usr/local/sbin';
+}
+elsif ($^O eq 'linux')
+{
+	$krb5_sbin_dir = '/usr/sbin';
+}
+
+my $krb5_config  = 'krb5-config';
+my $kinit= 'kinit';
+my $kdb5_util= 'kdb5_util';
+my $kadmin_local = 'kadmin.local';
+my $krb5kdc  = 'krb5kdc';
+
+if ($krb5_bin_dir && -d $krb5_bin_dir)
+{
+	$krb5_config = $krb5_bin_dir . '/' . $krb5_config;
+	$kinit   = $krb5_bin_dir . '/' . $kinit;
+}
+if ($krb5_sbin_dir && -d $krb5_sbin_dir)
+{
+	$kdb5_util= $krb5_sbin_dir . '/' . $kdb5_util;
+	$kadmin_local = $krb5_sbin_dir . '/' . $kadmin_local;
+	$krb5kdc  = $krb5_sbin_dir . '/' . $krb5kdc;
+}
+
+my $host = 'auth-test-localhost.postgresql.example.com';
+my $hostaddr = '127.0.0.1';
+my $realm = 'EXAMPLE.COM';
+
+my $krb5_conf   = "${TestLib::tmp_check}/krb5.conf";
+my $kdc_conf= "${TestLib::tmp_check}/kdc.conf";
+my $krb5_log= "${TestLib::tmp_check}/krb5libs.log";
+my $kdc_log = "${TestLib::tmp_check}/krb5kdc.log";
+my $kdc_port= int(rand() * 16384) + 49152;
+my $kdc_datadir = "${TestLib::tmp_check}/krb5kdc";
+my $kdc_pidfile = "${TestLib::tmp_check}/krb5kdc.pid";
+my $keytab  = "${TestLib::tmp_check}/krb5.keytab";
+
+note "setting up Kerberos";
+
+my ($stdout, $krb5_version);
+run_log [ $krb5_config, '--version' ], '>', \$stdout
+  or BAIL_OUT("could not execute krb5-config");
+BAIL_OUT("Heimdal is not supported") if $stdout =~ m/heimdal/;
+$stdout =~ m/Kerberos 5 release ([0-9]+\.[0-9]+)/
+  or BAIL_OUT("could not get Kerberos version");
+$krb5_version = $1;
+
+append_to_file(
+	$krb5_conf,
+	qq![logging]
+default = FILE:$krb5_log
+kdc = FILE:$kdc_log
+
+[libdefaults]
+default_realm = $realm
+
+[realms]
+$realm = {
+kdc = $hostaddr:$kdc_port
+}!);
+
+append_to_file(
+	$kdc_conf,
+	qq![kdcdefaults]
+!);
+
+# For new-enough versions of krb5, use the _listen settings rather
+# than the _ports settings so that we can bind to localhost only.
+if 

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> I don't know much about GSSAPI, but from what I can tell, this seems an
> attractive feature, and the implementation is compact enough.  I have
> done a bit of work on the internal SSL API refactoring, so I have some
> thoughts on this patch.
> 
> Looking at the file structure, we would have
> 
> be-secure.c
> be-secure-openssl.c
> be-secure-[othersslimpl].c
> be-secure-gssapi.c
> be-secure-common.c
> 
> This implies a code structure that isn't really there.
> be-secure-common.c is used by SSL implementations but not by the GSSAPI
> implementation.

be-secure-common.c seems very clearly mis-named, I mean, look at the
comment at the top of the file:

* common implementation-independent SSL support code

Seems we've been conflating SSL and "Secure" and we should probably fix
that.

What I also really don't like is that "secure_read()" is really only
*maybe* secure.  be-secure.c is really just an IO-abstraction layer that
lets other things hook in and implement the read/write themselves.

> Perhaps we should rename be-secure-openssl.c to be-ssl-openssl.c and
> be-secure-common.c to be-ssl-common.c.

This might be overly pedantic, but what we do in other parts of the tree
is use these things called directories..

I do think we need to rename be-secure-common since it's just flat out
wrong as-is, but that's independent of the GSSAPI encryption work,
really.

> Or maybe we avoid that, and you rename be-secure-gssapi.c to just
> be-gssapi.c and also combine that with the contents of be-gssapi-common.c.

I don't know why we would need to, or want to, combine
be-secure-gssapi.c and be-gssapi-common.c, they do have different roles
in that be-gssapi-common.c is used even if you aren't doing encryption,
while bs-secure-gssapi.c is specifically for handling the encryption
side of things.

Then again, be-gssapi-common.c does currently only have the one function
in it, and it's not like there's an option to compile for *just* GSS
authentication (and not encryption), or at least, I don't think that
would be a useful option to have..  Robbie?

> (And similarly in libpq.)

I do agree that we should try to keep the frontend and backend code
structures similar in this area, that seems to make sense.

> About pg_hba.conf: The "hostgss" keyword seems a bit confusing.  It only
> applies to encrypted gss-using connections, not all of them.  Maybe
> "hostgssenc" or "hostgsswrap"?

Not quite sure what you mean here, but 'hostgss' seems to be quite well
in-line with what we do for SSL...  as in, we have 'hostssl', we don't
say 'hostsslenc'.  I feel like I'm just not understanding what you mean
by "not all of them".

> I don't see any tests in the patch.  We have a Kerberos test suite at
> src/test/kerberos/ and an SSL test suite at src/test/ssl/.  You can get
> some ideas there.

Yeah, I was going to comment on that as well.  We definitely should
implement tests around this.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Peter Eisentraut]

I don't know much about GSSAPI, but from what I can tell, this seems an
attractive feature, and the implementation is compact enough.  I have
done a bit of work on the internal SSL API refactoring, so I have some
thoughts on this patch.

Looking at the file structure, we would have

be-secure.c
be-secure-openssl.c
be-secure-[othersslimpl].c
be-secure-gssapi.c
be-secure-common.c

This implies a code structure that isn't really there.
be-secure-common.c is used by SSL implementations but not by the GSSAPI
implementation.

Perhaps we should rename be-secure-openssl.c to be-ssl-openssl.c and
be-secure-common.c to be-ssl-common.c.

Or maybe we avoid that, and you rename be-secure-gssapi.c to just
be-gssapi.c and also combine that with the contents of be-gssapi-common.c.

(Or maybe both.)

(And similarly in libpq.)

About pg_hba.conf: The "hostgss" keyword seems a bit confusing.  It only
applies to encrypted gss-using connections, not all of them.  Maybe
"hostgssenc" or "hostgsswrap"?

I don't see any tests in the patch.  We have a Kerberos test suite at
src/test/kerberos/ and an SSL test suite at src/test/ssl/.  You can get
some ideas there.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH v20] GSSAPI encryption support

[Robbie Harwood]

Stephen Frost  writes:

> * Robbie Harwood (rharw...@redhat.com) wrote:
>> Stephen Frost  writes:
>>> * Robbie Harwood (rharw...@redhat.com) wrote:
>>>
 Sure!  I'll go ahead and hack up the checks and lucid stuff and get
 back to you.
>>>
>>> Great!  I'll finish fleshing out the basics of how to have this work
>>> server-side and once the checks and lucid stuff are in on the psql
>>> side, it should be pretty straight-forward to copy that same
>>> information into beentry alongside the SSL info, and expose it
>>> through pg_stat_get_activity() into a pg_stat_gss view.
>> 
>> When the mech is gss_mech_krb5 under MIT krb5:
>> 
>> psql (12devel)
>> GSSAPI encrypted connection (krb5 using aes256-cts-hmac-sha384-192)
>> Type "help" for help.
>> 
>> And the same under Heimdal:
>> 
>> psql (12devel)
>> GSSAPI encrypted connection (krb5 using aes256-cts-hmac-sha1-96)
>> Type "help" for help
>> 
>> If the mech weren't gss_krb5, or Lucid introspection failed, but we're a
>> SASL-aware mech (and using MIT):
>> 
>> psql (12devel)
>> GSSAPI encrypted connection (~256 bits)
>> Type "help" for help.
>> 
>> The third case (no lucid, no SASL SSF):
>> 
>> psql (12devel)
>> GSSAPI encrypted connection (unknown mechanism)
>> Type "help" for help.
>> 
>> Feel free to tweak these strings as needed.
>
> That all looks fantastic!  Do you see any reason to not say:
>
> GSSAPI encrypted connection (SASL SSF: ~256 bits)
>
> instead, since that's what we are technically reporting there?

Nope, that'd be fine with me!  (We'd probably want to get rid of the ~
in that case; I'd included it since SASL SSF is an approximate bit
measure, but 256 is the exact SSF.)

>> Another thing I've been thinking about here is whether the SASL SSF
>> logic is useful.  It'll only get invoked when the mechanism isn't
>> gss_mech_krb5, and only under MIT.  SPNEGO (krb5), NTLM
>> (gss-ntlmssp), IAKERB (krb5), and EAP (moonshot) all don't support
>> GSS_C_SEC_CONTEXT_SASL_SSF; I actually couldn't come up with another
>> mechanism that does.  I defer to you on whether to remove that,
>> though.
>
> Oh, hmm, I see.  Yeah, since there's no case where that could actually
> end up being used today then perhaps it makes sense to remove it- it's
> not a terribly good interface anyway since it doesn't provide the
> actual encryption algorithm, I had just gone down that route because I
> saw how to.  The lucid stuff is much better. :)
>
>> I've also adjusted the layering somewhat and moved the actual
>> printf() call down into fe-secure-gssapi.c I don't know whether this
>> model makes more sense in the long run, but for PoC code it was
>> marginally easier to reason about.
>
> No, I think we need to provide a way for libpq-using applications to
> get at that information easily..

Well, it's easier if there's only one type of thing (string) that can be
returned at least.  I imagine the interface there has to be pass
buffer-and-size into the function in fe-secure-gssapi.c then?  Do you
want me to make that change, or would you prefer to do it as part of the
server logging logic?

>> Patch attached after the break; apply on top of -20.
>
> Ok.  I'm pretty amazed at how little code it was to do..

The autotools part took the longest :)

> Is there somewhere that these functions are publicly documented and
> how they can be used from a GSSAPI handle is documented?

Not in the way you're hoping for, I suspect.  This interface is only
intended for consumption by NFS - which needs to pass contexts in and
out of the kernel.  Unlike GSSAPI, Kerberos5 interfaces aren't
standardized at all - parity between MIT and Heimdal is pretty low on
the krb5_*().

I was just referencing MIT's header files:
https://github.com/krb5/krb5/blob/master/src/lib/gssapi/krb5/gssapi_krb5.h#L229
(with the goal in mind of hitting krb5_enctype_to_name())
https://github.com/krb5/krb5/blob/master/src/include/krb5/krb5.hin#L6284-L6302

(Heimdal doesn't have any documentation/example code, but it works the
same way for lucid stuff; I had to look at the source to see how its
variant of krb5_enctype_to_string() worked.)

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Robbie Harwood (rharw...@redhat.com) wrote:
> Stephen Frost  writes:
> > * Robbie Harwood (rharw...@redhat.com) wrote:
> >> Sure!  I'll go ahead and hack up the checks and lucid stuff and get
> >> back to you.
> >
> > Great!  I'll finish fleshing out the basics of how to have this work
> > server-side and once the checks and lucid stuff are in on the psql
> > side, it should be pretty straight-forward to copy that same
> > information into beentry alongside the SSL info, and expose it through
> > pg_stat_get_activity() into a pg_stat_gss view.
> 
> When the mech is gss_mech_krb5 under MIT krb5:
> 
> psql (12devel)
> GSSAPI encrypted connection (krb5 using aes256-cts-hmac-sha384-192)
> Type "help" for help.
> 
> And the same under Heimdal:
> 
> psql (12devel)
> GSSAPI encrypted connection (krb5 using aes256-cts-hmac-sha1-96)
> Type "help" for help
> 
> If the mech weren't gss_krb5, or Lucid introspection failed, but we're a
> SASL-aware mech (and using MIT):
> 
> psql (12devel)
> GSSAPI encrypted connection (~256 bits)
> Type "help" for help.
> 
> The third case (no lucid, no SASL SSF):
> 
> psql (12devel)
> GSSAPI encrypted connection (unknown mechanism)
> Type "help" for help.
> 
> Feel free to tweak these strings as needed.

That all looks fantastic!  Do you see any reason to not say:

GSSAPI encrypted connection (SASL SSF: ~256 bits)

instead, since that's what we are technically reporting there?

> I've also adjusted the layering somewhat and moved the actual printf()
> call down into fe-secure-gssapi.c  I don't know whether this model makes
> more sense in the long run, but for PoC code it was marginally easier to
> reason about.

No, I think we need to provide a way for libpq-using applications to get
at that information easily..

> Another thing I've been thinking about here is whether the SASL SSF
> logic is useful.  It'll only get invoked when the mechanism isn't
> gss_mech_krb5, and only under MIT.  SPNEGO (krb5), NTLM (gss-ntlmssp),
> IAKERB (krb5), and EAP (moonshot) all don't support
> GSS_C_SEC_CONTEXT_SASL_SSF; I actually couldn't come up with another
> mechanism that does.  I defer to you on whether to remove that, though.

Oh, hmm, I see.  Yeah, since there's no case where that could actually
end up being used today then perhaps it makes sense to remove it- it's
not a terribly good interface anyway since it doesn't provide the
actual encryption algorithm, I had just gone down that route because I
saw how to.  The lucid stuff is much better. :)

> I did some testing with Heimdal since we're using some extensions here,
> and found out that the current build machinery doesn't support Heimdal.

Hah!

> (The configure.in detection logic only seems to work for MIT and
> Solaris.)  However, it's possible to explicitly pass in CFLAGS/LDFLAGS
> and it worked prior to my changes, so I've preserved that behavior.

Alright.  That seems like an independent change, if we decide to make
it easier for people to build with heimdal, but there hasn't been much
call for it, clearly.

> Finally, as this patch touches configure.in, configure needs to be
> regenerated; I'm not sure what project policy is on when that happens
> (and it produces rather a lot of churn on my machine).

There's some magic there due to vendor changes to autoconf, as I recall,
which is likely why you're seeing a lot of churn.

> Patch attached after the break; apply on top of -20.

Ok.  I'm pretty amazed at how little code it was to do..  Is there
somewhere that these functions are publicly documented and how they can
be used from a GSSAPI handle is documented?

Thanks a lot for this work!

Stephen


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Robbie Harwood]

Stephen Frost  writes:

> * Robbie Harwood (rharw...@redhat.com) wrote:
>> Stephen Frost  writes:
>>> * Robbie Harwood (rharw...@redhat.com) wrote:
 Stephen Frost  writes:
> * Robbie Harwood (rharw...@redhat.com) wrote:
>
>> Attached please find version 20 of the GSSAPI encryption support.
>> This has been rebased onto master (thanks Stephen for calling out
>> ab69ea9).
>
> I've looked over this again and have been playing with it
> off-and-on for a while now.  One thing I was actually looking at
> implementing before we push to commit this was to have similar
> information to what SSL provides on connection, specifically what
> printSSLInfo() prints by way of cipher, bits, etc for the
> client-side and then something like pg_stat_ssl for the server
> side.
>
> I went ahead and added a printGSSENCInfo(), and then
> PQgetgssSASLSSF(), which calls down into
> gss_inquire_sec_context_by_oid() for GSS_C_SEC_CONTEXT_SASL_SSF to
> get the bits (which also required including gssapi_ext.h), and now
> have psql producing:
>
> I don't think these things are absolutely required to push the
> patch forward, just to be clear, but it's helping my confidence
> level, at least, and would make it closer to on-par with our
> current SSL implementation.  They also seem, well, reasonably
> straight-forward to add and quite useful.
 
 Auditability is definitely reasonable.  I think there are a couple
 additional wrinkles discussed above, but we can probably get them
 sorted.
>>>
>>> Fantastic.  Do you think you'll have time to work through some of
>>> the above with me over the next couple of weeks?  I was just
>>> starting to look into adding things into the beentry to hold
>>> information on the server side.
>> 
>> Sure!  I'll go ahead and hack up the checks and lucid stuff and get
>> back to you.
>
> Great!  I'll finish fleshing out the basics of how to have this work
> server-side and once the checks and lucid stuff are in on the psql
> side, it should be pretty straight-forward to copy that same
> information into beentry alongside the SSL info, and expose it through
> pg_stat_get_activity() into a pg_stat_gss view.

When the mech is gss_mech_krb5 under MIT krb5:

psql (12devel)
GSSAPI encrypted connection (krb5 using aes256-cts-hmac-sha384-192)
Type "help" for help.

And the same under Heimdal:

psql (12devel)
GSSAPI encrypted connection (krb5 using aes256-cts-hmac-sha1-96)
Type "help" for help

If the mech weren't gss_krb5, or Lucid introspection failed, but we're a
SASL-aware mech (and using MIT):

psql (12devel)
GSSAPI encrypted connection (~256 bits)
Type "help" for help.

The third case (no lucid, no SASL SSF):

psql (12devel)
GSSAPI encrypted connection (unknown mechanism)
Type "help" for help.

Feel free to tweak these strings as needed.

I've also adjusted the layering somewhat and moved the actual printf()
call down into fe-secure-gssapi.c  I don't know whether this model makes
more sense in the long run, but for PoC code it was marginally easier to
reason about.

Another thing I've been thinking about here is whether the SASL SSF
logic is useful.  It'll only get invoked when the mechanism isn't
gss_mech_krb5, and only under MIT.  SPNEGO (krb5), NTLM (gss-ntlmssp),
IAKERB (krb5), and EAP (moonshot) all don't support
GSS_C_SEC_CONTEXT_SASL_SSF; I actually couldn't come up with another
mechanism that does.  I defer to you on whether to remove that, though.

I did some testing with Heimdal since we're using some extensions here,
and found out that the current build machinery doesn't support Heimdal.
(The configure.in detection logic only seems to work for MIT and
Solaris.)  However, it's possible to explicitly pass in CFLAGS/LDFLAGS
and it worked prior to my changes, so I've preserved that behavior.

Finally, as this patch touches configure.in, configure needs to be
regenerated; I'm not sure what project policy is on when that happens
(and it produces rather a lot of churn on my machine).

Patch attached after the break; apply on top of -20.

Thanks,
--Robbie



signature.asc
Description: PGP signature
>From 0f32efdb9b201a3b2d25d3fc41c9758790c84b93 Mon Sep 17 00:00:00 2001
From: Robbie Harwood 
Date: Wed, 20 Feb 2019 17:23:06 -0500
Subject: [PATCH] Log encryption strength in libpq GSSAPI connections

---
 configure.in|  20 
 src/bin/psql/command.c  |   2 +
 src/include/pg_config.h.in  |   6 ++
 src/interfaces/libpq/exports.txt|   1 +
 src/interfaces/libpq/fe-secure-gssapi.c | 127 
 src/interfaces/libpq/fe-secure.c|  11 ++
 src/interfaces/libpq/libpq-fe.h |   3 +
 7 files changed, 170 insertions(+)

diff --git a/configure.in b/configure.in
index 89a0fb2470..be011778f3 100644
--- a/configure.in
+++ b/configure.in
@@ -1191,6 +1191,26 @@ if test 

Re: [PATCH v20] GSSAPI encryption support

[Peter Eisentraut]

On 2019-02-18 16:32, Stephen Frost wrote:
> Considering this is only the second encryption protocol in the project's
> lifetime, I agree that using callbacks would be overkill here.  What
> other encryption protocols are you thinking we would be adding here?  I
> think most would be quite hard-pressed to name a second general-purpose
> one beyond TLS/SSL, and those who can almost certainly would say GSS,
> but a third?  Certainly OpenSSH has its own, but it's not intended to be
> general purpose and I can't see us adding support for OpenSSH's
> encryption protocol to PG.

I did look into an SSH-based encryption layer at one point.  It's
certainly attractive in terms of simplicity over SSL.  But your point
stands nonetheless, for two or three plausible implementations, we don't
necessarily need a generic plugin system.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Robbie Harwood (rharw...@redhat.com) wrote:
> Andres Freund  writes:
> 
> > On 2018-12-18 14:12:46 -0500, Robbie Harwood wrote:
> >
> >> Subject: [PATCH] libpq GSSAPI encryption support
> >
> > Could some of these be split into separate patches that could be more
> > eagerly merged? This is a somewhat large patch...
> 
> What splits do you propose?  (It's been a single patch since v3 as per
> your request in id:20151003161810.gd30...@alap3.anarazel.de and Michael
> Paquier's request in
> id:cab7npqtjd-ttrm1vu8p55_4kkvedx8dfz9v1d_xsqqvr_xa...@mail.gmail.com).

Yeah, I tend to agree with the earlier comments that this can be a
single patch.  There's a little bit that could maybe be split out (when
it comes to the bits that are mostly just moving things around and
removing things) but I'm not convinced it's necessary or, really,
particularly worth it.

> >> diff --git a/src/interfaces/libpq/fe-secure.c 
> >> b/src/interfaces/libpq/fe-secure.c
> >> index a06fc7dc82..f4f196e3b4 100644
> >> --- a/src/interfaces/libpq/fe-secure.c
> >> +++ b/src/interfaces/libpq/fe-secure.c
> >> @@ -220,6 +220,13 @@ pqsecure_read(PGconn *conn, void *ptr, size_t len)
> >>n = pgtls_read(conn, ptr, len);
> >>}
> >>else
> >> +#endif
> >> +#ifdef ENABLE_GSS
> >> +  if (conn->gssenc)
> >> +  {
> >> +  n = pg_GSS_read(conn, ptr, len);
> >> +  }
> >> +  else
> >>  #endif
> >>{
> >>n = pqsecure_raw_read(conn, ptr, len);
> >> @@ -287,7 +294,7 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len)
> >>   * to determine whether to continue/retry after error.
> >>   */
> >>  ssize_t
> >> -pqsecure_write(PGconn *conn, const void *ptr, size_t len)
> >> +pqsecure_write(PGconn *conn, void *ptr, size_t len)
> >>  {
> >>ssize_t n;
> >>  
> >> @@ -297,6 +304,13 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t 
> >> len)
> >>n = pgtls_write(conn, ptr, len);
> >>}
> >>else
> >> +#endif
> >> +#ifdef ENABLE_GSS
> >> +  if (conn->gssenc)
> >> +  {
> >> +  n = pg_GSS_write(conn, ptr, len);
> >> +  }
> >> +  else
> >>  #endif
> >>{
> >>n = pqsecure_raw_write(conn, ptr, len);
> >
> > Not a fan of this. Seems like we'll grow more and more such branches
> > over time?  Wouldn't the right thing be to have callbacks in PGconn
> > (and similarly in the backend)?
> 
> Is that really a problem?  Each branch is only seven lines, which is a
> lot less than adding callback support will be.  And we'll only get a new
> branch when we want to support a new encryption protocol - unlike
> authentication, there aren't too many of those.

Considering this is only the second encryption protocol in the project's
lifetime, I agree that using callbacks would be overkill here.  What
other encryption protocols are you thinking we would be adding here?  I
think most would be quite hard-pressed to name a second general-purpose
one beyond TLS/SSL, and those who can almost certainly would say GSS,
but a third?  Certainly OpenSSH has its own, but it's not intended to be
general purpose and I can't see us adding support for OpenSSH's
encryption protocol to PG.

Adding support for different libraries which implement TLS/SSL wouldn't
be changing this part of the code, if that was perhaps what was being
considered..?

> > Seems like if that's done reasonably it'd also make integration of
> > compression easier, because that could just layer itself between
> > encryption and wire?

Building a general-purpose filtering mechanism for streams is actually
quite a bit of work (we did it for pgBackRest, feel free to check it
out- and all that code is in C these days too) and I don't think it's
really necessary when the options are "optionally compress and
optionally encrypt using one of these two protocols".  I don't see any
reason why we'd need to, say, compress a stream multiple times, or
encrypt it multiple times (like with TLS first and then GSS).

> The current interface would allow a compress/decompress call in a way
> that makes sense to me (here for write, ignoring ifdefs):

[...]

> (pqsecure_read would look similarly, with decompression as the last step
> instead of the first.)

That certainly seems reasonable to me.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Robbie Harwood]

Andres Freund  writes:

> On 2018-12-18 14:12:46 -0500, Robbie Harwood wrote:
>
>> Subject: [PATCH] libpq GSSAPI encryption support
>
> Could some of these be split into separate patches that could be more
> eagerly merged? This is a somewhat large patch...

What splits do you propose?  (It's been a single patch since v3 as per
your request in id:20151003161810.gd30...@alap3.anarazel.de and Michael
Paquier's request in
id:cab7npqtjd-ttrm1vu8p55_4kkvedx8dfz9v1d_xsqqvr_xa...@mail.gmail.com).

>> diff --git a/src/interfaces/libpq/fe-secure.c 
>> b/src/interfaces/libpq/fe-secure.c
>> index a06fc7dc82..f4f196e3b4 100644
>> --- a/src/interfaces/libpq/fe-secure.c
>> +++ b/src/interfaces/libpq/fe-secure.c
>> @@ -220,6 +220,13 @@ pqsecure_read(PGconn *conn, void *ptr, size_t len)
>>  n = pgtls_read(conn, ptr, len);
>>  }
>>  else
>> +#endif
>> +#ifdef ENABLE_GSS
>> +if (conn->gssenc)
>> +{
>> +n = pg_GSS_read(conn, ptr, len);
>> +}
>> +else
>>  #endif
>>  {
>>  n = pqsecure_raw_read(conn, ptr, len);
>> @@ -287,7 +294,7 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len)
>>   * to determine whether to continue/retry after error.
>>   */
>>  ssize_t
>> -pqsecure_write(PGconn *conn, const void *ptr, size_t len)
>> +pqsecure_write(PGconn *conn, void *ptr, size_t len)
>>  {
>>  ssize_t n;
>>  
>> @@ -297,6 +304,13 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t 
>> len)
>>  n = pgtls_write(conn, ptr, len);
>>  }
>>  else
>> +#endif
>> +#ifdef ENABLE_GSS
>> +if (conn->gssenc)
>> +{
>> +n = pg_GSS_write(conn, ptr, len);
>> +}
>> +else
>>  #endif
>>  {
>>  n = pqsecure_raw_write(conn, ptr, len);
>
> Not a fan of this. Seems like we'll grow more and more such branches
> over time?  Wouldn't the right thing be to have callbacks in PGconn
> (and similarly in the backend)?

Is that really a problem?  Each branch is only seven lines, which is a
lot less than adding callback support will be.  And we'll only get a new
branch when we want to support a new encryption protocol - unlike
authentication, there aren't too many of those.

> Seems like if that's done reasonably it'd also make integration of
> compression easier, because that could just layer itself between
> encryption and wire?

The current interface would allow a compress/decompress call in a way
that makes sense to me (here for write, ignoring ifdefs):

ssize_t pqsecure_write(PGConn *conn, void *ptr, size_t len)
{
ssize_t n;

if (conn->compress)
{
do_compression(conn, , );
}
if (conn->ssl_in_use)
{
n = pgtls_write(conn, ptr, len);
}
else if (conn->gssenc)
{
n = pg_GSS_write(conn, ptr, len);
}
else
{
n = pqsecure_raw_write(conn, ptr, len);
}

return n;
}

(pqsecure_read would look similarly, with decompression as the last step
instead of the first.)

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Andres Freund]

Hi,

On 2018-12-18 14:12:46 -0500, Robbie Harwood wrote:
> From 6915ae2507bf7910c5eecfbd0b84805531c16a07 Mon Sep 17 00:00:00 2001
> From: Robbie Harwood 
> Date: Thu, 10 May 2018 16:12:03 -0400
> Subject: [PATCH] libpq GSSAPI encryption support
> 
> On both the frontend and backend, prepare for GSSAPI encryption
> support by moving common code for error handling into a separate file.
> Fix a TODO for handling multiple status messages in the process.
> Eliminate the OIDs, which have not been needed for some time.
> Add frontend and backend encryption support functions.  Keep the
> context initiation for authentication-only separate on both the
> frontend and backend in order to avoid concerns about changing the
> requested flags to include encryption support.
> 
> In postmaster, pull GSSAPI authorization checking into a shared
> function.  Also share the initiator name between the encryption and
> non-encryption codepaths.
> 
> Modify pqsecure_write() to take a non-const pointer.  The pointer will
> not be modified, but this change (or a const-discarding cast, or a
> malloc()+memcpy()) is necessary for GSSAPI due to const/struct
> interactions in C.
> 
> For HBA, add "hostgss" and "hostnogss" entries that behave similarly
> to their SSL counterparts.  "hostgss" requires either "gss", "trust",
> or "reject" for its authentication.
> 
> Simiarly, add a "gssmode" parameter to libpq.  Supported values are
> "disable", "require", and "prefer".  Notably, negotiation will only be
> attempted if credentials can be acquired.  Move credential acquisition
> into its own function to support this behavior.
> 
> Finally, add documentation for everything new, and update existing
> documentation on connection security.
> 
> Thanks to Michael Paquier for the Windows fixes.

Could some of these be split into separate patches that could be more
eagerly merged? This is a somewhat large patch...


> diff --git a/src/interfaces/libpq/fe-secure.c 
> b/src/interfaces/libpq/fe-secure.c
> index a06fc7dc82..f4f196e3b4 100644
> --- a/src/interfaces/libpq/fe-secure.c
> +++ b/src/interfaces/libpq/fe-secure.c
> @@ -220,6 +220,13 @@ pqsecure_read(PGconn *conn, void *ptr, size_t len)
>   n = pgtls_read(conn, ptr, len);
>   }
>   else
> +#endif
> +#ifdef ENABLE_GSS
> + if (conn->gssenc)
> + {
> + n = pg_GSS_read(conn, ptr, len);
> + }
> + else
>  #endif
>   {
>   n = pqsecure_raw_read(conn, ptr, len);
> @@ -287,7 +294,7 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len)
>   * to determine whether to continue/retry after error.
>   */
>  ssize_t
> -pqsecure_write(PGconn *conn, const void *ptr, size_t len)
> +pqsecure_write(PGconn *conn, void *ptr, size_t len)
>  {
>   ssize_t n;
>  
> @@ -297,6 +304,13 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len)
>   n = pgtls_write(conn, ptr, len);
>   }
>   else
> +#endif
> +#ifdef ENABLE_GSS
> + if (conn->gssenc)
> + {
> + n = pg_GSS_write(conn, ptr, len);
> + }
> + else
>  #endif
>   {
>   n = pqsecure_raw_write(conn, ptr, len);

Not a fan of this. Seems like we'll grow more and more such branches
over time? Wouldn't the right thing be to have callbacks in PGconn (and
similarly in the backend)?  Seems like if that's done reasonably it'd
also make integration of compression easier, because that could just
layer itself between encryption and wire?


Greetings,

Andres Freund

Re: [PATCH v20] GSSAPI encryption support

[Robbie Harwood]

Stephen Frost  writes:

> * Robbie Harwood (rharw...@redhat.com) wrote:
>> Stephen Frost  writes:
>>> * Robbie Harwood (rharw...@redhat.com) wrote:
 Stephen Frost  writes:
> * Robbie Harwood (rharw...@redhat.com) wrote:
>
>> Attached please find version 20 of the GSSAPI encryption support.
>> This has been rebased onto master (thanks Stephen for calling out
>> ab69ea9).
>
> I've looked over this again and have been playing with it
> off-and-on for a while now.  One thing I was actually looking at
> implementing before we push to commit this was to have similar
> information to what SSL provides on connection, specifically what
> printSSLInfo() prints by way of cipher, bits, etc for the
> client-side and then something like pg_stat_ssl for the server
> side.
>
> I went ahead and added a printGSSENCInfo(), and then
> PQgetgssSASLSSF(), which calls down into
> gss_inquire_sec_context_by_oid() for GSS_C_SEC_CONTEXT_SASL_SSF to
> get the bits (which also required including gssapi_ext.h), and now
> have psql producing:
 
 While I (a krb5 developer) am fine with a hard MIT dependency, the
 project doesn't currently have this.  (And if we went that road,
 I'd like to use gss_acquire_cred_from() instead of the setenv() in
 be-secure-gssapi.c.)
>>>
>>> We certainly don't want a hard MIT dependency, but if it's following
>>> an RFC and we can gracefully fall-back if it isn't available then I
>>> think it's acceptable.  If there's a better approach which would
>>> work with both MIT and Heimdal and ideally is defined through an
>>> RFC, that'd be better, but I'm guessing there isn't...
>> 
>> While I think the concept of SASL SSF is standardized, I don't think
>> the semantics of this OID are - the OID itself is in the MIT gssapi
>> extension space.
>
> We can adjust for that pretty easily, presumably, if another OID ends
> up being assigned (though if one already exists, isn't it something
> that Heimdal, for example, could potentially decide to just
> support..?).

Yes, exactly - Heimdal would probably just use the MIT OID with the
existing semantics.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Robbie Harwood (rharw...@redhat.com) wrote:
> Stephen Frost  writes:
> > * Robbie Harwood (rharw...@redhat.com) wrote:
> >> Stephen Frost  writes:
> >>> * Robbie Harwood (rharw...@redhat.com) wrote:
> >>>
>  Attached please find version 20 of the GSSAPI encryption support.
>  This has been rebased onto master (thanks Stephen for calling out
>  ab69ea9).
> >>>
> >>> I've looked over this again and have been playing with it off-and-on
> >>> for a while now.  One thing I was actually looking at implementing
> >>> before we push to commit this was to have similar information to
> >>> what SSL provides on connection, specifically what printSSLInfo()
> >>> prints by way of cipher, bits, etc for the client-side and then
> >>> something like pg_stat_ssl for the server side.
> >>>
> >>> I went ahead and added a printGSSENCInfo(), and then
> >>> PQgetgssSASLSSF(), which calls down into
> >>> gss_inquire_sec_context_by_oid() for GSS_C_SEC_CONTEXT_SASL_SSF to
> >>> get the bits (which also required including gssapi_ext.h), and now
> >>> have psql producing:
> >> 
> >> Also, this is an MIT-specific extension: Heimdal doesn't support it.
> >
> > Is that simply because they've not gotten around to it..?  From what I
> > saw, this approach is in an accepted RFC, so if they want to implement
> > it, they could do so..?
> 
> Yes.  Heimdal is less active these days than MIT; they were dormant for
> a while, though are active again.  They do have an issue open to track
> this feature: https://github.com/heimdal/heimdal/issues/400

Ok, good, when they add it then hopefully it'll be picked up
more-or-less 'for free'.

> >> While I (a krb5 developer) am fine with a hard MIT dependency, the
> >> project doesn't currently have this.  (And if we went that road, I'd
> >> like to use gss_acquire_cred_from() instead of the setenv() in
> >> be-secure-gssapi.c.)
> >
> > We certainly don't want a hard MIT dependency, but if it's following an
> > RFC and we can gracefully fall-back if it isn't available then I think
> > it's acceptable.  If there's a better approach which would work with
> > both MIT and Heimdal and ideally is defined through an RFC, that'd be
> > better, but I'm guessing there isn't...
> 
> While I think the concept of SASL SSF is standardized, I don't think the
> semantics of this OID are - the OID itself is in the MIT gssapi
> extension space.

We can adjust for that pretty easily, presumably, if another OID ends up
being assigned (though if one already exists, isn't it something that
Heimdal, for example, could potentially decide to just support..?).

> As a historical note, the reason this interface exists in krb5 is of
> course for SASL.  In particular, cyrus was reporting treating the SSF of
> all krb5 types as if they were DES.  So while technically "assume DES"
> could be a fallback... we probably shouldn't do that.

Agreed, we shouldn't do that.

> >> A call to gss_inquire_context() produces mech type, so you could
> >> print something like "GSS Encrypted connection (Kerberos 256 bits)"
> >> or "GSS encrypted connection (NTLM)".  I think this'd be pretty
> >> reasonable.
> >
> > I'm... not impressed with that approach, since "Kerberos" as an
> > encryption algorithm doesn't mean squat- that could be DES or RC4 for
> > all we know.
> 
> This is a fair concern.

I will say that I'm alright with listing Kerberos in the message if we
think it's useful- but we should also list the actual encryption
algorithm and bits, if possible (which it sounds like it is, so that's
definitely good).

> > To get where I'd like us to be, however, it sounds like we need to:
> >
> > a) determine if we've got encryption (that's easy, assuming I've done it
> >correctly so far)
> 
> Yes.
> 
> > b) Figure out if the encryption is being provided by Kerberos (using
> >gss_inquire_context() would do that, right?)
> 
> Right - check and log mech_out there, then proceed if it's
> gss_mech_krb5.  Probably not worth handling _old and _wrong, especially
> since Heimdal doesn't have them and they shouldn't be in use anyway.
> 
> > c) If we're using Kerberos, use the lucid contexts to ask what the
> >actual encryption algorithm used is
> 
> Yes.  This'll involve some introspection into krb5.h (for enctype
> names), but that's not too hard.

Sounds good.

> > That's a bit obnoxious but it at least seems doable.  I don't suppose
> > you know of any example code out there which provides this?  Any idea
> > if there's work underway on an RFC to make this, well, suck less?
> 
> MIT has some in our test suite (t_enctypes).  nfs-utils (GPLv2) also
> uses this interface (NFS is the intended consumer).
> 
> There isn't IETF effort in this regard that I'm aware of, but I'll add
> it to the kitten backlog.

Fantastic, thanks.

> >> (With my downstream maintainer hat on, I'd further ask that any such
> >> check not just be a version check in order to allow backporting; in
> >> particular, the el7 krb5-1.15 branch does 

Re: [PATCH v20] GSSAPI encryption support

[Robbie Harwood]

Stephen Frost  writes:

> * Robbie Harwood (rharw...@redhat.com) wrote:
>> Stephen Frost  writes:
>>> * Robbie Harwood (rharw...@redhat.com) wrote:
>>>
 Attached please find version 20 of the GSSAPI encryption support.
 This has been rebased onto master (thanks Stephen for calling out
 ab69ea9).
>>>
>>> I've looked over this again and have been playing with it off-and-on
>>> for a while now.  One thing I was actually looking at implementing
>>> before we push to commit this was to have similar information to
>>> what SSL provides on connection, specifically what printSSLInfo()
>>> prints by way of cipher, bits, etc for the client-side and then
>>> something like pg_stat_ssl for the server side.
>>>
>>> I went ahead and added a printGSSENCInfo(), and then
>>> PQgetgssSASLSSF(), which calls down into
>>> gss_inquire_sec_context_by_oid() for GSS_C_SEC_CONTEXT_SASL_SSF to
>>> get the bits (which also required including gssapi_ext.h), and now
>>> have psql producing:
>> 
>> Also, this is an MIT-specific extension: Heimdal doesn't support it.
>
> Is that simply because they've not gotten around to it..?  From what I
> saw, this approach is in an accepted RFC, so if they want to implement
> it, they could do so..?

Yes.  Heimdal is less active these days than MIT; they were dormant for
a while, though are active again.  They do have an issue open to track
this feature: https://github.com/heimdal/heimdal/issues/400

>> While I (a krb5 developer) am fine with a hard MIT dependency, the
>> project doesn't currently have this.  (And if we went that road, I'd
>> like to use gss_acquire_cred_from() instead of the setenv() in
>> be-secure-gssapi.c.)
>
> We certainly don't want a hard MIT dependency, but if it's following an
> RFC and we can gracefully fall-back if it isn't available then I think
> it's acceptable.  If there's a better approach which would work with
> both MIT and Heimdal and ideally is defined through an RFC, that'd be
> better, but I'm guessing there isn't...

While I think the concept of SASL SSF is standardized, I don't think the
semantics of this OID are - the OID itself is in the MIT gssapi
extension space.

As a historical note, the reason this interface exists in krb5 is of
course for SASL.  In particular, cyrus was reporting treating the SSF of
all krb5 types as if they were DES.  So while technically "assume DES"
could be a fallback... we probably shouldn't do that.

>>> ---
>>> psql (12devel)
>>> GSS Encrypted connection (bits: 256)
>>> Type "help" for help.
>>> ---
>>>
>>> I was curious though- is there a good way to get at the encryption
>>> type used..?  I haven't had much luck in reading the RFPs and poking
>>> around, but perhaps I'm just not looking in the right place.
>> 
>> A call to gss_inquire_context() produces mech type, so you could
>> print something like "GSS Encrypted connection (Kerberos 256 bits)"
>> or "GSS encrypted connection (NTLM)".  I think this'd be pretty
>> reasonable.
>
> I'm... not impressed with that approach, since "Kerberos" as an
> encryption algorithm doesn't mean squat- that could be DES or RC4 for
> all we know.

This is a fair concern.

>> For Kerberos-specific introspection, MIT krb5 and Heimdal both
>> support an interface called lucid contexts that allows this kind of
>> introspection (and some other gross layering violations) for use with
>> the kernel.  Look at gssapi_krb5.h (it's called that in both).  I
>> don't think it's worth supporting rfc1964 at this point (it's
>> 1DES-only).
>
> I agree that there's no sense in supporting 1DES-only.
>
> I also, frankly, don't agree with the characterization that wanting to
> know what the agreed-upon encryption algorithm is constitutes a gross
> layering violation, but that's really an independent discussion that we
> can have in a pub somewhere. ;)

Yeah I won't defend it very hard :)

> To get where I'd like us to be, however, it sounds like we need to:
>
> a) determine if we've got encryption (that's easy, assuming I've done it
>correctly so far)

Yes.

> b) Figure out if the encryption is being provided by Kerberos (using
>gss_inquire_context() would do that, right?)

Right - check and log mech_out there, then proceed if it's
gss_mech_krb5.  Probably not worth handling _old and _wrong, especially
since Heimdal doesn't have them and they shouldn't be in use anyway.

> c) If we're using Kerberos, use the lucid contexts to ask what the
>actual encryption algorithm used is

Yes.  This'll involve some introspection into krb5.h (for enctype
names), but that's not too hard.

> That's a bit obnoxious but it at least seems doable.  I don't suppose
> you know of any example code out there which provides this?  Any idea
> if there's work underway on an RFC to make this, well, suck less?

MIT has some in our test suite (t_enctypes).  nfs-utils (GPLv2) also
uses this interface (NFS is the intended consumer).

There isn't IETF effort in this regard that I'm aware of, but I'll add
it to the 

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings,

* Robbie Harwood (rharw...@redhat.com) wrote:
> Stephen Frost  writes:
> > * Robbie Harwood (rharw...@redhat.com) wrote:
> >> Attached please find version 20 of the GSSAPI encryption support.
> >> This has been rebased onto master (thanks Stephen for calling out
> >> ab69ea9).
> >
> > I've looked over this again and have been playing with it off-and-on
> > for a while now.  One thing I was actually looking at implementing
> > before we push to commit this was to have similar information to what
> > SSL provides on connection, specifically what printSSLInfo() prints by
> > way of cipher, bits, etc for the client-side and then something like
> > pg_stat_ssl for the server side.
> >
> > I went ahead and added a printGSSENCInfo(), and then
> > PQgetgssSASLSSF(), which calls down into
> > gss_inquire_sec_context_by_oid() for GSS_C_SEC_CONTEXT_SASL_SSF to get
> > the bits (which also required including gssapi_ext.h), and now have
> > psql producing:
> 
> Neat!  Two things here:
> 
> First, because this is a SASL extension, it requires existing mechanism
> integration with SASL.  For instance, NTLM (through gss-ntlmssp) doesn't
> implement it.  PQgetgssSASLSSF() logs an error message here, which I
> don't think is quite right - probably you should only log an error
> message if you don't get back GSS_S_COMPLETE or GSS_S_UNAVAILABLE.

Ok, that's easy enough to fix.

> (NTLM is an example here; I cannot in good conscience recommend its use,
> and neither does Microsoft.)

Agreed!

> Also, this is an MIT-specific extension: Heimdal doesn't support it.

Is that simply because they've not gotten around to it..?  From what I
saw, this approach is in an accepted RFC, so if they want to implement
it, they could do so..?

> While I (a krb5 developer) am fine with a hard MIT dependency, the
> project doesn't currently have this.  (And if we went that road, I'd
> like to use gss_acquire_cred_from() instead of the setenv() in
> be-secure-gssapi.c.)

We certainly don't want a hard MIT dependency, but if it's following an
RFC and we can gracefully fall-back if it isn't available then I think
it's acceptable.  If there's a better approach which would work with
both MIT and Heimdal and ideally is defined through an RFC, that'd be
better, but I'm guessing there isn't...

> > ---
> > psql (12devel)
> > GSS Encrypted connection (bits: 256)
> > Type "help" for help.
> > ---
> >
> > I was curious though- is there a good way to get at the encryption
> > type used..?  I haven't had much luck in reading the RFPs and poking
> > around, but perhaps I'm just not looking in the right place.
> 
> It's something of a layering issue.  GSSAPI might not be using Kerberos,
> and even if it is, Kerberos has algorithm agility.

Right, I'm aware that Kerberos could be using different encryption
algorithms, similar to SSL and that GSSAPI itself would allow for
different algorithms through different mechanisms- but the question is,
how do we know what the encryption algorithm ultimately used, regardless
of the rest, is?

> A call to gss_inquire_context() produces mech type, so you could print
> something like "GSS Encrypted connection (Kerberos 256 bits)" or "GSS
> encrypted connection (NTLM)".  I think this'd be pretty reasonable.

I'm... not impressed with that approach, since "Kerberos" as an
encryption algorithm doesn't mean squat- that could be DES or RC4 for
all we know.

> For Kerberos-specific introspection, MIT krb5 and Heimdal both support
> an interface called lucid contexts that allows this kind of
> introspection (and some other gross layering violations) for use with
> the kernel.  Look at gssapi_krb5.h (it's called that in both).  I don't
> think it's worth supporting rfc1964 at this point (it's 1DES-only).

I agree that there's no sense in supporting 1DES-only.

I also, frankly, don't agree with the characterization that wanting to
know what the agreed-upon encryption algorithm is constitutes a gross
layering violation, but that's really an independent discussion that we
can have in a pub somewhere. ;)

To get where I'd like us to be, however, it sounds like we need to:

a) determine if we've got encryption (that's easy, assuming I've done it
   correctly so far)

b) Figure out if the encryption is being provided by Kerberos (using
   gss_inquire_context() would do that, right?)

c) If we're using Kerberos, use the lucid contexts to ask what the
   actual encryption algorithm used is

That's a bit obnoxious but it at least seems doable.  I don't suppose
you know of any example code out there which provides this?  Any idea if
there's work underway on an RFC to make this, well, suck less?

> > Also, what information do you think would be particularly useful on
> > the server side?  I was thinking that the princ used to authenticate,
> > the encryption type/cipher, and bits used, at least, would be
> > meaningful.
> 
> I'm wary about indicating number of bits especially with the
> asymmetric/symmetric divide and the 

Re: [PATCH v20] GSSAPI encryption support

[Robbie Harwood]

Stephen Frost  writes:

> * Robbie Harwood (rharw...@redhat.com) wrote:
>
>> Attached please find version 20 of the GSSAPI encryption support.
>> This has been rebased onto master (thanks Stephen for calling out
>> ab69ea9).
>
> I've looked over this again and have been playing with it off-and-on
> for a while now.  One thing I was actually looking at implementing
> before we push to commit this was to have similar information to what
> SSL provides on connection, specifically what printSSLInfo() prints by
> way of cipher, bits, etc for the client-side and then something like
> pg_stat_ssl for the server side.
>
> I went ahead and added a printGSSENCInfo(), and then
> PQgetgssSASLSSF(), which calls down into
> gss_inquire_sec_context_by_oid() for GSS_C_SEC_CONTEXT_SASL_SSF to get
> the bits (which also required including gssapi_ext.h), and now have
> psql producing:

Neat!  Two things here:

First, because this is a SASL extension, it requires existing mechanism
integration with SASL.  For instance, NTLM (through gss-ntlmssp) doesn't
implement it.  PQgetgssSASLSSF() logs an error message here, which I
don't think is quite right - probably you should only log an error
message if you don't get back GSS_S_COMPLETE or GSS_S_UNAVAILABLE.

(NTLM is an example here; I cannot in good conscience recommend its use,
and neither does Microsoft.)

Also, this is an MIT-specific extension: Heimdal doesn't support it.
While I (a krb5 developer) am fine with a hard MIT dependency, the
project doesn't currently have this.  (And if we went that road, I'd
like to use gss_acquire_cred_from() instead of the setenv() in
be-secure-gssapi.c.)

> ---
> psql (12devel)
> GSS Encrypted connection (bits: 256)
> Type "help" for help.
> ---
>
> I was curious though- is there a good way to get at the encryption
> type used..?  I haven't had much luck in reading the RFPs and poking
> around, but perhaps I'm just not looking in the right place.

It's something of a layering issue.  GSSAPI might not be using Kerberos,
and even if it is, Kerberos has algorithm agility.

A call to gss_inquire_context() produces mech type, so you could print
something like "GSS Encrypted connection (Kerberos 256 bits)" or "GSS
encrypted connection (NTLM)".  I think this'd be pretty reasonable.

For Kerberos-specific introspection, MIT krb5 and Heimdal both support
an interface called lucid contexts that allows this kind of
introspection (and some other gross layering violations) for use with
the kernel.  Look at gssapi_krb5.h (it's called that in both).  I don't
think it's worth supporting rfc1964 at this point (it's 1DES-only).

> Also, what information do you think would be particularly useful on
> the server side?  I was thinking that the princ used to authenticate,
> the encryption type/cipher, and bits used, at least, would be
> meaningful.

I'm wary about indicating number of bits especially with the
asymmetric/symmetric divide and the importance of which algorithm
they're used with, but that may be a non-concern - especially if it
attains parity with the TLS code.

Principal used is definitely good (helps debugging the user mapping
rules, if nothing else).  Mechanism is also nice.  I can't think of
anything else right now that you haven't mentioned.

I think there's value in auth indcatorsp
http://web.mit.edu/kerberos/krb5-latest/doc/admin/auth_indicator.html
but supporting them would be a separate follow-on patch.

> I'm also guessing that we may need to add something to
> make these functions not fail on older systems which don't provide the
> SASL SSF OID..?  I haven't looked into that yet but we should.

MIT krb5 gained support in 2017, which corresponds to krb5-1.16; Heimdal
has no support for it.  There probably isn't a shortcut for a
buildsystem check, unfortunately: Heimdal has
gss_inquire_sec_context_by_oid() in another file (they don't have
gssapi_ext.h), and it's the OID is MIT-only and not a #define-d
constant.

(With my downstream maintainer hat on, I'd further ask that any such
check not just be a version check in order to allow backporting; in
particular, the el7 krb5-1.15 branch does include support for this OID.)

> I don't think these things are absolutely required to push the patch
> forward, just to be clear, but it's helping my confidence level, at
> least, and would make it closer to on-par with our current SSL
> implementation.  They also seem, well, reasonably straight-forward to
> add and quite useful.

Auditability is definitely reasonable.  I think there are a couple
additional wrinkles discussed above, but we can probably get them
sorted.

> I've attached my WIP patch for adding the bits, patched over-top of your
> v20 (would be neat if there was a way to tell cfbot to ignore it...).

I don't have additional concerns beyond the above.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [PATCH v20] GSSAPI encryption support

[Stephen Frost]

Greetings Robbie,

* Robbie Harwood (rharw...@redhat.com) wrote:
> Attached please find version 20 of the GSSAPI encryption support.  This
> has been rebased onto master (thanks Stephen for calling out ab69ea9).
> Other changes since v19 from Stephen's review:
> 
> - About 100 lines of new comments

Thanks for that.

> - pgindent run over code (only the stuff I'm changing; it reports other
>   problems on master, but if I understand correctly, that's not on me to
>   fix here)

That's correct.

> Thanks for the feedback!  And speaking of feedback, this patch is in the
> "Needs Review" state so if people have additional feedback, that would
> be appreciated.  I've CC'd people who I can remember reviewing before;
> they should of course feel no obligation to review again if time
> commitments/interests do not allow.

I've looked over this again and have been playing with it off-and-on for
a while now.  One thing I was actually looking at implementing before we
push to commit this was to have similar information to what SSL provides
on connection, specifically what printSSLInfo() prints by way of
cipher, bits, etc for the client-side and then something like
pg_stat_ssl for the server side.

I went ahead and added a printGSSENCInfo(), and then PQgetgssSASLSSF(),
which calls down into gss_inquire_sec_context_by_oid() for
GSS_C_SEC_CONTEXT_SASL_SSF to get the bits (which also required
including gssapi_ext.h), and now have psql producing:

---
psql (12devel)
GSS Encrypted connection (bits: 256)
Type "help" for help.
---

I was curious though- is there a good way to get at the encryption type
used..?  I haven't had much luck in reading the RFPs and poking around,
but perhaps I'm just not looking in the right place.  Also, what
information do you think would be particularly useful on the server
side?  I was thinking that the princ used to authenticate, the
encryption type/cipher, and bits used, at least, would be meaningful.
I'm also guessing that we may need to add something to make these
functions not fail on older systems which don't provide the SASL SSF
OID..?  I haven't looked into that yet but we should.

I don't think these things are absolutely required to push the patch
forward, just to be clear, but it's helping my confidence level, at
least, and would make it closer to on-par with our current SSL
implementation.  They also seem, well, reasonably straight-forward to
add and quite useful.

I've attached my WIP patch for adding the bits, patched over-top of your
v20 (would be neat if there was a way to tell cfbot to ignore it...).

Thoughts?

Thanks!

Stephen
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
new file mode 100644
index ab259c4..2599330
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*** static void print_with_linenumbers(FILE
*** 159,164 
--- 159,165 
  static void minimal_error_message(PGresult *res);
  
  static void printSSLInfo(void);
+ static void printGSSENCInfo(void);
  static bool printPsetInfo(const char *param, struct printQueryOpt *popt);
  static char *pset_value_string(const char *param, struct printQueryOpt *popt);
  
*** exec_command_conninfo(PsqlScanState scan
*** 621,626 
--- 622,628 
  		   db, PQuser(pset.db), host, PQport(pset.db));
  			}
  			printSSLInfo();
+ 			printGSSENCInfo();
  		}
  	}
  
*** connection_warnings(bool in_startup)
*** 3184,3189 
--- 3186,3192 
  		checkWin32Codepage();
  #endif
  		printSSLInfo();
+ 		printGSSENCInfo();
  	}
  }
  
*** printSSLInfo(void)
*** 3216,3221 
--- 3219,3244 
  		   (compression && strcmp(compression, "off") != 0) ? _("on") : _("off"));
  }
  
+ /*
+  * printGSSENCInfo
+  *
+  * Prints information about the current GSS-Encrypted connection, if GSS
+  * encryption is in use.
+  */
+ static void
+ printGSSENCInfo(void)
+ {
+ 	const char *bits;
+ 
+ 	if (!PQgssEncInUse(pset.db))
+ 		return;	/* no GSS Encryption */
+ 
+ 	bits = PQgetgssSASLSSF(pset.db);
+ 
+ 	printf(_("GSS Encrypted connection (bits: %s)\n"),
+ 		   bits ? bits : _("unknown"));
+ }
+ 
  
  /*
   * checkWin32Codepage
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
new file mode 100644
index cc9ee9c..3db936f
*** a/src/interfaces/libpq/exports.txt
--- b/src/interfaces/libpq/exports.txt
*** PQresultVerboseErrorMessage 171
*** 174,176 
--- 174,179 
  PQencryptPasswordConn 172
  PQresultMemorySize173
  PQhostaddr174
+ PQgssEncInUse 175
+ PQgetgssctx   176
+ PQgetgssSASLSSF   177
diff --git a/src/interfaces/libpq/fe-secure-gssapi.c b/src/interfaces/libpq/fe-secure-gssapi.c
new file mode 100644
index 5d9c1f8..df63882
*** a/src/interfaces/libpq/fe-secure-gssapi.c
--- b/src/interfaces/libpq/fe-secure-gssapi.c
***
*** 17,22 
--- 17,24 
  #include "libpq-int.h"
  #include "fe-gssapi-common.h"
  
+ #include "port/pg_bswap.h"
+ 
  /*
   

[PATCH v20] GSSAPI encryption support

[Robbie Harwood]

Hello friends,

Attached please find version 20 of the GSSAPI encryption support.  This
has been rebased onto master (thanks Stephen for calling out ab69ea9).
Other changes since v19 from Stephen's review:

- About 100 lines of new comments

- pgindent run over code (only the stuff I'm changing; it reports other
  problems on master, but if I understand correctly, that's not on me to
  fix here)

Thanks for the feedback!  And speaking of feedback, this patch is in the
"Needs Review" state so if people have additional feedback, that would
be appreciated.  I've CC'd people who I can remember reviewing before;
they should of course feel no obligation to review again if time
commitments/interests do not allow.

Thanks,
--Robbie


signature.asc
Description: PGP signature
>From 6915ae2507bf7910c5eecfbd0b84805531c16a07 Mon Sep 17 00:00:00 2001
From: Robbie Harwood 
Date: Thu, 10 May 2018 16:12:03 -0400
Subject: [PATCH] libpq GSSAPI encryption support

On both the frontend and backend, prepare for GSSAPI encryption
support by moving common code for error handling into a separate file.
Fix a TODO for handling multiple status messages in the process.
Eliminate the OIDs, which have not been needed for some time.

Add frontend and backend encryption support functions.  Keep the
context initiation for authentication-only separate on both the
frontend and backend in order to avoid concerns about changing the
requested flags to include encryption support.

In postmaster, pull GSSAPI authorization checking into a shared
function.  Also share the initiator name between the encryption and
non-encryption codepaths.

Modify pqsecure_write() to take a non-const pointer.  The pointer will
not be modified, but this change (or a const-discarding cast, or a
malloc()+memcpy()) is necessary for GSSAPI due to const/struct
interactions in C.

For HBA, add "hostgss" and "hostnogss" entries that behave similarly
to their SSL counterparts.  "hostgss" requires either "gss", "trust",
or "reject" for its authentication.

Simiarly, add a "gssmode" parameter to libpq.  Supported values are
"disable", "require", and "prefer".  Notably, negotiation will only be
attempted if credentials can be acquired.  Move credential acquisition
into its own function to support this behavior.

Finally, add documentation for everything new, and update existing
documentation on connection security.

Thanks to Michael Paquier for the Windows fixes.
---
 doc/src/sgml/client-auth.sgml   |  75 -
 doc/src/sgml/libpq.sgml |  57 +++-
 doc/src/sgml/runtime.sgml   |  77 -
 src/backend/libpq/Makefile  |   4 +
 src/backend/libpq/auth.c| 116 +++
 src/backend/libpq/be-gssapi-common.c|  74 +
 src/backend/libpq/be-gssapi-common.h|  26 ++
 src/backend/libpq/be-secure-gssapi.c| 374 ++
 src/backend/libpq/be-secure.c   |  16 +
 src/backend/libpq/hba.c |  51 ++-
 src/backend/postmaster/pgstat.c |   3 +
 src/backend/postmaster/postmaster.c |  69 -
 src/include/libpq/hba.h |   4 +-
 src/include/libpq/libpq-be.h|  13 +-
 src/include/libpq/libpq.h   |   3 +
 src/include/libpq/pqcomm.h  |   5 +-
 src/include/pgstat.h|   3 +-
 src/interfaces/libpq/Makefile   |   4 +
 src/interfaces/libpq/fe-auth.c  |  82 +
 src/interfaces/libpq/fe-connect.c   | 241 ++-
 src/interfaces/libpq/fe-gssapi-common.c | 130 
 src/interfaces/libpq/fe-gssapi-common.h |  23 ++
 src/interfaces/libpq/fe-secure-gssapi.c | 394 
 src/interfaces/libpq/fe-secure.c|  16 +-
 src/interfaces/libpq/libpq-fe.h |   3 +-
 src/interfaces/libpq/libpq-int.h|  30 +-
 src/tools/msvc/Mkvcbuild.pm |  10 +
 27 files changed, 1707 insertions(+), 196 deletions(-)
 create mode 100644 src/backend/libpq/be-gssapi-common.c
 create mode 100644 src/backend/libpq/be-gssapi-common.h
 create mode 100644 src/backend/libpq/be-secure-gssapi.c
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.c
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.h
 create mode 100644 src/interfaces/libpq/fe-secure-gssapi.c

diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index c2114021c3..6f9f2b7560 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -108,6 +108,8 @@ hostnossl  database  user
 host   database  user  IP-address  IP-mask  auth-method  auth-options
 hostssldatabase  user  IP-address  IP-mask  auth-method  auth-options
 hostnossl  database  user  IP-address  IP-mask  auth-method  auth-options
+hostgssdatabase  user  IP-address  IP-mask  auth-method  auth-options
+hostnogss  database  user  IP-address  IP-mask  auth-method  auth-options
 
The meaning of the fields is as follows:
 
@@ -128,9 +130,10 @@ hostnossl  database  user