From: Robbie Harwood <rharwood@redhat.com>
To: Stephen Frost <sfrost@snowman.net>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
 pgsql-hackers@postgresql.org, Nico Williams <nico@cryptonector.com>,
 Michael Paquier <michael@paquier.xyz>,
 Alvaro Herrera <alvherre@2ndquadrant.com>, David Steele <david@pgmasters.net>
Subject: Re: [PATCH v20] GSSAPI encryption support
In-Reply-To: <20190316102110.GU6197@tamriel.snowman.net>
References: <jlg1tgq1ktm.fsf@thriss.redhat.com> <jlgy38mir81.fsf@redhat.com>
 <5ec3f734-810e-36e0-030f-af92efe135da@2ndquadrant.com>
 <20190223162751.GO6197@tamriel.snowman.net> <jlg4l8gpsgk.fsf@redhat.com>
 <20190316102110.GU6197@tamriel.snowman.net>
Date: Fri, 22 Mar 2019 16:26:32 -0400
Message-ID: <jlgftre3bbb.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha512; protocol="application/pgp-signature"
Precedence: bulk

--=-=-=
Content-Type: text/plain

Stephen Frost <sfrost@snowman.net> writes:

> One of the things that I really didn't care for in this patch was the
> use of the string buffers, without any real checks (except for "oh,
> you tried to allocated over 1G"...) to make sure that the other side
> of the connection wasn't feeding us ridiculous packets, and with the
> resizing of the buffers, etc, that really shouldn't be necessary.
> After chatting with Robbie about these concerns while reading through
> the code, we agreed that we should be able to use fixed buffer sizes
> and use the quite helpful gss_wrap_size_limit() to figure out how much
> data we can encrypt without going over our fixed buffer size.  As
> Robbie didn't have time to implement those changes this past week, I
> did so, and added a bunch more comments and such too, and have now
> gone through and done more testing.  Robbie has said that he should
> have time this upcoming week to review the changes that I made, and
> I'm planning to go back and review other parts of the patch more
> closely now as well.

In general this looks good - there are a couple minor comments inline,
but it's fine.

I wanted to note a couple things about this approach.  It now uses one
more buffer than before (in contrast to the previous approach, which
reused a buffer for received data that was encrypted and decrypted).
Since these are static fixed size buffers, this increases the total
steady-state memory usage by 16k as opposed to re-using the buffer.
This may be fine; I don't know how tight RAM is here.

> Note that there's an issue with exporting the context to get the
> encryption algorithm used that I've asked Robbie to look into, so
> that's no longer done and instead we just print that the connection is
> encrypted, if it is.  If we can't figure out a way to make that work
> then obviously I'll pull out that code, and if we can get it to work
> then I'll update it to be done through libpq properly, as I had
> suggested earlier.  That's really more of a nice to have in any case
> though, so I may just exclude it for now anyway if it ends up adding
> any complications.

Correct.  Unfortunately I'd overlooked that the lucid interface won't
meet our needs (destroys the context).  So the two options here are:
SASL SSF (and I'll separately push more mechs to add support for that),
or nothing at all.

If you want a patch for that I can make one, but I think there was code
already... just needed a ./configure check program for whether the OID
is defined.

> +ssize_t
> +be_gssapi_write(Port *port, void *ptr, size_t len)
> +{
> +	size_t		bytes_to_encrypt = len;
> +	size_t		bytes_encrypted = 0;
> +
> +	/*
> +	 * Loop through encrypting data and sending it out until
> +	 * secure_raw_write() complains (which would likely mean that the socket
> +	 * is non-blocking and the requested send() would block, or there was some
> +	 * kind of actual error) and then return.
> +	 */
> +	while (bytes_to_encrypt || PqGSSSendPointer)
> +	{

I guess it's not a view everyone will share, but I think this block is
too long.  Maybe a helper function around secure_raw_write() would help?
(The check-and-send part at the start.)

I have similar nits about the other functions that don't fit on my
(86-line high) screen, though I guess a lot of this is due to project
style using a lot of vertical space.

> +	if (GSS_ERROR(major))
> +		pg_GSS_error(FATAL, gettext_noop("GSSAPI context error"),

I'd prefer this to be a different error message than the init/accept
errors - maybe something like "GSSAPI size check error"?

> +		if (GSS_ERROR(major))
> +			pg_GSS_error(libpq_gettext("GSSAPI context error"), conn,

Same here.

Again, these are nits, and I think I'm okay with the changes.

Thanks,
--Robbie

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

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEA5qc6hnelQjDaHWqJTL5F2qVpEIFAlyVRPkACgkQJTL5F2qV
pEJ/Xg/9G1GR4UI2OmEcwYjKkO0XAYSHMCWWUlbdMSU3JjHjXt3G8nndoKDwvmwx
Rmxl3UWPPprimcTSnCrzi7V4mM7F+G6Qi7F42fQ7QMLQL/IUM2j112Fbglt9EmrQ
c3zzmJBqOTNOS55AVeLVhBATwZP2L7+blQy8MPPbojVOIfvpdeMfXSPFKuiqV4hM
2rL3y8nhGf6ptSf56dErI8HCCoC8YJXm/3T0aIwhpQolSonw74VaeFzuEsLQhpyY
iUpqet6G0ShYFZCAuO0Hjd0wNJtEmVBIlDX0LMO6saaogTZqFRt7CT5mC6NzU78Z
Ir5skg/fYalSnzo5vHvaY6PDTT5nHnsyqK823Z3Chdr+cIpZkhydXh1G5/9M8oLE
hKRcvHbqvKU0dw2LRD7KWIDK+lxuK7CF0Gl1K+kQS+nXBVYOQPRd7BM3C8U2Qy/b
2mxnH3ft4wQUUxqiN+/prRRILZo7ZC5+eIlCnqJCcb0Y51iDfaLlOdLSq+QrOLR/
2rHhxVSNs7+7spOTpNDEsF9AV5pKNpSksgbUw4DrkZH/6UjyA13TON4np7BqeTpK
frGNUNDeyerxeuumdjALEf/AP1HZPTGujEcm3sHjK2puWsaCyULsqp2R5rvXIXDr
JqYyXofZ5bhJL+DGdhTATYwiQffIyyoDXvphBGLkZrtkBh0Drac=
=JKTs
-----END PGP SIGNATURE-----
--=-=-=--