Date: Fri, 19 Apr 2019 21:25:14 -0400
From: Stephen Frost <sfrost@snowman.net>
To: Michael Paquier <michael@paquier.xyz>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
	Magnus Hagander <magnus@hagander.net>,
	Joe Conway <mail@joeconway.com>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	David Steele <david@pgmasters.net>,
	Nico Williams <nico@cryptonector.com>,
	Robbie Harwood <rharwood@redhat.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH v20] GSSAPI encryption support
Message-ID: <20190420012514.GS6197@tamriel.snowman.net>
References: <7d0584ab-26a8-6731-aeee-9c8e195533e6@2ndquadrant.com>
 <20190405124803.GO6197@tamriel.snowman.net>
 <f56cacac-0693-23aa-353d-179adbe32f2e@2ndquadrant.com>
 <20190405213745.GR6197@tamriel.snowman.net>
 <160445e8-2ccf-7ffd-728f-80597c248a57@2ndquadrant.com>
 <20190409025114.GW6197@tamriel.snowman.net>
 <5bd413ae-719d-4db4-b25a-346f22054930@2ndquadrant.com>
 <8416d227-6e9d-092a-4475-b453e1d7d433@2ndquadrant.com>
 <20190415122452.GC6197@tamriel.snowman.net>
 <20190416043641.GE2673@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="y0/F5YZDTTPsdQ85"
Content-Disposition: inline
In-Reply-To: <20190416043641.GE2673@paquier.xyz>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--y0/F5YZDTTPsdQ85
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Mon, Apr 15, 2019 at 08:24:52AM -0400, Stephen Frost wrote:
> > The tests are really fast enough with one KDC that I don't think it
> > makes sense to have two independent tests.
>=20
> Perhaps you should add a comment about the need of unicity at the top
> of 001_auth.pl with a short description of the test?

I added some comments there that I think explain why it makes sense to
have just one test file there.

> > Please find attached a patch which updates the protocol.sgml docs that
> > Michael mentioned before, and merges the tests into one test file (while
> > adding in some additional tests to make sure that the server also agrees
> > with what our expectations are, using the pg_stat_gssapi view).
>=20
> Thanks for addressing all that feedback.  Parallel runs look more
> stable on my side.  At least it seems that I can re-enable it safely.

Great, glad to hear it.

> > I'll push this soon unless there are concerns.  If you get a chance to
> > test the patch out, that would be great.  It's working happily for me
> > locally.
>=20
> +    calling gss_init_sec_context() in a loop and sending the result to t=
he
> Some markups should be added here for all function names.  Not all the
> clients use C either, so you may want to say "or equivalent"?

I added the markups for function names along with a sentence fragment
saying that the functions referenced are the C GSSAPI bindings, and that
equivilants can be used.

> +test_access($node, 'test1', 'SELECT gss_authenticated AND encrypted
> from pg_stat_gssapi where pid =3D pg_backend_pid();', 0, '', 'succeeds
> with mapping with default gssencmode and host hba');
> +test_access($node, "test1", 'SELECT gss_authenticated AND encrypted
> from pg_stat_gssapi where pid =3D pg_backend_pid();', 0,
> "gssencmode=3Dprefer", "succeeds with GSS-encrypted access preferred
> with host hba");
> +test_access($node, "test1", 'SELECT gss_authenticated AND encrypted
> from pg_stat_gssapi where pid =3D pg_backend_pid();', 0,
> "gssencmode=3Drequire", "succeeds with GSS-encrypted access required
> with host hba");
> If you could rework a bit the indentation of the new code added in
> kerberos/t/001_auth.pl that would be nice.  I am afraid that the
> current format makes debugging harder than necessary.

I ran perltidy on it, sorry, should have done that before.

> +$node->append_conf('pg_hba.conf',
> +   qq{hostgssenc all all $hostaddr/32 gss map=3Dmymap});
> +$node->restart;
> A reload should be enough but not race-condition free, which is why a
> set of restarts is done in this test right?  (I have noticed that it
> is done this way since the beginning.)

Right, we want this to be a restart as Peter mentions downthread.

I've now pushed these changes and will mark the open item as addressed.

Thanks!

Stephen

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

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

iQIcBAEBCgAGBQJcunT5AAoJEO1sijiDR2RVIuYP/1/zgZ0DyAWLNxe2WcZSCsey
iNvDWL0wlWHDK+8IBe+F3MPfNln3HVim2QbUAYjoAICTgLVUdCr1r2Qx087jjOh8
nO/FGT8mzzEnTD6LtkWBSc0zHBLMgCgWcZUGHJ2vdykLuLKAGoyhZUxwbk4lj34S
fhQyax8/q3h+Di5BaPKvgyWfJKAw5brh+mvY6tW8Yu45Hnh3kZai0GJXzd9BaMd+
00P+fdAny0Mw8ISedjrXREfVEW1FqzxJ+VuGYFQvOOyd/iqDJUc0d4hOKxKf6HXT
Q0/k0Bl/yTUacPBZG/eeg7xsXULRGBCXYa+mAWUnBwtm8/74IVbdXzp0gkig2S9T
rLlLhfXceizcVE/o22UgqIiIQ9aVe5GFQduTk/6s1KnOobk37PFO0UbcIMd1Wn+B
UvA+tJVeBcUpPCBk8CW8visvxYFOVajawGbHl4pbmuRJUh3OLsggXOSARmpab907
4aUxTIl+67zYmJCGv8VGUVrIX0strlq3qYFt14Z4ExwPN2qQ2bSlMqKywcQDFeva
6hC4RsltkhdZpxddnzjURkWjCM4ETMsh0DdZH5c+JsXDgJF9ZXHNT52U6RkLcQ42
Q1C0QZrS1vxYeQxSYEMq/jDdMRtq3n81m2MJGHFLVSDrZOuK3+8XxjGrBPJbIp1P
JmPqZadl2L7/sZI1BJe9
=wOsy
-----END PGP SIGNATURE-----

--y0/F5YZDTTPsdQ85--