[PATCH v1] GSSAPI encryption support

Robbie,

* Robbie Harwood (rharwood@redhat.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@redhat.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.paquier@gmail.com> writes:

On Fri, Jul 3, 2015 at 3:22 AM, Robbie Harwood <rharwood@redhat.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.paquier@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier <michael.paquier@gmail.com> writes:

On Fri, Jul 3, 2015 at 3:22 AM, Robbie Harwood <rharwood@redhat.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier <michael.paquier@gmail.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@redhat.com> wrote:

Michael Paquier <michael.paquier@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Sep 10, 2015 at 4:27 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Thu, Sep 10, 2015 at 1:44 AM, Robbie Harwood <rharwood@redhat.com> wrote:

Michael Paquier <michael.paquier@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier <michael.paquier@gmail.com> writes:

On Thu, Sep 10, 2015 at 4:27 PM, Michael Paquier <michael.paquier@gmail.com> wrote:

On Thu, Sep 10, 2015 at 1:44 AM, Robbie Harwood <rharwood@redhat.com> wrote:

Michael Paquier <michael.paquier@gmail.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@redhat.com> writes:

Michael Paquier <michael.paquier@gmail.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@redhat.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, Oct 4, 2015 at 1:18 AM, Andres Freund <andres@anarazel.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund <andres@anarazel.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@redhat.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.paquier@gmail.com> writes:

On Sun, Oct 4, 2015 at 1:18 AM, Andres Freund <andres@anarazel.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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@redhat.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 14 October 2015 at 06:34, Robbie Harwood <rharwood@redhat.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers