Обсуждение: longfin missing gssapi_ext.h

Greetings,

Looks like longfin has a particularly old Kerberos/GSSAPI installation
on it which pre-dates MIT release 1.11 from circa 2012 and is missing
gssapi_ext.h, causing the recently committed patch to add Kerberos
credential delegation to fail to build.

I'm inclined to update our configure check to explicitly check for the
needed function (gss_store_cred_into) as no one should really be running
with such an out-dated (over a decade old...) version of MIT Kerberos.

Thoughts?

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> Looks like longfin has a particularly old Kerberos/GSSAPI installation
> on it

It's whatever Apple is shipping, or was shipping last year or so.

> I'm inclined to update our configure check to explicitly check for the
> needed function (gss_store_cred_into)

Sounds like a possible fix, although I wonder whether you shouldn't
explicitly check for the presence of this header.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > Looks like longfin has a particularly old Kerberos/GSSAPI installation
> > on it
>
> It's whatever Apple is shipping, or was shipping last year or so.

Sadly they've not been maintaining the Kerberos libraries at all on
their systems.

> > I'm inclined to update our configure check to explicitly check for the
> > needed function (gss_store_cred_into)
>
> Sounds like a possible fix, although I wonder whether you shouldn't
> explicitly check for the presence of this header.

I'm good with either.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> It's whatever Apple is shipping, or was shipping last year or so.

> Sadly they've not been maintaining the Kerberos libraries at all on
> their systems.

