Re: [PATCH v15] GSSAPI encryption support

Hello -hackers,

As previously discussed on this list, I have coded up GSSAPI encryption
support. If it is easier for anyone, this code is also available for
viewing on my github:
https://github.com/postgres/postgres/compare/master...frozencemetery:feature/gssencrypt

Fallback support is present in both directions for talking to old client
and old servers; GSSAPI encryption is by default auto-upgraded to where
available (for compatibility), but both client and server contain
settings for requiring it.

There are 8 commits in this series; I have tried to err on the side of
creating too much separation rather than too little. A patch for each
is attached. This is v1 of the series.

Thanks!

Robbie,

* Robbie Harwood (rharwood(at)redhat(dot)com) wrote:
> As previously discussed on this list, I have coded up GSSAPI encryption
> support. If it is easier for anyone, this code is also available for
> viewing on my github:
> https://github.com/postgres/postgres/compare/master...frozencemetery:feature/gssencrypt
>
> Fallback support is present in both directions for talking to old client
> and old servers; GSSAPI encryption is by default auto-upgraded to where
> available (for compatibility), but both client and server contain
> settings for requiring it.
>
> There are 8 commits in this series; I have tried to err on the side of
> creating too much separation rather than too little. A patch for each
> is attached. This is v1 of the series.

Excellent, thanks! I've got other things to tend to at the moment, but
this is definitely something I'm interested in and will work on (likely
in August).

If we could get a reviewer or two to take a look and take the patch out
for a test-drive, at least, that would be a huge help.

Thanks again!

Stephen

On Fri, Jul 3, 2015 at 3:22 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:

> Hello -hackers,
>
> As previously discussed on this list, I have coded up GSSAPI encryption
> support. If it is easier for anyone, this code is also available for
> viewing on my github:
>
> https://github.com/postgres/postgres/compare/master...frozencemetery:feature/gssencrypt
>
> Fallback support is present in both directions for talking to old client
> and old servers; GSSAPI encryption is by default auto-upgraded to where
> available (for compatibility), but both client and server contain
> settings for requiring it.
>
> There are 8 commits in this series; I have tried to err on the side of
> creating too much separation rather than too little. A patch for each
> is attached. This is v1 of the series.
>

I just had a quick look at this patch, and here are some comments:
+ <para>
+ If the client has probed <acronym>GSSAPI</acronym> encryption support
and
+ the connection is <acronym>GSSAPI</acronym>-authenticated, then after
the
+ server sends AuthenticationOk, all traffic between the client and
server
+ will be <acronym>GSSAPI</acronym>-encrypted. Because
+ <acronym>GSSAPI</acronym> does not provide framing,
+ <acronym>GSSAPI</acronym>-encrypted messages are modeled after
protocol-3
+ messages: the first byte is the caracter g, then four bytes of length,
and
+ then an encrypted message.
+ </para>
Message formats should be described in protocol.sgml in the section for
message formats.

+ network. In the <filename>pg_hba.conf</> file, the GSS authenticaion
+ method has a parameter to require encryption; otherwise, connections
+ will be encrypted if available and requiested by the client. On the
s/authenticaion/authentication
s/requiested/requested

+ Whether to require GSSAPI encryption. Default is off, which causes
+ GSSAPI encryption to be enabled if available and requested for
+ compatability with old clients. It is recommended to set this
unless
+ old clients are present.
s/compatability/compatibility

Going through the docs, the overall approach taken by the patch looks neat,
and the default values as designed for both the client and the server are
good things to do. Now actually looking at the code I am suspecting that
some code portions could be largely simplified in the authentication
protocol code, though I don't have the time yet to look at that in details.

Also, when trying to connect with GSSAPI, I found the following problem:
psql: lost synchronization with server: got message type "S", length 22
This happens whatever the value of require_encrypt on server-side is,
either 0 or 1.
Regards,
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> On Fri, Jul 3, 2015 at 3:22 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>
>> There are 8 commits in this series; I have tried to err on the side of
>> creating too much separation rather than too little. A patch for each
>> is attached. This is v1 of the series.
>
> I just had a quick look at this patch, and here are some comments:

Hi! Thanks for taking it for a spin.

> + <para>
> + If the client has probed <acronym>GSSAPI</acronym> encryption support and
> + the connection is <acronym>GSSAPI</acronym>-authenticated, then after the
> + server sends AuthenticationOk, all traffic between the client and server
> + will be <acronym>GSSAPI</acronym>-encrypted. Because
> + <acronym>GSSAPI</acronym> does not provide framing,
> + <acronym>GSSAPI</acronym>-encrypted messages are modeled after protocol-3
> + messages: the first byte is the caracter g, then four bytes of length, and
> + then an encrypted message.
> + </para>
>
> Message formats should be described in protocol.sgml in the section for
> message formats.

ACK. In next version of patch.

> + network. In the <filename>pg_hba.conf</> file, the GSS authenticaion
> + method has a parameter to require encryption; otherwise, connections
> + will be encrypted if available and requiested by the client. On the
> s/authenticaion/authentication
> s/requiested/requested
>
> + Whether to require GSSAPI encryption. Default is off, which causes
> + GSSAPI encryption to be enabled if available and requested for
> + compatability with old clients. It is recommended to set this
> unless
> + old clients are present.
> s/compatability/compatibility

Thanks for catching these. They'll be included in a new version of the
series, which I'll post once you and I have resolved the issue at the
bottom.

> Going through the docs, the overall approach taken by the patch looks neat,
> and the default values as designed for both the client and the server are
> good things to do. Now actually looking at the code I am suspecting that
> some code portions could be largely simplified in the authentication
> protocol code, though I don't have the time yet to look at that in details.

If there are ways to make it simpler without sacrificing clarity, I
welcome them. Fresh eyes could definitely help with that!

> Also, when trying to connect with GSSAPI, I found the following problem:
> psql: lost synchronization with server: got message type "S", length 22
> This happens whatever the value of require_encrypt on server-side is,
> either 0 or 1.

Well that's not good! Since I'm not seeing this failure (even after
rebuilding my setup with patches applied to master), can you give me
more information here? Since it's independent of require_encrypt, can
you verify it doesn't happen on master without my patches? What
messages went over the wire to/from the server before this occurred (and
what was it trying to send at the time)? Did you have valid
credentials?

On Sat, Aug 22, 2015 at 4:06 AM, Robbie Harwood wrote:
>
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> > Going through the docs, the overall approach taken by the patch looks neat,
> > and the default values as designed for both the client and the server are
> > good things to do. Now actually looking at the code I am suspecting that
> > some code portions could be largely simplified in the authentication
> > protocol code, though I don't have the time yet to look at that in details.
>
> If there are ways to make it simpler without sacrificing clarity, I
> welcome them. Fresh eyes could definitely help with that!

I'll look at that more at next week or the week after.

> > Also, when trying to connect with GSSAPI, I found the following problem:
> > psql: lost synchronization with server: got message type "S", length 22
> > This happens whatever the value of require_encrypt on server-side is,
> > either 0 or 1.
>
> Well that's not good! Since I'm not seeing this failure (even after
> rebuilding my setup with patches applied to master), can you give me
> more information here? Since it's independent of require_encrypt, can
> you verify it doesn't happen on master without my patches?

Well, I imagine that I have done nothing complicated... I have simply
set up a Kerberos KDC on a dev box, created necessary credentials on
this box in a keytab file that I have used afterwards to initialize a
Kerberos context with kinit for the psql client. On master things
worked fine, I was able to connect via gssapi. But with your patch the
communication protocol visibly lost track of the messages. I took a
memo about that, it's a bit rough, does not use pg_ident, but if that
can help:
http://michael.otacoo.com/manuals/postgresql/kerberos/

> What messages went over the wire to/from the server before this occurred (and
> what was it trying to send at the time)?

I haven't checked what were the messages sent over the network yet.

> Did you have valid credentials?

Yep. I just tried on master before switching to a build with your
patch that failed. After moving back to master things worked again.
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> On Fri, Jul 3, 2015 at 3:22 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>
>> Hello -hackers,
>>
>> As previously discussed on this list, I have coded up GSSAPI encryption
>> support. If it is easier for anyone, this code is also available for
>> viewing on my github:
>>
>> https://github.com/postgres/postgres/compare/master...frozencemetery:feature/gssencrypt
>>
>> Fallback support is present in both directions for talking to old client
>> and old servers; GSSAPI encryption is by default auto-upgraded to where
>> available (for compatibility), but both client and server contain
>> settings for requiring it.
>>
>> There are 8 commits in this series; I have tried to err on the side of
>> creating too much separation rather than too little. A patch for each
>> is attached. This is v1 of the series.
>
> I just had a quick look at this patch, and here are some comments:
> + <para>
> + If the client has probed <acronym>GSSAPI</acronym> encryption support
> and
> + the connection is <acronym>GSSAPI</acronym>-authenticated, then after
> the
> + server sends AuthenticationOk, all traffic between the client and
> server
> + will be <acronym>GSSAPI</acronym>-encrypted. Because
> + <acronym>GSSAPI</acronym> does not provide framing,
> + <acronym>GSSAPI</acronym>-encrypted messages are modeled after
> protocol-3
> + messages: the first byte is the caracter g, then four bytes of length,
> and
> + then an encrypted message.
> + </para>
> Message formats should be described in protocol.sgml in the section for
> message formats.
>
> + network. In the <filename>pg_hba.conf</> file, the GSS authenticaion
> + method has a parameter to require encryption; otherwise, connections
> + will be encrypted if available and requiested by the client. On the
> s/authenticaion/authentication
> s/requiested/requested
>
> + Whether to require GSSAPI encryption. Default is off, which causes
> + GSSAPI encryption to be enabled if available and requested for
> + compatability with old clients. It is recommended to set this
> unless
> + old clients are present.
> s/compatability/compatibility

As promised, here's a V2 to address your issues with comments. I
haven't heard back on the issues you found in testing, so no other
changes are present.

This means that only the last patch has changed. For convenience, I
will therefore only provide this new patch. I have also updated the
version available from my github.

Thanks!

On Wed, Sep 9, 2015 at 4:12 AM, Robbie Harwood wrote:
> Michael Paquier writes:
> As promised, here's a V2 to address your issues with comments. I
> haven't heard back on the issues you found in testing, so no other
> changes are present.

Well, the issue is still here: login through gssapi fails with your
patch, not with HEAD. This patch is next on my review list by the way
so I'll see what I can do about it soon even if I am in the US for
Postgres Open next week. Still, how did you test it? I am just
creating by myself a KDC, setting up a valid credential with kinit,
and after setting up Postgres for this purpose the protocol
communication just fails.

> This means that only the last patch has changed. For convenience, I
> will therefore only provide this new patch. I have also updated the
> version available from my github.

Thanks, this looks better. I have updated my local branch by replacing
the last patch of the previous series by this one, so I'll base any
potential hacking on this one.
Regards,
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> On Wed, Sep 9, 2015 at 4:12 AM, Robbie Harwood wrote:
>> Michael Paquier writes:
>> As promised, here's a V2 to address your issues with comments. I
>> haven't heard back on the issues you found in testing, so no other
>> changes are present.
>
> Well, the issue is still here: login through gssapi fails with your
> patch, not with HEAD. This patch is next on my review list by the way
> so I'll see what I can do about it soon even if I am in the US for
> Postgres Open next week. Still, how did you test it? I am just
> creating by myself a KDC, setting up a valid credential with kinit,
> and after setting up Postgres for this purpose the protocol
> communication just fails.

My KDC is setup through freeIPA; I create a service for postgres,
acquire a keytab, set it in the config file, and fire up the server. It
should go without saying that this is working for me, which is why I
asked you for more information so I could try to debug. I wrote a post
on this back in June when this was still in development:
http://mivehind.net/page/view-page-slug/16/postgres-kerberos

On Thu, Sep 10, 2015 at 1:44 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>
>> On Wed, Sep 9, 2015 at 4:12 AM, Robbie Harwood wrote:
>>> Michael Paquier writes:
>>> As promised, here's a V2 to address your issues with comments. I
>>> haven't heard back on the issues you found in testing, so no other
>>> changes are present.
>>
>> Well, the issue is still here: login through gssapi fails with your
>> patch, not with HEAD. This patch is next on my review list by the way
>> so I'll see what I can do about it soon even if I am in the US for
>> Postgres Open next week. Still, how did you test it? I am just
>> creating by myself a KDC, setting up a valid credential with kinit,
>> and after setting up Postgres for this purpose the protocol
>> communication just fails.
>
> My KDC is setup through freeIPA; I create a service for postgres,
> acquire a keytab, set it in the config file, and fire up the server. It
> should go without saying that this is working for me, which is why I
> asked you for more information so I could try to debug. I wrote a post
> on this back in June when this was still in development:
> http://mivehind.net/page/view-page-slug/16/postgres-kerberos

Hm. OK. I'll give it a try with freeipa and your patch with Fedora for
example. Could you as well try the configuration I have used? In any
case, it seems to me that we have a real problem with your patch: the
gss authentication protocol is broken with your patch and *not* HEAD
when using a custom kdc like the one I have set up manually on one of
my VMs.
--
Michael

On Thu, Sep 10, 2015 at 4:27 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Thu, Sep 10, 2015 at 1:44 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>>
>>> On Wed, Sep 9, 2015 at 4:12 AM, Robbie Harwood wrote:
>>>> Michael Paquier writes:
>>>> As promised, here's a V2 to address your issues with comments. I
>>>> haven't heard back on the issues you found in testing, so no other
>>>> changes are present.
>>>
>>> Well, the issue is still here: login through gssapi fails with your
>>> patch, not with HEAD. This patch is next on my review list by the way
>>> so I'll see what I can do about it soon even if I am in the US for
>>> Postgres Open next week. Still, how did you test it? I am just
>>> creating by myself a KDC, setting up a valid credential with kinit,
>>> and after setting up Postgres for this purpose the protocol
>>> communication just fails.
>>
>> My KDC is setup through freeIPA; I create a service for postgres,
>> acquire a keytab, set it in the config file, and fire up the server. It
>> should go without saying that this is working for me, which is why I
>> asked you for more information so I could try to debug. I wrote a post
>> on this back in June when this was still in development:
>> http://mivehind.net/page/view-page-slug/16/postgres-kerberos
> Hm. OK. I'll give it a try with freeipa and your patch with Fedora for
> example. Could you as well try the configuration I have used? In any
> case, it seems to me that we have a real problem with your patch: the
> gss authentication protocol is broken with your patch and *not* HEAD
> when using a custom kdc like the one I have set up manually on one of
> my VMs.

Looking more at this stuff. Your post assumes that you have an IPA
server available (I am not really familiar with this software stack)
already configured at hand so as you do not need to worry about any
low-level configuration and a KDC is provided as well, among other
things like ntpd or an apache instance. Well, the thing is that we
just need is a KDC for this patch to have an environment suitable for
testing, and some magic commands with kadmin.local, kinit, etc, and
not the whole set of features that an IPA server provides (when
kicking ipa-server-install one needs to provide a realm name, a KDC
admin password, so that's basically just a wrapper setting up
krb5.conf, which is handy when you want to have the full set in your
hands actually, though just to test this patch it does not seem worth
it). And I imagine that you do have an IPA server already set
facilitating your work.

Still, I gave it a try on a Fedora host, giving up after facing
several failures when trying to install the server. because of several
features.

Note that by duckduckging around I have bumped into some more
documentation to set up KDC with Postgres:
http://gpdb.docs.pivotal.io/4320/admin_guide/kerberos.html
This may be useful when testing this patch as well.
Regards,
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> On Thu, Sep 10, 2015 at 4:27 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Thu, Sep 10, 2015 at 1:44 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>>> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>>>> On Wed, Sep 9, 2015 at 4:12 AM, Robbie Harwood wrote:
>>>>> Michael Paquier writes:
>>>>> As promised, here's a V2 to address your issues with comments. I
>>>>> haven't heard back on the issues you found in testing, so no other
>>>>> changes are present.
>>>>
>>>> Well, the issue is still here: login through gssapi fails with your
>>>> patch, not with HEAD. This patch is next on my review list by the
>>>> way so I'll see what I can do about it soon even if I am in the US
>>>> for Postgres Open next week. Still, how did you test it? I am just
>>>> creating by myself a KDC, setting up a valid credential with kinit,
>>>> and after setting up Postgres for this purpose the protocol
>>>> communication just fails.
>>>
>>> My KDC is setup through freeIPA; I create a service for postgres,
>>> acquire a keytab, set it in the config file, and fire up the server.
>>> It should go without saying that this is working for me, which is
>>> why I asked you for more information so I could try to debug. I
>>> wrote a post on this back in June when this was still in
>>> development:
>>> http://mivehind.net/page/view-page-slug/16/postgres-kerberos
>>
>> Hm. OK. I'll give it a try with freeipa and your patch with Fedora
>> for example. Could you as well try the configuration I have used? In
>> any case, it seems to me that we have a real problem with your patch:
>> the gss authentication protocol is broken with your patch and *not*
>> HEAD when using a custom kdc like the one I have set up manually on
>> one of my VMs.
>
> Looking more at this stuff. Your post assumes that you have an IPA
> server available (I am not really familiar with this software stack)
> already configured at hand so as you do not need to worry about any
> low-level configuration and a KDC is provided as well, among other
> things like ntpd or an apache instance. Well, the thing is that we
> just need is a KDC for this patch to have an environment suitable for
> testing, and some magic commands with kadmin.local, kinit, etc, and
> not the whole set of features that an IPA server provides (when
> kicking ipa-server-install one needs to provide a realm name, a KDC
> admin password, so that's basically just a wrapper setting up
> krb5.conf, which is handy when you want to have the full set in your
> hands actually, though just to test this patch it does not seem worth
> it). And I imagine that you do have an IPA server already set
> facilitating your work.
>
> Still, I gave it a try on a Fedora host, giving up after facing
> several failures when trying to install the server. because of several
> features.

I'm sorry to hear that FreeIPA didn't work for you. I'd be remiss if I
didn't suggest you file bugs against the project for things that are
broken, though that's somewhat orthogonal to the patchset at hand.

I gave your setup a try; I spun up a fc22 machine (krb5v1.13.2), and
installed the KDC as per your instructions. I only did two things
differently: I'm using the default unit file for krb5kdc, and I'm using
the postgres base dir /var/lib/pgsql/data (this is a Fedora default and
I'm sure it doesn't matter for purposes of this).

I have no issues, no sync loss; nothing is amiss as far as I can see.
If there is actually a problem here, I need more information from you.
At the very least, as previously mentioned, I need to know what messages
went over the wire to/from the server before it occurred, and what
command (if it it made it to command processing) it was in the midst of
sending.

Thanks,
--Robbie

Robbie Harwood <rharwood(at)redhat(dot)com> writes:

>>>> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>>>>
>>>>> Well, the issue is still here: login through gssapi fails with
>>>>> your patch, not with HEAD. This patch is next on my review list by
>>>>> the way so I'll see what I can do about it soon even if I am in
>>>>> the US for Postgres Open next week. Still, how did you test it? I
>>>>> am just creating by myself a KDC, setting up a valid credential
>>>>> with kinit, and after setting up Postgres for this purpose the
>>>>> protocol communication just fails.
>
> I have no issues, no sync loss; nothing is amiss as far as I can see.
> If there is actually a problem here, I need more information from you.
> At the very least, as previously mentioned, I need to know what
> messages went over the wire to/from the server before it occurred, and
> what command (if it it made it to command processing) it was in the
> midst of sending.

Any follow-up on this? I'd really like my code to be bug-free.

Hi,

I quickly read through the patch, trying to understand what exactly is
happening here. To me the way the patch is split doesn't make much sense
- I don't mind incremental patches, but right now the steps don't
individually make sense.

On 2015-07-02 14:22:13 -0400, Robbie Harwood wrote:
> +#include <assert.h>

postgres.h should be the first header included.

