From: Robbie Harwood <rharwood@redhat.com>
To: Stephen Frost <sfrost@snowman.net>
Cc: pgsql-hackers@postgresql.org
Subject: Re: [PATCH v12] GSSAPI encryption support
In-Reply-To: <20160406112105.GG10850@tamriel.snowman.net>
References: <jlg1tgq1ktm.fsf@thriss.redhat.com>
 <jlgy48ssyvl.fsf@thriss.redhat.com>
 <20160406112105.GG10850@tamriel.snowman.net>
User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/24.5.1
 (x86_64-pc-linux-gnu)
Date: Wed, 06 Apr 2016 15:22:15 -0400
Message-ID: <jlg1t6isftk.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

Stephen Frost <sfrost@snowman.net> writes:

> Just an initial pass over the patch.

Thanks!  In the interest of brevity, if I haven't replied to something,
I plan to fix it.

>>  	/*
>> -	 * 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.  However, if we are doing GSSAPI encryption, that requ=
est
>> +	 * must go out immediately to ensure that all messages which follow the
>> +	 * AUTH_REQ_OK are not grouped with it and can therefore be encrypted.
>>  	 */
>> -	if (areq !=3D AUTH_REQ_OK)
>> +	if (areq !=3D AUTH_REQ_OK || port->gss !=3D NULL)
>>  		pq_flush();
>>=20=20
>>  	CHECK_FOR_INTERRUPTS();
>
> Do we actually need to send pq_flush *whenever* port->gss is not null?
> Shouldn't this actually be port->gss->encrypt?

I need to flush this any time we might be doing encryption because it
needs to be in a separate request to _secure_write() from what follows
it.  We don't know whether we should be doing encryption until
connection parameters are parsed; to put it another way,
port->gss->encrypt will never be true here because it hasn't been parsed
out of port->gss->gss_encrypt yet.

I could parse it earlier, but then I need another variable in the struct
(i.e., to hold whether AUTH_REQ_OK has been sent yet) to check as well.
Additionally, it seemed to me that there might be some value
security-wise in delaying parsing of connection parameters until after
auth is complete, though of course for just a bool this may not be as
important.

>> +	/* recur to send any buffered data */
>> +	gss_release_buffer(&minor, &output);
>> +	return be_gssapi_write(port, ptr, len);
>
> This feels a bit odd to be doing, honestly.  We try to take a lot of
> care to consider low-memory situation and to be careufl when it comes to
> potential for infinite recursion and there's already a way to ask for
> this function to be called again, isn't there?

This call should be okay because (1) it's a tail call and (2) we know
the buffer isn't empty.

That said, the other recursion is excessively complicated and I think
it's simpler to eliminate it entirely in favor of being called again.
I'll see what I can do.

>> +	/* we know the length of the packet at this point */
>> +	memcpy((char *)&input.length, port->gss->buf.data, 4);
>> +	input.length =3D ntohl(input.length);
>> +	enlargeStringInfo(&port->gss->buf, input.length - port->gss->buf.len +=
 4);
>
> I'm aware that enlargeStringInfo() does check and handle the case where
> the length ends up >1G, but that feels a bit grotty to me- are you sure
> you want the generic enlargeStringInfo() to handle that case?

This is a good point.  We definitely have to draw the line somewhere; 1G
is a high upper bound.  Digging around the server code I don't see a
precedent for what's a good size to stop at.  There's
PG_MAX_AUTH_TOKEN_LENGTH, which is 65k, and the password length in
auth.c is 1k.  Beyond that, SocketBackend() calls pq_getmessage() with
no maximum length, which causes the enlargeStringInfo() size
restrictions to be the only control (wrapped in a PG_TRY()).

I think what I'm trying to get at here is that I'm open to suggestion,
but don't see a clearly better way to do this.  We could use the
PG_TRY() approach here to preserve sync, though I'm not sure it's worth
it considering PQ_RECV_BUFFER_SIZE and PQ_SEND_BUFFER_SIZE are both 8k.

>> Subject: [PATCH 3/3] GSSAPI authentication cleanup
>
> Why wouldn't this be part of patch #2?

It's a cleanup of existing code, not my new code, so I thought the
separation would make it easier to understand.  I'm fine with it as part
of #2 so long as it happens, but the impression I had gotten from
earlier reviews was that it should have even more division (i.e., move
the gss_display_status() wrappers into their own patch), not less.

Thanks,
=2D-Robbie

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

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

iQIcBAEBAgAGBQJXBWHnAAoJECUy+RdqlaRCrb0P+wUKuk6urqRLbzo4XUBQ8Lf3
GPOnQ34HHkRUc9xmLneLNotFWN2QpQXI5n7q30iT7fK3cREFWWJsEJMizF00TdMK
oUsVQCj/uhgojSbI9n+sarrfpwLlrHNglqHbDkn7iNTnyiBemhSady4fWbdProLL
/5eMiv1moUQA9PNMAKkMN4RAWf3yMOK/G9pEFGFOjqLHL1X7uaY20FF8xPNt/hKD
uVwv+CH7lK3McqE28eVNI93b2dRXIi08BmeXeP7dA6FcTGOOF6l1DJpW+WgtfYA5
HDqiKhNS+AQyZ2TkDt0o+Kpzk5P90Smc1lbvIfWF0fHOg6kAFaL+NxCqxH7eaAiR
pQ34twUz2hkfl/YqCekKl0eIER0v+Rb2hHujAO1Pe6mmYmE7QQAc2UcILuBKO1n1
rVf6qXp+ozlatYxad+6sOeP7ucz6E79H+s9ZBTJebwflLt44gG5pH1/KzcHu0OxH
cgWejDzNC8nW3+JLj6CJcbtElsDaGXmg70Wn6CHaLJPxx/yFmGkI2I5RGO3uFzTW
ELpS8tDsK+9APeLDNX9seDXV2jvYXAyeONox2VPioD2R2FKQMgGWLDGqMP8Il0cD
VkX1ZePv0DgM18Bo+CBOGEA7W6Gly0FmiEKcONJwgqTZUOwCJMb2bDB2pnc2ybdm
21VfyjkxuHBQw+31Tj5i
=pXr/
-----END PGP SIGNATURE-----
--=-=-=--