From: Robbie Harwood <rharwood@redhat.com>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org
Subject: Re: [PATCH v1] GSSAPI encryption support
In-Reply-To: <20151003161810.GD30738@alap3.anarazel.de>
References: <jlg1tgq1ktm.fsf@thriss.redhat.com>
 <20151003161810.GD30738@alap3.anarazel.de>
User-Agent: Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1
 (x86_64-pc-linux-gnu)
Date: Fri, 09 Oct 2015 14:04:43 -0400
Message-ID: <jlgr3l33olg.fsf@thriss.redhat.com>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
 micalg=pgp-sha1; protocol="application/pgp-signature"
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

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 le=
n)
>> +{
>> +	OM_uint32 major, minor;
>> +	gss_buffer_desc input, output;
>> +	uint32 len_n;
>> +	int conf;
>> +	char *ptr =3D *((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 =3D 0;
>> +
>> +	input.length =3D inBuf->len;
>> +	input.value =3D inBuf->data;
>> +	output.length =3D 0;
>> +	output.value =3D NULL;
>> +
>> +	major =3D 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 =3D=3D 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 =3D ((char *)output.value)[0]; /* first byte is message type */
>> +	inBuf->len =3D output.length - 5; /* message starts */
>> +
>> +	memcpy((char *)&msglen, ((char *)output.value) + 1, 4);
>> +	msglen =3D ntohl(msglen);
>> +	if (msglen - 4 !=3D 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, si=
ze_t len)
>>  {
>>  	if (DoingCopyOut || PqCommBusy)
>>  		return 0;
>> +
>> +#ifdef ENABLE_GSS
>> +	/* Do not wrap auth requests. */
>> +	if (MyProcPort->hba->auth_method =3D=3D uaGSS && gss_encrypt &&
>> +		msgtype !=3D 'R' && msgtype !=3D 'g')
>> +	{
>> +		len =3D be_gss_encrypt(MyProcPort, msgtype, &s, len);
>> +		if (len < 0)
>> +			goto fail;
>> +		msgtype =3D '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
>>=20=20
>>  #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;
>>=20=20
>> +#ifdef ENABLE_GSS
>> +extern bool gss_encrypt;
>> +#endif
>
>> --- a/src/backend/utils/misc/guc.c
>> +++ b/src/backend/utils/misc/guc.c
>>=20=20
>> +#ifdef ENABLE_GSS
>> +static void assign_gss_encrypt(bool newval, void *extra);
>> +#endif
>> +
>>=20=20
>>  /*
>>   * Options for enum values defined in this module.
>> @@ -1618,6 +1622,15 @@ static struct config_bool ConfigureNamesBool[] =3D
>>  		NULL, NULL, NULL
>>  	},
>>=20=20
>> +	{
>> +		{"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
>>=20
>> 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(-)
>>=20
>> 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 =3D false;
>>  	}
>> +	else if (strcmp(name, "require_encrypt") =3D=3D 0)
>> +	{
>> +		if (hbaline->auth_method !=3D uaGSS)
>> +			INVALID_AUTH_OPTION("require_encrypt", "gssapi");
>> +		if (strcmp(val, "1") =3D=3D 0)
>> +			hbaline->require_encrypt =3D true;
>> +		else
>> +			hbaline->require_encrypt =3D 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[] =3D
>>  		 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
>>  	},
>>=20=20
>>  	/* End-of-list marker */
>> @@ -10133,4 +10134,13 @@ assign_gss_encrypt(bool newval, void *extra)
>>  	gss_encrypt =3D newval;
>>  }
>>=20=20
>> +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!
=2D-Robbie

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJWGAG9AAoJECUy+RdqlaRC7hsP/ig2DEzRwoqOG/xOziDwsthV
qiXy4M4kNd/BLweEETfVIep/lwSm2ZlIWjsWl3iet03q5SQKqsOhozBYSbykiNQT
FseeTw4IbfcPRLR+/85AwApnfc8IJVZ9YJhIpriJFqJ+CrAvdpmlmD8g3eAAe853
ICojXr4jQzXAbLpZ2X2pHBFn096743F0gJ950WSQykjTcb+2ppuvUrmDC4pf8lLA
IOmzgKGQg8W11e39MVOUjTfFdEyGPiyVo+vff8nex5h+srVtUfdk7RdYaWMLneLO
2kewfxdeOZdh/zKbgZJVJN4OYHA+ynnOgzowSktJydv57y129LrDv3rGSSIrO99g
Nrak6N5hw2LzDfTLiGf4TxbkJWCyczav8LHSx7UTFDNM+EncL/dymDcZTD6M6Hsj
41AXY2Ko4q0qtFP0d5fq4BwW619EETY/oNqMzZvn9rprHN+4immN/tJ2qXYp7Qbj
DINLSpcdH5eAKwjfzc69EpjqFIgrTJqbA1F3Hw9jBhZJo9GY1gwt+lTtK7FNzN8E
EKklXouLZ7OP0ag6ecNk37Lyis8jJi3D3Ovds5zI6rBf1rPQjmrC/WPwGZfQVw1T
vrvkiLqEIFi6lh/RFt7JBoMo4wqSlEL0YJoLjGDeIWNBqT7M/jALvvjFr6xfGEXU
l+GN3srETukVy6lDC2Gu
=G4TR
-----END PGP SIGNATURE-----
--=-=-=--