> +size_t
> +be_gss_encrypt(Port *port, char msgtype, const char **msgptr, size_t len)
> +{
> + OM_uint32 major, minor;
> + gss_buffer_desc input, output;
> + uint32 len_n;
> + int conf;
> + char *ptr = *((char **)msgptr);

trivial nitpick, missing spaces...

> +int
> +be_gss_inplace_decrypt(StringInfo inBuf)
> +{
> + OM_uint32 major, minor;
> + gss_buffer_desc input, output;
> + int qtype, conf;
> + size_t msglen = 0;
> +
> + input.length = inBuf->len;
> + input.value = inBuf->data;
> + output.length = 0;
> + output.value = NULL;
> +
> + major = gss_unwrap(&minor, MyProcPort->gss->ctx, &input, &output,
> + &conf, NULL);
> + if (GSS_ERROR(major))
> + {
> + pg_GSS_error(ERROR, gettext_noop("wrapping GSS message failed"),
> + major, minor);
> + return -1;
> + }
> + else if (conf == 0)
> + {
> + ereport(COMMERROR,
> + (errcode(ERRCODE_PROTOCOL_VIOLATION),
> + errmsg("Expected GSSAPI confidentiality but it was not received")));
> + return -1;
> + }

Hm. Aren't we leaking the gss buffer here?

> + qtype = ((char *)output.value)[0]; /* first byte is message type */
> + inBuf->len = output.length - 5; /* message starts */
> +
> + memcpy((char *)&msglen, ((char *)output.value) + 1, 4);
> + msglen = ntohl(msglen);
> + if (msglen - 4 != inBuf->len)
> + {
> + ereport(COMMERROR,
> + (errcode(ERRCODE_PROTOCOL_VIOLATION),
> + errmsg("Length value inside GSSAPI-encrypted packet was malformed")));
> + return -1;
> + }

and here?

Arguably it doesn't matter that much, since we'll usually disconnect
around here, but ...

> + memcpy(inBuf->data, ((char *)output.value) + 5, inBuf->len);
> + inBuf->data[inBuf->len] = '\0'; /* invariant */
> + gss_release_buffer(&minor, &output);
> +
> + return qtype;
> +}

> diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
> index a4b37ed..5a929a8 100644
> --- a/src/backend/libpq/pqcomm.c
> +++ b/src/backend/libpq/pqcomm.c
> @@ -1485,6 +1485,19 @@ socket_putmessage(char msgtype, const char *s, size_t len)
> {
> if (DoingCopyOut || PqCommBusy)
> return 0;
> +
> +#ifdef ENABLE_GSS
> + /* Do not wrap auth requests. */
> + if (MyProcPort->hba->auth_method == uaGSS && gss_encrypt &&
> + msgtype != 'R' && msgtype != 'g')
> + {
> + len = be_gss_encrypt(MyProcPort, msgtype, &s, len);
> + if (len < 0)
> + goto fail;
> + msgtype = 'g';
> + }
> +#endif

Putting encryption specific code here feels rather wrong to me.

> diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
> index 6171ef3..58712fc 100644
> --- a/src/include/libpq/libpq-be.h
> +++ b/src/include/libpq/libpq-be.h
> @@ -30,6 +30,8 @@
> #endif
>
> #ifdef ENABLE_GSS
> +#include "lib/stringinfo.h"
> +

Conditionally including headers providing generic infrastructure strikes
me as a recipe for build failures in different configurations.

> /* TCP keepalives configuration. These are no-ops on an AF_UNIX socket. */
> diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h
> index c408e5b..e788cc8 100644
> --- a/src/include/libpq/libpq.h
> +++ b/src/include/libpq/libpq.h
> @@ -99,4 +99,8 @@ extern char *SSLCipherSuites;
> extern char *SSLECDHCurve;
> extern bool SSLPreferServerCiphers;
>
> +#ifdef ENABLE_GSS
> +extern bool gss_encrypt;
> +#endif

> --- a/src/backend/utils/misc/guc.c
> +++ b/src/backend/utils/misc/guc.c
>
> +#ifdef ENABLE_GSS
> +static void assign_gss_encrypt(bool newval, void *extra);
> +#endif
> +
>
> /*
> * Options for enum values defined in this module.
> @@ -1618,6 +1622,15 @@ static struct config_bool ConfigureNamesBool[] =
> NULL, NULL, NULL
> },
>
> + {
> + {"gss_encrypt", PGC_USERSET, CONN_AUTH_SECURITY,
> + gettext_noop("Whether client wants encryption for this connection."),
> + NULL,
> + GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
> + },
> + &gss_encrypt, false, NULL, assign_gss_encrypt, NULL
> + },
> +
> /* End-of-list marker */
> {
> {NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL, NULL
> @@ -10114,4 +10127,10 @@ show_log_file_mode(void)
> return buf;
> }

The guc should always be present, not just when gss is built in. It
should error out when not supported. As is you're going to get linker
errors around gss_encrypt, assign_gss_encrypt.

> From e55795e0638ca37447ef200f21f74db80b07ebf4 Mon Sep 17 00:00:00 2001
> From: "Robbie Harwood (frozencemetery)" <rharwood(at)redhat(dot)com>
> Date: Fri, 12 Jun 2015 13:27:50 -0400
> Subject: Error when receiving plaintext on GSS-encrypted connections

I don't see why this makes sense as a separate patch.

> Subject: server: hba option for requiring GSSAPI encryption
>
> Also includes logic for failing clients that do not request encryption
> in the startup packet when encryption is required.
> ---
> src/backend/libpq/hba.c | 9 +++++++++
> src/backend/utils/init/postinit.c | 7 ++++++-
> src/backend/utils/misc/guc.c | 12 +++++++++++-
> src/include/libpq/hba.h | 1 +
> 4 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
> index 23c8b5d..90fe57f 100644
> --- a/src/backend/libpq/hba.c
> +++ b/src/backend/libpq/hba.c
> @@ -1570,6 +1570,15 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline, int line_num)
> else
> hbaline->include_realm = false;
> }
> + else if (strcmp(name, "require_encrypt") == 0)
> + {
> + if (hbaline->auth_method != uaGSS)
> + INVALID_AUTH_OPTION("require_encrypt", "gssapi");
> + if (strcmp(val, "1") == 0)
> + hbaline->require_encrypt = true;
> + else
> + hbaline->require_encrypt = false;
> + }

So this is a new, undocumented, option that makes a connection require
encryption? But despite the generic name, it's gss specific?

> @@ -1628,7 +1629,7 @@ static struct config_bool ConfigureNamesBool[] =
> NULL,
> GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
> },
> - &gss_encrypt, false, NULL, assign_gss_encrypt, NULL
> + &gss_encrypt, false, check_gss_encrypt, assign_gss_encrypt, NULL
> },
>
> /* End-of-list marker */
> @@ -10133,4 +10134,13 @@ assign_gss_encrypt(bool newval, void *extra)
> gss_encrypt = newval;
> }
>
> +static bool
> +check_gss_encrypt(bool *newval, void **extra, GucSource source)
> +{
> + if (MyProcPort && MyProcPort->hba && MyProcPort->hba->require_encrypt &&
> + !*newval)
> + return false;
> + return true;
> +}

Doing such checks in a guc assign hook seems like a horrible idea. Yes,
there's some precedent, but still.

Greetings,

Andres Freund

On Sun, Oct 4, 2015 at 1:18 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> Hi,
>
> I quickly read through the patch, trying to understand what exactly is
> happening here. To me the way the patch is split doesn't make much sense
> - I don't mind incremental patches, but right now the steps don't
> individually make sense.

I agree with Andres. While I looked a bit at this patch, I just had a
look at them a whole block and not individually.

> On 2015-07-02 14:22:13 -0400, Robbie Harwood wrote:
> [Andres' comments]

Here are some comments on top of what Andres has mentioned.

--- a/configure.in
+++ b/configure.in
@@ -636,6 +636,7 @@ PGAC_ARG_BOOL(with, gssapi, no, [build with GSSAPI support],
krb_srvtab="FILE:\$(sysconfdir)/krb5.keytab"
])
AC_MSG_RESULT([$with_gssapi])
+AC_SUBST(with_gssapi)

I think that using a new configure variable like that with a dedicated
file fe-secure-gss.c and be-secure-gss.c has little sense done this
way, and that it would be more adapted to get everything grouped in
fe-auth.c for the frontend and auth.c for the backend, or move all the
GSSAPI-related stuff in its own file. I can understand the move
though: this is to imitate OpenSSL in a way somewhat similar to what
has been done for it with a rather generic set if routines, but with
GSSAPI that's a bit different, we do not have such a set of routines,
hence based on this argument moving it to its own file has little
sense. Now, a move that would make sense though is to move all the
GSSAPI stuff in its own file, for example pg_GSS_recvauth and
pg_GSS_error for the backend, and you should do the same for the
frontend with all the pg_GSS_* routines. This should be as well a
refactoring patch on top of the actual feature.

diff --git a/src/interfaces/libpq/fe-secure-gss.c
b/src/interfaces/libpq/fe-secure-gss.c
new file mode 100644
index 0000000..afea9c3
--- /dev/null
+++ b/src/interfaces/libpq/fe-secure-gss.c
@@ -0,0 +1,92 @@
+#include <assert.h>
You should add a proper header to those new files.
--
Michael

Andres Freund <andres(at)anarazel(dot)de> writes:

> Hi,

Hi, thanks for the review; I really appreciate your time in going
through this. I have questions about some of your comments, so I'll
wait a bit before sending a v3. (By the way, there is a v2 of this I've
already posted, though you seem to have replied to the v1. The only
difference is in documentation, though.)

> I quickly read through the patch, trying to understand what exactly is
> happening here. To me the way the patch is split doesn't make much sense
> - I don't mind incremental patches, but right now the steps don't
> individually make sense.

That's fair. Can you suggest a better organization?

> On 2015-07-02 14:22:13 -0400, Robbie Harwood wrote:
>> +#include <assert.h>
>
> postgres.h should be the first header included.

Okay, will fix.

>> +size_t
>> +be_gss_encrypt(Port *port, char msgtype, const char **msgptr, size_t len)
>> +{
>> + OM_uint32 major, minor;
>> + gss_buffer_desc input, output;
>> + uint32 len_n;
>> + int conf;
>> + char *ptr = *((char **)msgptr);
>
> trivial nitpick, missing spaces...

Got it.

>> +int
>> +be_gss_inplace_decrypt(StringInfo inBuf)
>> +{
>> + OM_uint32 major, minor;
>> + gss_buffer_desc input, output;
>> + int qtype, conf;
>> + size_t msglen = 0;
>> +
>> + input.length = inBuf->len;
>> + input.value = inBuf->data;
>> + output.length = 0;
>> + output.value = NULL;
>> +
>> + major = gss_unwrap(&minor, MyProcPort->gss->ctx, &input, &output,
>> + &conf, NULL);
>> + if (GSS_ERROR(major))
>> + {
>> + pg_GSS_error(ERROR, gettext_noop("wrapping GSS message failed"),
>> + major, minor);
>> + return -1;
>> + }
>> + else if (conf == 0)
>> + {
>> + ereport(COMMERROR,
>> + (errcode(ERRCODE_PROTOCOL_VIOLATION),
>> + errmsg("Expected GSSAPI confidentiality but it was not received")));
>> + return -1;
>> + }
>
> Hm. Aren't we leaking the gss buffer here?
>
>> + qtype = ((char *)output.value)[0]; /* first byte is message type */
>> + inBuf->len = output.length - 5; /* message starts */
>> +
>> + memcpy((char *)&msglen, ((char *)output.value) + 1, 4);
>> + msglen = ntohl(msglen);
>> + if (msglen - 4 != inBuf->len)
>> + {
>> + ereport(COMMERROR,
>> + (errcode(ERRCODE_PROTOCOL_VIOLATION),
>> + errmsg("Length value inside GSSAPI-encrypted packet was malformed")));
>> + return -1;
>> + }
>
> and here?
>
> Arguably it doesn't matter that much, since we'll usually disconnect
> around here, but ...

Probably better to be cautious about it. Will fix.

>> diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
>> index a4b37ed..5a929a8 100644
>> --- a/src/backend/libpq/pqcomm.c
>> +++ b/src/backend/libpq/pqcomm.c
>> @@ -1485,6 +1485,19 @@ socket_putmessage(char msgtype, const char *s, size_t len)
>> {
>> if (DoingCopyOut || PqCommBusy)
>> return 0;
>> +
>> +#ifdef ENABLE_GSS
>> + /* Do not wrap auth requests. */
>> + if (MyProcPort->hba->auth_method == uaGSS && gss_encrypt &&
>> + msgtype != 'R' && msgtype != 'g')
>> + {
>> + len = be_gss_encrypt(MyProcPort, msgtype, &s, len);
>> + if (len < 0)
>> + goto fail;
>> + msgtype = 'g';
>> + }
>> +#endif
>
> Putting encryption specific code here feels rather wrong to me.

Do you have a suggestion about where this code *should* go? I need to
filter on the message type since some can't be encrypted. I was unable
to find another place, but I may have missed it.

>> diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
>> index 6171ef3..58712fc 100644
>> --- a/src/include/libpq/libpq-be.h
>> +++ b/src/include/libpq/libpq-be.h
>> @@ -30,6 +30,8 @@
>> #endif
>>
>> #ifdef ENABLE_GSS
>> +#include "lib/stringinfo.h"
>> +
>
> Conditionally including headers providing generic infrastructure strikes
> me as a recipe for build failures in different configurations.

That's fair, will fix.

>> /* TCP keepalives configuration. These are no-ops on an AF_UNIX socket. */
>> diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h
>> index c408e5b..e788cc8 100644
>> --- a/src/include/libpq/libpq.h
>> +++ b/src/include/libpq/libpq.h
>> @@ -99,4 +99,8 @@ extern char *SSLCipherSuites;
>> extern char *SSLECDHCurve;
>> extern bool SSLPreferServerCiphers;
>>
>> +#ifdef ENABLE_GSS
>> +extern bool gss_encrypt;
>> +#endif
>
>> --- a/src/backend/utils/misc/guc.c
>> +++ b/src/backend/utils/misc/guc.c
>>
>> +#ifdef ENABLE_GSS
>> +static void assign_gss_encrypt(bool newval, void *extra);
>> +#endif
>> +
>>
>> /*
>> * Options for enum values defined in this module.
>> @@ -1618,6 +1622,15 @@ static struct config_bool ConfigureNamesBool[] =
>> NULL, NULL, NULL
>> },
>>
>> + {
>> + {"gss_encrypt", PGC_USERSET, CONN_AUTH_SECURITY,
>> + gettext_noop("Whether client wants encryption for this connection."),
>> + NULL,
>> + GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
>> + },
>> + &gss_encrypt, false, NULL, assign_gss_encrypt, NULL
>> + },
>> +
>> /* End-of-list marker */
>> {
>> {NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL, NULL
>> @@ -10114,4 +10127,10 @@ show_log_file_mode(void)
>> return buf;
>> }
>
> The guc should always be present, not just when gss is built in. It
> should error out when not supported. As is you're going to get linker
> errors around gss_encrypt, assign_gss_encrypt.

If that is the style I will conform to it.

>> From e55795e0638ca37447ef200f21f74db80b07ebf4 Mon Sep 17 00:00:00 2001
>> From: "Robbie Harwood (frozencemetery)" <rharwood(at)redhat(dot)com>
>> Date: Fri, 12 Jun 2015 13:27:50 -0400
>> Subject: Error when receiving plaintext on GSS-encrypted connections
>
> I don't see why this makes sense as a separate patch.

As previously stated, if there is another organization you prefer,
please suggest it. As stated in my first email, I have attempted to err
on the side of having too many patches since splitting changesets later
is nontrivial, and squashing is easy.

>> Subject: server: hba option for requiring GSSAPI encryption
>>
>> Also includes logic for failing clients that do not request encryption
>> in the startup packet when encryption is required.
>> ---
>> src/backend/libpq/hba.c | 9 +++++++++
>> src/backend/utils/init/postinit.c | 7 ++++++-
>> src/backend/utils/misc/guc.c | 12 +++++++++++-
>> src/include/libpq/hba.h | 1 +
>> 4 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
>> index 23c8b5d..90fe57f 100644
>> --- a/src/backend/libpq/hba.c
>> +++ b/src/backend/libpq/hba.c
>> @@ -1570,6 +1570,15 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline, int line_num)
>> else
>> hbaline->include_realm = false;
>> }
>> + else if (strcmp(name, "require_encrypt") == 0)
>> + {
>> + if (hbaline->auth_method != uaGSS)
>> + INVALID_AUTH_OPTION("require_encrypt", "gssapi");
>> + if (strcmp(val, "1") == 0)
>> + hbaline->require_encrypt = true;
>> + else
>> + hbaline->require_encrypt = false;
>> + }
>
> So this is a new, undocumented, option that makes a connection require
> encryption? But despite the generic name, it's gss specific?

It was not my intent to leave it undocumented; I believe I documented it
as part of my changes. If there is a place I have missed where it
should be documented, please tell me and I will happily document it there.

>> @@ -1628,7 +1629,7 @@ static struct config_bool ConfigureNamesBool[] =
>> NULL,
>> GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
>> },
>> - &gss_encrypt, false, NULL, assign_gss_encrypt, NULL
>> + &gss_encrypt, false, check_gss_encrypt, assign_gss_encrypt, NULL
>> },
>>
>> /* End-of-list marker */
>> @@ -10133,4 +10134,13 @@ assign_gss_encrypt(bool newval, void *extra)
>> gss_encrypt = newval;
>> }
>>
>> +static bool
>> +check_gss_encrypt(bool *newval, void **extra, GucSource source)
>> +{
>> + if (MyProcPort && MyProcPort->hba && MyProcPort->hba->require_encrypt &&
>> + !*newval)
>> + return false;
>> + return true;
>> +}
>
> Doing such checks in a guc assign hook seems like a horrible idea. Yes,
> there's some precedent, but still.

Where would you prefer they go? Isn't this what check hooks are for -
checking that it's valid to assign?

Thanks!
--Robbie

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> On Sun, Oct 4, 2015 at 1:18 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> Hi,
>>
>> I quickly read through the patch, trying to understand what exactly is
>> happening here. To me the way the patch is split doesn't make much sense
>> - I don't mind incremental patches, but right now the steps don't
>> individually make sense.
>
> I agree with Andres. While I looked a bit at this patch, I just had a
> look at them a whole block and not individually.

I'm hearing block from both of you! Okay, if block is desired, I'll
squish for v3. Sorry for the inconvenience.

>> On 2015-07-02 14:22:13 -0400, Robbie Harwood wrote:
>> [Andres' comments]
>
> Here are some comments on top of what Andres has mentioned.
>
> --- a/configure.in
> +++ b/configure.in
> @@ -636,6 +636,7 @@ PGAC_ARG_BOOL(with, gssapi, no, [build with GSSAPI support],
> krb_srvtab="FILE:\$(sysconfdir)/krb5.keytab"
> ])
> AC_MSG_RESULT([$with_gssapi])
> +AC_SUBST(with_gssapi)
>
> I think that using a new configure variable like that with a dedicated
> file fe-secure-gss.c and be-secure-gss.c has little sense done this
> way, and that it would be more adapted to get everything grouped in
> fe-auth.c for the frontend and auth.c for the backend, or move all the
> GSSAPI-related stuff in its own file. I can understand the move
> though: this is to imitate OpenSSL in a way somewhat similar to what
> has been done for it with a rather generic set if routines, but with
> GSSAPI that's a bit different, we do not have such a set of routines,
> hence based on this argument moving it to its own file has little
> sense. Now, a move that would make sense though is to move all the
> GSSAPI stuff in its own file, for example pg_GSS_recvauth and
> pg_GSS_error for the backend, and you should do the same for the
> frontend with all the pg_GSS_* routines. This should be as well a
> refactoring patch on top of the actual feature.

My understanding is that frontend and backend code need to be separate
(for linking), so it's automatically in two places. I really don't want
to put encryption-related code in files called "auth.c" and "fe-auth.c"
since those files are presumably for authentication, not encryption.

I'm not sure what you mean about "rather generic set if routines";
GSSAPI is a RFC-standardized interface. I think I also don't understand
the last half of your above paragraph.

> diff --git a/src/interfaces/libpq/fe-secure-gss.c
> b/src/interfaces/libpq/fe-secure-gss.c
> new file mode 100644
> index 0000000..afea9c3
> --- /dev/null
> +++ b/src/interfaces/libpq/fe-secure-gss.c
> @@ -0,0 +1,92 @@
> +#include <assert.h>
> You should add a proper header to those new files.

Sorry, what?

On Sat, Oct 10, 2015 at 3:10 AM, Robbie Harwood wrote:
> Michael Paquier writes:
>>> On 2015-07-02 14:22:13 -0400, Robbie Harwood wrote:
>>> [Andres' comments]
>>
>> Here are some comments on top of what Andres has mentioned.
>>
>> --- a/configure.in
>> +++ b/configure.in
>> @@ -636,6 +636,7 @@ PGAC_ARG_BOOL(with, gssapi, no, [build with GSSAPI support],
>> krb_srvtab="FILE:\$(sysconfdir)/krb5.keytab"
>> ])
>> AC_MSG_RESULT([$with_gssapi])
>> +AC_SUBST(with_gssapi)
>>
>> I think that using a new configure variable like that with a dedicated
>> file fe-secure-gss.c and be-secure-gss.c has little sense done this
>> way, and that it would be more adapted to get everything grouped in
>> fe-auth.c for the frontend and auth.c for the backend, or move all the
>> GSSAPI-related stuff in its own file. I can understand the move
>> though: this is to imitate OpenSSL in a way somewhat similar to what
>> has been done for it with a rather generic set if routines, but with
>> GSSAPI that's a bit different, we do not have such a set of routines,
>> hence based on this argument moving it to its own file has little
>> sense. Now, a move that would make sense though is to move all the
>> GSSAPI stuff in its own file, for example pg_GSS_recvauth and
>> pg_GSS_error for the backend, and you should do the same for the
>> frontend with all the pg_GSS_* routines. This should be as well a
>> refactoring patch on top of the actual feature.
>
> My understanding is that frontend and backend code need to be separate
> (for linking), so it's automatically in two places. I really don't want
> to put encryption-related code in files called "auth.c" and "fe-auth.c"
> since those files are presumably for authentication, not encryption.
>
> I'm not sure what you mean about "rather generic set if routines";
> GSSAPI is a RFC-standardized interface. I think I also don't understand
> the last half of your above paragraph.

src/interfaces/libpq/fe-auth.c contains the following set of routines
related to GSS (frontend code in libpq):
- pg_GSS_error_int
- pg_GSS_error
- pg_GSS_continue
- pg_GSS_startup
src/backend/libpq/auth.c contains the following routines related to
GSS (backend code):
- pg_GSS_recvauth
- pg_GSS_error
My point would be simply to move all those routines in two new files
dedicated to GSS, then add your new routines for encryption in it.
Still, the only reason why the OpenSSL routines have been moved out of
be-secure.c to be-secure-openssl.c is to allow other libraries to be
plugged into that, the primary target being SChannel on Windows. And
that's not the case of GSS, so I think that the separation done as in
your patch is not adapted.

>> diff --git a/src/interfaces/libpq/fe-secure-gss.c
>> b/src/interfaces/libpq/fe-secure-gss.c
>> new file mode 100644
>> index 0000000..afea9c3
>> --- /dev/null
>> +++ b/src/interfaces/libpq/fe-secure-gss.c
>> @@ -0,0 +1,92 @@
>> +#include <assert.h>
>> You should add a proper header to those new files.
>
> Sorry, what?

All the files in the source tree need to have a header like that:
/*-------------------------------------------------------------------------
*
* file_name.c
* Description
*
* Portions Copyright (c) 2015, PostgreSQL Global Development Group
*
* IDENTIFICATION
* path/to/file/file_name.c
*
*-------------------------------------------------------------------------
*/
--
Michael

Alright, here's v3. As requested, it's one patch now. Other things
addressed herein include:

- postgres.h/assert.h ordering fix
- spacing around casts
- leaking of GSS buffer in be_gss_inplace_decrypt
- libpq-be.h not having a conditional internal include
- always exposing guc veriable gss_encrypt
- copyright/description headers on all new files
- movement of GSSAPI methods from fe-auth.c and auth.c to fe-gss.c and
be-gss.c respectively
- renaming GSSAPI files to fe-gss.c and be-gss.c (drops -secure)

Andres, one thing you mentioned as "feels rather wrong" was the
GSSAPI-specific code in pqcomm.c; while looking at that again, I have a
slightly better explanation than what I said previously.

Essentially, the problem is that socket_putmessage_noblock() needs to
know the size of the message to put in the buffer but we can't know
that until we've encrypted the message. socket_putmessage_noblock()
calls socket_putmessage() after ensuring the call will not block;
however, other code paths simply call directly into socket_putmessage()
and so socket_putmessage() needs to have a path to encryption as well.

If you have other potential solutions to this problem, I would love to
hear them; right now though I don't see a better way.

Patch follows. Thanks!

On Wed, Oct 14, 2015 at 7:34 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> Alright, here's v3. As requested, it's one patch now. Other things
> addressed herein include:
> Essentially, the problem is that socket_putmessage_noblock() needs to
> know the size of the message to put in the buffer but we can't know
> that until we've encrypted the message. socket_putmessage_noblock()
> calls socket_putmessage() after ensuring the call will not block;
> however, other code paths simply call directly into socket_putmessage()
> and so socket_putmessage() needs to have a path to encryption as well.
>
> If you have other potential solutions to this problem, I would love to
> hear them; right now though I don't see a better way.
>
> Patch follows. Thanks!

After giving a quick shot at this patch, I am still seeing the same problem:
psql: lost synchronization with server: got message type "S", length 22
(And unpatched works, I will try to put my head into your code...)
Regards,
--
Michael

