Re: weird libpq GSSAPI comment

[Robbie Harwood]

Stephen Frost  writes:

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

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

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: weird libpq GSSAPI comment

[Robbie Harwood]

Alvaro Herrera  writes:

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

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

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

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

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

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: weird libpq GSSAPI comment

[Robbie Harwood]

Stephen Frost  writes:

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

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

> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
>> I found this comment in fe-connect.c:
>> 
>> /*
>>  * If GSSAPI is enabled and we have a credential cache, try 
>> to
>>  * set it up before sending startup messages.  If it's 
>> already
>>  * operating, don't try SSL and instead just build the 
>> startup
>>  * packet.
>>  */
>> 
>> I'm not sure I understand this correctly.  Why does it say "just
>> build the startup" packet about the SSL thing, when in reality the
>> SSL block below is unrelated to the GSS logic?  If I consider that
>> SSL is just a typo for GSS, then the comment doesn't seem to describe
>> the logic either, because what it does is go to
>> CONNECTION_GSS_STARTUP state which *doesn't* "build the startup
>> packet" in the sense of pqBuildStartupPacket2/3, but instead it just
>> does pqPacketSend (which is what the SSL block below calls "request
>> SSL instead of sending the startup packet").
>
> SSL there isn't a typo for GSS.  The "startup packet" being referred to
> in that comment is specifically the "request GSS" that's sent via the
> following pqPacketSend, not the pqBuildStartupPacket one.  I agree
> that's a bit confusing and we should probably reword that, since
> "Startup Packet" has a specific meaning in this area of the code.

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

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

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

>> Also, it says "... and we have a credential cache, try to set it
>> up..." but I think it should say "if we *don't* have a credential
>> cache".
>
> No, we call pg_GSS_have_cred_cache() here, which goes on to call
> gss_acquire_cred(), which, as the comment above that function says,
> checks to see if we can acquire credentials by making sure that we *do*
> have a credential cache.  If we *don't* have a credential cache, then we
> fall back to SSL (and then to non-SSL).

Right.

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

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

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

>> Can we discuss a better wording for this comment?  I wrote this, but I
>> don't think it captures all the nuances in this code:
>> 
>> /*
>>  * If GSSAPI is enabled, we need a credential cache; we may
>>  * already have it, or set it up if not.  Then, if we don't
>>  * have a GSS context, request it and switch to
>>  * CONNECTION_GSS_STARTUP to wait for the response.
>>  *
>>  * Fail altogether if GSS is required but cannot be had.
>>  */
>
> We don't set up a credential cache at any point in this code, we only
> check to see if one exists, and only in that case do we send a request
> to start GSSAPI encryption (if it's allowed for us to do so).
>
> Maybe part of the confusion here is that there's two different things- a
> credential cache, and then a credential *handle*.  Calling
> gss_acquire_cred() will, if a credential *cache* exists, return to us a
> credential *handle* (in the form of conn->gcred) that we then pass to
> gss_init_sec_context().

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

> There's then also a GSS *context* (conn->gctx), which gets set up when
> we first call gss_init_sec_context(), and is then used throughout a
> connection.
>
> Typically, the credential cache is actually created when you log into a
> kerberized system, but if not, you can create one by using 'kinit'
> manually.
>
> Hopefully that helps.  I'm certainly happy to work with you to reword
> the comment, of course, but let's make sure there's agreement and
> understanding of what the code does first.

How do you feel about something like this:

If GSSAPI encryption 

Re: PostgreSQL 12 Beta 1 press release draft

[Robbie Harwood]

"Jonathan S. Katz"  writes:

> Attached is a draft of the PG12 Beta 1 press release that is going out
> this Thursday. The primary goals of this release announcement are to
> introduce new features, enhancements, and changes that are available in
> PG12, as well as encourage our users to test and provide feedback to
> help ensure the stability of the release.

Awesome!

> ### Authentication
>
> GSSAPI now supports client and server-side encryption and can be
> specified in the
> [`pg_hba.conf`](https://www.postgresql.org/docs/devel/auth-pg-hba-conf.html)
> file using the `hostgssenc` and `hostnogssenc` record
> types. PostgreSQL 12 also allows for LDAP servers to be discovered
> based on `DNS SRV` records if PostgreSQL was compiled with OpenLDAP.

Maybe a better title for this section would be "Authentication /
Encryption" or maybe even "Connection security"?  I get that this is a
press release though, so feel free to disregard.

Thanks,
--Robbie


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

[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

[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

[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

[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

[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 v22] GSSAPI encryption support

[Robbie Harwood]

Stephen Frost  writes:

> * Robbie Harwood (rharw...@redhat.com) wrote:
>> Stephen Frost  writes:
>> 
>> 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).
>
> Yeah, I don't see that as too much of an issue and it certainly seems
> cleaner and simpler to reason about to me, which seems worth the
> modest additional buffer cost.  That's certainly something we could
> change if others feel differently.
>
>> 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.
>
> It seems unlikely to be an issue to me- and I would contend that the
> prior implementation didn't actually take any steps to prevent the
> other side from sending packets of nearly arbitrary size (up to 1G),
> so while the steady-state memory usage of the prior implementation was
> less when everyone was playing nicely, it could have certainly been
> abused.  I'm a lot happier having an explicit cap on how much memory
> will be used, even if that cap is a bit higher.

Yeah, that's definitely a fair point.  We could combine the two
approaches, but I don't really see a reason to if it's unlikely to be an
issue - as you say, this is more readable.  It can always be a follow-on
if needed.

> I did add in a simple pg_stat_gssapi view, modeled on pg_stat_ssl, so
> that you can check server-side if GSSAPI was used for authentication
> and/or encryption, and what principal was used if GSSAPI was used for
> authentication.

Good idea.

>> Again, these are nits, and I think I'm okay with the changes.
>
> Great, thanks again for reviewing!
>
> Updated patch attached, if you could take another look through it,
> that'd be great.

I'm happy with this!  Appreciate you exploring my concerns.

Thanks,
--Robbie


signature.asc
Description: PGP signature

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

[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]
+$r

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

[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 

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: libpq compression

[Robbie Harwood]

Konstantin Knizhnik  writes:

> First of all thank you for attempting to push this patch, because
> there is really seems to be some disagreement which blocks progress of
> this patch. Unfortunately first reviewer (Robbie Harwood) think that
> my approach cause some layering violation and should be done in other
> way. Robbie several times suggested me to look "how the buffering in
> TLS or GSSAPI works" and do it in similar way.  Frankly speaking I do
> not see some principle differences differences.

Hello,

In order to comply with your evident desires, consider this message a
courtesy notice that I will no longer be reviewing this patch or
accepting future code from you.

Thanks,
--Robbie


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

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

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: libpq compression

[Robbie Harwood]

Konstantin Knizhnik  writes:

> On 08.02.2019 10:01, Iwata, Aya wrote:
>
>>> I fixed all issues you have reported except using list of supported
>>> compression algorithms.
>>
>> Sure. I confirmed that.
>>
>>> It will require extra round of communication between client and
>>> server to make a decision about used compression algorithm.
>>
>> In beginning of this thread, Robbie Harwood said that no extra
>> communication needed.  I think so, too.
>
> Well, I think that this problem is more complex and requires more 
> discussion.
> There are three places determining choice of compression algorithm:
> 1. Specification of compression algorithm by client. Right now it is 
> just boolean "compression" parameter in connection string,
> but it is obviously possible to specify concrete algorithm here.
> 2. List of compression algorithms supported by client.
> 3. List of compression algorithms supported by server.
>
> Concerning first option I have very serious doubt that it is good idea 
> to let client choose compression protocol.
> Without extra round-trip it can be only implemented in this way:
> if client toggles compression option in connection string, then libpq 
> includes in startup packet list of supported compression algorithms.
> Then server intersects this list with its own set of supported 
> compression algorithms and if result is not empty, then
> somehow choose  one of the commonly supported algorithms and sends it to 
> the client with 'z' command.

The easiest way, which I laid out earlier in
id:jlgfu1gqjbk@redhat.com, is to have the server perform selection.
The client sends a list of supported algorithms in startup.  Startup has
a reply, so if the server wishes to use compression, then its startup
reply contains which algorithm to use.  Compression then begins after
startup.

If you really wanted to compress half the startup for some reason, you
can even have the server send a packet which consists of the choice of
compression algorithm and everything else in it subsequently
compressed.  I don't see this being useful.  However, you can use a
similar approach to let the client choose the algorithm if there were
some compelling reason for that (there is none I'm aware of to prefer
one to the other) - startup from client requests compression, reply from
server lists supported algorithms, next packet from client indicates
which one is in use along with compressed payload.

It may help to keep in mind that you are defining your own message type
here.

> Frankly speaking, I do not think that such flexibility in choosing 
> compression algorithms is really needed.
> I do not expect that there will be many situations where old client has 
> to communicate with new server or visa versa.
> In most cases both client and server belongs to the same postgres 
> distributive and so implements the same compression algorithm.
> As far as we are compressing only temporary data (traffic), the problem 
> of providing backward compatibility seems to be not so important.

Your comments have been heard, but this is the model that numerous folks
from project has told you we have.  Your code will not pass review
without algorithm agility.

>> src/backend/libpq/pqcomm.c :
>> In current Postgres source code, pq_recvbuf() calls secure_read()
>> and pq_getbyte_if_available() also calls secure_read().
>> It means these functions are on the same level.
>> However in your change, pq_getbyte_if_available() calls pq_recvbuf(),
>> and  pq_recvbuf() calls secure_read(). The level of these functions is 
>> different.
>>
>> I think the purpose of pq_getbyte_if_available() is to get a
>> character if it exists and the purpose of pq_recvbuf() is to acquire
>> data up to the expected length.  In your change,
>> pq_getbyte_if_available() may have to do unnecessary process waiting
>> or something.
>
> Sorry, but this change is essential. We can have some available data
> in compression buffer and we need to try to fetch it in
> pq_getbyte_if_available() instead of just returning EOF.

Aya is correct about the purposes of these functions.  Take a look at
how the buffering in TLS or GSSAPI works for an example of how to do
this correctly.

As with agility, this is what multiple folks from the project have told
you is a hard requirement.  None of us will be okaying your code without
proper transport layering.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: Commit Fest 2019-01 is now closed

[Robbie Harwood]

Andres Freund  writes:

> There's plenty stuff that's chugging along in development but ought to
> be processed at less urgency / by different people, than the stuff
> targeted to be committed soon. It's already frustrating to contribute
> to postgresql for new people, but if they don't get feedback for half
> a year because they submitted around December / January it's almost
> guaranteed that they vanish.  Additionally, there's an increasing
> amount of development projects that are too large to complete in a
> single cycle, and if we just stop looking at them for half a year,
> they'll also not succeed.

I definitely did this - and I don't think success can be declared in my
case yet.  Would like to talk about that for a moment if that's alright.

GSSAPI encryption was first submitted 2015-07-02.  Discussion on it
continued until 2016-08-01, when I vanished.  Discussion included 118
messages, 49 of which I sent myself, and 13 separate revisions.  At that
point I had gone way over the allotted time to spend on this, and had to
move on to other things.  The account tracking on the commitfest app
wasn't as good then, but this corresponds to 4 commitfests.

Second push started 2018-05-23 and is ongoing.  Discussion has been much
quieter - 30 messages, 10 mine - and 7 revisions (mostly due to cfbot).
Since the commitfest webpage supports github login now, the count for
second push is accurate: 5 commitfests.

So I'm at 9 commitfests total (over 2 years).  The total amount of time
spent on this is incredibly daunting.  And this isn't an isolated case;
for instance, at the top of the current commitfest is
https://commitfest.postgresql.org/22/528/ which has been in 14
commitfests.  14 - this'll be 3 years next month.  Others have 11, 10,
10, 8...  (I didn't dig into the tracking on this, so they might be
higher for the same reason my count is higher than is reflected.)

postgresql is an amazing piece of software, and it's really cool to have
something to contribute to it.  And I think that the reviews I've
received have been from people who care genuinely that it keeps being
amazing.  But if I weren't regularly dealing with open source and quirky
upstreams for my day job, I would be long gone.  Even so, no contributor
has unlimited time; even for us corporate contributors, management will
eventually decide to solve the problem a different way.

Thanks for letting me get that off my chest.  Happy hacking!

Thanks,
--Robbie


signature.asc
Description: PGP signature

[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-met

cfbot run pgindent?

[Robbie Harwood]

Hi friends,

First, I've found cfbot really useful as tool for improving code
correctness.  So thanks for that.

Second, since the project does have a defined style checker, do you
think it would be possible to run it as part of cfbot and report errors?

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [PATCH v19] GSSAPI encryption support

[Robbie Harwood]

Stephen Frost  writes:

> Greetings Robbie,
>
> * Dmitry Dolgov (9erthali...@gmail.com) wrote:
>> > On Tue, Oct 2, 2018 at 11:12 PM Robbie Harwood  wrote:
>> >
>> > Michael Paquier  writes:
>> >
>> > > On Mon, Aug 06, 2018 at 05:23:28PM -0400, Robbie Harwood wrote:
>> > >> If you're in a position where you're using Kerberos (or most other
>> > >> things from the GSSAPI) for authentication, the encryption comes at
>> > >> little to no additional setup cost.  And then you get all the security
>> > >> benefits outlined in the docs changes.
>> > >
>> > > Please note that the last patch set does not apply anymore, so I have
>> > > moved it to CF 2018-11 and marked it as waiting on author.
>> >
>> > Indeed.  Please find v19 attached.  It's just a rebase; no architecture
>> > changes.
>> 
>> Unfortunately, patch needs another rebase, could you do this? In the meantime
>> I'll move it to the next CF.
>
> This patch needs a few minor changes to get it back to working, but I'm
> happy to say that with those changes, it seems to be working rather well
> for me.

Thanks Stephen!

Dmitry, I will make an update (and address Stephen's feedback) hopefully
soon.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [PATCH v19] GSSAPI encryption support

[Robbie Harwood]

Michael Paquier  writes:

> On Mon, Aug 06, 2018 at 05:23:28PM -0400, Robbie Harwood wrote:
>> If you're in a position where you're using Kerberos (or most other
>> things from the GSSAPI) for authentication, the encryption comes at
>> little to no additional setup cost.  And then you get all the security
>> benefits outlined in the docs changes.
>
> Please note that the last patch set does not apply anymore, so I have
> moved it to CF 2018-11 and marked it as waiting on author.

Indeed.  Please find v19 attached.  It's just a rebase; no architecture
changes.

I'll set commitfest status back to "Needs Review" once I've sent this
mail.

Thanks,
--Robbie


signature.asc
Description: PGP signature
>From 4a91571db6873da46becf2f7ad0cd9055df2 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| 107 +++-
 src/backend/libpq/be-gssapi-common.c|  64 +
 src/backend/libpq/be-gssapi-common.h|  26 ++
 src/backend/libpq/be-secure-gssapi.c| 321 ++
 src/backend/libpq/be-secure.c   |  16 ++
 src/backend/libpq/hba.c |  51 +++-
 src/backend/postmaster/pgstat.c |   3 +
 src/backend/postmaster/postmaster.c |  64 -
 src/include/libpq/hba.h |   4 +-
 src/include/libpq/libpq-be.h|  11 +-
 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   | 232 +++-
 src/interfaces/libpq/fe-gssapi-common.c | 128 +
 src/interfaces/libpq/fe-gssapi-common.h |  23 ++
 src/interfaces/libpq/fe-secure-gssapi.c | 345 
 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, 1571 insertions(+), 193 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 fi

Re: [PATCH v18] GSSAPI encryption support

[Robbie Harwood]

Stephen Frost  writes:

> * Heikki Linnakangas (hlinn...@iki.fi) wrote:
>
>> What is the point of this patch? What's the advantage of GSSAPI
>> encryption over SSL? I was hoping to find the answer by reading the
>> documentation changes, but all I can see is "how" to set it up, and
>> nothing about "why".
>
> If you've already got an existing Kerberos environment, then it's a
> lot nicer to leverage that rather than having to also implement a full
> PKI to support and use SSL-based encryption.
>
> There's also something to be said for having alternatives to OpenSSL.

This exactly.

If you're in a position where you're using Kerberos (or most other
things from the GSSAPI) for authentication, the encryption comes at
little to no additional setup cost.  And then you get all the security
benefits outlined in the docs changes.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] WIP Add ALWAYS DEFERRED option for constraints

[Robbie Harwood]

Nico Williams  writes:

> Attached is an additional patch, as well as a new, rebased patch.
>
> This includes changes responsive to Álvaro Herrera's commentary about
> the SET CONSTRAINTS manual page.

This patch looks good to me.  +1; Álvaro, please update the CF entry
when you're also satisfied.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] WIP Add ALWAYS DEFERRED option for constraints

[Robbie Harwood]

Nico Williams  writes:

> [Re-send; first attempt appears to have hit /dev/null somewhere.  My
> apologies if you get two copies.]
>
> I've finally gotten around to rebasing this patch and making the change
> that was requested, which was: merge the now-would-be-three deferral-
> related bool columns in various pg_catalog tables into one char column.
>
> Instead of (deferrable, initdeferred, alwaysdeferred), now there is just
> (deferral).

This design seems correct to me.  I have a couple questions inline, and
some nits to go with them.

> All tests (make check) pass.

Thank you for adding tests!

> diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
> index 3ed9021..e82e39b 100644
> --- a/doc/src/sgml/catalogs.sgml
> +++ b/doc/src/sgml/catalogs.sgml
> @@ -2239,17 +2239,15 @@ SCRAM-SHA-256$iteration 
> count:
>   
>  
>   
> -  condeferrable
> -  bool
> -  
> -  Is the constraint deferrable?
> - 
> -
> - 
> -  condeferred
> -  bool
> +  condeferral
> +  char
>
> -  Is the constraint deferred by default?
> +  Constraint deferral option:
> +a = always deferred,
> +d = deferrable,
> +d = deferrable initially deferred,

From the rest of the code, I think this is supposed to be 'i', not 'd'.

> +n = not deferrable
> +  
>   
>  
>   

> diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
> index 8b276bc..795a7a9 100644
> --- a/src/backend/catalog/index.c
> +++ b/src/backend/catalog/index.c
> @@ -1070,6 +1070,7 @@ index_create(Relation heapRelation,
>  
>   recordDependencyOn(, , 
> deptype);
>   }
> + Assert((flags & INDEX_CREATE_ADD_CONSTRAINT) == 0);

What does this ensure, and why is it in this part of the code?  We're in
the `else` branch here - isn't this always the case (i.e., Assert can
never fire), since `flags` isn't manipulated in this block?

>   }
>  
>   /* Store dependency on parent index, if any */

> diff --git a/src/backend/catalog/information_schema.sql 
> b/src/backend/catalog/information_schema.sql
> index f4e69f4..bde6199 100644
> --- a/src/backend/catalog/information_schema.sql
> +++ b/src/backend/catalog/information_schema.sql
> @@ -891,10 +891,14 @@ CREATE VIEW domain_constraints AS
> CAST(current_database() AS sql_identifier) AS domain_catalog,
> CAST(n.nspname AS sql_identifier) AS domain_schema,
> CAST(t.typname AS sql_identifier) AS domain_name,
> -   CAST(CASE WHEN condeferrable THEN 'YES' ELSE 'NO' END
> +   CAST(CASE WHEN condeferral = 'n' THEN 'NO' ELSE 'YES' END
>   AS yes_or_no) AS is_deferrable,
> -   CAST(CASE WHEN condeferred THEN 'YES' ELSE 'NO' END
> +   CAST(CASE WHEN condeferral = 'i' OR condeferral = 'a' THEN 'YES' 
> ELSE 'NO' END
>   AS yes_or_no) AS initially_deferred
> +/*
> + * XXX Can we add is_always_deferred here?  Are there
> + * standards considerations?
> + */

I'm not familiar enough to speak to this.  Hopefully someone else can.
Absent other input, I think we should add is_always_deferred.  (And if
we were building this today, probably just expose a single character
instead of three booleans.)

>  FROM pg_namespace rs, pg_namespace n, pg_constraint con, pg_type t
>  WHERE rs.oid = con.connamespace
>AND n.oid = t.typnamespace

> diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
> index 57519fe..41dc6a4 100644
> --- a/src/backend/commands/trigger.c
> +++ b/src/backend/commands/trigger.c
> @@ -3632,6 +3627,7 @@ typedef struct AfterTriggerSharedData
>   TriggerEvent ats_event; /* event type indicator, see trigger.h 
> */
>   Oid ats_tgoid;  /* the trigger's ID */
>   Oid ats_relid;  /* the relation it's on 
> */
> + boolats_alwaysdeferred; /* whether this can be 
> deferred */

"Whether this must be deferred"?  Also, I'm not sure what this is for -
it doesn't seem to be used anywhere.

>   CommandId   ats_firing_id;  /* ID for firing cycle */
>   struct AfterTriggersTableData *ats_table;   /* transition table 
> access */
>  } AfterTriggerSharedData;

> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> index 90dfac2..dab721a 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -184,8 +185,8 @@ static void SplitColQualList(List *qualList,
>List **constraintList, 
> CollateClause **collClause,
>core_yyscan_t 
> yyscanner);
>  static void processCASbits(int cas_bits, int location, const char 
> *constrType,
> -bool *deferrable, bool 

Re: libpq compression

[Robbie Harwood]

Konstantin Knizhnik  writes:

> On 22.06.2018 18:59, Robbie Harwood wrote:
>> Konstantin Knizhnik  writes:
>>> On 21.06.2018 20:14, Robbie Harwood wrote:
>>>> Konstantin Knizhnik  writes:
>>>>> On 21.06.2018 17:56, Robbie Harwood wrote:
>>>>>> Konstantin Knizhnik  writes:
>>>>>>> On 20.06.2018 23:34, Robbie Harwood wrote:
>>>>>>>> Konstantin Knizhnik  writes:
>>>>>>>>
>>>>>>>>> My idea was the following: client want to use compression. But
>>>>>>>>> server may reject this attempt (for any reasons: it doesn't
>>>>>>>>> support it, has no proper compression library, do not want to
>>>>>>>>> spend CPU for decompression,...) Right now compression
>>>>>>>>> algorithm is hardcoded. But in future client and server may
>>>>>>>>> negotiate to choose proper compression protocol.  This is why
>>>>>>>>> I prefer to perform negotiation between client and server to
>>>>>>>>> enable compression.
>>>>>>>>
>>>>>>>> Well, for negotiation you could put the name of the algorithm
>>>>>>>> you want in the startup.  It doesn't have to be a boolean for
>>>>>>>> compression, and then you don't need an additional round-trip.
>>>>>>>
>>>>>>> Sorry, I can only repeat arguments I already mentioned:
>>>>>>>
>>>>>>> - in future it may be possible to specify compression algorithm
>>>>>>>
>>>>>>> - even with boolean compression option server may have some
>>>>>>> reasons to reject client's request to use compression
>>>>>>>
>>>>>>> Extra flexibility is always good thing if it doesn't cost too
>>>>>>> much. And extra round of negotiation in case of enabling
>>>>>>> compression seems to me not to be a high price for it.
>>>>>>
>>>>>> You already have this flexibility even without negotiation.  I
>>>>>> don't want you to lose your flexibility.  Protocol looks like
>>>>>> this:
>>>>>>
>>>>>> - Client sends connection option "compression" with list of
>>>>>> algorithms it wants to use (comma-separated, or something).
>>>>>>
>>>>>> - First packet that the server can compress one of those algorithms
>>>>>> (or none, if it doesn't want to turn on compression).
>>>>>>
>>>>>> No additional round-trips needed.
>>>>>
>>>>> This is exactly how it works now...  Client includes compression
>>>>> option in connection string and server replies with special
>>>>> message ('Z') if it accepts request to compress traffic between
>>>>> this client and server.
>>>>
>>>> No, it's not.  You don't need this message.  If the server receives
>>>> a compression request, it should just turn compression on (or not),
>>>> and then have the client figure out whether it got compression
>>>> back.
>>>
>>> How it will managed to do it. It receives some reply and first of
>>> all it should know whether it has to be decompressed or not.
>>
>> You can tell whether a message is compressed by looking at it.  The
>> way the protocol works, every message has a type associated with it:
>> a single byte, like 'R', that says what kind of message it is.
>
> Compressed message can contain any sequence of bytes, including 'R':)

Then tag your messages with a type byte.  Or do it the other way around
- look for the zstd framing within a message.

Please, try to work with me on this instead of fighting every design
change.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: libpq compression

[Robbie Harwood]

Konstantin Knizhnik  writes:

> On 21.06.2018 20:14, Robbie Harwood wrote:
>> Konstantin Knizhnik  writes:
>>> On 21.06.2018 17:56, Robbie Harwood wrote:
>>>> Konstantin Knizhnik  writes:
>>>>> On 20.06.2018 23:34, Robbie Harwood wrote:
>>>>>> Konstantin Knizhnik  writes:
>>>>>>
>>>>>> Well, that's a design decision you've made.  You could put
>>>>>> lengths on chunks that are sent out - then you'd know exactly how
>>>>>> much is needed.  (For instance, 4 bytes of network-order length
>>>>>> followed by a complete payload.)  Then you'd absolutely know
>>>>>> whether you have enough to decompress or not.
>>>>>
>>>>> Do you really suggest to send extra header for each chunk of data?
>>>>> Please notice that chunk can be as small as one message: dozen of
>>>>> bytes because libpq is used for client-server communication with
>>>>> request-reply pattern.
>>>>
>>>> I want you to think critically about your design.  I *really* don't
>>>> want to design it for you - I have enough stuff to be doing.  But
>>>> again, the design I gave you doesn't necessarily need that - you
>>>> just need to properly buffer incomplete data.
>>>
>>> Right now secure_read may return any number of available bytes. But
>>> in case of using streaming compression, it can happen that available
>>> number of bytes is not enough to perform decompression. This is why
>>> we may need to try to fetch additional portion of data. This is how
>>> zpq_stream is working now.
>>
>> No, you need to buffer and wait until you're called again.  Which is
>> to say: decompress() shouldn't call secure_read().  secure_read()
>> should call decompress().
>
> I this case I will have to implement this code twice: both for backend
> and frontend, i.e. for secure_read/secure_write and
> pqsecure_read/pqsecure_write.

Likely, yes.  You can see that this is how TLS does it (which you should
be using as a model, architecture-wise).

> Frankly speaking i was very upset by design of libpq communication
> layer in Postgres: there are two different implementations of almost
> the same stuff for cbackend and frontend.

Changing the codebases so that more could be shared is not necessarily a
bad idea; however, it is a separate change from compression.

>>> I do not understand how it is possible to implement in different way
>>> and what is wrong with current implementation.
>>
>> The most succinct thing I can say is: absolutely don't modify
>> pq_recvbuf().  I gave you pseudocode for how to do that.  All of your
>> logic should be *below* the secure_read/secure_write functions.
>>
>> I cannot approve code that modifies pq_recvbuf() in the manner you
>> currently do.
>
> Well. I understand you arguments.  But please also consider my
> argument above (about avoiding code duplication).
>
> In any case, secure_read function is called only from pq_recvbuf() as
> well as pqsecure_read is called only from pqReadData.  So I do not
> think principle difference in handling compression in secure_read or
> pq_recvbuf functions and do not understand why it is "destroying the
> existing model".
>
> Frankly speaking, I will really like to destroy existed model, moving
> all system dependent stuff in Postgres to SAL and avoid this awful mix
> of code sharing and duplication between backend and frontend. But it
> is a another story and I do not want to discuss it here.

I understand you want to avoid code duplication.  I will absolutely
agree that the current setup makes it difficult to share code between
postmaster and libpq clients.  But the way I see it, you have two
choices:

1. Modify the code to make code sharing easier.  Once this has been
   done, *then* build a compression patch on top, with the nice new
   architecture.

2. Leave the architecture as-is and add compression support.
   (Optionally, you can make code sharing easier at a later point.)

Fundamentally, I think you value code sharing more than I do.  So while
I might advocate for (2), you might personally prefer (1).

But right now you're not doing either of those.

> If we are speaking about the "right design", then neither your
> suggestion, neither my implementation are good and I do not see
> principle differences between them.
>
> The right approach is using "decorator pattern": this is how streams
> are designed in .Net/Java. You can construct pipe of "secure",
> "compressed" and whatever else streams.

I *strongly* disagree, but I don't think y

Re: libpq compression

[Robbie Harwood]

Konstantin Knizhnik  writes:

> On 21.06.2018 17:56, Robbie Harwood wrote:
>> Konstantin Knizhnik  writes:
>>> On 20.06.2018 23:34, Robbie Harwood wrote:
>>>> Konstantin Knizhnik  writes:
>>>>
>>>> Well, that's a design decision you've made.  You could put lengths
>>>> on chunks that are sent out - then you'd know exactly how much is
>>>> needed.  (For instance, 4 bytes of network-order length followed by
>>>> a complete payload.)  Then you'd absolutely know whether you have
>>>> enough to decompress or not.
>>>
>>> Do you really suggest to send extra header for each chunk of data?
>>> Please notice that chunk can be as small as one message: dozen of
>>> bytes because libpq is used for client-server communication with
>>> request-reply pattern.
>>
>> I want you to think critically about your design.  I *really* don't
>> want to design it for you - I have enough stuff to be doing.  But
>> again, the design I gave you doesn't necessarily need that - you just
>> need to properly buffer incomplete data.
>
> Right now secure_read may return any number of available bytes. But in
> case of using streaming compression, it can happen that available
> number of bytes is not enough to perform decompression. This is why we
> may need to try to fetch additional portion of data. This is how
> zpq_stream is working now.

No, you need to buffer and wait until you're called again.  Which is to
say: decompress() shouldn't call secure_read().  secure_read() should
call decompress().

> I do not understand how it is possible to implement in different way
> and what is wrong with current implementation.

The most succinct thing I can say is: absolutely don't modify
pq_recvbuf().  I gave you pseudocode for how to do that.  All of your
logic should be *below* the secure_read/secure_write functions.

I cannot approve code that modifies pq_recvbuf() in the manner you
currently do.

>>>> My idea was the following: client want to use compression. But
>>>> server may reject this attempt (for any reasons: it doesn't support
>>>> it, has no proper compression library, do not want to spend CPU for
>>>> decompression,...) Right now compression algorithm is
>>>> hardcoded. But in future client and server may negotiate to choose
>>>> proper compression protocol.  This is why I prefer to perform
>>>> negotiation between client and server to enable compression.  Well,
>>>> for negotiation you could put the name of the algorithm you want in
>>>> the startup.  It doesn't have to be a boolean for compression, and
>>>> then you don't need an additional round-trip.
>>>
>>> Sorry, I can only repeat arguments I already mentioned:
>>> - in future it may be possible to specify compression algorithm
>>> - even with boolean compression option server may have some reasons to
>>> reject client's request to use compression
>>>
>>> Extra flexibility is always good thing if it doesn't cost too
>>> much. And extra round of negotiation in case of enabling compression
>>> seems to me not to be a high price for it.
>>
>> You already have this flexibility even without negotiation.  I don't
>> want you to lose your flexibility.  Protocol looks like this:
>>
>> - Client sends connection option "compression" with list of
>>   algorithms it wants to use (comma-separated, or something).
>>
>> - First packet that the server can compress one of those algorithms
>>   (or none, if it doesn't want to turn on compression).
>>
>> No additional round-trips needed.
>
> This is exactly how it works now...  Client includes compression
> option in connection string and server replies with special message
> ('Z') if it accepts request to compress traffic between this client
> and server.

No, it's not.  You don't need this message.  If the server receives a
compression request, it should just turn compression on (or not), and
then have the client figure out whether it got compression back.  This
is of course made harder by your refusal to use packet framing, but
still shouldn't be particularly difficult.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: libpq compression

[Robbie Harwood]

Konstantin Knizhnik  writes:

> On 20.06.2018 23:34, Robbie Harwood wrote:
>> Konstantin Knizhnik  writes:
>>
>>
>> My idea was the following: client want to use compression. But server
>> may reject this attempt (for any reasons: it doesn't support it, has
>> no proper compression library, do not want to spend CPU for
>> decompression,...) Right now compression algorithm is hardcoded. But
>> in future client and server may negotiate to choose proper compression
>> protocol.  This is why I prefer to perform negotiation between client
>> and server to enable compression.
>> Well, for negotiation you could put the name of the algorithm you want
>> in the startup.  It doesn't have to be a boolean for compression, and
>> then you don't need an additional round-trip.
>
> Sorry, I can only repeat arguments I already mentioned:
> - in future it may be possible to specify compression algorithm
> - even with boolean compression option server may have some reasons to 
> reject client's request to use compression
>
> Extra flexibility is always good thing if it doesn't cost too much. And 
> extra round of negotiation in case of enabling compression seems to me 
> not to be a high price for it.

You already have this flexibility even without negotiation.  I don't
want you to lose your flexibility.  Protocol looks like this:

- Client sends connection option "compression" with list of algorithms
  it wants to use (comma-separated, or something).

- First packet that the server can compress one of those algorithms (or
  none, if it doesn't want to turn on compression).

No additional round-trips needed.

>> Well, that's a design decision you've made.  You could put lengths on
>> chunks that are sent out - then you'd know exactly how much is needed.
>> (For instance, 4 bytes of network-order length followed by a complete
>> payload.)  Then you'd absolutely know whether you have enough to
>> decompress or not.
>
> Do you really suggest to send extra header for each chunk of data?
> Please notice that chunk can be as small as one message: dozen of bytes 
> because libpq is used for client-server communication with request-reply 
> pattern.

I want you to think critically about your design.  I *really* don't want
to design it for you - I have enough stuff to be doing.  But again, the
design I gave you doesn't necessarily need that - you just need to
properly buffer incomplete data.

> Frankly speaking I do not completely understand the source of your
> concern.  My primary idea was to preseve behavior of libpq function as
> much as possible, so there is no need to rewrite all places in
> Postgres code when them are used.  It seems to me that I succeed in
> reaching this goal. Incase of enabled compression zpq_stream functions
> (zpq-read/write) are used instead of (pq)secure_read/write and in turn
> are using them to fetch/send more data. I do not see any bad flaws,
> encapsulation violation or some other problems in such solution.
>
> So before discussing some alternative ways of embedding compression in 
> libpq, I will want to understand what's wrong with this approach.

You're destroying the existing model for no reason.  If you needed to, I
could understand some argument for the way you've done it, but what I've
tried to tell you is that you don't need to do so.  It's longer this
way, and it *significantly* complicates the (already difficult to reason
about) connection state machine.

I get that rewriting code can be obnoxious, and it feels like a waste of
time when we have to do so.  (I've been there; I'm on version 19 of my
postgres patchset.)

>>> So loop should be something like this:
>>>
>>>decompress(ptr, n)
>>>   ZSTD_inBuffer in = {0}
>>>   ZSTD_outBuffer out = {0}
>>>
>>>   in.src = ptr
>>>   in.size = n
>>>
>>>   while true
>>>   ret = ZSTD_decompressStream(out, in)
>>>   if ZSTD_isError(ret):
>>>   give_up()
>>> if out.pos != 0
>>> // if we deomcpress soemthing
>>> return out.pos;
>>> read_input(in);
>>
>> The last line is what I'm taking issue with.  The interface we have
>> already in postgres's network code has a notion of partial reads, or
>> that reads might not return data.  (Same with writing and partial
>> writes.)  So you'd buffer what compressed data you have and return -
>> this is the preferred idiom so that we don't block or spin on a
>> nonblocking socket.
>
> If socket is in non-blocking mode and there is available data, then 
> secure_read  function will also im

Re: libpq compression

[Robbie Harwood]

Konstantin Knizhnik  writes:

> On 20.06.2018 00:04, Robbie Harwood wrote:
>> Konstantin Knizhnik  writes:
>>> On 18.06.2018 23:34, Robbie Harwood wrote:
>>>
>>>> I also don't like that you've injected into the *startup* path -
>>>> before authentication takes place.  Fundamentally, authentication
>>>> (if it happens) consists of exchanging some combination of short
>>>> strings (e.g., usernames) and binary blobs (e.g., keys).  None of
>>>> this will compress well, so I don't see it as worth performing this
>>>> negotiation there - it can wait.  It's also another message in
>>>> every startup.  I'd leave it to connection parameters, personally,
>>>> but up to you.
>>>
>>>   From my point of view compression of libpq traffic is similar with
>>> SSL and should be toggled at the same stage.
>>
>> But that's not what you're doing.  This isn't where TLS gets toggled.
>>
>> TLS negotiation happens as the very first packet: after completing
>> the TCP handshake, the client will send a TLS negotiation request.
>> If it doesn't happen there, it doesn't happen at all.
>>
>> (You *could* configure it where TLS is toggled.  This is, I think,
>> not a good idea.  TLS encryption is a probe: the server can reject
>> it, at which point the client tears everything down and connects
>> without TLS.  So if you do the same with compression, that's another
>> point of tearing down an starting over.  The scaling on it isn't good
>> either: if we add another encryption method into the mix, you've
>> doubled the number of teardowns.)
>
> Yes, you are right. There is special message for enabling TLS
> procotol.  But I do not think that the same think is needed for
> compression.  This is why I prefer to specify compression in
> connectoin options. So compression may be enabled straight after
> processing of startup package.  Frankly speaking I still do no see
> reasons to postpone enabling compression till some later moment.

I'm arguing for connection option only (with no additional negotiation
round-trip).  See below.

>>> Definitely authentication parameter are not so large to be
>>> efficiently compressed, by compression (may be in future password
>>> protected) can somehow protect this data.  In any case I do not
>>> think that compression of authentication data may have any influence
>>> on negotiation speed.  So I am not 100% sure that toggling
>>> compression just after receiving startup package is the only right
>>> solution.  But I am not also convinced in that there is some better
>>> place where compressor should be configured.  Do you have some
>>> concrete suggestions for it? In InitPostgres just after
>>> PerformAuthentication ?
>>
>> Hmm, let me try to explain this differently.
>>
>> pq_configure() (as you've called it) shouldn't send a packet.  At its
>> callsite, we *already know* whether we want to use compression -
>> that's what the port->use_compression option says.  So there's no
>> point in having a negotiation there - it's already happened.
>
> My idea was the following: client want to use compression. But server
> may reject this attempt (for any reasons: it doesn't support it, has
> no proper compression library, do not want to spend CPU for
> decompression,...) Right now compression algorithm is hardcoded. But
> in future client and server may negotiate to choose proper compression
> protocol.  This is why I prefer to perform negotiation between client
> and server to enable compression.

Well, for negotiation you could put the name of the algorithm you want
in the startup.  It doesn't have to be a boolean for compression, and
then you don't need an additional round-trip.

>>>> I don't like where you've put the entry points to the compression
>>>> logic: it's a layering violation.  A couple people have expressed
>>>> similar reservations I think, so let me see if I can explain using
>>>> `pqsecure_read()` as an example.  In pseudocode, `pqsecure_read()`
>>>> looks like this:
>>>>
>>>>   if conn->is_tls:
>>>>   n = tls_read(conn, ptr, len)
>>>>   else:
>>>>   n = pqsecure_raw_read(conn, ptr, len)
>>>>   return n
>>>>
>>>> I want to see this extended by your code to something like:
>>>>
>>>>   if conn->is_tls:
>>>>   n = tls_read(conn, ptr, len)
>>>>   else:
>>>>   n = pqsecure_raw_read(conn, ptr, len)
>>>>
>

Re: libpq compression

[Robbie Harwood]

Konstantin Knizhnik  writes:

> On 18.06.2018 23:34, Robbie Harwood wrote:
>
>> I also don't like that you've injected into the *startup* path -
>> before authentication takes place.  Fundamentally, authentication (if
>> it happens) consists of exchanging some combination of short strings
>> (e.g., usernames) and binary blobs (e.g., keys).  None of this will
>> compress well, so I don't see it as worth performing this negotiation
>> there - it can wait.  It's also another message in every startup.
>> I'd leave it to connection parameters, personally, but up to you.
>
>  From my point of view compression of libpq traffic is similar with SSL 
> and should be toggled at the same stage.

But that's not what you're doing.  This isn't where TLS gets toggled.

TLS negotiation happens as the very first packet: after completing the
TCP handshake, the client will send a TLS negotiation request.  If it
doesn't happen there, it doesn't happen at all.

(You *could* configure it where TLS is toggled.  This is, I think, not a
good idea.  TLS encryption is a probe: the server can reject it, at
which point the client tears everything down and connects without TLS.
So if you do the same with compression, that's another point of tearing
down an starting over.  The scaling on it isn't good either: if we add
another encryption method into the mix, you've doubled the number of
teardowns.)

> Definitely authentication parameter are not so large to be efficiently 
> compressed, by compression (may be in future password protected) can 
> somehow protect this data.
> In any case I do not think that compression of authentication data may 
> have any influence on negotiation speed.
> So I am not 100% sure that toggling compression just after receiving 
> startup package is the only right solution.
> But I am not also convinced in that there is some better place where 
> compressor should be configured.
> Do you have some concrete suggestions for it? In InitPostgres just after 
> PerformAuthentication ?

Hmm, let me try to explain this differently.

pq_configure() (as you've called it) shouldn't send a packet.  At its
callsite, we *already know* whether we want to use compression - that's
what the port->use_compression option says.  So there's no point in
having a negotiation there - it's already happened.

The other thing you do in pq_configure() is call zpq_create(), which
does a bunch of initialization for you.  I am pretty sure that all of
this can be deferred until the first time you want to send a compressed
message - i.e., when compress()/decompress() is called for the first
time from *secure_read() or *secure_write().

> Also please notice that compression is useful not only for client-server 
> communication, but also for replication channels.
> Right now it is definitely used in both cases, but if we move 
> pq_configure somewhere else, we should check that this code is invoked 
> in both for normal backends and walsender.

"We" meaning you, at the moment, since I don't think any of the rest of
us have set up tests with this code :)

If there's common code to be shared around, that's great.  But it's not
imperative; in a lot of ways, the network stacks are very different from
each other, as I'm sure you've seen.  Let's not have the desire for code
reuse get in the way of good, maintainable design.

>> Using terminology from https://facebook.github.io/zstd/zstd_manual.html :
>>
>> Right now you use streaming (ZSTD_{compress,decompress}Stream()) as the
>> basis for your API.  I think this is probably a mismatch for what we're
>> doing here - we're doing one-shot compression/decompression of packets,
>> not sending video or something.
>>
>> I think our use case is better served by the non-streaming interface, or
>> what they call the "Simple API" (ZSTD_{decompress,compress}()).
>> Documentation suggests it may be worth it to keep an explicit context
>> around and use that interface instead (i.e.,
>> ZSTD_{compressCCTx,decompressDCtx}()), but that's something you'll have
>> to figure out.
>>
>> You may find making this change helpful for addressing the next issue.
>
> Sorry, but here I completely disagree with you.
> What we are doing is really streaming compression, not compression of 
> individual messages/packages.
> Yes, it is not a video, but actually COPY data has the same nature as 
> video data.
> The main benefit of streaming compression is that we can use the same 
> dictionary for compressing all messages (and adjust this dictionary 
> based on new data).
> We do not need to write dictionary and separate header for each record. 
> Otherwize compression of libpq messages will be completely useless: 
> typical size of message is too short to be efficiently compres

Re: libpq compression

[Robbie Harwood]

tKonstantin Knizhnik  writes:

> On 06.06.2018 02:03, Thomas Munro wrote:
>> On Wed, Jun 6, 2018 at 2:06 AM, Konstantin Knizhnik
>>  wrote:
>>> Thank you for review. Updated version of the patch fixing all reported
>>> problems is attached.
>> Small problem on Windows[1]:
>>
>>C:\projects\postgresql\src\include\common/zpq_stream.h(17): error
>> C2143: syntax error : missing ')' before '*'
>> [C:\projects\postgresql\libpq.vcxproj]
>> 2395
>>
>> You used ssize_t in zpq_stream.h, but Windows doesn't have that type.
>> We have our own typedef in win32_port.h.  Perhaps zpq_stream.c should
>> include postgres.h/postgres_fe.h (depending on FRONTEND) like the
>> other .c files in src/common, before it includes zpq_stream.h?
>> Instead of "c.h".
>>
>> [1] 
>> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.1106
>>
> Thank you very much for reporting the problem.
> I attached new patch with include of postgres_fe.h added to zpq_stream.c

Hello!

Due to being in a similar place, I'm offering some code review.  I'm
excited that you're looking at throughput on the network stack - it's
not usually what we think of in database performance.  Here are some
first thoughts, which have some overlap with what others have said on
this thread already:

###

This build still doesn't pass Windows:
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.2277

You can find more about what the bot is doing here:
http://cfbot.cputube.org/

###

I have a few misgivings about pq_configure(), starting with the name.
The *only* thing this function does is set up compression, so it's
mis-named.  (There's no point in making something generic unless it's
needed - it's just confusing.)

I also don't like that you've injected into the *startup* path - before
authentication takes place.  Fundamentally, authentication (if it
happens) consists of exchanging some combination of short strings (e.g.,
usernames) and binary blobs (e.g., keys).  None of this will compress
well, so I don't see it as worth performing this negotiation there - it
can wait.  It's also another message in every startup.  I'd leave it to
connection parameters, personally, but up to you.

###

Documentation!  You're going to need it.  There needs to be enough
around for other people to implement the protocol (or if you prefer,
enough for us to debug the protocol as it exists).

In conjunction with that, please add information on how to set up
compressed vs. uncompressed connections - similarly to how we've
documentation on setting up TLS connection (though presumably compressed
connection documentation will be shorter).

###

Using terminology from https://facebook.github.io/zstd/zstd_manual.html :

Right now you use streaming (ZSTD_{compress,decompress}Stream()) as the
basis for your API.  I think this is probably a mismatch for what we're
doing here - we're doing one-shot compression/decompression of packets,
not sending video or something.

I think our use case is better served by the non-streaming interface, or
what they call the "Simple API" (ZSTD_{decompress,compress}()).
Documentation suggests it may be worth it to keep an explicit context
around and use that interface instead (i.e.,
ZSTD_{compressCCTx,decompressDCtx}()), but that's something you'll have
to figure out.

You may find making this change helpful for addressing the next issue.

###

I don't like where you've put the entry points to the compression logic:
it's a layering violation.  A couple people have expressed similar
reservations I think, so let me see if I can explain using
`pqsecure_read()` as an example.  In pseudocode, `pqsecure_read()` looks
like this:

if conn->is_tls:
n = tls_read(conn, ptr, len)
else:
n = pqsecure_raw_read(conn, ptr, len)
return n

I want to see this extended by your code to something like:

if conn->is_tls:
n = tls_read(conn, ptr, len)
else:
n = pqsecure_raw_read(conn, ptr, len)

if conn->is_compressed:
n = decompress(ptr, n)

return n

In conjunction with the above change, this should also significantly
reduce the size of the patch (I think).

###

The compression flag has proven somewhat contentious, as you've already
seen on this thread.  I recommend removing it for now and putting it in
a separate patch to be merged later, since it's separable.

###

It's not worth flagging style violations in your code right now, but you
should be aware that there are quite a few style and whitespace
problems.  In particular, please be sure that you're using hard tabs
when appropriate, and that line lengths are fewer than 80 characters
(unless they contain error messages), and that pointers are correctly
declared (`void *arg`, not `void* arg`).

###

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [PATCH v18] GSSAPI encryption support

[Robbie Harwood]

Nico Williams  writes:

> On Mon, Jun 11, 2018 at 04:11:10PM -0400, Robbie Harwood wrote:
>> Nico was kind enough to provide me with some code review.  This should
>> those concerns (clarify short-read behavior and fixing error checking on
>> GSS functions).
>
> Besides the bug you fixed and which I told you about off-list (on IRC,
> specifically), I only have some commentary that does not need any
> action:
>
>  - support for non-Kerberos/default GSS mechanisms
>
>This might require new values for gssmode: prefer-
>and require-.  One could always use SPNEGO if there
>are multiple mechanisms to choose from.  And indeed, you could just
>use SPNEGO if the user has credentials for multiple mechanism.
>
>(Because GSS has no standard mechanism _names_, this means making
>some up.  This is one obnoxious shortcoming of the GSS-API...)

As long as it's better than passing raw OIDs on the CLI...

>  - when the SCRAM channel binding work is done, it might be good to add
>an option for TLS + GSS w/ channel binding to TLS and no gss wrap
>tokens

I think both of these are neat ideas if they'll be used.  Getting GSSAPI
encryption in shouldn't preclude either in its present form (should make
it easier, I hope), but I'm glad to hear of possible future work as
well!

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: Fix some error handling for read() and errno

[Robbie Harwood]

Michael Paquier  writes:

> diff --git a/src/backend/access/transam/slru.c 
> b/src/backend/access/transam/slru.c
> index 87942b4cca..d487347cc6 100644
> --- a/src/backend/access/transam/slru.c
> +++ b/src/backend/access/transam/slru.c
> @@ -683,6 +683,11 @@ SlruPhysicalReadPage(SlruCtl ctl, int pageno, int slotno)
>   pgstat_report_wait_start(WAIT_EVENT_SLRU_READ);
>   if (read(fd, shared->page_buffer[slotno], BLCKSZ) != BLCKSZ)
>   {
> + /*
> +  * XXX: Note that this may actually report sucess if the number
> +  * of bytes read is positive, but lacking data so that errno is
> +  * not set.
> +  */
>   pgstat_report_wait_end();
>   slru_errcause = SLRU_READ_FAILED;
>   slru_errno = errno;

It might be less confusing to just set errno if it's not set already
(e.g., to EIO, or something).  Up to you though - this is a bit of a
niche case.

The rest of the patch looks good to me.

Thanks,
--Robbie



signature.asc
Description: PGP signature

Re: [PATCH v18] GSSAPI encryption support

[Robbie Harwood]

Hi,

Nico was kind enough to provide me with some code review.  This should
those concerns (clarify short-read behavior and fixing error checking on
GSS functions).

Thanks,
--Robbie


signature.asc
Description: PGP signature
>From 20b9f7d5cd9a35e7210ccc309bada789411b6cdb 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| 103 +++
 src/backend/libpq/be-gssapi-common.c|  64 +
 src/backend/libpq/be-gssapi-common.h|  26 ++
 src/backend/libpq/be-secure-gssapi.c| 321 ++
 src/backend/libpq/be-secure.c   |  16 ++
 src/backend/libpq/hba.c |  51 +++-
 src/backend/postmaster/pgstat.c |   3 +
 src/backend/postmaster/postmaster.c |  64 -
 src/include/libpq/hba.h |   4 +-
 src/include/libpq/libpq-be.h|  11 +-
 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  |  90 +--
 src/interfaces/libpq/fe-connect.c   | 232 +++-
 src/interfaces/libpq/fe-gssapi-common.c | 128 +
 src/interfaces/libpq/fe-gssapi-common.h |  23 ++
 src/interfaces/libpq/fe-secure-gssapi.c | 345 
 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 |  22 +-
 27 files changed, 1578 insertions(+), 202 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 656d5f9417..38cf32e3be 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
  
   
This record matches connection attempts made using TCP/IP.
-   host records match either
+   host records match
SSL or non-SSL connection
-   attempts.
+   attempts as well as GSSAPI or
+   non-GSSAPI connection attempts.
   
  
   
@@ -176,6 +179,42 @@ hostnossl  database  user
  
 
 
+
+ hostgss
+ 
+  
+   This record matches connection 

Re: [PATCH v17] GSSAPI encryption support

[Robbie Harwood]

Thomas Munro  writes:

> On Sat, May 26, 2018 at 6:58 AM, Robbie Harwood  wrote:
>> Me and the bot are having an argument.  This should green Linux but I
>> dunno about Windows.
>
> BTW if you're looking for a way to try stuff out on Windows exactly
> the way cfbot does it without posting a new patch to the mailing list,
> I put some instructions here:
>
> https://wiki.postgresql.org/wiki/Continuous_Integration
>
> The .patch there could certainly be improved (ideally, I think, it'd
> run the whole build farm animal script) but it's a start.

Appreciated.  I think I've got it now.

Thanks,
--Robbie


signature.asc
Description: PGP signature
>From 43a83485a487f8aaf78109dea5d63b26b395c683 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| 103 +++
 src/backend/libpq/be-gssapi-common.c|  64 +
 src/backend/libpq/be-gssapi-common.h|  26 ++
 src/backend/libpq/be-secure-gssapi.c| 319 ++
 src/backend/libpq/be-secure.c   |  16 ++
 src/backend/libpq/hba.c |  51 +++-
 src/backend/postmaster/pgstat.c |   3 +
 src/backend/postmaster/postmaster.c |  64 -
 src/include/libpq/hba.h |   4 +-
 src/include/libpq/libpq-be.h|  11 +-
 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  |  90 +--
 src/interfaces/libpq/fe-connect.c   | 232 +++-
 src/interfaces/libpq/fe-gssapi-common.c | 128 +
 src/interfaces/libpq/fe-gssapi-common.h |  23 ++
 src/interfaces/libpq/fe-secure-gssapi.c | 343 
 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 |  22 +-
 27 files changed, 1574 insertions(+), 202 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 656d5f9417..38cf32e3be 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  databa

Re: [PATCH v16] GSSAPI encryption support

[Robbie Harwood]

Robbie Harwood <rharw...@redhat.com> writes:

> Thomas Munro <thomas.mu...@enterprisedb.com> writes:
>
>> On Thu, May 24, 2018 at 8:00 AM, Robbie Harwood <rharw...@redhat.com> wrote:
>>
>>> Zombie patch is back from the dead.
>>
>> Hi Robbie,
>>
>> Robots[1] vs zombies:
>>
>> + $postgres->RemoveFile('src/backennd/libpq/be-gssapi-common.c');
>>
>> Typo, breaks on Windows.
>>
>> runtime.sgml:2032: parser error : Opening and ending tag mismatch:
>> para line 2026 and sect1
>>  
>>  ^
>>
>> Docs malformed.
>>
>> [1] http://cfbot.cputube.org/robbie-harwood.html
>
> Hah, this is great!  Looks like I failed to build the docs.
>
> Here's an updated version that should fix those.

Me and the bot are having an argument.  This should green Linux but I
dunno about Windows.

Thanks,
--Robbie


signature.asc
Description: PGP signature
>From 1bf21be9d9cd05bf2bcb37c474888b4d8ff6fb63 Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharw...@redhat.com>
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| 103 +++
 src/backend/libpq/be-gssapi-common.c|  64 +
 src/backend/libpq/be-gssapi-common.h|  26 ++
 src/backend/libpq/be-secure-gssapi.c| 319 ++
 src/backend/libpq/be-secure.c   |  16 ++
 src/backend/libpq/hba.c |  51 +++-
 src/backend/postmaster/pgstat.c |   3 +
 src/backend/postmaster/postmaster.c |  64 -
 src/include/libpq/hba.h |   4 +-
 src/include/libpq/libpq-be.h|  11 +-
 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  |  90 +--
 src/interfaces/libpq/fe-connect.c   | 232 +++-
 src/interfaces/libpq/fe-gssapi-common.c | 128 +
 src/interfaces/libpq/fe-gssapi-common.h |  23 ++
 src/interfaces/libpq/fe-secure-gssapi.c | 343 
 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 |  24 +-
 27 files changed, 1576 insertions(+), 202 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 656d5f9417..38cf32e3be 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-mas

Re: [PATCH v15] GSSAPI encryption support

[Robbie Harwood]

Thomas Munro <thomas.mu...@enterprisedb.com> writes:

> On Thu, May 24, 2018 at 8:00 AM, Robbie Harwood <rharw...@redhat.com> wrote:
>
>> Zombie patch is back from the dead.
>
> Hi Robbie,
>
> Robots[1] vs zombies:
>
> + $postgres->RemoveFile('src/backennd/libpq/be-gssapi-common.c');
>
> Typo, breaks on Windows.
>
> runtime.sgml:2032: parser error : Opening and ending tag mismatch:
> para line 2026 and sect1
>  
>  ^
>
> Docs malformed.
>
> [1] http://cfbot.cputube.org/robbie-harwood.html

Hah, this is great!  Looks like I failed to build the docs.

Here's an updated version that should fix those.

Thanks,
--Robbie


signature.asc
Description: PGP signature
>From 53235b7922bd37e361338d160120657a757d7448 Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharw...@redhat.com>
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| 103 +++
 src/backend/libpq/be-gssapi-common.c|  64 +
 src/backend/libpq/be-gssapi-common.h|  26 ++
 src/backend/libpq/be-secure-gssapi.c| 319 ++
 src/backend/libpq/be-secure.c   |  16 ++
 src/backend/libpq/hba.c |  51 +++-
 src/backend/postmaster/pgstat.c |   3 +
 src/backend/postmaster/postmaster.c |  64 -
 src/include/libpq/hba.h |   4 +-
 src/include/libpq/libpq-be.h|  11 +-
 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  |  90 +--
 src/interfaces/libpq/fe-connect.c   | 232 +++-
 src/interfaces/libpq/fe-gssapi-common.c | 128 +
 src/interfaces/libpq/fe-gssapi-common.h |  23 ++
 src/interfaces/libpq/fe-secure-gssapi.c | 343 
 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 |  20 +-
 27 files changed, 1572 insertions(+), 202 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 656d5f9417..38cf32e3be 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
 
 

Re: [PATCH v14] GSSAPI encryption support

[Robbie Harwood]

Hello -hackers,

Zombie patch is back from the dead.  It's been a bit more than two years
since v12 (the last major revision) and almost three since it was
originally submitted.  (I do have enough pride to point out that it did
not actually /take/ anywhere close to two years to update it.)

CC'd are reviewers from before; I appreciate their input from before,
but there is of course no obligation for them to page all this back in,
especially if they don't want to.  A large chunk of this code is
unchanged from previous iterations of the patch, but this is a major
re-architect.  Various things have also been previously fixed as part of
the GSSAPI testing efforts, for which I am grateful.

So: this is GSSAPI encryption support for libpq.  Based on feedback on
previous versions, GSSAPI encryption has a separate negotiation step -
similar to SSL negotiation.  I've tried to incorporate all other
feedback I've received thus far, but very likely missed things (and
introduced new problems).

To actually see encryption, you'll first need to configure the server as
for GSSAPI authentication.  You'll also need to ensure the HBA
configuration has a rule that will permit it.  However, there should
hopefully be enough information to set this up in the corresponding docs
changes (and if there isn't, I should fix it).  The Kerberos/GSSAPI
implementation shouldn't matter, but I am testing using MIT krb5
(through freeIPA); I wrote a post a while back for my setup here:
https://mivehind.net/2015/06/11/kerberized-postgresql/

Finally, I've submitted this as a single patch because it was requested
previously.  I'm happy to break it apart into many commits instead, if
that's helpful.

Thanks,
--Robbie


signature.asc
Description: PGP signature
>From 45de59244e4b9ef887cf910a17cbe63c9043f17e Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharw...@redhat.com>
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 |  60 -
 doc/src/sgml/runtime.sgml   |  80 +-
 src/backend/libpq/Makefile  |   4 +
 src/backend/libpq/auth.c| 103 +++
 src/backend/libpq/be-gssapi-common.c|  64 +
 src/backend/libpq/be-gssapi-common.h|  26 ++
 src/backend/libpq/be-secure-gssapi.c| 319 ++
 src/backend/libpq/be-secure.c   |  16 ++
 src/backend/libpq/hba.c |  51 +++-
 src/backend/postmaster/pgstat.c |   3 +
 src/backend/postmaster/postmaster.c |  64 -
 src/include/libpq/hba.h |   4 +-
 src/include/libpq/libpq-be.h|  11 +-
 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  |  90 +--
 src/interfaces/libpq/fe-connect.c   | 232 +++-
 src/interfaces/libpq/fe-gssapi-common.c | 128 +
 src/interfaces/libpq/fe-gssapi-common.h |  23 ++
 src/interfaces/libpq/fe-secure-gssapi.c | 343 
 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 |  20 +-
 27 files change

Re: Kerberos test suite

[Robbie Harwood]

Peter Eisentraut  writes:

> On 3/5/18 21:08, Michael Paquier wrote:
>
>> Perhaps the tests should be skipped on Windows or just produce an error?
>> Like LDAP tests, libraries are supported on Windows but the hardcoded
>> paths make things harder to handle there.
>
> Hmm, why couldn't someone install MIT krb5 on Windows and give it a try?
>  We don't need to block the platform outright.

krb5kdc doesn't support windows; only the client portion works there.

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: Kerberos test suite

[Robbie Harwood]

Thomas Munro  writes:

> Peter Eisentraut  wrote:
>> On 2/27/18 00:56, Thomas Munro wrote:
>>> FWIW it passes for me if I add this:
>>>
>>> +elsif ($^O eq 'freebsd')
>>> +{
>>> +   $krb5_bin_dir = '/usr/local/bin';
>>> +   $krb5_sbin_dir = '/usr/local/sbin';
>>
>> I suppose you only need the second one, right?  The first one should be
>> in the path.
>
> Erm.  Turned out I had a couple of different versions in my path, but
> your test only works with the binaries in the paths shown above (the
> binaries installed from ports, or rather pkg install krb5, not the
> ones from the base system).
>
> munro@asterix $ /usr/bin/krb5-config --version
> FreeBSD heimdal 1.1.0

This one's Heimdal; the test is intended to only work with MIT.  Perhaps
it should check the output of krb5-config instead of failing
confusingly?

> munro@asterix $ /usr/local/bin/krb5-config --version
> Kerberos 5 release 1.15.2

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: Kerberos test suite

[Robbie Harwood]

Michael Paquier  writes:

> On Wed, Feb 14, 2018 at 09:27:04AM -0500, Peter Eisentraut wrote:
>
>> (If it appears to hang for you in the "setting up Kerberos" step, you
>> might need more entropy/wait a while.  That problem appears to be
>> limited to some virtual machine setups, but the specifics are not
>> clear.)
>
> That's one of those "move your mouse" or "type randomly your keyboard"
> to generate more entropy for the installation setup?

Right.

Whether this message can show up will depend on how the krb5 was built
(and how new it is).  krb5 > 1.15 has support for getrandom(), so on
most Linux distros, if the machine has successfully ever created 256
bits of entropy, it won't block.  On other platforms, use of
/dev/urandom requires a build flag.

Thanks,
--Robbie


signature.asc
Description: PGP signature