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 v22] GSSAPI encryption support
In-Reply-To: <20190330142404.GO6197@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> <jlgftre3bbb.fsf@redhat.com>
 <20190330142404.GO6197@tamriel.snowman.net>
Date: Mon, 01 Apr 2019 11:13:24 -0400
Message-ID: <jlgv9zx4v3f.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
Content-Transfer-Encoding: quoted-printable

Stephen Frost <sfrost@snowman.net> writes:

> * Robbie Harwood (rharwood@redhat.com) wrote:
>> Stephen Frost <sfrost@snowman.net> writes:
>>=20
>> 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).
>
> Yeah, I don't see that as too much of an issue and it certainly seems
> cleaner and simpler to reason about to me, which seems worth the
> modest additional buffer cost.  That's certainly something we could
> change if others feel differently.
>
>> 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.
>
> It seems unlikely to be an issue to me- and I would contend that the
> prior implementation didn't actually take any steps to prevent the
> other side from sending packets of nearly arbitrary size (up to 1G),
> so while the steady-state memory usage of the prior implementation was
> less when everyone was playing nicely, it could have certainly been
> abused.  I'm a lot happier having an explicit cap on how much memory
> will be used, even if that cap is a bit higher.

Yeah, that's definitely a fair point.  We could combine the two
approaches, but I don't really see a reason to if it's unlikely to be an
issue - as you say, this is more readable.  It can always be a follow-on
if needed.

> I did add in a simple pg_stat_gssapi view, modeled on pg_stat_ssl, so
> that you can check server-side if GSSAPI was used for authentication
> and/or encryption, and what principal was used if GSSAPI was used for
> authentication.

Good idea.

>> Again, these are nits, and I think I'm okay with the changes.
>
> Great, thanks again for reviewing!
>
> Updated patch attached, if you could take another look through it,
> that'd be great.

I'm happy with this!  Appreciate you exploring my concerns.

Thanks,
=2D-Robbie

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

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

iQIzBAEBCgAdFiEEA5qc6hnelQjDaHWqJTL5F2qVpEIFAlyiKpQACgkQJTL5F2qV
pEIRUg/+Mj3ieE+pxFureaScBKKlO/cuxePsWMgLtjWNmNwQLypCSDC52/oFOR+h
difKnbhWPkehNHVXaxQ5N6W47L9yt6u2677TnxOCYbpaA9/UXHyJx7kKuBvVv9GU
5wbXFTB4OE4uct+3yM8K4+jSJyH47H4iiJVebaO6+C5waxP4KekrlxBT/2JxnYBn
HSnHzV6BwM73yd3u1ts65nzEFVolUp12hZrPuxKsY3H9UJ+EJbfFDqaJ29T5518B
UjjmPqK03hgoz8wGL/633aFW3Snp+UmXxh4IsAbmkWVywwu3faOl2cSuVwOdGSfx
ZVCuyODvDwJB/ndsViV0uE5OV9caEE0wl9IBhZfZozyaZkO8U0IoVIjxSCTyRQc0
5FOiv/b0T7e1v+pe4XcKctm3jpmDLMIRUJDpz2BNgEIYcz68UyPMca5DB9mjzGOq
hMEUN2hloiY4StNVZkOUpknjJ3RqcsXflNAzA/iJVrOgM81ZWysCRO+8FuWTchrN
JGuMTglewGQhFi9pr19wYjIePIzGF05LFagYyf0cRfi9NtqzOKUQldd+JPwCXUuw
MTXQO6IRSZm6B+iVfp2Acus15ghNcalAu0tYBojRxwX4EE3mVOW2d5RT8BSTl5mJ
iXvSdX7UMOHO3Z795N5HCJDH1LN/tJvQefyoV4IX2yCCaYbcoNc=
=yDHZ
-----END PGP SIGNATURE-----
--=-=-=--