On 14 October 2015 at 06:34, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> Alright, here's v3. As requested, it's one patch now.

I hate to ask, but have you looked at how this interacts with Windows?

We support Windows SSPI (on a domain-member host) authenticating to a
PostgreSQL server using gssapi with spnego.

We also support a PostgreSQL client on *nix authenticating using
gssapi with spnego to a PostgreSQL server that's requesting sspi mode.

The relevant code is all a bit tangled, since there's support in there
for using Kerberos libraries on Windows instead of SSPI too. I doubt
anybody uses that last one, tests it, or cares about it, though, given
the painful hoop-jumping, registry key permission changes, etc
required to make it work.

For bonus fun, RC4, DES, AES128 or AES256 are available/used for
Kerberos encryption on Windows. See
http://blogs.msdn.com/b/openspecification/archive/2011/05/31/windows-configurations-for-kerberos-supported-encryption-type.aspx
. Though given that Win7 defaults to AES256 it's probably reasonable
to simply not care about anything else.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Craig Ringer <craig(at)2ndquadrant(dot)com> writes:

> On 14 October 2015 at 06:34, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>> Alright, here's v3. As requested, it's one patch now.
>
> I hate to ask, but have you looked at how this interacts with Windows?
>
> We support Windows SSPI (on a domain-member host) authenticating to a
> PostgreSQL server using gssapi with spnego.
>
> We also support a PostgreSQL client on *nix authenticating using
> gssapi with spnego to a PostgreSQL server that's requesting sspi mode.
>
> The relevant code is all a bit tangled, since there's support in there
> for using Kerberos libraries on Windows instead of SSPI too. I doubt
> anybody uses that last one, tests it, or cares about it, though, given
> the painful hoop-jumping, registry key permission changes, etc
> required to make it work.
>
> For bonus fun, RC4, DES, AES128 or AES256 are available/used for
> Kerberos encryption on Windows. See
> http://blogs.msdn.com/b/openspecification/archive/2011/05/31/windows-configurations-for-kerberos-supported-encryption-type.aspx
> . Though given that Win7 defaults to AES256 it's probably reasonable
> to simply not care about anything else.

The short - and probably most important - answer is that no, I haven't
tested it, and it would be difficult for me to do so quickly.

In more depth:

Looking at
http://www.postgresql.org/docs/9.4/static/protocol-message-formats.html
suggests that SSPI follows a separate codepath from the GSS code;
certainly it's a different auth request, which means it shouldn't
trigger the encryption path.

There is no reason that using GSSAPI from, e.g., MIT Kerberos for
Windows, should not work here. Even more broadly than that, GSSAPI is a
RFC-standardized protocol with rigorously tested interop etc. etc.,
though whether anyone outside of MIT cares about MIT Kerberos for
Windows I have no idea. As for encryption types, MIT out-of-the-box
supports aes256 + aes128 (in several variants), rc4-hmac and friends,
camellia in several variants, and des3-cbc-sha1. A much wider selection
is available on setting the appropriately named "allow_weak_crypto" in
krb5.conf, though I would hope that would not be needed.

I think the important takeaway here is that I haven't actually changed
how *authentication* works; these changes only affect GSSAPI
post-authentication add encryption functions as defined by that
specification. So if the authentication was working before, and the
GSSAPI implementation is following RFC, I would hope that this would
work still.

Thanks,
--Robbie

On 16 October 2015 at 01:07, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:

> The short - and probably most important - answer is that no, I haven't
> tested it, and it would be difficult for me to do so quickly.

IIRC it's pretty easy to fire up AWS instances that're primary domain
controllers, and then join a Pg box to them with Kerberos. I realise
that's different to having the time and desire to do it, though.

> Looking at
> http://www.postgresql.org/docs/9.4/static/protocol-message-formats.html
> suggests that SSPI follows a separate codepath from the GSS code;
> certainly it's a different auth request, which means it shouldn't
> trigger the encryption path.

It's a different auth request, but the handling in be-auth.c is
co-mingled to handle the cases:

* We're compiled with Kerberos, handling SSPI from Windows
* We're compiled with Kerberos, handling a GSSAPI request
* We're compiled with SSPI, and we're using Windows SSPI to handle a
GSSAPI auth request
* We're compiled with SSPI, and we're using it to handle a SSP client request

SSPI is a wrapper. For Windows Domain members it carries
GSSAPI/Kerberos data with the spnego extension. (For local loopback it
does NTLM, but you don't need to care about that).

> There is no reason that using GSSAPI from, e.g., MIT Kerberos for
> Windows, should not work here.

Except that *nobody* does it. The EDB installer builds Pg with SSPI,
and that's what the overwhelming majority of Windows users use.

Personally I don't care if this patch doesn't add support for GSSAPI
encryption for sspi connections, only that it doesn't break them.

> Even more broadly than that, GSSAPI is a
> RFC-standardized protocol with rigorously tested interop etc. etc.,
> though whether anyone outside of MIT cares about MIT Kerberos for
> Windows I have no idea.

It's maintained enough that AFAIK it works, and Pg supports compiling
with it. No idea if anyone tests it with Pg anymore.

> I think the important takeaway here is that I haven't actually changed
> how *authentication* works; these changes only affect GSSAPI
> post-authentication add encryption functions as defined by that
> specification. So if the authentication was working before, and the
> GSSAPI implementation is following RFC, I would hope that this would
> work still.

Hope so.

I'll put this on my "hope to test it at some stage" list, but I don't
have a prebuilt environment for it and have a lot to get ready for the
next CF, so it'll be a while...

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

* Craig Ringer (craig(at)2ndquadrant(dot)com) wrote:
> On 16 October 2015 at 01:07, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> > Looking at
> > http://www.postgresql.org/docs/9.4/static/protocol-message-formats.html
> > suggests that SSPI follows a separate codepath from the GSS code;
> > certainly it's a different auth request, which means it shouldn't
> > trigger the encryption path.
>
> It's a different auth request, but the handling in be-auth.c is
> co-mingled to handle the cases:

be-auth.c? You mean src/backend/libpq/auth.c?

> * We're compiled with Kerberos, handling SSPI from Windows
> * We're compiled with Kerberos, handling a GSSAPI request

Eh, *kerberos* was removed with 9.4.0. I'm guessing you mean GSSAPI
above.

> * We're compiled with SSPI, and we're using Windows SSPI to handle a
> GSSAPI auth request
> * We're compiled with SSPI, and we're using it to handle a SSP client request
>
> SSPI is a wrapper. For Windows Domain members it carries
> GSSAPI/Kerberos data with the spnego extension. (For local loopback it
> does NTLM, but you don't need to care about that).

I have to admit that I'm not following what you're getting at here.

src/backend/libpq/auth.c and src/interfaces/libpq/fe-auth.c have pretty
clearly independent paths for SSPI and GSSAPI and which is used
(server-side) is based on what the auth method in pg_hba.conf is. In a
properly set up environment, the client could be using either, but it's
not like we try to figure out (or care) which the client is using from
the server's perspective.

Now, if you set up GSSAPI on the client side and tell it to require
encryption and you're using SSPI on the server side (or the other way
around) it's not likely to work without changes to the SSPI code paths,
which we should be looking at doing. I've not looked at the patch in
much depth yet, but hopefully there's a straight-forward way to say
"we're using SSPI on (client/server) and that doesn't support
encryption yet, so don't try to require it" and throw an error if
someone does. I don't think we necessairly have to have encryption with
SSPI working initially (and that's assuming SSPI can actually do
encryption the way GSSAPI can, I don't know that it can).

> > There is no reason that using GSSAPI from, e.g., MIT Kerberos for
> > Windows, should not work here.
>
> Except that *nobody* does it. The EDB installer builds Pg with SSPI,
> and that's what the overwhelming majority of Windows users use.

Apparently *somebody* uses MIT KfW; I doubt MIT would have put out
KfW 4.0 if no one wanted it. I agree that it's clearly a small number
of environments (I don't know of any currently) and it's entirely
possible that none of them use PG with GSSAPI on Windows for
authentication.

> Personally I don't care if this patch doesn't add support for GSSAPI
> encryption for sspi connections, only that it doesn't break them.

Agreed and we should certainly test it, but I'm not seeing what you're
getting at as the concern.

> > Even more broadly than that, GSSAPI is a
> > RFC-standardized protocol with rigorously tested interop etc. etc.,
> > though whether anyone outside of MIT cares about MIT Kerberos for
> > Windows I have no idea.
>
> It's maintained enough that AFAIK it works, and Pg supports compiling
> with it. No idea if anyone tests it with Pg anymore.

The EDB installers were changed to use SSPI instead of MIT KfW (or
perhaps just dropped building with GSSAPI, Dave would probably remember
better than I do) because KfW wasn't being maintained or updated any
more and the latest version had known security issues. That was quite a
while ago and it looks like MIT has decided to revive KfW (note the
drought between about 2007 and 2012 or so though..) with KfW 4.0.

I doubt anyone's tried building or running PG with GSSAPI under Windows
though. I agree it'd be good to test and see and perhaps even the EDB
installers could be changed to add support for it back in (not sure if
Dave really wants to though as it was a bit of a pain..). However,
presumably MIT has updated KfW and contains to maintain it because
someone is using it.

Really, though, assuming that GSSAPI as provided by KfW works per the
spec-defined API, I don't see why it wouldn't work under Windows just
the same as it does under *nix, it's not like we have any
Windows-specific code in backend/libpq/auth.c or
interfaces/libpq/fe-auth.c for GSSAPI except a MingW-specific symbol
definition.

> > I think the important takeaway here is that I haven't actually changed
> > how *authentication* works; these changes only affect GSSAPI
> > post-authentication add encryption functions as defined by that
> > specification. So if the authentication was working before, and the
> > GSSAPI implementation is following RFC, I would hope that this would
> > work still.
>
> Hope so.
>
> I'll put this on my "hope to test it at some stage" list, but I don't
> have a prebuilt environment for it and have a lot to get ready for the
> next CF, so it'll be a while...

Ditto.

Thanks!

Stephen

On 16 October 2015 at 21:34, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Craig Ringer (craig(at)2ndquadrant(dot)com) wrote:
>> On 16 October 2015 at 01:07, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>> > Looking at
>> > http://www.postgresql.org/docs/9.4/static/protocol-message-formats.html
>> > suggests that SSPI follows a separate codepath from the GSS code;
>> > certainly it's a different auth request, which means it shouldn't
>> > trigger the encryption path.
>>
>> It's a different auth request, but the handling in be-auth.c is
>> co-mingled to handle the cases:
>
> be-auth.c? You mean src/backend/libpq/auth.c?

Ahem. Yes.

Also, I was actually thinking of the libpq front-end code, as it turns
out. The backend's handling of each method is much better separated.

>> * We're compiled with Kerberos, handling SSPI from Windows
>> * We're compiled with Kerberos, handling a GSSAPI request
>
> Eh, *kerberos* was removed with 9.4.0. I'm guessing you mean GSSAPI
> above.

Yes, being sloppy.

> src/backend/libpq/auth.c and src/interfaces/libpq/fe-auth.c have pretty
> clearly independent paths for SSPI and GSSAPI

fe-auth.c doesn't. See pg_fe_sendauth(...)'s case AUTH_REQ_GSS: and
AUTH_REQ_SSPI:

Not what I call clearly independent. I've had to deal with that tangle
of joy a few times...

> and which is used
> (server-side) is based on what the auth method in pg_hba.conf is.

Yep, agreed there. I was thinking that the backend overlapped more,
like the frontend does.

> In a
> properly set up environment, the client could be using either, but it's
> not like we try to figure out (or care) which the client is using from
> the server's perspective.

Exactly. I want to make sure we still don't have to care.

> Now, if you set up GSSAPI on the client side and tell it to require
> encryption and you're using SSPI on the server side (or the other way
> around) it's not likely to work without changes to the SSPI code paths,
> which we should be looking at doing.

So long as setting up gssapi auth on the backend without requiring
encryption still works with sspi clients, like it did before, I'm
happy. My concern is avoiding a regression in what works, not
requiring that the new functionality be supported everywhere.

>> > There is no reason that using GSSAPI from, e.g., MIT Kerberos for
>> > Windows, should not work here.
>>
>> Except that *nobody* does it. The EDB installer builds Pg with SSPI,
>> and that's what the overwhelming majority of Windows users use.
>
> Apparently *somebody* uses MIT KfW

Yeah, I meant nobody uses it with Pg on Windows.

> I doubt anyone's tried building or running PG with GSSAPI under Windows
> though. I agree it'd be good to test and see and perhaps even the EDB
> installers could be changed to add support for it back in (not sure if
> Dave really wants to though as it was a bit of a pain..).

It's a lot of a pain, and not just because of maintenance. Kerberos on
Windows needs access to various innards that you don't need when just
using SSPI to delegate SSO to the OS. It's less secure, fiddly, and
annoying. At least it was last time I had to deal with it, when I was
working around some SSPI bugs in psqlODBC before landing up patching
them in the driver instead.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

* Craig Ringer (craig(at)2ndquadrant(dot)com) wrote:
> On 16 October 2015 at 21:34, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> >> It's a different auth request, but the handling in be-auth.c is
> >> co-mingled to handle the cases:
> >
> > be-auth.c? You mean src/backend/libpq/auth.c?
>
> Ahem. Yes.

No worries. :)

> Also, I was actually thinking of the libpq front-end code, as it turns
> out. The backend's handling of each method is much better separated.

Ok.

> > src/backend/libpq/auth.c and src/interfaces/libpq/fe-auth.c have pretty
> > clearly independent paths for SSPI and GSSAPI
>
> fe-auth.c doesn't. See pg_fe_sendauth(...)'s case AUTH_REQ_GSS: and
> AUTH_REQ_SSPI:
>
> Not what I call clearly independent. I've had to deal with that tangle
> of joy a few times...

Hmm, yea, the actual choice of how to respond to the auth method the
server wants us to use is kind of tricky, but once we've settled on one
or the other, the rest of the code is pretty independent.

Still, I agree with your point that we need to be careful to not break
what we have there by, say, trying to move to encrypted GSSAPI with an
SSPI server.

I'm not sure that I want to really change that fe-auth.c code but I
really think there may be a simpler way to have the same behavior. That
mishmash of #ifdef's and swith/case statements strikes me as possibly
being overly cute. Clear blocks of "we got this message, this is what
we do when we're compiled with X, or Y, or X+Y" would probably be a lot
easier to follow.

> > In a
> > properly set up environment, the client could be using either, but it's
> > not like we try to figure out (or care) which the client is using from
> > the server's perspective.
>
> Exactly. I want to make sure we still don't have to care.

Agreed.

> > Now, if you set up GSSAPI on the client side and tell it to require
> > encryption and you're using SSPI on the server side (or the other way
> > around) it's not likely to work without changes to the SSPI code paths,
> > which we should be looking at doing.
>
> So long as setting up gssapi auth on the backend without requiring
> encryption still works with sspi clients, like it did before, I'm
> happy. My concern is avoiding a regression in what works, not
> requiring that the new functionality be supported everywhere.

Great, we're definitely agreed on that.

> >> > There is no reason that using GSSAPI from, e.g., MIT Kerberos for
> >> > Windows, should not work here.
> >>
> >> Except that *nobody* does it. The EDB installer builds Pg with SSPI,
> >> and that's what the overwhelming majority of Windows users use.
> >
> > Apparently *somebody* uses MIT KfW
>
> Yeah, I meant nobody uses it with Pg on Windows.

It'd be nice if it did, in case any of those users *do* end up wanting
to use PG.

> > I doubt anyone's tried building or running PG with GSSAPI under Windows
> > though. I agree it'd be good to test and see and perhaps even the EDB
> > installers could be changed to add support for it back in (not sure if
> > Dave really wants to though as it was a bit of a pain..).
>
> It's a lot of a pain, and not just because of maintenance. Kerberos on
> Windows needs access to various innards that you don't need when just
> using SSPI to delegate SSO to the OS. It's less secure, fiddly, and
> annoying. At least it was last time I had to deal with it, when I was
> working around some SSPI bugs in psqlODBC before landing up patching
> them in the driver instead.

Yeah, I remember working with KfW back-in-the-day. I never played with
the new 4.0 version, so perhaps it's better, but I'm not particularly
anxious to get into that mess again..

As for this patch, the reason I've not been as involved (beyond being
ridiculously busy) is that Michael's environment, which at least appears
perfectly reasonable (and works with PG unpatched) isn't working. If we
can get that working (and I've not looked at what's happening, so I have
no idea how easy or hard that would be), then I'd be a lot more excited
to spend time doing review of the patch.

Thanks!

Stephen

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

> As for this patch, the reason I've not been as involved (beyond being
> ridiculously busy) is that Michael's environment, which at least appears
> perfectly reasonable (and works with PG unpatched) isn't working. If we
> can get that working (and I've not looked at what's happening, so I have
> no idea how easy or hard that would be), then I'd be a lot more excited
> to spend time doing review of the patch.

I would also really like to see this fixed. Unfortunately, I haven't
been able to replicate; I followed Michael's (very nicely written)
writeup and didn't see the issues that Michael did. The only thing I
know about the problem is that it logs:

> psql: lost synchronization with server: got message type "S", length 22

which unfortunately could be a great many things. I've said this a
couple times now, but I really do need more information - a traffic
dump, a list of commands that were run, etc.; unfortunately, the surface
here is pretty large, and while I totally am willing to believe there
are bugs in the code I've written, I do not yet see them.

On Tue, Oct 20, 2015 at 3:01 AM, Robbie Harwood wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>> psql: lost synchronization with server: got message type "S", length 22
>
> which unfortunately could be a great many things. I've said this a
> couple times now, but I really do need more information - a traffic
> dump, a list of commands that were run, etc.; unfortunately, the surface
> here is pretty large, and while I totally am willing to believe there
> are bugs in the code I've written, I do not yet see them.

--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -129,6 +129,58 @@ pqParseInput3(PGconn *conn)
return;
}

+#ifdef ENABLE_GSS
+ /* We want to be ready in both IDLE and BUSY states
for encryption */
+ if (id == 'g' && !conn->gss_disable_enc && conn->gctx)
+ {
+ ssize_t encEnd, next;
[...]
+ }
+ else if (!conn->gss_disable_enc && conn->gss_auth_done &&
+ !conn->gss_decrypted_cur && id != 'E')
+ /* This could be a sync error, so let's handle
it as such. */
+ handleSyncLoss(conn, id, msgLength);
+#endif

Hm. The out-of-sync error I am seeing in my environment is caused by
this block when parsing 'g' messages coming from the backend that are
considered as being GSSAPI-encrypted messages. I am still looking at
that...
--
Michael

Robbie,

On Wed, Oct 21, 2015 at 3:54 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Oct 20, 2015 at 3:01 AM, Robbie Harwood wrote:
>> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>>> psql: lost synchronization with server: got message type "S", length 22
>>
>> which unfortunately could be a great many things. I've said this a
>> couple times now, but I really do need more information - a traffic
>> dump, a list of commands that were run, etc.; unfortunately, the surface
>> here is pretty large, and while I totally am willing to believe there
>> are bugs in the code I've written, I do not yet see them.
>
> --- a/src/interfaces/libpq/fe-protocol3.c
> +++ b/src/interfaces/libpq/fe-protocol3.c
> @@ -129,6 +129,58 @@ pqParseInput3(PGconn *conn)
> return;
> }
>
> +#ifdef ENABLE_GSS
> + /* We want to be ready in both IDLE and BUSY states
> for encryption */
> + if (id == 'g' && !conn->gss_disable_enc && conn->gctx)
> + {
> + ssize_t encEnd, next;
> [...]
> + }
> + else if (!conn->gss_disable_enc && conn->gss_auth_done &&
> + !conn->gss_decrypted_cur && id != 'E')
> + /* This could be a sync error, so let's handle
> it as such. */
> + handleSyncLoss(conn, id, msgLength);
> +#endif
>
> Hm. The out-of-sync error I am seeing in my environment is caused by
> this block when parsing 'g' messages coming from the backend that are
> considered as being GSSAPI-encrypted messages. I am still looking at
> that...

@@ -604,6 +604,11 @@ pqPutMsgEnd(PGconn *conn)
memcpy(conn->outBuffer + conn->outMsgStart, &msgLen, 4);
}

+#ifdef ENABLE_GSS
+ if (pggss_encrypt(conn) < 0)
+ return EOF;
+#endif

@@ -1528,10 +1541,20 @@ socket_putmessage(char msgtype, const char *s,
size_t len)
if (internal_putbytes(s, len))
goto fail;
PqCommBusy = false;
+#ifdef ENABLE_GSS
+ /* if we're GSSAPI encrypting, s was allocated in be_gss_encrypt */
+ if (msgtype == 'g')
+ pfree((char *)s);
+#endif

Looking at this patch in more details... Why is it necessary to wrap
all the encrypted messages into a dedicated message type 'g'? Cannot
we rely on the context on client and backend that should be set up
after authentication to perform the encryption and decryption
operations? This patch is enforcing the message type in
socket_putmessage for backend and pggss_encrypt/pqPutMsgEnd for
frontend, this just feels wrong and I think that the patch could be
really simplified, this includes the crafting added in fe-protocol3.c
that should be IMO reserved only for messages received from the
backend and should not be made aware of any protocol-level logic.
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> Robbie,
>
> +#ifdef ENABLE_GSS
> + if (pggss_encrypt(conn) < 0)
> + return EOF;
> +#endif
>
> @@ -1528,10 +1541,20 @@ socket_putmessage(char msgtype, const char *s,
> size_t len)
> if (internal_putbytes(s, len))
> goto fail;
> PqCommBusy = false;
> +#ifdef ENABLE_GSS
> + /* if we're GSSAPI encrypting, s was allocated in be_gss_encrypt */
> + if (msgtype == 'g')
> + pfree((char *)s);
> +#endif
>
> Looking at this patch in more details... Why is it necessary to wrap
> all the encrypted messages into a dedicated message type 'g'? Cannot
> we rely on the context on client and backend that should be set up
> after authentication to perform the encryption and decryption
> operations? This patch is enforcing the message type in
> socket_putmessage for backend and pggss_encrypt/pqPutMsgEnd for
> frontend, this just feels wrong and I think that the patch could be
> really simplified, this includes the crafting added in fe-protocol3.c
> that should be IMO reserved only for messages received from the
> backend and should not be made aware of any protocol-level logic.

Well, it's not strictly necessary in the general case, but it makes
debugging a *lot* easier to have it present, and it simplifies some of
the handling logic. For instance, in the part quoted above, with
socket_putmessage() and socket_putmessage_noblock(), we need some way to
tell whether a message blob has already been encrypted.

Some enforcement of message type *will need to be carried out* anyway to
avoid security issues with tampering on the wire, whether that be by
sanity-checking the stated message type and then handling accordingly,
or trying to decrypt and detonating the connection if it fails.

GSSAPI does not define a wire protocol for the transport of messages the
way, e.g., TLS does, so there must be some handling of incomplete
messages, multiple messages at once, etc. There is already adequate
handling of these present in postgres already, so I have structured the
code to take advantage of it. That said, I could absolutely reimplement
them - but it would not be simpler, I'm reasonably sure.

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> On Tue, Oct 20, 2015 at 3:01 AM, Robbie Harwood wrote:
>> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>>> psql: lost synchronization with server: got message type "S", length 22
>>
>> which unfortunately could be a great many things. I've said this a
>> couple times now, but I really do need more information - a traffic
>> dump, a list of commands that were run, etc.; unfortunately, the surface
>> here is pretty large, and while I totally am willing to believe there
>> are bugs in the code I've written, I do not yet see them.
>
> --- a/src/interfaces/libpq/fe-protocol3.c
> +++ b/src/interfaces/libpq/fe-protocol3.c
> @@ -129,6 +129,58 @@ pqParseInput3(PGconn *conn)
> return;
> }
>
> +#ifdef ENABLE_GSS
> + /* We want to be ready in both IDLE and BUSY states
> for encryption */
> + if (id == 'g' && !conn->gss_disable_enc && conn->gctx)
> + {
> + ssize_t encEnd, next;
> [...]
> + }
> + else if (!conn->gss_disable_enc && conn->gss_auth_done &&
> + !conn->gss_decrypted_cur && id != 'E')
> + /* This could be a sync error, so let's handle
> it as such. */
> + handleSyncLoss(conn, id, msgLength);
> +#endif
>
> Hm. The out-of-sync error I am seeing in my environment is caused by
> this block when parsing 'g' messages coming from the backend that are
> considered as being GSSAPI-encrypted messages. I am still looking at
> that...

If you're hitting the else-block, that suggests a GSSAPI context is not
present at the time a GSSAPI message was received, I think.

On Thu, Oct 22, 2015 at 1:28 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>
>> Robbie,
>>
>> +#ifdef ENABLE_GSS
>> + if (pggss_encrypt(conn) < 0)
>> + return EOF;
>> +#endif
>>
>> @@ -1528,10 +1541,20 @@ socket_putmessage(char msgtype, const char *s,
>> size_t len)
>> if (internal_putbytes(s, len))
>> goto fail;
>> PqCommBusy = false;
>> +#ifdef ENABLE_GSS
>> + /* if we're GSSAPI encrypting, s was allocated in be_gss_encrypt */
>> + if (msgtype == 'g')
>> + pfree((char *)s);
>> +#endif
>>
>> Looking at this patch in more details... Why is it necessary to wrap
>> all the encrypted messages into a dedicated message type 'g'? Cannot
>> we rely on the context on client and backend that should be set up
>> after authentication to perform the encryption and decryption
>> operations? This patch is enforcing the message type in
>> socket_putmessage for backend and pggss_encrypt/pqPutMsgEnd for
>> frontend, this just feels wrong and I think that the patch could be
>> really simplified, this includes the crafting added in fe-protocol3.c
>> that should be IMO reserved only for messages received from the
>> backend and should not be made aware of any protocol-level logic.
>
> Well, it's not strictly necessary in the general case, but it makes
> debugging a *lot* easier to have it present, and it simplifies some of
> the handling logic. For instance, in the part quoted above, with
> socket_putmessage() and socket_putmessage_noblock(), we need some way to
> tell whether a message blob has already been encrypted.
> Some enforcement of message type *will need to be carried out* anyway to
> avoid security issues with tampering on the wire, whether that be by
> sanity-checking the stated message type and then handling accordingly,
> or trying to decrypt and detonating the connection if it fails.
> GSSAPI does not define a wire protocol for the transport of messages the
> way, e.g., TLS does, so there must be some handling of incomplete
> messages, multiple messages at once, etc. There is already adequate
> handling of these present in postgres already, so I have structured the
> code to take advantage of it. That said, I could absolutely reimplement
> them - but it would not be simpler, I'm reasonably sure.

Hm, and that's why you chose this way of going. My main concern about
this patch is that it adds on top of the existing Postgres protocol a
layer to encrypt and decrypt the messages between server and client
based on GSSAPI. All messages transmitted between client and server
are changed to 'g' messages on the fly and switched back to their
original state at reception. This is symbolized by the four routines
you added in the patch in this purpose, two for frontend and two for
backend, each one for encryption and decryption. I may be wrong of
course, but it seems to me that this approach will not survive
committer-level screening because of the fact that context-level
things invade higher level protocol messages.

IMO, we would live better with something at a lower level, like in
be-secure.c and fe-secure.c to exchange the encrypted and decrypted
packages using the GSSAPI context created at authentication, that's
where the context-level switches are living after all.

So I am thinking that this patch needs a rework, and should be
returned with feedback.
Stephen, others, what do you think?
--
Michael

On 2015-10-22 16:47:09 +0900, Michael Paquier wrote:
> Hm, and that's why you chose this way of going. My main concern about
> this patch is that it adds on top of the existing Postgres protocol a
> layer to encrypt and decrypt the messages between server and client
> based on GSSAPI. All messages transmitted between client and server
> are changed to 'g' messages on the fly and switched back to their
> original state at reception. This is symbolized by the four routines
> you added in the patch in this purpose, two for frontend and two for
> backend, each one for encryption and decryption. I may be wrong of
> course, but it seems to me that this approach will not survive
> committer-level screening because of the fact that context-level
> things invade higher level protocol messages.

Agreed. At least one committer here indeed thinks this approach is not
acceptable (and I've said so upthread).

Greetings,

Andres Freund

On Thu, Oct 22, 2015 at 6:00 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2015-10-22 16:47:09 +0900, Michael Paquier wrote:
>> Hm, and that's why you chose this way of going. My main concern about
>> this patch is that it adds on top of the existing Postgres protocol a
>> layer to encrypt and decrypt the messages between server and client
>> based on GSSAPI. All messages transmitted between client and server
>> are changed to 'g' messages on the fly and switched back to their
>> original state at reception. This is symbolized by the four routines
>> you added in the patch in this purpose, two for frontend and two for
>> backend, each one for encryption and decryption. I may be wrong of
>> course, but it seems to me that this approach will not survive
>> committer-level screening because of the fact that context-level
>> things invade higher level protocol messages.
>
> Agreed. At least one committer here indeed thinks this approach is not
> acceptable (and I've said so upthread).

OK, so marked as returned with feedback.
--
Michael

Andres Freund <andres(at)anarazel(dot)de> writes:

> On 2015-10-22 16:47:09 +0900, Michael Paquier wrote:
>> Hm, and that's why you chose this way of going. My main concern about
>> this patch is that it adds on top of the existing Postgres protocol a
>> layer to encrypt and decrypt the messages between server and client
>> based on GSSAPI. All messages transmitted between client and server
>> are changed to 'g' messages on the fly and switched back to their
>> original state at reception. This is symbolized by the four routines
>> you added in the patch in this purpose, two for frontend and two for
>> backend, each one for encryption and decryption. I may be wrong of
>> course, but it seems to me that this approach will not survive
>> committer-level screening because of the fact that context-level
>> things invade higher level protocol messages.
>
> Agreed. At least one committer here indeed thinks this approach is not
> acceptable (and I've said so upthread).

Okay, I'll make some changes. Before I do, though, since this is not
the approach I came up with, can you explicitly state what you're
looking for here? It subjectively seems that I'm getting a lot of
feedback of "this feels wrong" without suggestion for improvement.

To be clear, what I need to know is:

- What changes do you want to see in the wire protocol? (And how will
fallback be supported if that's affected?)

- Since this seems to be an important sticking point, what files am I
encouraged to change (or prohibited from changing)? (Fallback makes
this complex.)

- I've been assuming that we care about fallback, but I'd like to be
told that it's something postgres actually wants to see because it's
the most intricate part of these changes. (I'm reasonably confident
that the code becomes simpler without it, and I myself have no use for
it.)

If I understand what you're asking for (and the above is intended to be
sure that I will), this will not be a trivial rework, so I want to be
really sure before doing that because writing this code a third time is
something I don't relish.

Thanks,
--Robbie

On Thu, Oct 22, 2015 at 11:36 PM, Robbie Harwood wrote:
> To be clear, what I need to know is:
> - What changes do you want to see in the wire protocol? (And how will
> fallback be supported if that's affected?)

Hm. Something essential will be to send the length of the wrapped
gss_buffer_t object to be sent in the first 4 bytes of the message so
as the receiver can know how much it has to unwrap and can perform
sanity checks on what has been received.

> - Since this seems to be an important sticking point, what files am I
> encouraged to change (or prohibited from changing)? (Fallback makes
> this complex.)

If we want to make that stick into Postgres, I think that we are going
to need be_gss_read and be_gss_write in be-secure.c, and pqgss_write
and pqgss_read in fe-secure.c, the use the context initialized at
authentication time to wrap and unwrap messages between the server and
client.

> - I've been assuming that we care about fallback, but I'd like to be
> told that it's something postgres actually wants to see because it's
> the most intricate part of these changes. (I'm reasonably confident
> that the code becomes simpler without it, and I myself have no use for
> it.)

As a first shot for this patch, I would not mind if there is no
fallback at protocol level, it seems to me that it is challenging
enough to get a solid core feature first. Perhaps others have
different opinions?

> If I understand what you're asking for (and the above is intended to be
> sure that I will), this will not be a trivial rework, so I want to be
> really sure before doing that because writing this code a third time is
> something I don't relish.

This makes sense. Let's be sure that we come up with a clear picture
of what to do first.
--
Michael

On Tue, Sep 29, 2015 at 7:53 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> Robbie Harwood <rharwood(at)redhat(dot)com> writes:
>
>>>>> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>>>>>
>>>>>> Well, the issue is still here: login through gssapi fails with
>>>>>> your patch, not with HEAD. This patch is next on my review list by
>>>>>> the way so I'll see what I can do about it soon even if I am in
>>>>>> the US for Postgres Open next week. Still, how did you test it? I
>>>>>> am just creating by myself a KDC, setting up a valid credential
>>>>>> with kinit, and after setting up Postgres for this purpose the
>>>>>> protocol communication just fails.
>>
>> I have no issues, no sync loss; nothing is amiss as far as I can see.
>> If there is actually a problem here, I need more information from you.
>> At the very least, as previously mentioned, I need to know what
>> messages went over the wire to/from the server before it occurred, and
>> what command (if it it made it to command processing) it was in the
>> midst of sending.
>
> Any follow-up on this? I'd really like my code to be bug-free.

I don't know if this is worth posting as the patch is currently
returned with feedback and you are redoing it in a different way, but
with your patch I get this error when connecting:

lost synchronization with server: got message type "T", length 27
The connection to the server was lost. Attempting reset: Failed.

I only get the error when connection to a patched server from a
patched libpq. If either is unpatched, then there is no problem.

Let me know if this is worth looking into.

Cheers,

Jeff

Jeff Janes <jeff(dot)janes(at)gmail(dot)com> writes:

> On Tue, Sep 29, 2015 at 7:53 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>> Robbie Harwood <rharwood(at)redhat(dot)com> writes:
>>
>>>>>> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>>>>>>
>>>>>>> Well, the issue is still here: login through gssapi fails with
>>>>>>> your patch, not with HEAD. This patch is next on my review list by
>>>>>>> the way so I'll see what I can do about it soon even if I am in
>>>>>>> the US for Postgres Open next week. Still, how did you test it? I
>>>>>>> am just creating by myself a KDC, setting up a valid credential
>>>>>>> with kinit, and after setting up Postgres for this purpose the
>>>>>>> protocol communication just fails.
>>>
>>> I have no issues, no sync loss; nothing is amiss as far as I can see.
>>> If there is actually a problem here, I need more information from you.
>>> At the very least, as previously mentioned, I need to know what
>>> messages went over the wire to/from the server before it occurred, and
>>> what command (if it it made it to command processing) it was in the
>>> midst of sending.
>>
>> Any follow-up on this? I'd really like my code to be bug-free.
>
> I don't know if this is worth posting as the patch is currently
> returned with feedback and you are redoing it in a different way, but
> with your patch I get this error when connecting:
>
> lost synchronization with server: got message type "T", length 27
> The connection to the server was lost. Attempting reset: Failed.
>
> I only get the error when connection to a patched server from a
> patched libpq. If either is unpatched, then there is no problem.
>
> Let me know if this is worth looking into.

Definitely good to know, and I appreciate your testing. It's probably
not worth looking into right now, but please do test the next version of
the code as well.

Thanks!
--Robbie

Andreas, can you please weigh in here since your voice is important to
this process?

Robbie Harwood <rharwood(at)redhat(dot)com> writes:

> Andres Freund <andres(at)anarazel(dot)de> writes:
>
>> On 2015-10-22 16:47:09 +0900, Michael Paquier wrote:
>>> Hm, and that's why you chose this way of going. My main concern about
>>> this patch is that it adds on top of the existing Postgres protocol a
>>> layer to encrypt and decrypt the messages between server and client
>>> based on GSSAPI. All messages transmitted between client and server
>>> are changed to 'g' messages on the fly and switched back to their
>>> original state at reception. This is symbolized by the four routines
>>> you added in the patch in this purpose, two for frontend and two for
>>> backend, each one for encryption and decryption. I may be wrong of
>>> course, but it seems to me that this approach will not survive
>>> committer-level screening because of the fact that context-level
>>> things invade higher level protocol messages.
>>
>> Agreed. At least one committer here indeed thinks this approach is not
>> acceptable (and I've said so upthread).
>
> Okay, I'll make some changes. Before I do, though, since this is not
> the approach I came up with, can you explicitly state what you're
> looking for here? It subjectively seems that I'm getting a lot of
> feedback of "this feels wrong" without suggestion for improvement.
>
> To be clear, what I need to know is:
>
> - What changes do you want to see in the wire protocol? (And how will
> fallback be supported if that's affected?)
>
> - Since this seems to be an important sticking point, what files am I
> encouraged to change (or prohibited from changing)? (Fallback makes
> this complex.)
>
> - I've been assuming that we care about fallback, but I'd like to be
> told that it's something postgres actually wants to see because it's
> the most intricate part of these changes. (I'm reasonably confident
> that the code becomes simpler without it, and I myself have no use for
> it.)
>
> If I understand what you're asking for (and the above is intended to be
> sure that I will), this will not be a trivial rework, so I want to be
> really sure before doing that because writing this code a third time is
> something I don't relish.
>
> Thanks,
> --Robbie

Hello friends,

For your consideration, here is a new version of GSSAPI encryption
support. For those who prefer, it's also available on my github:
https://github.com/frozencemetery/postgres/commit/c92275b6605d7929cda5551de47a4c60aab7179e

Some thoughts:

- The overall design is different this time - GSS encryption sits in
parallel construction to SSL encryption rather than at the protocol
level - so a strict diff probably isn't useful.

- The GSSAPI authentication code has been moved without modification.
In doing so, the temptation to modify it (flags, error checking, that
big comment at the top about things from Athena, etc.) is very large.
I do not know whether these changes are best suited to another patch
in this series or should be reviewed separately. I am also hesitant
to add things beyond the core before I am told this is the right
approach.

- There's no fallback here. I wrote fallback support for versions 1-3,
and the same design could apply here without too much trouble, but I
am hesitant to port it over before the encryption design is approved.
I strongly suspect you will not want to merge this without fallback
support, and that makes sense to me.

- The client and server code look a lot like each other. This
resemblance is not exact, and my understanding is that server and
client need to compile independently, so I do not know of a way to
rectify this. Suggestions are welcome.

Thanks!

On Thu, Feb 11, 2016 at 6:06 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> For your consideration, here is a new version of GSSAPI encryption
> support. For those who prefer, it's also available on my github:
> https://github.com/frozencemetery/postgres/commit/c92275b6605d7929cda5551de47a4c60aab7179e

Yeah! Glad to see you back.

> Some thoughts:
>
> - The overall design is different this time - GSS encryption sits in
> parallel construction to SSL encryption rather than at the protocol
> level - so a strict diff probably isn't useful.
>
> - The GSSAPI authentication code has been moved without modification.
> In doing so, the temptation to modify it (flags, error checking, that
> big comment at the top about things from Athena, etc.) is very large.
> I do not know whether these changes are best suited to another patch
> in this series or should be reviewed separately. I am also hesitant
> to add things beyond the core before I am told this is the right
> approach.

I would recommend a different patch if code needs to be moved around.
The move may make sense taken as an independent piece of the
integration.

> - There's no fallback here. I wrote fallback support for versions 1-3,
> and the same design could apply here without too much trouble, but I
> am hesitant to port it over before the encryption design is approved.
> I strongly suspect you will not want to merge this without fallback
> support, and that makes sense to me.
>
> - The client and server code look a lot like each other. This
> resemblance is not exact, and my understanding is that server and
> client need to compile independently, so I do not know of a way to
> rectify this. Suggestions are welcome.

At quick glance, I like the direction this is taking. You moved all
the communication protocol at a lower level where SSL and secure reads
are located, so this results in a neat integration.

+ * Portions Copyright (c) 2015-2016, Red Hat, Inc.
+ * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
I think that this part may be a problem... Not sure the feeling of the
others regarding additional copyright notices.

It would be good to add that to the next CF, I will be happy to get a
look at it.
--
Michael

On Thu, Feb 11, 2016 at 8:42 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> + * Portions Copyright (c) 2015-2016, Red Hat, Inc.
> + * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
> I think that this part may be a problem... Not sure the feeling of the
> others regarding additional copyright notices.

Yep. "PostgreSQL Global Development Group" means "whoever it was that
contributed", not a specific legal organization.

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

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> On Thu, Feb 11, 2016 at 6:06 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>>
>> - The GSSAPI authentication code has been moved without modification.
>> In doing so, the temptation to modify it (flags, error checking, that
>> big comment at the top about things from Athena, etc.) is very large.
>> I do not know whether these changes are best suited to another patch
>> in this series or should be reviewed separately. I am also hesitant
>> to add things beyond the core before I am told this is the right
>> approach.
>
> I would recommend a different patch if code needs to be moved around.
> The move may make sense taken as an independent piece of the
> integration.

Sorry, are you suggesting separate patch for moving the GSS auth code,
or separate patch for changes to said code? I am happy to move it if
so, just want to be sure.

> + * Portions Copyright (c) 2015-2016, Red Hat, Inc.
> + * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
> I think that this part may be a problem... Not sure the feeling of the
> others regarding additional copyright notices.

Good catch. That's an accident (force of habit). Since I'm pretty sure
this version won't be merged anyway, I'll drop it from the next one.

> It would be good to add that to the next CF, I will be happy to get a
> look at it.

Sounds good. Thanks for looking at it!

On Fri, Feb 12, 2016 at 3:56 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> On Thu, Feb 11, 2016 at 6:06 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>>> - The GSSAPI authentication code has been moved without modification.
>>> In doing so, the temptation to modify it (flags, error checking, that
>>> big comment at the top about things from Athena, etc.) is very large.
>>> I do not know whether these changes are best suited to another patch
>>> in this series or should be reviewed separately. I am also hesitant
>>> to add things beyond the core before I am told this is the right
>>> approach.
>>
>> I would recommend a different patch if code needs to be moved around.
>> The move may make sense taken as an independent piece of the
>> integration.
>
> Sorry, are you suggesting separate patch for moving the GSS auth code,
> or separate patch for changes to said code? I am happy to move it if
> so, just want to be sure.

This is based on my first impressions on the patch. Let's discuss more
those points once I got a more in-depth look at the patch with what it
actually does. In short, there is no need to put more efforts in the
coding now :) Sorry to confuse you.

>> + * Portions Copyright (c) 2015-2016, Red Hat, Inc.
>> + * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
>> I think that this part may be a problem... Not sure the feeling of the
>> others regarding additional copyright notices.
>
> Good catch. That's an accident (force of habit). Since I'm pretty sure
> this version won't be merged anyway, I'll drop it from the next one.
>
>> It would be good to add that to the next CF, I will be happy to get a
>> look at it.
>
> Sounds good. Thanks for looking at it!

Okay, let's do this.
--
Michael

Hi Robbie,

On 2/10/16 4:06 PM, Robbie Harwood wrote:
> Hello friends,
>
> For your consideration, here is a new version of GSSAPI encryption
> support. For those who prefer, it's also available on my github:
> https://github.com/frozencemetery/postgres/commit/c92275b6605d7929cda5551de47a4c60aab7179e

It tried out this patch and ran into a few problems:

1) It didn't apply cleanly to HEAD. It did apply cleanly on a455878
which I figured was recent enough for testing. I didn't bisect to find
the exact commit that broke it.

2) While I was able to apply the patch and get it compiled it seemed
pretty flaky - I was only able to logon about 1 in 10 times on average.
Here was my testing methodology:

a) Build Postgres from a455878 (without your patch), install/configure
Kerberos and get everything working. I was able the set the auth method
to gss in pg_hba.conf and logon successfully every time.

b) On the same system rebuild Postgres from a455878 including your patch
and attempt authentication.

The problems arose after step 2b. Sometimes I would try to logon twenty
times without success and sometimes it only take five or six attempts.
I was never able to logon successfully twice in a row.

When not successful the client always output this incomplete message
(without terminating LF):

psql: expected authentication request from server, but received

From the logs I can see the server is reporting EOF from the client,
though the client does not core dump and prints the above message before
exiting.

I have attached files that contain server logs at DEBUG5 and tcpdump
output for both the success and failure cases.

Please let me know if there's any more information you would like me to
provide.

--
-David
david(at)pgmasters(dot)net

David Steele <david(at)pgmasters(dot)net> writes:

> Hi Robbie,
>
> On 2/10/16 4:06 PM, Robbie Harwood wrote:
>> Hello friends,
>>
>> For your consideration, here is a new version of GSSAPI encryption
>> support. For those who prefer, it's also available on my github:
>> https://github.com/frozencemetery/postgres/commit/c92275b6605d7929cda5551de47a4c60aab7179e
>
> It tried out this patch and ran into a few problems:
>
> 1) It didn't apply cleanly to HEAD. It did apply cleanly on a455878
> which I figured was recent enough for testing. I didn't bisect to find
> the exact commit that broke it.

It applied to head of master (57c932475504d63d8f8a68fc6925d7decabc378a)
for me (`patch -p1 < v4-GSSAPI-encryption-support.patch`). I rebased it
anyway and cut a v5 anyway, just to be sure. It's attached, and
available on github as well:
https://github.com/frozencemetery/postgres/commit/dc10e3519f0f6c67f79abd157dc8ff1a1c293f53