Indeed :-(.  I wouldn't be surprised if there are security issues in
their version.  Perhaps what we really ought to do is refuse to build
with their version --- but if so, we need some clearer error message
about it.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> It's whatever Apple is shipping, or was shipping last year or so.
>
> > Sadly they've not been maintaining the Kerberos libraries at all on
> > their systems.
>
> Indeed :-(.  I wouldn't be surprised if there are security issues in
> their version.  Perhaps what we really ought to do is refuse to build
> with their version --- but if so, we need some clearer error message
> about it.

The attached should (I believe?) at least add the needed check for
gssapi_ext.h which will cause builds to fail and complain about the
header being missing from their installation.

I'm certainly open to ideas about how to provide a better error message,
particularly on OSX systems which have an ancient version, to make it
clear that people need to install an updated version.  I don't have an
OSX system at hand though.

Should I push this to at least address the header check ... ?

Thanks,

Stephen

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > Stephen Frost <sfrost@snowman.net> writes:
> > > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > >> It's whatever Apple is shipping, or was shipping last year or so.
> >
> > > Sadly they've not been maintaining the Kerberos libraries at all on
> > > their systems.
> >
> > Indeed :-(.  I wouldn't be surprised if there are security issues in
> > their version.  Perhaps what we really ought to do is refuse to build
> > with their version --- but if so, we need some clearer error message
> > about it.
>
> The attached should (I believe?) at least add the needed check for
> gssapi_ext.h which will cause builds to fail and complain about the
> header being missing from their installation.
>
> I'm certainly open to ideas about how to provide a better error message,
> particularly on OSX systems which have an ancient version, to make it
> clear that people need to install an updated version.  I don't have an
> OSX system at hand though.
>
> Should I push this to at least address the header check ... ?

Looks like buildfarm animal hake, at least, has a version recent enough
to have gssapi_ext.h ... but still older than 1.11 and therefore
doesn't have the type gss_key_value_element_desc defined, so maybe the
check for gss_store_cred_into would be better?

Certainly interesting how many old kerberos library installations there
are, even in our buildfarm..

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> Looks like buildfarm animal hake, at least, has a version recent enough
> to have gssapi_ext.h ... but still older than 1.11 and therefore
> doesn't have the type gss_key_value_element_desc defined, so maybe the
> check for gss_store_cred_into would be better?

Well, now we're getting into value judgements about which gssapi
versions are still worth supporting.  Are you really willing to toss
overboard all versions that don't support gss_store_cred_into?  Or
should credential delegation be viewed as an incremental feature that
we can support or not?

TBH, committing things with significant portability hazards ten hours
before feature freeze is not high on my list of good development
practices.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > Looks like buildfarm animal hake, at least, has a version recent enough
> > to have gssapi_ext.h ... but still older than 1.11 and therefore
> > doesn't have the type gss_key_value_element_desc defined, so maybe the
> > check for gss_store_cred_into would be better?
>
> Well, now we're getting into value judgements about which gssapi
> versions are still worth supporting.  Are you really willing to toss
> overboard all versions that don't support gss_store_cred_into?  Or
> should credential delegation be viewed as an incremental feature that
> we can support or not?

I'm open to considering support for older versions, however ...

> TBH, committing things with significant portability hazards ten hours
> before feature freeze is not high on my list of good development
> practices.

but as pointed out, these APIs are all over a decade old and systems
which don't support them have a pretty high risk of having security
issues due to shipping these out-dated libraries.

I agree it's a value judgement and something to consider but I don't see
Apple changing their mind any time soon on actually updating the
Kerberos version they ship and no one should really be using what they
do ship.  The same is true for any other system that's shipping a
version of a core security library that's not been updated in over a
decade.

We are currently requiring at least OpenSSL 1.0.1 which was released in
2012.  Having a similar requirement for MIT Kerberos, for our release of
PG in 2023, doesn't strike me as unreasonable.

Attached is a more fully-formed patch with a regenerated configure that
adds in a check for gssapi_ext.h and updates the function check to look
for gss_store_cred_into().

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> I'm open to considering support for older versions, however ...

NetBSD 9.3, which is their *latest production release*, doesn't have
gssapi_ext.h [1].  For that matter, it doesn't look like their
not-yet-released 10.0BETA does either (my NetBSD 10 animals would
be failing if they had --with-gssapi).  I do not think it's going
to be acceptable to require this feature.

I'm now going to reiterate my opinion that this patch should not
have been pushed in at this point of the dev cycle.  A month ago,
there was time to deal with these sorts of issues.

            regards, tom lane

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sidewinder&dt=2023-04-08%2002%3A38%3A31

Hi,

On 2023-04-07 22:50:18 -0400, Tom Lane wrote:
> Or should credential delegation be viewed as an incremental feature that we
> can support or not?

That seems like the best way forward here.

Greetings,

Andres Freund

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > I'm open to considering support for older versions, however ...
>
> NetBSD 9.3, which is their *latest production release*, doesn't have
> gssapi_ext.h [1].  For that matter, it doesn't look like their
> not-yet-released 10.0BETA does either (my NetBSD 10 animals would
> be failing if they had --with-gssapi).  I do not think it's going
> to be acceptable to require this feature.

I'm certainly curious to understand what Kerberos library they're using
and how they're maintaining it.  At least some of the documentation I've
found seems to indicate that it might be Heimdal, which does have
gss_store_cred_into in gssapi.h.

> I'm now going to reiterate my opinion that this patch should not
> have been pushed in at this point of the dev cycle.  A month ago,
> there was time to deal with these sorts of issues.

I suspected there would be an issue with OSX but hadn't expected an
issue with NetBSD.  I had tested this across a few Linux platforms and
cfbot showed it wasn't causing issues on Windows or the platforms that
are run there.  Would be really great to have a way to test these things
out on these other platforms other than just committing them and seeing
what happens on the buildfarm.

In any case, I've reverted it and we can pick this up for the next
cycle.  I'll play around with Heimdal locally as it appears to be
available on Ubuntu (I had actually thought Heimdal to be mostly gone at
this point since it only ever existed really due to silly export
restrictions...) and see if there's actually anything other than making
gssapi_ext.h itself be optional to be pulled in that's needed to make it
work.

Thanks,

Stephen

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2023-04-07 22:50:18 -0400, Tom Lane wrote:
> > Or should credential delegation be viewed as an incremental feature that we
> > can support or not?
>
> That seems like the best way forward here.

Yeah, that's certainly doable too, though I'm really not sure we should
be accepting OSX's GSSAPI library and that might really be the only case
at issue here.  Either way, I've reverted it and will see about picking
it up for the next cycle (again) and hopefully be able to work through
these issues either by having it be optional or, if NetBSD and Heimdal
actually support all the APIs just with different headers, perhaps
deciding we're willing to require them.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> I suspected there would be an issue with OSX but hadn't expected an
> issue with NetBSD.  I had tested this across a few Linux platforms and
> cfbot showed it wasn't causing issues on Windows or the platforms that
> are run there.  Would be really great to have a way to test these things
> out on these other platforms other than just committing them and seeing
> what happens on the buildfarm.

I poked around a bit more and found that:

* NetBSD's package collection[1] includes both Heimdal and MIT Kerberos
(mit-krb5).  Apparently what's installed on at least some of the buildfarm
animals is the former.

* FreeBSD seems to offer *only* Heimdal [2]; OpenBSD ditto [3].

* I cannot find any sign of either gss_store_cred_into or gssapi_ext.h
in FreeBSD's Heimdal (7.8.0_6).

So it does not look like supporting Heimdal is going to be optional,
and that means the credential delegation feature is going to have
to be optional, or else we need to find some equivalent Heimdal APIs.

I share your feeling that we could probably blow off Apple's built-in
GSSAPI.  MacPorts offers both Heimdal and kerberos5, and I imagine
Homebrew has at least one of them, so Mac people could easily get
hold of newer implementations.  But the BSDen are going to be a
problem.

            regards, tom lane

[1] https://cdn.netbsd.org/pub/pkgsrc/current/pkgsrc/security/index.html
[2] https://ports.freebsd.org/cgi/ports.cgi?query=kerberos&stype=all&sektion=all
[3] https://cdn.openbsd.org/pub/OpenBSD/snapshots/packages/amd64/

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > I suspected there would be an issue with OSX but hadn't expected an
> > issue with NetBSD.  I had tested this across a few Linux platforms and
> > cfbot showed it wasn't causing issues on Windows or the platforms that
> > are run there.  Would be really great to have a way to test these things
> > out on these other platforms other than just committing them and seeing
> > what happens on the buildfarm.
>
> I poked around a bit more and found that:
>
> * NetBSD's package collection[1] includes both Heimdal and MIT Kerberos
> (mit-krb5).  Apparently what's installed on at least some of the buildfarm
> animals is the former.
>
> * FreeBSD seems to offer *only* Heimdal [2]; OpenBSD ditto [3].
>
> * I cannot find any sign of either gss_store_cred_into or gssapi_ext.h
> in FreeBSD's Heimdal (7.8.0_6).
>
> So it does not look like supporting Heimdal is going to be optional,
> and that means the credential delegation feature is going to have
> to be optional, or else we need to find some equivalent Heimdal APIs.

Thanks for doing that digging!

I've been looking too and while Heimdal added gss_store_cred_into in
their development branch 5 years ago[1] (!), it's not made it into an
actual release.  Good that they seem to at least be maintaining it
enough to deal with CVEs, but unfortunately I'm fairly confident that
there won't be a way to support constrained delegation (which is the
next goal, once unconstrained delegation is in and working) on the
Heimdal platforms.  I suspected that would have to be optional anyway,
but I hadn't expected it to hit all the BSD platforms.

In any case, for this I'm working switching over to gss_store_cred()
which does seem to be available in the Heimdal Debian packages that I
was able to install locally (looks to be 7.7.0) and should work just
fine for these purposes, though it requires a bit more work on the
libpq side as we need to tell libpq explicitly the name which was on
the delegated credential when we call gss_acquire_cred().

Once that's done, should be able to drop the gssapi_ext.h include
entirely and still have the test suite able to run with MIT Kerberos.

One thing I'm on the fence about is trying to make the test suite
actually work with Heimdal..  I'm planning to install the Heimdal KDC,
et al, and see what happens but if it ends up looking like it's a lot of
work then I might forgo that effort.  I'm not sure it's really necessary
but I could be argued out of that position without too much effort.  The
stated Heimdal goal is to be a re-implementation of MIT Kerberos and
these are all documented APIs with RFCs, after all.

> I share your feeling that we could probably blow off Apple's built-in
> GSSAPI.  MacPorts offers both Heimdal and kerberos5, and I imagine
> Homebrew has at least one of them, so Mac people could easily get
> hold of newer implementations.  But the BSDen are going to be a
> problem.

Yeah.  Unfortunate that Heimdal doesn't seem to really be moving forward
in terms of new development.

Thanks,

Stephen

[1] https://github.com/heimdal/heimdal/commit/e0bb9c10cad0fd98245caecf8af8fca855b2df49

I wrote:
> * NetBSD's package collection[1] includes both Heimdal and MIT Kerberos
> (mit-krb5).  Apparently what's installed on at least some of the buildfarm
> animals is the former.

Oh!  New data: the core NetBSD OS includes a copy of Heimdal (looks
to be 7.7.0 in the 10.0_BETA sources).  The installable package is
a slightly newer version, 7.8.0, but I think it's a very solid bet
that the relevant buildfarm animals are just using the core copy
and haven't installed the add-on package.  Even if they had, it
would take some fooling around with include and link paths to pull
in the packaged version rather than the built-in one.

The exact same thing applies to FreeBSD, except that their in-core
Heimdal is ancient (1.5.2).  Also, they do have MIT Kerberos
available as a package [1].  I'd been misled by the lack of a hit
on "kerberos", but "krb5" finds it.  Our code does compile against
that version of Heimdal, but src/test/kerberos/ refuses to try to
run.

I've not dug into OpenBSD any further.

            regards, tom lane

[1] https://ports.freebsd.org/cgi/ports.cgi?query=krb5&stype=all&sektion=all

On Sun, Apr 9, 2023 at 6:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The exact same thing applies to FreeBSD, except that their in-core
> Heimdal is ancient (1.5.2).  Also, they do have MIT Kerberos
> available as a package [1].  I'd been misled by the lack of a hit
> on "kerberos", but "krb5" finds it.  Our code does compile against
> that version of Heimdal, but src/test/kerberos/ refuses to try to
> run.

FWIW my FBSD animal elver has krb5 installed.  Sorry it wasn't running
when the relevant commit landed.  Stupid network cable wriggled out.

Greetings,

* Thomas Munro (thomas.munro@gmail.com) wrote:
> On Sun, Apr 9, 2023 at 6:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > The exact same thing applies to FreeBSD, except that their in-core
> > Heimdal is ancient (1.5.2).  Also, they do have MIT Kerberos
> > available as a package [1].  I'd been misled by the lack of a hit
> > on "kerberos", but "krb5" finds it.  Our code does compile against
> > that version of Heimdal, but src/test/kerberos/ refuses to try to
> > run.
>
> FWIW my FBSD animal elver has krb5 installed.  Sorry it wasn't running
> when the relevant commit landed.  Stupid network cable wriggled out.

Yeah, I wouldn't be the least bit surprised if many folks running
FreeBSD with any interest in Kerberos have MIT Kerberos installed given
that Heimdal doesn't seem to be under any kind of ongoing active
development and is just in this maintenance mode.

Have you tried running the tests in src/test/kerberos with elver?  Or is
it configured to run them?  Would be awesome if it could be, or if
there's issues with running the tests on FBSD w/ MIT Kerberos, I'd be
happy to try and help work through them.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> Yeah, I wouldn't be the least bit surprised if many folks running
> FreeBSD with any interest in Kerberos have MIT Kerberos installed given
> that Heimdal doesn't seem to be under any kind of ongoing active
> development and is just in this maintenance mode.

Yeah, that's a pretty scary situation for security-critical software.
Maybe we should just desupport Heimdal, rather than investing effort
to the contrary?

Also, the core-code versions of Heimdal in these BSDen are even scarier
than the upstream releases, so I'm thinking that the fact that we
currently compile against them is more a net negative than a positive.
(Same logic as for macOS, really.)

IOW, maybe it'd be okay to de-revert 3d4fa227b and add documentation
saying that --with-gssapi requires MIT Kerberos not Heimdal.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > Yeah, I wouldn't be the least bit surprised if many folks running
> > FreeBSD with any interest in Kerberos have MIT Kerberos installed given
> > that Heimdal doesn't seem to be under any kind of ongoing active
> > development and is just in this maintenance mode.
>
> Yeah, that's a pretty scary situation for security-critical software.

Agreed.

> Maybe we should just desupport Heimdal, rather than investing effort
> to the contrary?

As this is for a new major PG release, I'd be in support of that.  I
would like to get the kerberos tests working on a FreeBSD buildfarm
animal with MIT Kerberos installed, if possible.

> Also, the core-code versions of Heimdal in these BSDen are even scarier
> than the upstream releases, so I'm thinking that the fact that we
> currently compile against them is more a net negative than a positive.
> (Same logic as for macOS, really.)

Agreed.  Still, I wouldn't go and break it for minor releases, but for a
new major version saying we no longer support Heimdal seems reasonable.
Then folks have the usual 5-ish years (if they want to delay as long as
possible) to move to MIT Kerberos.

> IOW, maybe it'd be okay to de-revert 3d4fa227b and add documentation
> saying that --with-gssapi requires MIT Kerberos not Heimdal.

I'd be happy with that and can add the appropriate documentation noting
that we require MIT Kerberos.  Presumably the appropriate step at this
point would be to check with the RMT?

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> IOW, maybe it'd be okay to de-revert 3d4fa227b and add documentation
>> saying that --with-gssapi requires MIT Kerberos not Heimdal.

> I'd be happy with that and can add the appropriate documentation noting
> that we require MIT Kerberos.  Presumably the appropriate step at this
> point would be to check with the RMT?

Yeah, RMT would have final say at this stage.

If you pull the trigger, a note to buildfarm-members would be
appropriate too, so people will know if they need to remove
--with-gssapi from animal configurations.

            regards, tom lane

On 4/10/23 11:37 AM, Tom Lane wrote:
> Stephen Frost <sfrost@snowman.net> writes:
>> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>>> IOW, maybe it'd be okay to de-revert 3d4fa227b and add documentation
>>> saying that --with-gssapi requires MIT Kerberos not Heimdal.
> 
>> I'd be happy with that and can add the appropriate documentation noting
>> that we require MIT Kerberos.  Presumably the appropriate step at this
>> point would be to check with the RMT?
> 
> Yeah, RMT would have final say at this stage.
> 
> If you pull the trigger, a note to buildfarm-members would be
> appropriate too, so people will know if they need to remove
> --with-gssapi from animal configurations.

The RMT discussed this thread and agrees to a "de-revert" of the "Add 
support for Kerberos delegation" patch, provided that:

1. The appropriate documentation is added AND
2. The de-revert occurs no later than 2023-04-15 (upper bound 2023-04-16 
0:00 AoE).

There's an open item[1] for this task.

Thanks,

Jonathan

[1] https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items#Open_Issues

On Tue, Apr 11, 2023 at 2:31 AM Stephen Frost <sfrost@snowman.net> wrote:
> Have you tried running the tests in src/test/kerberos with elver?  Or is
> it configured to run them?  Would be awesome if it could be, or if
> there's issues with running the tests on FBSD w/ MIT Kerberos, I'd be
> happy to try and help work through them.

I'm also happy to test/help/improve the animal/teach CI to do
it/whatever.  I've made a note to test out the reverted commit later
today when I'll be in front of the right computers.

On Tue, Apr 11, 2023 at 2:53 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Tue, Apr 11, 2023 at 2:31 AM Stephen Frost <sfrost@snowman.net> wrote:
> > Have you tried running the tests in src/test/kerberos with elver?  Or is
> > it configured to run them?  Would be awesome if it could be, or if
> > there's issues with running the tests on FBSD w/ MIT Kerberos, I'd be
> > happy to try and help work through them.

Oh, the FreeBSD CI already runs the kerberos test and it uses the krb5
package, because the image it uses installs that[1] and at least the
Meson build automatically prefers that over Heimdal.  So the CI in the
postgres/postgres account tested it[2] as soon as you committed, and
cfbot was testing all along in the commitfest.  It's not skipped and
that test would clearly BAIL_OUT if it detected Heimdal.  Is that good
enough?

I have OpenBSD and NetBSD vagrant images around, do you want me to
test those too?

As for elver, I remembered an unfortunate detail: it doesn't currently
have kerberos enabled in PG_TEXT_EXTRA, because it the test depends on
localhost being 127.0.0.1 which isn't quite true on this box
(container tech with an unusual network stack, long boring story) and
I hadn't got around to figuring out what to do about that.  I can look
into it if you want, or perhaps you are satisfied with CI proving that
FreeBSD likes your patch.

[1] https://github.com/anarazel/pg-vm-images/blob/main/packer/freebsd.pkr.hcl
[2] https://cirrus-ci.com/task/6378179762323456?logs=test_world#L43

Greetings,

* Jonathan S. Katz (jkatz@postgresql.org) wrote:
> On 4/10/23 11:37 AM, Tom Lane wrote:
> > Stephen Frost <sfrost@snowman.net> writes:
> > > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > > > IOW, maybe it'd be okay to de-revert 3d4fa227b and add documentation
> > > > saying that --with-gssapi requires MIT Kerberos not Heimdal.
> >
> > > I'd be happy with that and can add the appropriate documentation noting
> > > that we require MIT Kerberos.  Presumably the appropriate step at this
> > > point would be to check with the RMT?
> >
> > Yeah, RMT would have final say at this stage.
> >
> > If you pull the trigger, a note to buildfarm-members would be
> > appropriate too, so people will know if they need to remove
> > --with-gssapi from animal configurations.
>
> The RMT discussed this thread and agrees to a "de-revert" of the "Add
> support for Kerberos delegation" patch, provided that:
>
> 1. The appropriate documentation is added AND
> 2. The de-revert occurs no later than 2023-04-15 (upper bound 2023-04-16
> 0:00 AoE).
>
> There's an open item[1] for this task.

Understood.  Please find attached the updated patch with changes to the
commit message to indicate that we now require MIT Kerberos, an
additional explicit check for gssapi_ext.h in configure.ac/configure,
along with updated documentation explicitly saying we require MIT
Kerberos for GSSAPI support.

I'll plan to push this tomorrow.

Of course, suggestions/improvements on documentation or anything else
always welcome.

Thanks all!

Stephen

>  configure                                     |  27 ++
>  configure.ac                                  |   2 +

Does meson.build need the corresponding change ?

Greetings,

* Justin Pryzby (pryzby@telsasoft.com) wrote:
> >  configure                                     |  27 ++
> >  configure.ac                                  |   2 +
>
> Does meson.build need the corresponding change ?

Ah, yes, presumably.

Something like the attached?

Thanks,

Stephen

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:
> Greetings,
>
> * Justin Pryzby (pryzby@telsasoft.com) wrote:
> > >  configure                                     |  27 ++
> > >  configure.ac                                  |   2 +
> >
> > Does meson.build need the corresponding change ?
>
> Ah, yes, presumably.

No, more like attached actually.  Picks up on the dependency properly
with this when I ran meson/ninja, at least.

I'll include this then (along with any other suggestions, of course).

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> Understood.  Please find attached the updated patch with changes to the
> commit message to indicate that we now require MIT Kerberos, an
> additional explicit check for gssapi_ext.h in configure.ac/configure,
> along with updated documentation explicitly saying we require MIT
> Kerberos for GSSAPI support.

Um ... could you package this as a straight un-revert of the
previous commit, then a delta patch?  Would be easier to review.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > Understood.  Please find attached the updated patch with changes to the
> > commit message to indicate that we now require MIT Kerberos, an
> > additional explicit check for gssapi_ext.h in configure.ac/configure,
> > along with updated documentation explicitly saying we require MIT
> > Kerberos for GSSAPI support.
>
> Um ... could you package this as a straight un-revert of the
> previous commit, then a delta patch?  Would be easier to review.

Sure, reworked that way and attached.

Thanks,

Stephen

On 4/12/23 10:33 AM, Stephen Frost wrote:
> Greetings,
> 
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Stephen Frost <sfrost@snowman.net> writes:
>>> Understood.  Please find attached the updated patch with changes to the
>>> commit message to indicate that we now require MIT Kerberos, an
>>> additional explicit check for gssapi_ext.h in configure.ac/configure,
>>> along with updated documentation explicitly saying we require MIT
>>> Kerberos for GSSAPI support.
>>
>> Um ... could you package this as a straight un-revert of the
>> previous commit, then a delta patch?  Would be easier to review.
> 
> Sure, reworked that way and attached.

Docs read well. A few questions/commenets:

* On [1] -- do we want to add a note that it's not just Kerberos, but 
MIT Kerberos?

* On [2] -- we mention "kadmin tool of MIT-compatible Kerberos 5" which 
is AIUI is still technically correct, but do we want to drop the 
"-compatible?" (precedent in [3])

Thanks,

Jonathan

[1] https://www.postgresql.org/docs/devel/install-requirements.html
[2] https://www.postgresql.org/docs/devel/gssapi-auth.html
[3] https://www.postgresql.org/docs/devel/regress-run.html

> On 12 Apr 2023, at 16:33, Stephen Frost <sfrost@snowman.net> wrote:

> Sure, reworked that way and attached.

While not changed in this hunk, does the comment regarding Heimdal still apply?

@@ -918,6 +919,7 @@ pg_GSS_recvauth(Port *port)
     int            mtype;
     StringInfoData buf;
     gss_buffer_desc gbuf;
+    gss_cred_id_t delegated_creds;

     /*
      * Use the configured keytab, if there is one.  Unfortunately, Heimdal

--
Daniel Gustafsson

Greetings,

* Jonathan S. Katz (jkatz@postgresql.org) wrote:
> On 4/12/23 10:33 AM, Stephen Frost wrote:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > > Stephen Frost <sfrost@snowman.net> writes:
> > > > Understood.  Please find attached the updated patch with changes to the
> > > > commit message to indicate that we now require MIT Kerberos, an
> > > > additional explicit check for gssapi_ext.h in configure.ac/configure,
> > > > along with updated documentation explicitly saying we require MIT
> > > > Kerberos for GSSAPI support.
> > >
> > > Um ... could you package this as a straight un-revert of the
> > > previous commit, then a delta patch?  Would be easier to review.
> >
> > Sure, reworked that way and attached.
>
> Docs read well. A few questions/commenets:
>
> * On [1] -- do we want to add a note that it's not just Kerberos, but MIT
> Kerberos?

Yes, makes sense, updated.

> * On [2] -- we mention "kadmin tool of MIT-compatible Kerberos 5" which is
> AIUI is still technically correct, but do we want to drop the "-compatible?"
> (precedent in [3])

Yup, cleaned that up also.

Updated patch set attached.

Thanks!

Stephen

On 4/12/23 10:47 AM, Stephen Frost wrote:
> Greetings,
> 
> * Jonathan S. Katz (jkatz@postgresql.org) wrote:
>> On 4/12/23 10:33 AM, Stephen Frost wrote:
>>> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>>>> Stephen Frost <sfrost@snowman.net> writes:
>>>>> Understood.  Please find attached the updated patch with changes to the
>>>>> commit message to indicate that we now require MIT Kerberos, an
>>>>> additional explicit check for gssapi_ext.h in configure.ac/configure,
>>>>> along with updated documentation explicitly saying we require MIT
>>>>> Kerberos for GSSAPI support.
>>>>
>>>> Um ... could you package this as a straight un-revert of the
>>>> previous commit, then a delta patch?  Would be easier to review.
>>>
>>> Sure, reworked that way and attached.
>>
>> Docs read well. A few questions/commenets:
>>
>> * On [1] -- do we want to add a note that it's not just Kerberos, but MIT
>> Kerberos?
> 
> Yes, makes sense, updated.
> 
>> * On [2] -- we mention "kadmin tool of MIT-compatible Kerberos 5" which is
>> AIUI is still technically correct, but do we want to drop the "-compatible?"
>> (precedent in [3])
> 
> Yup, cleaned that up also.
> 
> Updated patch set attached.

Thanks! I'll sign off on the docs portion.

The meson build code looks good to me (I just compared it to what 
already exists). Similar comment to the autoconf code.

Thanks,

Jonathan

Greetings,

* Daniel Gustafsson (daniel@yesql.se) wrote:
> > On 12 Apr 2023, at 16:33, Stephen Frost <sfrost@snowman.net> wrote:
> > Sure, reworked that way and attached.
>
> While not changed in this hunk, does the comment regarding Heimdal still apply?
>
> @@ -918,6 +919,7 @@ pg_GSS_recvauth(Port *port)
>      int            mtype;
>      StringInfoData buf;
>      gss_buffer_desc gbuf;
> +    gss_cred_id_t delegated_creds;
>
>      /*
>       * Use the configured keytab, if there is one.  Unfortunately, Heimdal

Good catch.  No, it doesn't.  I'm not anxious to actually change that
code at this point but we could certainly consider changing it in the
future.  I'll update this comment (and the identical one in
secure_open_gssapi) accordingly.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> Updated patch set attached.

LGTM

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > Updated patch set attached.
>
> LGTM

Great, thanks.

I cleaned up the commit messages a bit more and added links to the
discussion.  If there isn't anything more then I'll plan to push these
later today or tomorrow.

Thanks again!

Stephen

> On 12 Apr 2023, at 16:55, Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> * Daniel Gustafsson (daniel@yesql.se) wrote:
>>> On 12 Apr 2023, at 16:33, Stephen Frost <sfrost@snowman.net> wrote:
>>> Sure, reworked that way and attached.
>>
>> While not changed in this hunk, does the comment regarding Heimdal still apply?
>>
>> @@ -918,6 +919,7 @@ pg_GSS_recvauth(Port *port)
>>     int            mtype;
>>     StringInfoData buf;
>>     gss_buffer_desc gbuf;
>> +    gss_cred_id_t delegated_creds;
>>
>>     /*
>>      * Use the configured keytab, if there is one.  Unfortunately, Heimdal
>
> Good catch.  No, it doesn't.  I'm not anxious to actually change that
> code at this point but we could certainly consider changing it in the
> future.  I'll update this comment (and the identical one in
> secure_open_gssapi) accordingly.

Sounds like a good plan.

--
Daniel Gustafsson

On 4/12/23 12:22 PM, Stephen Frost wrote:
> Greetings,
> 
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Stephen Frost <sfrost@snowman.net> writes:
>>> Updated patch set attached.
>>
>> LGTM
> 
> Great, thanks.
> 
> I cleaned up the commit messages a bit more and added links to the
> discussion.  If there isn't anything more then I'll plan to push these
> later today or tomorrow.

Great -- thanks for your attention to this. I'm glad we have an 
opportunity to de-revert (devert?).

Jonathan

Greetings,

* Jonathan S. Katz (jkatz@postgresql.org) wrote:
> On 4/12/23 12:22 PM, Stephen Frost wrote:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > > Stephen Frost <sfrost@snowman.net> writes:
> > > > Updated patch set attached.
> > >
> > > LGTM
> >
> > Great, thanks.
> >
> > I cleaned up the commit messages a bit more and added links to the
> > discussion.  If there isn't anything more then I'll plan to push these
> > later today or tomorrow.
>
> Great -- thanks for your attention to this. I'm glad we have an opportunity
> to de-revert (devert?).

Pushed, thanks again to everyone.

I'll monitor the buildfarm and assuming there isn't anything unexpected
then I'll mark the open item as resolved now.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> Pushed, thanks again to everyone.
> I'll monitor the buildfarm and assuming there isn't anything unexpected
> then I'll mark the open item as resolved now.

The Debian 7 (Wheezy) members of the buildfarm (lapwing, skate, snapper)
are all getting past the gssapi_ext.h check you added and then failing
like this:

ccache gcc -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla
-Wendif-labels-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g
-O2-Werror -I../../../src/include  -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS -D_GNU_SOURCE -I/usr/include/libxml2
-I/usr/include/et -c -o be-gssapi-common.o be-gssapi-common.c 
be-gssapi-common.c: In function 'pg_store_delegated_credential':
be-gssapi-common.c:110:2: error: unknown type name 'gss_key_value_element_desc'
be-gssapi-common.c:111:2: error: unknown type name 'gss_key_value_set_desc'
be-gssapi-common.c:113:4: error: request for member 'key' in something not a structure or union
be-gssapi-common.c:114:4: error: request for member 'value' in something not a structure or union
be-gssapi-common.c:115:7: error: request for member 'count' in something not a structure or union
be-gssapi-common.c:116:7: error: request for member 'elements' in something not a structure or union
be-gssapi-common.c:119:2: error: implicit declaration of function 'gss_store_cred_into'
[-Werror=implicit-function-declaration]

Debian 7 has been EOL five years or so, so I don't mind saying "get a
newer OS or disable gssapi".  However, is it worth adding another
configure check to fail a little faster with whatever Kerberos
version this is?  Checking that gss_store_cred_into() exists
seems like the most obvious one of these things to test for.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > Pushed, thanks again to everyone.
> > I'll monitor the buildfarm and assuming there isn't anything unexpected
> > then I'll mark the open item as resolved now.
>
> The Debian 7 (Wheezy) members of the buildfarm (lapwing, skate, snapper)
> are all getting past the gssapi_ext.h check you added and then failing
> like this:
>
> ccache gcc -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla
-Wendif-labels-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g
-O2-Werror -I../../../src/include  -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS -D_GNU_SOURCE -I/usr/include/libxml2
-I/usr/include/et -c -o be-gssapi-common.o be-gssapi-common.c 
> be-gssapi-common.c: In function 'pg_store_delegated_credential':
> be-gssapi-common.c:110:2: error: unknown type name 'gss_key_value_element_desc'
> be-gssapi-common.c:111:2: error: unknown type name 'gss_key_value_set_desc'
> be-gssapi-common.c:113:4: error: request for member 'key' in something not a structure or union
> be-gssapi-common.c:114:4: error: request for member 'value' in something not a structure or union
> be-gssapi-common.c:115:7: error: request for member 'count' in something not a structure or union
> be-gssapi-common.c:116:7: error: request for member 'elements' in something not a structure or union
> be-gssapi-common.c:119:2: error: implicit declaration of function 'gss_store_cred_into'
[-Werror=implicit-function-declaration]
>
> Debian 7 has been EOL five years or so, so I don't mind saying "get a
> newer OS or disable gssapi".  However, is it worth adding another
> configure check to fail a little faster with whatever Kerberos
> version this is?  Checking that gss_store_cred_into() exists
> seems like the most obvious one of these things to test for.

Sure, I can certainly do that and agreed that it makes sense to check
for gss_store_cred_into().

How about the attached which just switches from testing for
gss_init_sec_context to testing for gss_store_cred_into?

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> How about the attached which just switches from testing for
> gss_init_sec_context to testing for gss_store_cred_into?

WFM.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > How about the attached which just switches from testing for
> > gss_init_sec_context to testing for gss_store_cred_into?
>
> WFM.

Done that way.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> Done that way.

Looks like you neglected to update the configure script proper?

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > Done that way.
>
> Looks like you neglected to update the configure script proper?

Pah, indeed.  Will fix.  Sorry about that.

Thanks,

Stephen

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > Stephen Frost <sfrost@snowman.net> writes:
> > > Done that way.
> >
> > Looks like you neglected to update the configure script proper?
>
> Pah, indeed.  Will fix.  Sorry about that.

Fixed.

I'm guessing it's not really an issue but it does make changing
configure a bit annoying on my Ubuntu 22.04, when I run autoconf2.69, I
end up with this additional hunk as changed from what our configure
currently has.

Thoughts?

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> I'm guessing it's not really an issue but it does make changing
> configure a bit annoying on my Ubuntu 22.04, when I run autoconf2.69, I
> end up with this additional hunk as changed from what our configure
> currently has.

Not surprising.  Thanks to autoconf's long release cycles, individual
distros often are carrying local patches that affect its output.
To ensure consistent results across committers, our policy is that you
should use built-from-upstream-source autoconf not a vendor's version.
(In principle that could bite us sometime, but it hasn't yet.)

Also, you should generally run autoheader after autoconf.
Checking things here, I notice that pg_config.h.in hasn't been
updated for the last few gssapi-related commits:

$ git diff
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 3665e79..6d572c3 100644
*** a/src/include/pg_config.h.in
--- b/src/include/pg_config.h.in
***************
*** 196,201 ****
--- 196,207 ----
  /* Define to 1 if you have the `getpeerucred' function. */
  #undef HAVE_GETPEERUCRED
  
+ /* Define to 1 if you have the <gssapi_ext.h> header file. */
+ #undef HAVE_GSSAPI_EXT_H
+ 
+ /* Define to 1 if you have the <gssapi/gssapi_ext.h> header file. */
+ #undef HAVE_GSSAPI_GSSAPI_EXT_H
+ 
  /* Define to 1 if you have the <gssapi/gssapi.h> header file. */
  #undef HAVE_GSSAPI_GSSAPI_H
  
Shall I push that, or do you want to?

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > I'm guessing it's not really an issue but it does make changing
> > configure a bit annoying on my Ubuntu 22.04, when I run autoconf2.69, I
> > end up with this additional hunk as changed from what our configure
> > currently has.
>
> Not surprising.  Thanks to autoconf's long release cycles, individual
> distros often are carrying local patches that affect its output.
> To ensure consistent results across committers, our policy is that you
> should use built-from-upstream-source autoconf not a vendor's version.
> (In principle that could bite us sometime, but it hasn't yet.)

... making me more excited about the idea of getting over to meson.

> Also, you should generally run autoheader after autoconf.
> Checking things here, I notice that pg_config.h.in hasn't been
> updated for the last few gssapi-related commits:
>
> $ git diff
> diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
> index 3665e79..6d572c3 100644
> *** a/src/include/pg_config.h.in
> --- b/src/include/pg_config.h.in
> ***************
> *** 196,201 ****
> --- 196,207 ----
>   /* Define to 1 if you have the `getpeerucred' function. */
>   #undef HAVE_GETPEERUCRED
>
> + /* Define to 1 if you have the <gssapi_ext.h> header file. */
> + #undef HAVE_GSSAPI_EXT_H
> +
> + /* Define to 1 if you have the <gssapi/gssapi_ext.h> header file. */
> + #undef HAVE_GSSAPI_GSSAPI_EXT_H
> +
>   /* Define to 1 if you have the <gssapi/gssapi.h> header file. */
>   #undef HAVE_GSSAPI_GSSAPI_H
>
> Shall I push that, or do you want to?

Hrmpf.  Sorry about that.  Please feel free and thanks for pointing it
out.

Thanks again,

Stephen