> 2) While I was able to apply the patch and get it compiled it seemed
> pretty flaky - I was only able to logon about 1 in 10 times on average.
> Here was my testing methodology:
>
> a) Build Postgres from a455878 (without your patch), install/configure
> Kerberos and get everything working. I was able the set the auth method
> to gss in pg_hba.conf and logon successfully every time.
>
> b) On the same system rebuild Postgres from a455878 including your patch
> and attempt authentication.
>
> The problems arose after step 2b. Sometimes I would try to logon twenty
> times without success and sometimes it only take five or six attempts.
> I was never able to logon successfully twice in a row.
>
> When not successful the client always output this incomplete message
> (without terminating LF):
>
> psql: expected authentication request from server, but received
>
> From the logs I can see the server is reporting EOF from the client,
> though the client does not core dump and prints the above message before
> exiting.
>
> I have attached files that contain server logs at DEBUG5 and tcpdump
> output for both the success and failure cases.
>
> Please let me know if there's any more information you would like me to
> provide.

What I can't tell from looking at your methodology is whether both the
client and server were running my patches or no. There's no fallback
here (I'd like to talk about how that should work, with example from
v1-v3, if people have ideas). This means that both the client and the
server need to be running my patches for the moment. Is this your
setup?

Thanks for taking it for a spin!
--Robbie

On 2/15/16 12:45 PM, Robbie Harwood wrote:
> David Steele <david(at)pgmasters(dot)net> writes:
>
>> 1) It didn't apply cleanly to HEAD. It did apply cleanly on a455878
>> which I figured was recent enough for testing. I didn't bisect to find
>> the exact commit that broke it.
>
> It applied to head of master (57c932475504d63d8f8a68fc6925d7decabc378a)
> for me (`patch -p1 < v4-GSSAPI-encryption-support.patch`). I rebased it
> anyway and cut a v5 anyway, just to be sure. It's attached, and
> available on github as well:
> https://github.com/frozencemetery/postgres/commit/dc10e3519f0f6c67f79abd157dc8ff1a1c293f53

It could have been my mistake. I'll give it another try when you have a
new patch.

>> 2) While I was able to apply the patch and get it compiled it seemed
>> pretty flaky - I was only able to logon about 1 in 10 times on average.
>> Here was my testing methodology:
>
> What I can't tell from looking at your methodology is whether both the
> client and server were running my patches or no. There's no fallback
> here (I'd like to talk about how that should work, with example from
> v1-v3, if people have ideas). This means that both the client and the
> server need to be running my patches for the moment. Is this your
> setup?

I was testing on a system with no version of PostgreSQL installed. I
applied your patch to master and then ran both server and client from
that patched version. Is there something I'm missing?

--
-David
david(at)pgmasters(dot)net

David Steele <david(at)pgmasters(dot)net> writes:

> On 2/15/16 12:45 PM, Robbie Harwood wrote:
>> David Steele <david(at)pgmasters(dot)net> writes:
>>
>>> 1) It didn't apply cleanly to HEAD. It did apply cleanly on a455878
>>> which I figured was recent enough for testing. I didn't bisect to find
>>> the exact commit that broke it.
>>
>> It applied to head of master (57c932475504d63d8f8a68fc6925d7decabc378a)
>> for me (`patch -p1 < v4-GSSAPI-encryption-support.patch`). I rebased it
>> anyway and cut a v5 anyway, just to be sure. It's attached, and
>> available on github as well:
>> https://github.com/frozencemetery/postgres/commit/dc10e3519f0f6c67f79abd157dc8ff1a1c293f53
>
> It could have been my mistake. I'll give it another try when you have a
> new patch.

Please do let me know how v5 goes. If you run into trouble, in addition
to the logs you helpfully provided before, I'd like a traffic dump (pcap
preferable; I need tcp/udp port 88 for Kerberos and tcp port 5432 or
whatever you're running postgres on) if possible. Thanks!

>>> 2) While I was able to apply the patch and get it compiled it seemed
>>> pretty flaky - I was only able to logon about 1 in 10 times on average.
>>> Here was my testing methodology:
>>
>> What I can't tell from looking at your methodology is whether both the
>> client and server were running my patches or no. There's no fallback
>> here (I'd like to talk about how that should work, with example from
>> v1-v3, if people have ideas). This means that both the client and the
>> server need to be running my patches for the moment. Is this your
>> setup?
>
> I was testing on a system with no version of PostgreSQL installed. I
> applied your patch to master and then ran both server and client from
> that patched version. Is there something I'm missing?

Not that I can immediately see. As long as the client and server are
both patched, everything should work. My process is the same as with
previous versions of this patchset [0], and though I'm using FreeIPA
there is no reason it shouldn't work with any other KDC (MIT, for
instance[1]) provided the IPA calls are converted.

I am curious, though - I haven't changed any of the authentication code
in v4/v5 from what's in ~master, so how often can you log in using
GSSAPI using master?

[0]: https://mivehind.net/2015/06/11/kerberized-postgresql/
[1]: http://web.mit.edu/kerberos/krb5-devel/doc/admin/install_kdc.html

On Tue, Feb 16, 2016 at 2:45 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> David Steele <david(at)pgmasters(dot)net> writes:
>> On 2/10/16 4:06 PM, Robbie Harwood wrote:
>>> Hello friends,
>>>
>>> For your consideration, here is a new version of GSSAPI encryption
>>> support. For those who prefer, it's also available on my github:
>>> https://github.com/frozencemetery/postgres/commit/c92275b6605d7929cda5551de47a4c60aab7179e
>>
>> It tried out this patch and ran into a few problems:
>>
>> 1) It didn't apply cleanly to HEAD. It did apply cleanly on a455878
>> which I figured was recent enough for testing. I didn't bisect to find
>> the exact commit that broke it.
>
> It applied to head of master (57c932475504d63d8f8a68fc6925d7decabc378a)
> for me (`patch -p1 < v4-GSSAPI-encryption-support.patch`). I rebased it
> anyway and cut a v5 anyway, just to be sure. It's attached, and
> available on github as well:
> https://github.com/frozencemetery/postgres/commit/dc10e3519f0f6c67f79abd157dc8ff1a1c293f53

v5 is applying fine for me. There were two diff hunks in routine.sgml
but nothing to worry about.

>> 2) While I was able to apply the patch and get it compiled it seemed
>> pretty flaky - I was only able to logon about 1 in 10 times on average.
>> Here was my testing methodology:
>>
>> a) Build Postgres from a455878 (without your patch), install/configure
>> Kerberos and get everything working. I was able the set the auth method
>> to gss in pg_hba.conf and logon successfully every time.
>>
>> b) On the same system rebuild Postgres from a455878 including your patch
>> and attempt authentication.
>>
>> The problems arose after step 2b. Sometimes I would try to logon twenty
>> times without success and sometimes it only take five or six attempts.
>> I was never able to logon successfully twice in a row.
>>
>> When not successful the client always output this incomplete message
>> (without terminating LF):
>>
>> psql: expected authentication request from server, but received
>>
>> From the logs I can see the server is reporting EOF from the client,
>> though the client does not core dump and prints the above message before
>> exiting.
>>
>> I have attached files that contain server logs at DEBUG5 and tcpdump
>> output for both the success and failure cases.
>>
>> Please let me know if there's any more information you would like me to
>> provide.
>
> What I can't tell from looking at your methodology is whether both the
> client and server were running my patches or no. There's no fallback
> here (I'd like to talk about how that should work, with example from
> v1-v3, if people have ideas). This means that both the client and the
> server need to be running my patches for the moment. Is this your
> setup?

We need to be careful here, backward-compatibility is critical for
both the client and the server, or to put it in other words, an
uptables client should still be able to connect a patched server, and
vice-versa. This is an area where it is important to not break any
third-part tool, either using libpq or reimplementing the frontend
protocol.

So I finally began to dive into your new patch... And after testing
this is breaking the authentication protocol for GSS. I have been able
to connect to a server once, but at the second attempt and after
connection is failing with the following error:
psql: expected authentication request from server, but received ioltas

Also, something that is missing is the parametrization that has been
discussed the last time this patch was on the mailing list. Where is
the capacity to control if a client is connecting to a server that is
performing encryption and downgrade to non-ecrypted connection should
the server not be able to support it? Enforcing a client to require
encryption support using pg_hba.conf was as well a good thing. Some
more infrastructure is needed here, I thought that there was an
agreement previously regarding that.

Also, and to make the review a bit easier, it would be good to split
the patch into smaller pieces (thanks for waiting on my input here,
this became quite clear after looking at this code). Basically,
pg_GSS_error is moved to a new file, bringing with it pg_GSS_recvauth
because the error routine is being used by the new ones you are
introducing: be_gssapi_write, etc. The split that this patch is doing
is a bit confusing, all the GSS-related stuff is put within one single
file:
- read/write routines
- authentication routine
- constants
- routines for error handling
Mixing read/write routines with the authentication routine looks
wrong, because the read-write routines just need to create a
dependency with for example be-secure.c on the backend. In short,
where before authentication and secure read/writes after
authentication get linked to each other, and create a dependency that
did not exist before.

For the sake of clarity I would suggest the following split:
- be-gss-common.c, where all the constants and the error handling
routine are located.
- Let authrecv in auth.c
- Move only the read/write routines to the new file be-[secure-]gssapi.c
Splitting the patches into one that moves around the routines, and a
second that introduces the new logic would bring more clarity.

As In understood from the patch, enforcing encryption is not good
either, and any patched clients would not be able to 9.5 or older
servers. Regarding the parametrization, I liked the previous v1-v3
layout to control the encryption, and this allows to set up the
session context on backend and frontend at authentication. The
connection errors are also something to look at, I guess that David
and I ran into the same thing. I am wondering how this patch is tested
to be honest.
--
Michael

On Wed, Feb 24, 2016 at 7:12 PM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> David Steele <david(at)pgmasters(dot)net> writes:
>
>> On 2/15/16 12:45 PM, Robbie Harwood wrote:
>>> David Steele <david(at)pgmasters(dot)net> writes:
>>>
>>>> 1) It didn't apply cleanly to HEAD. It did apply cleanly on a455878
>>>> which I figured was recent enough for testing. I didn't bisect to find
>>>> the exact commit that broke it.
>>>
>>> It applied to head of master (57c932475504d63d8f8a68fc6925d7decabc378a)
>>> for me (`patch -p1 < v4-GSSAPI-encryption-support.patch`). I rebased it
>>> anyway and cut a v5 anyway, just to be sure. It's attached, and
>>> available on github as well:
>>> https://github.com/frozencemetery/postgres/commit/dc10e3519f0f6c67f79abd157dc8ff1a1c293f53
>>
>> It could have been my mistake. I'll give it another try when you have a
>> new patch.
>
> Please do let me know how v5 goes. If you run into trouble, in addition
> to the logs you helpfully provided before, I'd like a traffic dump (pcap
> preferable; I need tcp/udp port 88 for Kerberos and tcp port 5432 or
> whatever you're running postgres on) if possible. Thanks!
>
>>>> 2) While I was able to apply the patch and get it compiled it seemed
>>>> pretty flaky - I was only able to logon about 1 in 10 times on average.
>>>> Here was my testing methodology:
>>>
>>> What I can't tell from looking at your methodology is whether both the
>>> client and server were running my patches or no. There's no fallback
>>> here (I'd like to talk about how that should work, with example from
>>> v1-v3, if people have ideas). This means that both the client and the
>>> server need to be running my patches for the moment. Is this your
>>> setup?
>>
>> I was testing on a system with no version of PostgreSQL installed. I
>> applied your patch to master and then ran both server and client from
>> that patched version. Is there something I'm missing?
>
> Not that I can immediately see. As long as the client and server are
> both patched, everything should work. My process is the same as with
> previous versions of this patchset [0], and though I'm using FreeIPA
> there is no reason it shouldn't work with any other KDC (MIT, for
> instance[1]) provided the IPA calls are converted.

I used a custom krb5kdc set up manually, and all my connection
attempts are working on HEAD, not with your patch (both client and
server patched).

> I am curious, though - I haven't changed any of the authentication code
> in v4/v5 from what's in ~master, so how often can you log in using
> GSSAPI using master?

My guess is that there is something not been correctly cleaned up when
closing the connection. The first attempt worked for me, not after.
--
Michael

On 2/25/16 2:08 AM, Michael Paquier wrote:
> On Wed, Feb 24, 2016 at 7:12 PM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>>
>> Not that I can immediately see. As long as the client and server are
>> both patched, everything should work. My process is the same as with
>> previous versions of this patchset [0], and though I'm using FreeIPA
>> there is no reason it shouldn't work with any other KDC (MIT, for
>> instance[1]) provided the IPA calls are converted.
>
> I used a custom krb5kdc set up manually, and all my connection
> attempts are working on HEAD, not with your patch (both client and
> server patched).

I've got the same setup with the same results.

>> I am curious, though - I haven't changed any of the authentication code
>> in v4/v5 from what's in ~master, so how often can you log in using
>> GSSAPI using master?
>
> My guess is that there is something not been correctly cleaned up when
> closing the connection. The first attempt worked for me, not after.

I was able to get in again after a number of failed attempts, though the
number varied.

--
-David
david(at)pgmasters(dot)net

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> On Tue, Feb 16, 2016 at 2:45 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>> David Steele <david(at)pgmasters(dot)net> writes:
>>> On 2/10/16 4:06 PM, Robbie Harwood wrote:
>>>> Hello friends,
>>>>
>>>> For your consideration, here is a new version of GSSAPI encryption
>>>> support. For those who prefer, it's also available on my github:
>>>> https://github.com/frozencemetery/postgres/commit/c92275b6605d7929cda5551de47a4c60aab7179e
>>>
>>> It tried out this patch and ran into a few problems:
>>>
>>> 2) While I was able to apply the patch and get it compiled it seemed
>>> pretty flaky - I was only able to logon about 1 in 10 times on average.
>>>
>>> When not successful the client always output this incomplete message
>>> (without terminating LF):
>>>
>>> psql: expected authentication request from server, but received
>>>
>>> From the logs I can see the server is reporting EOF from the client,
>>> though the client does not core dump and prints the above message before
>>> exiting.
>>>
>>> I have attached files that contain server logs at DEBUG5 and tcpdump
>>> output for both the success and failure cases.
>>>
>>> Please let me know if there's any more information you would like me to
>>> provide.
>>
>> What I can't tell from looking at your methodology is whether both the
>> client and server were running my patches or no. There's no fallback
>> here (I'd like to talk about how that should work, with example from
>> v1-v3, if people have ideas). This means that both the client and the
>> server need to be running my patches for the moment. Is this your
>> setup?
>
> We need to be careful here, backward-compatibility is critical for
> both the client and the server, or to put it in other words, an
> uptables client should still be able to connect a patched server, and
> vice-versa. This is an area where it is important to not break any
> third-part tool, either using libpq or reimplementing the frontend
> protocol.

Which is why in my introduction to the v4 patch I explicitly mentioned
that this was missing. I wanted to talk about how this should be
implemented, since I feel that I received no feedback on that design
From last time. It's not hard to port that design over, if desired.

> So I finally began to dive into your new patch... And after testing
> this is breaking the authentication protocol for GSS. I have been able
> to connect to a server once, but at the second attempt and after
> connection is failing with the following error:
> psql: expected authentication request from server, but received ioltas

Interesting. I will see if I can find anything. The capture posted
earlier (thanks David!) suggests that the client doesn't expect the
encrypted data.

I suspect what has happened is a race between the client buffering data
From the socket and the client processing the completion of GSSAPI
authentication. This kind of issue is why my v1 handled GSSAPI at the
protocol level, not at the transport level. I think we end up with
encrypted data in the buffer that's supposed to be decrypted, and since
the GSSAPI blob starts with \x00, it doesn't know what to do.

I'll cut a v6 with most of the changes we've talked about here. It
should address this issue, but I suspect that no one will be happy about
how, since the client essentially needs to "un-read" some data.

As a side note, this would also explain why I can't reproduce the issue,
since I'm running in very low-latency environments (three VMs on my
laptop).

> Also, something that is missing is the parametrization that has been
> discussed the last time this patch was on the mailing list. Where is
> the capacity to control if a client is connecting to a server that is
> performing encryption and downgrade to non-ecrypted connection should
> the server not be able to support it? Enforcing a client to require
> encryption support using pg_hba.conf was as well a good thing. Some
> more infrastructure is needed here, I thought that there was an
> agreement previously regarding that.

This does not match my impression of the discussion, but I would be
happy to be wrong about that since it is less work for me.

> Also, and to make the review a bit easier, it would be good to split
> the patch into smaller pieces (thanks for waiting on my input here,
> this became quite clear after looking at this code). Basically,
> pg_GSS_error is moved to a new file, bringing with it pg_GSS_recvauth
> because the error routine is being used by the new ones you are
> introducing: be_gssapi_write, etc. The split that this patch is doing
> is a bit confusing, all the GSS-related stuff is put within one single
> file:
> - read/write routines
> - authentication routine
> - constants
> - routines for error handling
> Mixing read/write routines with the authentication routine looks
> wrong, because the read-write routines just need to create a
> dependency with for example be-secure.c on the backend. In short,
> where before authentication and secure read/writes after
> authentication get linked to each other, and create a dependency that
> did not exist before.
>
> For the sake of clarity I would suggest the following split:
> - be-gss-common.c, where all the constants and the error handling
> routine are located.
> - Let authrecv in auth.c
> - Move only the read/write routines to the new file be-[secure-]gssapi.c
> Splitting the patches into one that moves around the routines, and a
> second that introduces the new logic would bring more clarity.

So... auth.c is now going to depend on be-gss-common.c, and
be-secure-gssapi.c is also going to depend on be-gss-common.c. That
doesn't seem better in terms of dependencies: now I'm creating two that
didn't exist before.

That said, I would believe that it makes everything clearer. I will do
that shortly.

> I am wondering how this patch is tested to be honest.

We've already had this conversation; please let's stay constructive.

On Fri, Feb 26, 2016 at 5:02 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> We need to be careful here, backward-compatibility is critical for
>> both the client and the server, or to put it in other words, an
>> uptables client should still be able to connect a patched server, and
>> vice-versa. This is an area where it is important to not break any
>> third-part tool, either using libpq or reimplementing the frontend
>> protocol.
>
> Which is why in my introduction to the v4 patch I explicitly mentioned
> that this was missing. I wanted to talk about how this should be
> implemented, since I feel that I received no feedback on that design
> From last time. It's not hard to port that design over, if desired.

I gave my opinion about the parametrization here a couple of months
back, and thought that it was rather a neat design. I still think so:
http://www.postgresql.org/message-id/CAB7nPqRGQ60PWLb-CLLrvMoQwPAbnu-961W+xGPvG62RMSkcZQ@mail.gmail.com
In short:
1) Introduction of new pg_hba parameter require_encrypt, defaulting to
off, enforcing the clients to have encryption. This way an
administrator of a new server can prevent connections of old clients
if he/she wants to have all the connections excrypted.
2) Client-side parameter, that you named previously gss_encrypt, to
let a client decide if he wishes to do encryption or not.

>> So I finally began to dive into your new patch... And after testing
>> this is breaking the authentication protocol for GSS. I have been able
>> to connect to a server once, but at the second attempt and after
>> connection is failing with the following error:
>> psql: expected authentication request from server, but received ioltas
>
> Interesting. I will see if I can find anything. The capture posted
> earlier (thanks David!) suggests that the client doesn't expect the
> encrypted data.
>
> I suspect what has happened is a race between the client buffering data
> From the socket and the client processing the completion of GSSAPI
> authentication. This kind of issue is why my v1 handled GSSAPI at the
> protocol level, not at the transport level. I think we end up with
> encrypted data in the buffer that's supposed to be decrypted, and since
> the GSSAPI blob starts with \x00, it doesn't know what to do.
>
> I'll cut a v6 with most of the changes we've talked about here. It
> should address this issue, but I suspect that no one will be happy about
> how, since the client essentially needs to "un-read" some data.

Let's be sure that we come out with something rock-solid here, the
code paths taken for authentication do not require an authenticated
user, so any bugs introduced could have dangerous consequences for the
backend.

> As a side note, this would also explain why I can't reproduce the issue,
> since I'm running in very low-latency environments (three VMs on my
> laptop).

I'm doing my tests in single VM, with both krb5kdc and Postgres
running together.

>> Also, something that is missing is the parametrization that has been
>> discussed the last time this patch was on the mailing list. Where is
>> the capacity to control if a client is connecting to a server that is
>> performing encryption and downgrade to non-ecrypted connection should
>> the server not be able to support it? Enforcing a client to require
>> encryption support using pg_hba.conf was as well a good thing. Some
>> more infrastructure is needed here, I thought that there was an
>> agreement previously regarding that.
>
> This does not match my impression of the discussion, but I would be
> happy to be wrong about that since it is less work for me.

OK, good to know. I had the opposite impression actually :) See above.

>> Also, and to make the review a bit easier, it would be good to split
>> the patch into smaller pieces (thanks for waiting on my input here,
>> this became quite clear after looking at this code). Basically,
>> pg_GSS_error is moved to a new file, bringing with it pg_GSS_recvauth
>> because the error routine is being used by the new ones you are
>> introducing: be_gssapi_write, etc. The split that this patch is doing
>> is a bit confusing, all the GSS-related stuff is put within one single
>> file:
>> - read/write routines
>> - authentication routine
>> - constants
>> - routines for error handling
>> Mixing read/write routines with the authentication routine looks
>> wrong, because the read-write routines just need to create a
>> dependency with for example be-secure.c on the backend. In short,
>> where before authentication and secure read/writes after
>> authentication get linked to each other, and create a dependency that
>> did not exist before.
>>
>> For the sake of clarity I would suggest the following split:
>> - be-gss-common.c, where all the constants and the error handling
>> routine are located.
>> - Let authrecv in auth.c
>> - Move only the read/write routines to the new file be-[secure-]gssapi.c
>> Splitting the patches into one that moves around the routines, and a
>> second that introduces the new logic would bring more clarity.
>
> So... auth.c is now going to depend on be-gss-common.c, and
> be-secure-gssapi.c is also going to depend on be-gss-common.c. That
> doesn't seem better in terms of dependencies: now I'm creating two that
> didn't exist before.
>
> That said, I would believe that it makes everything clearer. I will do
> that shortly.

That's a personal suggestion on the matter. If you feel that the way
you did the separation is better, nothing prevents you from submitting
the patch as such. IMO making a clear separation between the
authentication code path and the read/write code path matters, and
this separation exists today. In any case, a patch doing the
refactoring, and a second patch adding the feature would greatly
facilitate the review. After looking at the patch once this is pretty
clear.
--
Michael

Hello friends,

Here's yet another version of GSSAPI encryption support. It's also
available for viewing on my github:

https://github.com/frozencemetery/postgres/tree/feature/gssencrypt6

Let me hit the highlights of this time around:

- Fallback code is back! It's almost unchanged from early versions of
this patchset. Corresponding doc changes for this and the next item
are of course included.

- Minor protocol change. I did not realize that connection parameters
were not read until after auth was complete, which means that in this
version I go back to sending the AUTH_REQ_OK in the clear. Though I
found this initially irritating since it required re-working the
should_crypto conditions, it ends up being a net positive since I can
trade a library call for a couple variables.

- Client buffer flush on completion of authentication. This should
prevent the issue with the client getting unexpected message type of
NUL due to encrypted data not getting decrypted. I continue to be
unable to replicate this issue, but since the codepath triggers in the
"no data buffered case" all the math is sound. (Famous last words I'm
sure.)

- Code motion is its own patch. This was requested and hopefully
clarifies what's going on.

- Some GSSAPI authentication fixes have been applied. I've been staring
at this code too long now and writing this made me feel better. If it
should be a separate change that's fine and easy to do.

Thanks!

Hi Robbie,

On 3/8/16 5:44 PM, Robbie Harwood wrote:
> Hello friends,
>
> Here's yet another version of GSSAPI encryption support. It's also
> available for viewing on my github:

I got this warning when applying the first patch in the set:

../other/v6-0001-Move-common-GSSAPI-code-into-its-own-files.patch:245:
new blank line at EOF.
+
warning: 1 line adds whitespace errors.

I know it's minor but I'm always happier when patches apply cleanly.

The build went fine but when testing I was unable to logon at all. I'm
using the same methodology as in
http://www.postgresql.org/message-id/56BE0FF9.70302@pgmasters.net except
that I'm running against 51c0f63 and using the v6 patch set.

psql simply hangs and never returns. I have attached a pcap of the
psql/postgres session generated with:

tcpdump -i lo -nnvvXSs 1514 port 5432 -w gssapi.pcap

If you would like me to capture more information please let me know
specifically how you would like me to capture it.

I reverted to v5 and got the same behavior I was seeing with v4 and v5,
namely that I can only logon occasionally and usually get this error:

psql: expected authentication request from server, but received

Using a fresh build from 51c0f63 I can logon reliably every time so I
don't think there's an issue in my environment.

--
-David
david(at)pgmasters(dot)net

David Steele <david(at)pgmasters(dot)net> writes:

> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>>
>> Here's yet another version of GSSAPI encryption support. It's also
>> available for viewing on my github:
>
> I got this warning when applying the first patch in the set:
>
> ../other/v6-0001-Move-common-GSSAPI-code-into-its-own-files.patch:245:
> new blank line at EOF.
> +
> warning: 1 line adds whitespace errors.

Hah, so it does. Thanks for catching it; will fix.

> The build went fine but when testing I was unable to logon at all. I'm
> using the same methodology as in
> http://www.postgresql.org/message-id/56BE0FF9.70302@pgmasters.net except
> that I'm running against 51c0f63 and using the v6 patch set.
>
> psql simply hangs and never returns. I have attached a pcap of the
> psql/postgres session generated with:
>
> tcpdump -i lo -nnvvXSs 1514 port 5432 -w gssapi.pcap
>
> If you would like me to capture more information please let me know
> specifically how you would like me to capture it.

Thank you for the pcap! (I'm using wireshark so formats it can open are
greatly appreciated.) This suggests that the hang is my client code's
fault, but just in case: I assume nothing unusual was logged on the
server?

v6-0002-Connection-encryption-support-for-GSSAPI.patch in fe-connect.c
at around line 2518 adds a call to appendBinaryPQExpBuffer and sets
conn->inEnd. Can you try without those lines?

Can you also (e.g., with gdb or by adding printf calls) tell me what the
values of conn->inStart, conn->inEnd, and conn->inCursor any time
(should only be once) that those lines are triggered?

> I reverted to v5 and got the same behavior I was seeing with v4 and v5,
> namely that I can only logon occasionally and usually get this error:
>
> psql: expected authentication request from server, but received
>
> Using a fresh build from 51c0f63 I can logon reliably every time so I
> don't think there's an issue in my environment.

Agreed, I'm sure I've caused it somehow, though I don't know what's
wrong yet. (And if it weren't my fault but you didn't get useful errors
out, that'd be my fault anyway for not checking enough stuff!)

I don't know if this would say anything relevant, but it might be
interesting to see what the results are of applying [1] to the v5 code.
It's the same approach to solving the problem, though it happens at a
different time due to the aforementioned protocol change between v5 and
v6.

Thanks,
--Robbie

[1] https://github.com/frozencemetery/postgres/commit/82c89227a6b499ac9273044f91cff747c154629f

David Steele <david(at)pgmasters(dot)net> writes:

> Hi Robbie,
>
> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>> Hello friends,
>>
>> Here's yet another version of GSSAPI encryption support. It's also
>> available for viewing on my github:
>
> The build went fine but when testing I was unable to logon at all. I'm
> using the same methodology as in
> http://www.postgresql.org/message-id/56BE0FF9.70302@pgmasters.net except
> that I'm running against 51c0f63 and using the v6 patch set.
>
> psql simply hangs and never returns. I have attached a pcap of the
> psql/postgres session generated with:
>
> tcpdump -i lo -nnvvXSs 1514 port 5432 -w gssapi.pcap
>
> If you would like me to capture more information please let me know
> specifically how you would like me to capture it.

Please disregard my other email. I think I've found the issue; will
post a new version in a moment.

On 3/14/16 4:10 PM, Robbie Harwood wrote:
> David Steele <david(at)pgmasters(dot)net> writes:
>
>> Hi Robbie,
>>
>> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>>> Hello friends,
>>>
>>> Here's yet another version of GSSAPI encryption support. It's also
>>> available for viewing on my github:
>>
>> The build went fine but when testing I was unable to logon at all. I'm
>> using the same methodology as in
>> http://www.postgresql.org/message-id/56BE0FF9.70302@pgmasters.net except
>> that I'm running against 51c0f63 and using the v6 patch set.
>>
>> psql simply hangs and never returns. I have attached a pcap of the
>> psql/postgres session generated with:
>>
>> tcpdump -i lo -nnvvXSs 1514 port 5432 -w gssapi.pcap
>>
>> If you would like me to capture more information please let me know
>> specifically how you would like me to capture it.
>
> Please disregard my other email. I think I've found the issue; will
> post a new version in a moment.

Strange timing since I was just testing this. Here's what I got:

$ pg/bin/psql -h localhost -U vagrant(at)PGMASTERS(dot)NET postgres
conn->inStart = 179, conn->inEnd = 179, conn->inCursor = 179
psql (9.6devel)
Type "help" for help.

postgres=>

This was after commenting out:

// appendBinaryPQExpBuffer(&conn->gwritebuf,
// conn->inBuffer + conn->inStart,
// conn->inEnd - conn->inStart);
// conn->inEnd = conn->inStart;

The good news I can log on every time now!

--
-David
david(at)pgmasters(dot)net

Hello friends,

New week, new version. GitHub link:
https://github.com/frozencemetery/postgres/tree/feature/gssencrypt7

Changes in this version:

- Removed extra whitespace in auth code movement.

- Fixed connection desync issue. A diff of this and v6 will reveal
three issues:

- First, that pg_GSS_read() didn't properly handle having a full
buffer when called because pqsecure_raw_read() doesn't handle reads of
size 0. I've elected to change my own code here only, but it may be
desirable to change pqsecure_raw_read() as well depending on whether
other people are likely to hit that.

- Second, that I was shunting data into the wrong buffer (don't know
how this was overlooked; it has "write" right there in the name).

- Third, that I'm now immediately decrypting that data into
conn->inBuffer rather than deferring that step until later. This
removes the hang because now the connection will not erroneously get
stuck polling while data is buffered.

Thanks!

David Steele <david(at)pgmasters(dot)net> writes:

> On 3/14/16 4:10 PM, Robbie Harwood wrote:
>
>> David Steele <david(at)pgmasters(dot)net> writes:
>>
>>> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>>>
>>>> Here's yet another version of GSSAPI encryption support. It's also
>>>> available for viewing on my github:
>>>
>>> psql simply hangs and never returns. I have attached a pcap of the
>>> psql/postgres session generated with:
>>
>> Please disregard my other email. I think I've found the issue; will
>> post a new version in a moment.
>
> Strange timing since I was just testing this. Here's what I got:
>
> $ pg/bin/psql -h localhost -U vagrant(at)PGMASTERS(dot)NET postgres
> conn->inStart = 179, conn->inEnd = 179, conn->inCursor = 179
> psql (9.6devel)
> Type "help" for help.
>
> postgres=>

Thanks, that certainly is interesting! I did finally manage to
reproduce the issue on my end, but the rate of incidence is much lower
than what you and Michael were seeing: I have to run connections in a
loop for about 10-20 minutes before it makes itself apparent (and no,
it's not due to entropy). Apparently I just wasn't patient enough.

> This was after commenting out:
>
> // appendBinaryPQExpBuffer(&conn->gwritebuf,
> // conn->inBuffer + conn->inStart,
> // conn->inEnd - conn->inStart);
> // conn->inEnd = conn->inStart;
>
> The good news I can log on every time now!

Since conn->inStart == conn->inEnd in the case you were testing, the
lines you commented out would have been a no-op anyway (that's the
normal case of operation, as far as I can tell). That said, the chances
of hitting the race for me seemed very dependent on how much code wants
to run in that conditional: I got it up to 30-40 minutes when I added a
lot of printf()s (can't just run in gdb because it's nondeterministic
and rr has flushing bugs at the moment).

All that is to say: thank you very much for investigating that!

On 3/14/16 7:20 PM, Robbie Harwood wrote:

> David Steele <david(at)pgmasters(dot)net> writes:
>
>>
>> Strange timing since I was just testing this. Here's what I got:
>>
>> $ pg/bin/psql -h localhost -U vagrant(at)PGMASTERS(dot)NET postgres
>> conn->inStart = 179, conn->inEnd = 179, conn->inCursor = 179
>> psql (9.6devel)
>> Type "help" for help.
>>
>> postgres=>
>
> Thanks, that certainly is interesting! I did finally manage to
> reproduce the issue on my end, but the rate of incidence is much lower
> than what you and Michael were seeing: I have to run connections in a
> loop for about 10-20 minutes before it makes itself apparent (and no,
> it's not due to entropy). Apparently I just wasn't patient enough.

I'm running client and server on the same VM - perhaps that led to less
latency than your setup?

> All that is to say: thank you very much for investigating that!

My pleasure!

--
-David
david(at)pgmasters(dot)net

On 3/8/16 5:44 PM, Robbie Harwood wrote:

> Here's yet another version of GSSAPI encryption support.

OK, everything seems to be working fine with a 9.6 client and server so
next I tested older clients and I got this error:

$ /usr/lib/postgresql/9.1/bin/psql -h localhost \
-U vagrant(at)PGMASTERS(dot)NET postgres
psql: FATAL: GSSAPI did no provide required flags

There's a small typo in the error message, BTW.

Here's the output from the server logs:

DEBUG: forked new backend, pid=7746 socket=9
DEBUG: postgres child[7746]: starting with (
DEBUG: postgres
DEBUG: )
DEBUG: InitPostgres
DEBUG: my backend ID is 2
DEBUG: StartTransaction
DEBUG: name: unnamed; blockState: DEFAULT; state: INPROGR,
xid/subid/cid: 0/1/0, nestlvl: 1, children:
DEBUG: Processing received GSS token of length 667
DEBUG: gss_accept_sec_context major: 0, minor: 0, outlen: 161,
outflags: 1b2
DEBUG: sending GSS response token of length 161
DEBUG: sending GSS token of length 161
FATAL: GSSAPI did no provide required flags
DEBUG: shmem_exit(1): 1 before_shmem_exit callbacks to make
DEBUG: shmem_exit(1): 6 on_shmem_exit callbacks to make
DEBUG: proc_exit(1): 3 callbacks to make
DEBUG: exit(1)
DEBUG: shmem_exit(-1): 0 before_shmem_exit callbacks to make
DEBUG: shmem_exit(-1): 0 on_shmem_exit callbacks to make
DEBUG: proc_exit(-1): 0 callbacks to make
DEBUG: reaping dead processes
DEBUG: server process (PID 7746) exited with exit code 1

I got the same result with a 9.4 client so didn't try any others.

--
-David
david(at)pgmasters(dot)net

On Tue, Mar 15, 2016 at 3:12 PM, David Steele <david(at)pgmasters(dot)net> wrote:
> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>> Here's yet another version of GSSAPI encryption support.

This looks far more stable than last versions, cool to see the
progress. pgbench -C does not complain on my side so that's a good
thing. This is not yet a detailed review, there are a couple of things
that I find strange in what you did and are potential subject to bugs,
but I need a bit of time to let that mature a bit. This is not
something yet committable, but I really like the direction that the
patch is taking.

For now, regarding 0002:
/*
- * Flush message so client will see it, except for AUTH_REQ_OK, which need
- * not be sent until we are ready for queries.
+ * In most cases, we do not need to send AUTH_REQ_OK until we are ready
+ * for queries, but if we are doing GSSAPI encryption that request must go
+ * out now.
*/
- if (areq != AUTH_REQ_OK)
- pq_flush();
+ pq_flush();
Er, this sends unconditionally the message without caring about the
protocol, and so this is incorrect, no?

+#ifdef ENABLE_GSS
+ if (conn->gss->buf.data)
+ pfree(conn->gss->buf.data);
+ if (conn->gss->writebuf.data)
+ pfree(conn->gss->writebuf.data);
+#endif
You should use resetStringInfo here.

> OK, everything seems to be working fine with a 9.6 client and server so
> next I tested older clients and I got this error:
>
> $ /usr/lib/postgresql/9.1/bin/psql -h localhost \
> -U vagrant(at)PGMASTERS(dot)NET postgres
> psql: FATAL: GSSAPI did no provide required flags
>
> There's a small typo in the error message, BTW.

And in 0003, the previous error is caused by that:
+ target_flags = GSS_C_MUTUAL_FLAG | GSS_C_REPLAY_FLAG |
+ GSS_C_SEQUENCE_FLAG | GSS_C_CONF_FLAG | GSS_C_INTEG_FLAG;
+ if ((gflags & target_flags) != target_flags)
+ {
+ ereport(FATAL, (errmsg("GSSAPI did no provide required flags")));
+ return STATUS_ERROR;
+ }
Yeah, this is a recipe for protocol incompatibility and here the
connection context is not yet fully defined I believe. We need to be
careful.

- maj_stat = gss_accept_sec_context(
- &min_stat,
+ maj_stat = gss_accept_sec_context(&min_stat,
This is just noise.
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> On Tue, Mar 15, 2016 at 3:12 PM, David Steele <david(at)pgmasters(dot)net> wrote:
>> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>>> Here's yet another version of GSSAPI encryption support.
>>
>> This looks far more stable than last versions, cool to see the
>> progress. pgbench -C does not complain on my side so that's a good
>> thing. This is not yet a detailed review, there are a couple of
>> things that I find strange in what you did and are potential subject
>> to bugs, but I need a bit of time to let that mature a bit. This is
>> not something yet committable, but I really like the direction that
>> the patch is taking.

Thanks! I must admit a preference for receiving all feedback at once
(reduces back-and-forth overhead), but if you feel there are still
design-type problems then that's very reasonable. (I also admit to
feeling the pressure of feature freeze in less than a month.)

> For now, regarding 0002:
> /*
> - * Flush message so client will see it, except for AUTH_REQ_OK, which need
> - * not be sent until we are ready for queries.
> + * In most cases, we do not need to send AUTH_REQ_OK until we are ready
> + * for queries, but if we are doing GSSAPI encryption that request must go
> + * out now.
> */
> - if (areq != AUTH_REQ_OK)
> - pq_flush();
> + pq_flush();
> Er, this sends unconditionally the message without caring about the
> protocol, and so this is incorrect, no?

My impression from reading the old version of the comment above it was
that on other protocols it could go either way. I think last time I
made it conditional; I can do so again if it is desired. It's certainly
not /incorrect/ to send it immediately; there's just a question of
performance by minimizing the number of writes as far as I can tell.

> +#ifdef ENABLE_GSS
> + if (conn->gss->buf.data)
> + pfree(conn->gss->buf.data);
> + if (conn->gss->writebuf.data)
> + pfree(conn->gss->writebuf.data);
> +#endif
> You should use resetStringInfo here.

That will leak since resetStringInfo() doesn't free the underlying
representation.

>> OK, everything seems to be working fine with a 9.6 client and server so
>> next I tested older clients and I got this error:
>>
>> $ /usr/lib/postgresql/9.1/bin/psql -h localhost \
>> -U vagrant(at)PGMASTERS(dot)NET postgres
>> psql: FATAL: GSSAPI did no provide required flags
>>
>> There's a small typo in the error message, BTW.

Thanks, will fix. I forgot that MIT doesn't provide GSS_C_REPLAY_FLAG
and GSS_C_SEQUENCE_FLAG by default. Removing those from auth.c should
temporarily resolve the problem, which is what I'll do in the next
version. (Tested with MIT krb5.)

On the subject of older code, I realized (one of those wake up in the
middle of the night-type moments) that the old server has a potential
problem with new clients now in that we try to decrypt the "help I don't
recognize connection parameter gss_encrypt" error message. v8 will have
some sort of a fix, though I don't know what yet. The only thing I've
come up with so far is to have the client decrypt check the first time
through for packets beginning with 'E'. Pretty much anything I can do
will end up being a poor substitute for being at the protocol layer
anyway.

> And in 0003, the previous error is caused by that:
> + target_flags = GSS_C_MUTUAL_FLAG | GSS_C_REPLAY_FLAG |
> + GSS_C_SEQUENCE_FLAG | GSS_C_CONF_FLAG | GSS_C_INTEG_FLAG;
> + if ((gflags & target_flags) != target_flags)
> + {
> + ereport(FATAL, (errmsg("GSSAPI did no provide required flags")));
> + return STATUS_ERROR;
> + }
> Yeah, this is a recipe for protocol incompatibility and here the
> connection context is not yet fully defined I believe. We need to be
> careful.

Nope, it's done. This is happening immediately prior to username
checks. By the time we exit the do/while the context is fully
complete.

> - maj_stat = gss_accept_sec_context(
> - &min_stat,
> + maj_stat = gss_accept_sec_context(&min_stat,
>
> This is just noise.

You're not wrong, though I do think it makes the code more readable by
enforcing style and this is a "cleanup" commit. I'll take it out if it
bothers you.

Robbie,

* Robbie Harwood (rharwood(at)redhat(dot)com) wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> > - maj_stat = gss_accept_sec_context(
> > - &min_stat,
> > + maj_stat = gss_accept_sec_context(&min_stat,
> >
> > This is just noise.
>
> You're not wrong, though I do think it makes the code more readable by
> enforcing style and this is a "cleanup" commit. I'll take it out if it
> bothers you.

First, thanks much for working on this, I've been following along the
discussions and hope to be able to help move it to commit, once it's
ready.

Secondly, generally, speaking, we prefer that 'cleanup' type changes
(particularly whitespace-only ones) are in independent commits which are
marked as just whitespace/indentation changes. We have a number of
organizations which follow our code changes and it makes it more
difficult on them to include whitespace/indentation changes with code
changes.

Thanks!

Stephen

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

> Robbie,
>
> * Robbie Harwood (rharwood(at)redhat(dot)com) wrote:
>> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> > - maj_stat = gss_accept_sec_context(
>> > - &min_stat,
>> > + maj_stat = gss_accept_sec_context(&min_stat,
>> >
>> > This is just noise.
>>
>> You're not wrong, though I do think it makes the code more readable by
>> enforcing style and this is a "cleanup" commit. I'll take it out if it
>> bothers you.
>
> First, thanks much for working on this, I've been following along the
> discussions and hope to be able to help move it to commit, once it's
> ready.
>
> Secondly, generally, speaking, we prefer that 'cleanup' type changes
> (particularly whitespace-only ones) are in independent commits which are
> marked as just whitespace/indentation changes. We have a number of
> organizations which follow our code changes and it makes it more
> difficult on them to include whitespace/indentation changes with code
> changes.

Thanks for the clarification. I'll be sure to take it out!

Hello friends,

A new version of my GSSAPI encryption patchset is available, both in
this email and on my github:
https://github.com/frozencemetery/postgres/tree/feature/gssencrypt8

What changed:

- Fixed fallback in the new server/old client case. The server flag
checking has been reduced. In the (distant) future when all old
clients are gone, the checking should be increased in strength once
again.

- Fixed fallback in the old server/new client case. This consists of
checking whether the first message we receive after auth is done is an
error, and raising it without decrypting if it is so that reconnection
can happen if desired. The quality of the fallback path has also
generally been improved.

- Removed overzealous whitespace cleanup. I'm a packager and have been
bitten by upstreams doing this; I should have known better.

- Made flushing the AUTH_REQ_OK message conditional again.

- Fixed typo in server error message for insufficient GSSAPI protection.

Thanks!

On 3/20/16 12:09 AM, Robbie Harwood wrote:
> Hello friends,
>
> A new version of my GSSAPI encryption patchset is available, both in
> this email and on my github:
> https://github.com/frozencemetery/postgres/tree/feature/gssencrypt8

Excellent, Robbie! I've run this patch through my test cases and
everything works.

Now that it's working I'll be writing up an actual review so expect that
by Monday.

--
-David
david(at)pgmasters(dot)net

On Fri, Mar 25, 2016 at 10:10 PM, David Steele <david(at)pgmasters(dot)net> wrote:
> Excellent, Robbie! I've run this patch through my test cases and
> everything works.
>
> Now that it's working I'll be writing up an actual review so expect that
> by Monday.

(I haven't given up on this patch yet, sorry for the low activity on
this thread)
--
Michael

Hi Robbie,

On 3/20/16 12:09 AM, Robbie Harwood wrote:

> A new version of my GSSAPI encryption patchset is available

Here's a more thorough review:

* PATCH - Move common GSSAPI code into its own files

diff --git a/src/backend/libpq/be-gssapi-common.c

+ if (msg_ctx)
+
+ /*
+ * More than one message available. XXX: Should we loop and read all
+ * messages? (same below)
+ */

It seems like it would be a good idea to pull all error messages if only
for debugging purposes.

+ /* Fetch mechanism minor status message */
+ msg_ctx = 0;
+ gss_display_status(&lmin_s, min_stat, GSS_C_MECH_CODE,
+ GSS_C_NO_OID, &msg_ctx, &gmsg);
+ strlcpy(msg_minor, gmsg.value, sizeof(msg_minor));
+ gss_release_buffer(&lmin_s, &gmsg);
+
+ if (msg_ctx)
+ ereport(WARNING,
+ (errmsg_internal("incomplete GSS minor error report")));

Same here.

* PATCH - Connection encryption support for GSSAPI

+++ b/doc/src/sgml/client-auth.sgml

data sent over the database connection will be sent unencrypted unless
- <acronym>SSL</acronym> is used.
+ <acronym>SSL</acronym> is used, or <acronym>GSSAPI</acronym> encryption
+ is in use.

Perhaps "... unless SSL or GSSAPI encryption is used."

+ <term><literal>require_encrypt</literal></term>
+ <listitem>
+ <para>
+ Whether to require GSSAPI encryption. Default is off, which causes
+ GSSAPI encryption to be enabled if available and requested for
+ compatability with old clients. It is recommended to set this
unless
+ old clients are present.

Some rewording:

Require GSSAPI encryption. The default is off which enables GSSAPI
encryption only when available and requested to maintain compatability
with older clients. This setting should be enabled unless older clients
are present.

+++ b/doc/src/sgml/libpq.sgml

@@ -1356,6 +1356,18 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
</listitem>
</varlistentry>

+ <term><literal>gss_enc_require</literal></term>
+ <listitem>
+ <para>
+ If set, whether to require GSSAPI encryption support from the
remote
+ server. Defaults to unset, which will cause the client to fall
back
+ to not using GSSAPI encryption if the server does not support
+ encryption through GSSAPI.
+ </para>

Some rewording:

Require GSSAPI encryption support from the remote server when set. By
default clients will fall back to not using GSSAPI encryption if the
server does not support encryption through GSSAPI.

+++ b/doc/src/sgml/runtime.sgml

I think you mean postgresql.conf?

+ <para>
+ GSSAPI connections can also encrypt all data sent across the network.
+ In the <filename>pg_hba.conf</> file, the GSSAPI authentication
method
+ has a parameter to require encryption; otherwise connections will be
+ encrypted if available and requested by the client. On the
client side,
+ there is also a parameter to require GSSAPI encryption support
from the
+ server.
+ </para>

I think a link to the client parameter would be valuable here.

+++ b/src/backend/libpq/auth.c

+ * In most cases, we do not need to send AUTH_REQ_OK until we are ready
+ * for queries, but if we are doing GSSAPI encryption that request must go
+ * out now.

Why?

+++ b/src/backend/libpq/be-secure-gssapi.c

Some of the functions in this file are missing comment headers and
should have more inline comments for each major section.

+ ret = be_gssapi_should_crypto(port);

Add LF here.

+ port->gss->writebuf.cursor += ret;

And here.

+ /* need to be called again */

And here. Well, you get the idea.

These functions could all use more whitespace and comments.

+++ b/src/backend/utils/misc/guc.c

+ {
+ {"gss_encrypt", PGC_USERSET, CONN_AUTH_SECURITY,
+ gettext_noop("Whether client wants encryption for this connection."),

Perhaps, "Require encryption for all GSSAPI connections."

+++ b/src/interfaces/libpq/fe-connect.c

+ if (n > 0)
+ conn->inEnd += n;
+ /*
+ * If n < 0, then this wasn't a full request and
+ * either more data will be available later to
+ * complete it or we will error out then.
+ */

Shouldn't this comment block go above the if?

+ /*
+ * We tried to request GSSAPI encryption, but the
+ * server doesn't support it. Retries are permitted
+ * here, so hang up and try again. A connection that
+ * doesn't rupport appname will also not support
+ * GSSAPI encryption.
+ */

typo - "doesn't support appname".

+++ b/src/interfaces/libpq/fe-secure-gssapi.c

Comments are a bit better here than in be-secure-gssapi.c but could
still be better. And again, more whitespace.

* PATCH 3 - GSSAPI authentication cleanup

+++ b/src/backend/libpq/auth.c

+ * GSS_C_REPLAY_FLAG and GSS_C_SEQUENCE_FLAG are missing for compatability
+ * with older clients and should be added in as soon as possible.

Please elaborate here. Why should they be added and what functionality
is missing before they are.

+++ b/src/backend/libpq/be-gssapi-common.c

-#if defined(WIN32) && !defined(WIN32_ONLY_COMPILER)
-/*
- * MIT Kerberos GSSAPI DLL doesn't properly export the symbols for MingW
- * that contain the OIDs required. Redefine here, values copied
- * from src/athena/auth/krb5/src/lib/gssapi/generic/gssapi_generic.c
- */
-static const gss_OID_desc GSS_C_NT_USER_NAME_desc =
-{10, (void *) "\x2a\x86\x48\x86\xf7\x12\x01\x02\x01\x02"};
-static GSS_DLLIMP gss_OID GSS_C_NT_USER_NAME = &GSS_C_NT_USER_NAME_desc;
-#endif

Can you explain why it's OK to remove this now? I can see you've
replaced it in gss_init_sec_context() with GSS_C_MUTUAL_FLAG. Have you
tested this on Win32?

Things look pretty good in general but fe-secure-gssapi.c and
be-secure-gssapi.c should both be reworked with better comments and more
whitespace before this patch goes to a committer.

--
-David
david(at)pgmasters(dot)net

David Steele <david(at)pgmasters(dot)net> writes:

> On 3/20/16 12:09 AM, Robbie Harwood wrote:
>
>> A new version of my GSSAPI encryption patchset is available
>
> Here's a more thorough review:

Thanks for the review! To keep this a manageable size, I'm going to
trim pretty heavily. If I haven't replied to something, please
interpret it to mean that I think it's a good suggestion and will
fix/change in v9.

> +++ b/doc/src/sgml/runtime.sgml
>
> I think you mean postgresql.conf?

Sorry, what does this review comment go with?

> +++ b/src/backend/libpq/auth.c
>
> + * In most cases, we do not need to send AUTH_REQ_OK until we are ready
> + * for queries, but if we are doing GSSAPI encryption that request must go
> + * out now.
>
> Why?

Because otherwise we will send the connection spew (connection
parameters and such) unencrypted, since it will get grouped with the
AUTH_REQ_OK packet. I'll note this in the comment.

> + ret = be_gssapi_should_crypto(port);
>
> Add LF here.
>
>
> + port->gss->writebuf.cursor += ret;
>
> And here.
>
> + /* need to be called again */
>
> And here. Well, you get the idea.

Sorry, what is LF? ASCII newline?

> * PATCH 3 - GSSAPI authentication cleanup
>
> +++ b/src/backend/libpq/auth.c
>
> + * GSS_C_REPLAY_FLAG and GSS_C_SEQUENCE_FLAG are missing for compatability
> + * with older clients and should be added in as soon as possible.
>
> Please elaborate here. Why should they be added and what functionality
> is missing before they are.

It's request reply detection and out-of-sequence detection. We can't
require them because old clients don't request them, and so would be
unable to connect. (There's some history of this in the last couple
versions I've posted, but it's not really interesting beyond "it doesn't
work".) I will clarify this comment.

> +++ b/src/backend/libpq/be-gssapi-common.c
>
> -#if defined(WIN32) && !defined(WIN32_ONLY_COMPILER)
> -/*
> - * MIT Kerberos GSSAPI DLL doesn't properly export the symbols for MingW
> - * that contain the OIDs required. Redefine here, values copied
> - * from src/athena/auth/krb5/src/lib/gssapi/generic/gssapi_generic.c
> - */
> -static const gss_OID_desc GSS_C_NT_USER_NAME_desc =
> -{10, (void *) "\x2a\x86\x48\x86\xf7\x12\x01\x02\x01\x02"};
> -static GSS_DLLIMP gss_OID GSS_C_NT_USER_NAME = &GSS_C_NT_USER_NAME_desc;
> -#endif
>
> Can you explain why it's OK to remove this now? I can see you've
> replaced it in gss_init_sec_context() with GSS_C_MUTUAL_FLAG. Have you
> tested this on Win32?

This comment is old enough that it references sources from Athena. If
this is in fact a current krb5 bug (which I doubt, since MIT tests
windows rather heavily), then it should be filed against krb5 instead of
kludged around here. I do not however have win32 machines to test this
with. (GSS_C_MUTUAL_FLAG is unrelated; it just requests that the client
and server are both authenticated to each other.)

Thanks,
--Robbie

On 3/29/16 5:05 PM, Robbie Harwood wrote:
> David Steele <david(at)pgmasters(dot)net> writes:
>
>> On 3/20/16 12:09 AM, Robbie Harwood wrote:
>>
>>> A new version of my GSSAPI encryption patchset is available
>>
>> Here's a more thorough review:
>
> Thanks for the review! To keep this a manageable size, I'm going to
> trim pretty heavily. If I haven't replied to something, please
> interpret it to mean that I think it's a good suggestion and will
> fix/change in v9.
>
>> +++ b/doc/src/sgml/runtime.sgml
>>
>> I think you mean postgresql.conf?
>
> Sorry, what does this review comment go with?

Oops! That was a comment I made then realized I had misunderstood what
was going on. I removed the code but not the comment apparently.

Thanks for the other clarifications.

--
-David
david(at)pgmasters(dot)net

Hello friends,

A new version of my GSSAPI encryption patchset is available, both in
this email and on my github:
https://github.com/frozencemetery/postgres/tree/feature/gssencrypt9

This version is intended to address David's review suggestions:

- The only code change (not counting SGML and comments) is that I've
resolved the pre-existing "XXX: Should we loop and read all messages?"
comment in the affirmative by... looping and reading all messages in
pg_GSS_error. Though commented on as part of the first patch, this is
bundled with the rest of the auth cleanup since the first patch is
code motion only.

- Several changes to comments, documentation rewordings, and whitespace
additions. I look forward to finding out what needs even more of the
same treatment. Of all the changesets I've posted thus far, this
might be the one for which it makes the most sense to see what changed
by diffing from the previous changeset.

Thanks!

On Wed, Mar 30, 2016 at 1:01 PM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> A new version of my GSSAPI encryption patchset is available, both in
> this email and on my github:
> https://github.com/frozencemetery/postgres/tree/feature/gssencrypt9
>
> This version is intended to address David's review suggestions:
>
> - The only code change (not counting SGML and comments) is that I've
> resolved the pre-existing "XXX: Should we loop and read all messages?"
> comment in the affirmative by... looping and reading all messages in
> pg_GSS_error. Though commented on as part of the first patch, this is
> bundled with the rest of the auth cleanup since the first patch is
> code motion only.
>
> - Several changes to comments, documentation rewordings, and whitespace
> additions. I look forward to finding out what needs even more of the
> same treatment. Of all the changesets I've posted thus far, this
> might be the one for which it makes the most sense to see what changed
> by diffing from the previous changeset.

Thanks for the new versions. For now I have been having a look at only 0001.

The first thing I have noticed is that 0001 breaks the Windows build
using Visual Studio. When building without GSSAPI, fe-gssapi-common.c
and be-gssapi-common.c should be removed from the list of files used
so that's simple enough to fix. Now when GSSAPI build is enabled,
there are some declaration conflicts with your patch, leading to many
compilation errors. This took me some time to figure out but the cause
is this diff in be-gssapi-common.c:
+#include "libpq/be-gssapi-common.h"
+
+#include "postgres.h"

postgres.h should be declared *before* be-gssapi-common.h. As I
suggested the refactoring and I guess you don't have a Windows
environment at hand, attached is a patch that fixes the build with and
without gssapi for Visual Studio, that can be applied on top of your
0001. Feel free to rebase using it.

Note for others: I had as well to patch the MSVC scripts because in
the newest installations of Kerberos:
- inc/ does not exist anymore, include/ does
- The current VS scripts ignore completely x64 builds, the libraries
linked to are for x86 unconditionally.
I am not sure if there are *any* people building the code with
Kerberos on Windows except I, but as things stand those scripts are
rather broken in my view.

Now looking at 0002 and 0003...
--
Michael

On Thu, Mar 31, 2016 at 2:14 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Mar 30, 2016 at 1:01 PM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>> A new version of my GSSAPI encryption patchset is available, both in
>> this email and on my github:
>> https://github.com/frozencemetery/postgres/tree/feature/gssencrypt9
>>
>> This version is intended to address David's review suggestions:
>>
>> - The only code change (not counting SGML and comments) is that I've
>> resolved the pre-existing "XXX: Should we loop and read all messages?"
>> comment in the affirmative by... looping and reading all messages in
>> pg_GSS_error. Though commented on as part of the first patch, this is
>> bundled with the rest of the auth cleanup since the first patch is
>> code motion only.
>>
>> - Several changes to comments, documentation rewordings, and whitespace
>> additions. I look forward to finding out what needs even more of the
>> same treatment. Of all the changesets I've posted thus far, this
>> might be the one for which it makes the most sense to see what changed
>> by diffing from the previous changeset.
>
> Thanks for the new versions. For now I have been having a look at only 0001.
>
> The first thing I have noticed is that 0001 breaks the Windows build
> using Visual Studio. When building without GSSAPI, fe-gssapi-common.c
> and be-gssapi-common.c should be removed from the list of files used
> so that's simple enough to fix. Now when GSSAPI build is enabled,
> there are some declaration conflicts with your patch, leading to many
> compilation errors. This took me some time to figure out but the cause
> is this diff in be-gssapi-common.c:
> +#include "libpq/be-gssapi-common.h"
> +
> +#include "postgres.h"
>
> postgres.h should be declared *before* be-gssapi-common.h. As I
> suggested the refactoring and I guess you don't have a Windows
> environment at hand, attached is a patch that fixes the build with and
> without gssapi for Visual Studio, that can be applied on top of your
> 0001. Feel free to rebase using it.
>
> Note for others: I had as well to patch the MSVC scripts because in
> the newest installations of Kerberos:
> - inc/ does not exist anymore, include/ does
> - The current VS scripts ignore completely x64 builds, the libraries
> linked to are for x86 unconditionally.
> I am not sure if there are *any* people building the code with
> Kerberos on Windows except I, but as things stand those scripts are
> rather broken in my view.

(Lonely feeling after typing that)

> Now looking at 0002 and 0003...

(Thanks for the split btw, this is far easier to look at)

Patch 0002 has the same problems, because it introduces the
gssapi-secure stuff (see attached patch that can be applied on top of
0002 fixing the windows build). When gss build is not enabled,
compilation is able to work, now when gss is enabled... See below.

+ <varlistentry id="libpq-connect-gss-enc-require"
xreflabel="gss_enc_require">
+ <term><literal>gss_enc_require</literal></term>
+ <listitem>
Perhaps renaming that gss_require_encrypt?

+++ b/src/backend/libpq/be-secure-gssapi.c
[...]
+#include "libpq/be-gssapi-common.h"
+
+#include "postgres.h"
Those should be reversed.

+ Require GSSAPI encryption. Defaults to off, which enables GSSAPI
+ encryption only when both available and requested to maintain
+ compatability with old clients. This setting should be enabled unless
+ old clients are present.
s/compatability/compatibility/

+ <para>
+ Require GSSAPI encryption support from the remote server when set. By
+ default, clients will fall back to not using GSSAPI encryption if the
+ server does not support encryption through GSSAPI.
+ </para>
Nitpick: the use of the <acronym> markup for GSSAPI may be more
adapted (I know, the docs are a bit lazy on that).

+ iov[0].iov_base = lenbuf;
+ iov[0].iov_len = 4;
+ iov[1].iov_base = output.value;
+ iov[1].iov_len = output.length;
+
+ ret = writev(port->sock, iov, 2);
writev and iovec are not present on Windows, so this code would never
compile there, and it does not sound that this patch is a reason
sufficient enough to drop support of GSSAPI on Windows.

+static ssize_t
+be_gssapi_should_crypto(Port *port)
+{
+ if (port->gss->ctx == GSS_C_NO_CONTEXT)
+ return 0;
+ return gss_encrypt;
+}
This should return a boolean, gss_encrypt being one.

+ if (!gss_encrypt && port->hba->require_encrypt)
+ {
+ ereport(FATAL, (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
+ errmsg("GSSAPI encryption required from user \"%s\"",
+ port->user_name)));
+ }
Could be clearer here, with some error detail message, like:
"User has requested GSSAPI encryption, but server disallows it".

+static void
+assign_gss_encrypt(bool newval, void *extra)
+{
+ gss_encrypt = newval;
+}
Assigning a variable is done by the GUC machinery, you don't need that.

+ {
+ {"gss_encrypt", PGC_USERSET, CONN_AUTH_SECURITY,
+ gettext_noop("Require encryption for all GSSAPI connections."),
+ NULL,
+ GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+ },
+ &gss_encrypt, false, check_gss_encrypt, assign_gss_encrypt, NULL
+ },
Hm. I think that this would be better as PGC_POSTMASTER, the idea
behind this parameter being that the server enforces any connections
from clients to be encrypted. Clients should not have the right to
disable that at will depending on their session, and this is used in
authentication code paths. Also, this parameter is not documented, the
new parameters in pg_hba.conf and client-side are though.

A consequence of that, is that you would not need the following thingy:
+static bool
+check_gss_encrypt(bool *newval, void **extra, GucSource source)
+{
+ if (MyProcPort && MyProcPort->hba && MyProcPort->hba->require_encrypt &&
+ !*newval)
+ return false;
+ return true;
+}
So I'd suggest go with PGC_POSTMASTER first to simplify the code. We
could always relax that later on as need be, but that's not something
I think the code patch should bother much about.

+/*
+ * Only consider encryption when GSS context is complete
+ */
+ssize_t
+pg_GSS_should_crypto(PGconn *conn)
+{
s/crypto/encrypt?

+ /*
+ * We tried to request GSSAPI encryption, but the
+ * server doesn't support it. Retries are permitted
+ * here, so hang up and try again. A connection that
+ * doesn't support appname will also not support
+ * GSSAPI encryption.
+ */
+ const char *sqlstate;
Hm... I'm having second thoughts regarding gss_enc_require... This
code path would happen with a 9.6 client and a ~9.5 server, it seems a
bit treacherous to not ERROR here and fallback to an unencrypted
connection, client want to have an encrypted connection at the end.

I ran as well some tests, and I was able to crash the server. For
example take this SQL sequence:
CREATE TABLE aa (a int);
INSERT INTO aa VALUES (generate_series(1,1000000));
COPY aa TO STDOUT; -- crash
Which resulted in the following crash:
1046 AssertArg(MemoryContextIsValid(context));
(gdb) bt
#0 0x0000000000980552 in repalloc (pointer=0x2aa1bb0, size=8192) at mcxt.c:1046
#1 0x00000000006b75d5 in enlargeStringInfo (str=0x2aa6b70,
needed=7784) at stringinfo.c:286
#2 0x00000000006b7447 in appendBinaryStringInfo (str=0x2aa6b70,
data=0x2b40f25
"\337\321\261\026jy\352\334#\275l)\030\021s\235\f;\237\336\222PZsd\025>ioS\313`9C\375\a\340Z\354E\355\235\276y\307)D\261\344$D\347\323\036\177S\265\374\373\332~\264\377\317\375<\017\330\214P\a\237\321\375\002$=6\326\263\265{\237\344\214\344.W\303\216\373\206\325\257E\223N\t\324\223\030\363\252&\374\241T\322<\343,\233\203\320\252\343\344\f\036*\274\311\066\206\n\337\300\320L,>-A\016D\346\263pv+A>y\324\254k\003)\264\212zc\344\n\223\224\211\243\"\224\343\241Q\264\233\223\303\"\b\275\026%\302\352\065]8\207\244\304\353\220p\364\272\240\307\247l\216}N\325\aUO6\322\352\273"...,
datalen=7783) at stringinfo.c:213
#3 0x00000000006c8359 in be_gssapi_write (port=0x2aa31d0,
ptr=0x2aa8b48, len=8192) at be-secure-gssapi.c:158
#4 0x00000000006b9697 in secure_write (port=0x2aa31d0, ptr=0x2aa8b48,
len=8192) at be-secure.c:248
Which is actually related to your use of StringInfoData, pointing out
that the buffer handling should be reworked.

Also, to sum up with this patch, things behave like that:
1) server enforces encryption, client asks for encryption: all good,
encryption is done.
2) server enforces encryption, client does not ask for it: encryption
is enforced. Perhaps a FATAL on backend would be more adapted to let
the caller know the incompatibility? I am afraid of cases where the
client intentionally does *not* want encryption because of the
overhead that it creates at low-level to encrypt/decrypt the messages.
We'd lose control of that with this patch, and encryption can get
really a lot slower..
3) server does not enforce encryption, client asks for it: FATAL on
backend side.
4) server does not enforce encryption, client not asking for it, no encryption.

-#if defined(WIN32) && !defined(WIN32_ONLY_COMPILER)
-/*
- * MIT Kerberos GSSAPI DLL doesn't properly export the symbols for MingW
- * that contain the OIDs required. Redefine here, values copied
- * from src/athena/auth/krb5/src/lib/gssapi/generic/gssapi_generic.c
- */
-static const gss_OID_desc GSS_C_NT_USER_NAME_desc =
-{10, (void *) "\x2a\x86\x48\x86\xf7\x12\x01\x02\x01\x02"};
-static GSS_DLLIMP gss_OID GSS_C_NT_USER_NAME = &GSS_C_NT_USER_NAME_desc;
-#endif
What makes you say that it is fine to remove that? If that's actually
the case, we should remove it as part of a separate patch.
--
Michael

On Thu, Mar 31, 2016 at 4:48 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> [long review]

Note as well that I have switched the patch as "waiting on author" for
the time being. Missing symbols on Windows as well as crashes are
pointing out that this should be returned with feedback for now (sorry
for the late reviews btw).
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> On Thu, Mar 31, 2016 at 2:14 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Wed, Mar 30, 2016 at 1:01 PM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>>> A new version of my GSSAPI encryption patchset is available, both in
>>> this email and on my github:
>>> https://github.com/frozencemetery/postgres/tree/feature/gssencrypt9
>>>
>> postgres.h should be declared *before* be-gssapi-common.h. As I
>> suggested the refactoring and I guess you don't have a Windows
>> environment at hand, attached is a patch that fixes the build with and
>> without gssapi for Visual Studio, that can be applied on top of your
>> 0001. Feel free to rebase using it.

Thank you for the patch to fix 0001. There is basically no way I would
have been able to make it work on Windows myself.

> + iov[0].iov_base = lenbuf;
> + iov[0].iov_len = 4;
> + iov[1].iov_base = output.value;
> + iov[1].iov_len = output.length;
> +
> + ret = writev(port->sock, iov, 2);
>
> writev and iovec are not present on Windows, so this code would never
> compile there, and it does not sound that this patch is a reason
> sufficient enough to drop support of GSSAPI on Windows.

Um. Okay. I guess on Windows I'll make two write calls then, since the
only other option I see is to hit alloc again here.

> + {
> + {"gss_encrypt", PGC_USERSET, CONN_AUTH_SECURITY,
> + gettext_noop("Require encryption for all GSSAPI connections."),
> + NULL,
> + GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
> + },
> + &gss_encrypt, false, check_gss_encrypt, assign_gss_encrypt, NULL
> + },
>
> Hm. I think that this would be better as PGC_POSTMASTER, the idea
> behind this parameter being that the server enforces any connections
> from clients to be encrypted. Clients should not have the right to
> disable that at will depending on their session, and this is used in
> authentication code paths. Also, this parameter is not documented, the
> new parameters in pg_hba.conf and client-side are though.

Negative, setting PGC_POSTMASTER breaks the fallback support; I get

"psql: FATAL: parameter "gss_encrypt" cannot be changed without
restarting the server"

when trying to connect a new client to a new server.

I will add documentation.

> + /*
> + * We tried to request GSSAPI encryption, but the
> + * server doesn't support it. Retries are permitted
> + * here, so hang up and try again. A connection that
> + * doesn't support appname will also not support
> + * GSSAPI encryption.
> + */
> + const char *sqlstate;
> Hm... I'm having second thoughts regarding gss_enc_require... This
> code path would happen with a 9.6 client and a ~9.5 server, it seems a
> bit treacherous to not ERROR here and fallback to an unencrypted
> connection, client want to have an encrypted connection at the end.

I'm confused by what you're saying here.

1. If you have a 9.6 client and a 9.5 server, with no additional
connection parameters the client will reconnect and get an
unencrypted connection. This is the "fallback" support.

2. If the client passes gss_enc_require=1, then it will just fail
because the server cannot provide encryption.

> I ran as well some tests, and I was able to crash the server. For
> example take this SQL sequence:
> CREATE TABLE aa (a int);
> INSERT INTO aa VALUES (generate_series(1,1000000));
> COPY aa TO STDOUT; -- crash
> Which resulted in the following crash:
> 1046 AssertArg(MemoryContextIsValid(context));
> (gdb) bt
> #0 0x0000000000980552 in repalloc (pointer=0x2aa1bb0, size=8192) at mcxt.c:1046
> #1 0x00000000006b75d5 in enlargeStringInfo (str=0x2aa6b70,
> needed=7784) at stringinfo.c:286
> #2 0x00000000006b7447 in appendBinaryStringInfo (str=0x2aa6b70,
> data=0x2b40f25
> "\337\321\261\026jy\352\334#\275l)\030\021s\235\f;\237\336\222PZsd\025>ioS\313`9C\375\a\340Z\354E\355\235\276y\307)D\261\344$D\347\323\036\177S\265\374\373\332~\264\377\317\375<\017\330\214P\a\237\321\375\002$=6\326\263\265{\237\344\214\344.W\303\216\373\206\325\257E\223N\t\324\223\030\363\252&\374\241T\322<\343,\233\203\320\252\343\344\f\036*\274\311\066\206\n\337\300\320L,>-A\016D\346\263pv+A>y\324\254k\003)\264\212zc\344\n\223\224\211\243\"\224\343\241Q\264\233\223\303\"\b\275\026%\302\352\065]8\207\244\304\353\220p\364\272\240\307\247l\216}N\325\aUO6\322\352\273"...,
> datalen=7783) at stringinfo.c:213
> #3 0x00000000006c8359 in be_gssapi_write (port=0x2aa31d0,
> ptr=0x2aa8b48, len=8192) at be-secure-gssapi.c:158
> #4 0x00000000006b9697 in secure_write (port=0x2aa31d0, ptr=0x2aa8b48,
> len=8192) at be-secure.c:248
>
> Which is actually related to your use of StringInfoData, pointing out
> that the buffer handling should be reworked.

This crash is not deterministic. I do observe it though and will try to
debug. Any tips on debugging this are appreciated.

> 2) server enforces encryption, client does not ask for it: encryption
> is enforced. Perhaps a FATAL on backend would be more adapted to let
> the caller know the incompatibility? I am afraid of cases where the
> client intentionally does *not* want encryption because of the
> overhead that it creates at low-level to encrypt/decrypt the messages.
> We'd lose control of that with this patch, and encryption can get
> really a lot slower..

This is really late in the process to bring this up, but alright, let's
talk about it. I think about this in a manner similar to HTTPS: if
support for it is there, you don't want to have to have people think
about using it, you want that security by default. Wherever possible,
it should just happen.

The overhead of reasonable encryption is in general quite low, even more
so since Kerberos will be using symmetric cryptography (typically AES at
the time of writing, which specifically has CPU support in many cases).
Even running on something on lower-power hardware, you will typically
notice other problems (especially since we're working on a performant
database right now) before you notice performance impact due to
encryption.

To back that up, here's an article from a Google employee:
https://www.imperialviolet.org/2010/06/25/overclocking-ssl.html
To summarize, when they turned HTTPS on by default for GMail in 2010, no
new hardware was required and the performance impact was strongly
negligible.

> -#if defined(WIN32) && !defined(WIN32_ONLY_COMPILER)
> -/*
> - * MIT Kerberos GSSAPI DLL doesn't properly export the symbols for MingW
> - * that contain the OIDs required. Redefine here, values copied
> - * from src/athena/auth/krb5/src/lib/gssapi/generic/gssapi_generic.c
> - */
> -static const gss_OID_desc GSS_C_NT_USER_NAME_desc =
> -{10, (void *) "\x2a\x86\x48\x86\xf7\x12\x01\x02\x01\x02"};
> -static GSS_DLLIMP gss_OID GSS_C_NT_USER_NAME = &GSS_C_NT_USER_NAME_desc;
> -#endif
> What makes you say that it is fine to remove that? If that's actually
> the case, we should remove it as part of a separate patch.

Please see earlier comments on David's review. I feel that part of a
"GSSAPI auth cleanup" patch it is in the right place.

Robbie Harwood wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> > + iov[0].iov_base = lenbuf;
> > + iov[0].iov_len = 4;
> > + iov[1].iov_base = output.value;
> > + iov[1].iov_len = output.length;
> > +
> > + ret = writev(port->sock, iov, 2);
> >
> > writev and iovec are not present on Windows, so this code would never
> > compile there, and it does not sound that this patch is a reason
> > sufficient enough to drop support of GSSAPI on Windows.
>
> Um. Okay. I guess on Windows I'll make two write calls then, since the
> only other option I see is to hit alloc again here.

Hmm, I wouldn't push my luck by using writev here at all. We don't use
writev/readv anywhere, and it's quite possible that they are not present
on older Unixen which we still support.
http://pubs.opengroup.org/onlinepubs/009695399/functions/writev.html
says writev was introduced in "issue 4 version 2", which AFAICT is the
2004 version, but our baseline is SUSv2 (1997). So it's definitely not
workable.

> > + {
> > + {"gss_encrypt", PGC_USERSET, CONN_AUTH_SECURITY,
> > + gettext_noop("Require encryption for all GSSAPI connections."),
> > + NULL,
> > + GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
> > + },
> > + &gss_encrypt, false, check_gss_encrypt, assign_gss_encrypt, NULL
> > + },

Why is this marked NOT_IN_SAMPLE?

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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:

> Robbie Harwood wrote:
>> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>
>> > + iov[0].iov_base = lenbuf;
>> > + iov[0].iov_len = 4;
>> > + iov[1].iov_base = output.value;
>> > + iov[1].iov_len = output.length;
>> > +
>> > + ret = writev(port->sock, iov, 2);
>> >
>> > writev and iovec are not present on Windows, so this code would never
>> > compile there, and it does not sound that this patch is a reason
>> > sufficient enough to drop support of GSSAPI on Windows.
>>
>> Um. Okay. I guess on Windows I'll make two write calls then, since the
>> only other option I see is to hit alloc again here.
>
> Hmm, I wouldn't push my luck by using writev here at all. We don't use
> writev/readv anywhere, and it's quite possible that they are not present
> on older Unixen which we still support.
> http://pubs.opengroup.org/onlinepubs/009695399/functions/writev.html
> says writev was introduced in "issue 4 version 2", which AFAICT is the
> 2004 version, but our baseline is SUSv2 (1997). So it's definitely not
> workable.

Understood. What do you suggest instead? To give some context here,
writev() is being used here because I have a GSSAPI payload that is
(effectively) opaque and need to include length in it. The only
alternatives I can see are either allocating a new buffer and reading
the payload + length into it (incurs additional memory overhead), or
calling a write/send function twice (incurs syscall overhead at
minimum).

>> > + {
>> > + {"gss_encrypt", PGC_USERSET, CONN_AUTH_SECURITY,
>> > + gettext_noop("Require encryption for all GSSAPI connections."),
>> > + NULL,
>> > + GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
>> > + },
>> > + &gss_encrypt, false, check_gss_encrypt, assign_gss_encrypt, NULL
>> > + },
>
> Why is this marked NOT_IN_SAMPLE?

Well, because it's not something that's supposed to be set in the file
(and indeed, you can't set it there, if I understand
GUC_DISALLOW_IN_FILE). It's used only as a connection parameter, and I
use its presence / absence for the client and server to negotiate GSSAPI
encryption support.

Robbie Harwood <rharwood(at)redhat(dot)com> writes:
>>> + {
>>> + {"gss_encrypt", PGC_USERSET, CONN_AUTH_SECURITY,
>>> + gettext_noop("Require encryption for all GSSAPI connections."),
>>> + NULL,
>>> + GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
>>> + },
>>> + &gss_encrypt, false, check_gss_encrypt, assign_gss_encrypt, NULL
>>> + },

>> Why is this marked NOT_IN_SAMPLE?

> Well, because it's not something that's supposed to be set in the file
> (and indeed, you can't set it there, if I understand
> GUC_DISALLOW_IN_FILE). It's used only as a connection parameter, and I
> use its presence / absence for the client and server to negotiate GSSAPI
> encryption support.

If that's what it is, it seems fairly broken to have it connected up to a
GUC variable. Especially one that's USERSET; some people will wonder why
frobbing it with SET does nothing, and others will bitch that they think
it should be superuser-only or some such. I'd keep it localized to the
connection logic, myself. There's already logic in ProcessStartupPacket
for connection options that aren't GUC variables, so I'd suggest adding
another case there instead of pretending this is a settable GUC variable.

regards, tom lane

On Fri, Apr 1, 2016 at 6:10 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> If that's what it is, it seems fairly broken to have it connected up to a
> GUC variable. Especially one that's USERSET; some people will wonder why
> frobbing it with SET does nothing, and others will bitch that they think
> it should be superuser-only or some such. I'd keep it localized to the
> connection logic, myself. There's already logic in ProcessStartupPacket
> for connection options that aren't GUC variables, so I'd suggest adding
> another case there instead of pretending this is a settable GUC variable.

Argh, yes right. That's what we should look for.
--
Michael

Hello friends,

Here is another version of GSSAPI encryption support, both on this email
and on my github:
https://github.com/frozencemetery/postgres/tree/feature/gssencrypt10

This version is intended to address Michael's review:

- Fixed Windows build. Thanks to Michael for patches.

- Fixed buffering of large replies on the serverside. This should fix
the traceback that was being seen. The issue had to do with the
difference between the server and client calling conventions for the
_read and _write functions.

- gss_enc_require renamed to gss_require_encrypt. Slightly longer, but
half the stomach churn.

- Move gss_encrypt out of the GUCs and into connection-specific logic.
Thanks to Tom Lane for pointing me in the right direction here.

- Replace writev() with two calls to _raw_write(). I'm not attached to
this design; if someone has a preference for allocating a buffer and
making a single write from that, I could be persuaded. I don't know
what the performance tradeoffs are.

- Typo fixes. I need to figure out spellchecking in my editor.

- More use of <acronym>.

- Change _should_crypto() functions to return bool. Also rename them to
be the _should_encrypt functions.

- Error message cleanup.

Thanks!

On Fri, Apr 1, 2016 at 12:31 PM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> - Fixed Windows build. Thanks to Michael for patches.

This looks fine. I am not seeing build failures now.

> - Fixed buffering of large replies on the serverside. This should fix
> the traceback that was being seen. The issue had to do with the
> difference between the server and client calling conventions for the
> _read and _write functions.

This does not fix the issue. I am still able to crash the server with
the same trace.

> - Move gss_encrypt out of the GUCs and into connection-specific logic.
> Thanks to Tom Lane for pointing me in the right direction here.

+ /* delay processing until after AUTH_REQ_OK has been sent */
+ if (port->gss->gss_encrypt != NULL)
+ port->gss->encrypt = !strcmp(port->gss->gss_encrypt, "on");
You should use parse_bool here, because contrary to libpq, clients
should be able to use other values like "1", "0", "off", 'N', 'Y',
etc.

> - Replace writev() with two calls to _raw_write(). I'm not attached to
> this design; if someone has a preference for allocating a buffer and
> making a single write from that, I could be persuaded. I don't know
> what the performance tradeoffs are.

Relying on pqsecure_raw_write and pqsecure_raw_read is a better bet
IMO, there is already some low-level error handling.

static ConfigVariable *ProcessConfigFileInternal(GucContext context,
bool applySettings, int elevel);

-
/*
Spurious noise.
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> On Fri, Apr 1, 2016 at 12:31 PM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>
>> - Fixed buffering of large replies on the serverside. This should fix
>> the traceback that was being seen. The issue had to do with the
>> difference between the server and client calling conventions for the
>> _read and _write functions.
>
> This does not fix the issue. I am still able to crash the server with
> the same trace.

Interesting. I guess I'll keep trying to reproduce this.

>> - Move gss_encrypt out of the GUCs and into connection-specific logic.
>> Thanks to Tom Lane for pointing me in the right direction here.
>
> + /* delay processing until after AUTH_REQ_OK has been sent */
> + if (port->gss->gss_encrypt != NULL)
> + port->gss->encrypt = !strcmp(port->gss->gss_encrypt, "on");
>
> You should use parse_bool here, because contrary to libpq, clients
> should be able to use other values like "1", "0", "off", 'N', 'Y',
> etc.

Missed that function somehow. Will fix.

>> - Replace writev() with two calls to _raw_write(). I'm not attached to
>> this design; if someone has a preference for allocating a buffer and
>> making a single write from that, I could be persuaded. I don't know
>> what the performance tradeoffs are.
>
> Relying on pqsecure_raw_write and pqsecure_raw_read is a better bet
> IMO, there is already some low-level error handling.

Indeed, it looks like it should especially lead to better behavior on
Windows.

> static ConfigVariable *ProcessConfigFileInternal(GucContext context,
> bool applySettings, int elevel);
>
> -
> /*
>
> Spurious noise.

Indeed, will fix.

Hello friends,

Song and dance, here's v11 both here and on my github:
https://github.com/frozencemetery/postgres/tree/feature/gssencrypt11

Changes from v10:

- Attempt to address a crash Michael is observing by switching to using
the StringInfo/pqExpBuffer management functions over my own code as
much as possible. Michael, if this doesn't fix it, I'm out of ideas.
Since I still can't reproduce this locally (left a client machine and
a process on the same machine retrying for over an hour on your test
case and didn't see it), could you provide me with some more
information on why repalloc is complaining? Is this a low memory
situation where alloc might have failed? What's your setup look like?
That pointer looks like it's on the heap, is that correct? I really
don't have a handle on what's gone wrong here.

- Switch to using parse_bool for handling gss_encrypt.

- Remove accidental whitespace change.

Thanks!

On Sat, Apr 2, 2016 at 7:34 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> - Attempt to address a crash Michael is observing by switching to using
> the StringInfo/pqExpBuffer management functions over my own code as
> much as possible. Michael, if this doesn't fix it, I'm out of ideas.

Nope, it doesn't.

> Since I still can't reproduce this locally (left a client machine and
> a process on the same machine retrying for over an hour on your test
> case and didn't see it), could you provide me with some more
> information on why repalloc is complaining?
> Is this a low memory situation where alloc might have failed?

No, this is an assertion failure, and it seems that you are compiling
this code without --enable-cassert, without the switch the code
actually works.

> What's your setup look like?

Just a simple Linux VM running krb5kdc with 386MB of memory, with
Postgres running locally as well.

> That pointer looks like it's on the heap, is that correct?

appendBinaryStringInfo depends on palloc calls that allocate memory
depending on the memory context used. It looks that what's just
missing in your logic is a private memory context that be_gssapi_write
and be_gssapi_read can use to handle the allocation of the
communication buffers.
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> On Sat, Apr 2, 2016 at 7:34 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>
>> Since I still can't reproduce this locally (left a client machine and
>> a process on the same machine retrying for over an hour on your test
>> case and didn't see it), could you provide me with some more
>> information on why repalloc is complaining?
>> Is this a low memory situation where alloc might have failed?
>
> No, this is an assertion failure, and it seems that you are compiling
> this code without --enable-cassert, without the switch the code
> actually works.

You are right. I now see the assertion failure.

>> That pointer looks like it's on the heap, is that correct?
>
> appendBinaryStringInfo depends on palloc calls that allocate memory
> depending on the memory context used. It looks that what's just
> missing in your logic is a private memory context that be_gssapi_write
> and be_gssapi_read can use to handle the allocation of the
> communication buffers.

Thank you very much for the pointer! I will work in memory context
management for the next version.

Hello friends,

Here's v12, both here and on my github:
https://github.com/frozencemetery/postgres/tree/feature/gssencrypt12

What changed:

- The code is aware of memory contexts now. I actually really like the
memory context stuff; just didn't see any indication of its existence
in the code I had read. Anyway, we allocate server buffers in the
connection-lifetime context. The other alternative that we discussed
on IRC a bit was to avoid palloc()/pfree() entirely in favor of raw
calloc()/free(), but I think if possible I prefer this approach since
I find the StringInfo handy to work with. This eliminates the
traceback for me with --enable-cassert.

- Error cleanup. I've been looking very hard at this code in order to
try to fix the assert, and I've fixed up a couple error paths that
hadn't been taken. This involves replacing the double-send with a
buffer-and-then-send, which turns out to be not only shorter but
easier for me to reason about.

Thanks!

On Tue, Apr 5, 2016 at 9:06 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> Here's v12, both here and on my github:
> https://github.com/frozencemetery/postgres/tree/feature/gssencrypt12
>
> What changed:
>
> - The code is aware of memory contexts now. I actually really like the
> memory context stuff; just didn't see any indication of its existence
> in the code I had read. Anyway, we allocate server buffers in the
> connection-lifetime context. The other alternative that we discussed
> on IRC a bit was to avoid palloc()/pfree() entirely in favor of raw
> calloc()/free(), but I think if possible I prefer this approach since
> I find the StringInfo handy to work with. This eliminates the
> traceback for me with --enable-cassert.

Affirnative, I am not seeing the failure anymore.

+#ifdef ENABLE_GSS
+ {
+ MemoryContext save = CurrentMemoryContext;
+ CurrentMemoryContext = TopMemoryContext;
+
+ initStringInfo(&port->gss->buf);
+ initStringInfo(&port->gss->writebuf);
+
+ CurrentMemoryContext = save;
+ }
+#endif
So you are saving everything in the top memory context. I am fine to
give the last word to a committer here but I would just go with
calloc/free to simplify those hunks.

+#ifdef ENABLE_GSS
+ if (conn->gss->buf.data)
+ pfree(conn->gss->buf.data);
+ if (conn->gss->writebuf.data)
+ pfree(conn->gss->writebuf.data);
+#endif
This should happen in its own memory context, no

> - Error cleanup. I've been looking very hard at this code in order to
> try to fix the assert, and I've fixed up a couple error paths that
> hadn't been taken. This involves replacing the double-send with a
> buffer-and-then-send, which turns out to be not only shorter but
> easier for me to reason about.

@@ -775,6 +775,7 @@ infodir
docdir
oldincludedir
includedir
+runstatedir
localstatedir
sharedstatedir
sysconfdir
@@ -896,6 +897,7 @@ datadir='${datarootdir}'
sysconfdir='${prefix}/etc'
sharedstatedir='${prefix}/com'
localstatedir='${prefix}/var'
+runstatedir='${localstatedir}/run'
What's that? I would recommend re-running autoconf to remove this
portion (committers do it at the end btw, so that's actually no bug
deal).

-#if defined(WIN32) && !defined(WIN32_ONLY_COMPILER)
-/*
- * MIT Kerberos GSSAPI DLL doesn't properly export the symbols for MingW
- * that contain the OIDs required. Redefine here, values copied
- * from src/athena/auth/krb5/src/lib/gssapi/generic/gssapi_generic.c
- */
-static const gss_OID_desc GSS_C_NT_USER_NAME_desc =
-{10, (void *) "\x2a\x86\x48\x86\xf7\x12\x01\x02\x01\x02"};
-static GSS_DLLIMP gss_OID GSS_C_NT_USER_NAME = &GSS_C_NT_USER_NAME_desc;
-#endif
Regarding patch 0003 it may be fine to remove that... Robbie, do you
know how long ago this has been fixed upstream? I'd rather not have
this bit removed if this could impact some users.

On 0003, +1 for reading the whole stack of messages. That's definitely
worth a separate patch.

Btw, those seem like small things to me, and my comments have been
addressed, so I have switched the patch as ready for committer. I
guess that Stephen would be the one to look at it.
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> On Tue, Apr 5, 2016 at 9:06 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>> Here's v12, both here and on my github:
>> https://github.com/frozencemetery/postgres/tree/feature/gssencrypt12
>>
> +#ifdef ENABLE_GSS
> + {
> + MemoryContext save = CurrentMemoryContext;
> + CurrentMemoryContext = TopMemoryContext;
> +
> + initStringInfo(&port->gss->buf);
> + initStringInfo(&port->gss->writebuf);
> +
> + CurrentMemoryContext = save;
> + }
> +#endif
>
> So you are saving everything in the top memory context. I am fine to
> give the last word to a committer here but I would just go with
> calloc/free to simplify those hunks.

Yeah, it's definitely worth thinking/talking about; this came up in IRC
discussion as well.

If we go the memory context route, it has to be TopMemoryContext since
nothing else lives long enough (i.e., entire connection).

On the other hand, if we go with calloc/free here, I can't use
StringInfo for these buffers because StringInfo uses palloc. Using
StringInfo is really handy; if I don't use StringInfo, I'd need to
reimplement enlargeStringInfo() for the ad-hoc buffer structure. What
I'm trying to articulate is that it'd simplify the initialization code
since it removes MemoryContext management, but it makes everything else
more complicated.

I prefer the StringInfo approach since I think the abstraction is a good
one, but if you've got a case for explicit calloc()/free() in light of
that, I'd be interested to hear it.

> +#ifdef ENABLE_GSS
> + if (conn->gss->buf.data)
> + pfree(conn->gss->buf.data);
> + if (conn->gss->writebuf.data)
> + pfree(conn->gss->writebuf.data);
> +#endif
>
> This should happen in its own memory context, no

It turns out that's not actually required, but could easily be made
explicit here. According to the README for the memory context system,
pfree() and repalloc() do not require setting CurrentMemoryContext
(since 7.1).

I'm not opposed to making this one explicit if you think it adds
clarity. I would not want to make all the calls to StringInfo functions
explicit just because they can call repalloc, though.

> @@ -775,6 +775,7 @@ infodir
> docdir
> oldincludedir
> includedir
> +runstatedir
> localstatedir
> sharedstatedir
> sysconfdir
> @@ -896,6 +897,7 @@ datadir='${datarootdir}'
> sysconfdir='${prefix}/etc'
> sharedstatedir='${prefix}/com'
> localstatedir='${prefix}/var'
> +runstatedir='${localstatedir}/run'
> What's that? I would recommend re-running autoconf to remove this
> portion (committers do it at the end btw, so that's actually no bug
> deal).