Re: [PATCH v12] GSSAPI encryption support

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> On Tue, Apr 5, 2016 at 9:06 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>> Here's v12, both here and on my github:
>> https://github.com/frozencemetery/postgres/tree/feature/gssencrypt12
>>
> +#ifdef ENABLE_GSS
> + {
> + MemoryContext save = CurrentMemoryContext;
> + CurrentMemoryContext = TopMemoryContext;
> +
> + initStringInfo(&port->gss->buf);
> + initStringInfo(&port->gss->writebuf);
> +
> + CurrentMemoryContext = save;
> + }
> +#endif
>
> So you are saving everything in the top memory context. I am fine to
> give the last word to a committer here but I would just go with
> calloc/free to simplify those hunks.

Yeah, it's definitely worth thinking/talking about; this came up in IRC
discussion as well.

If we go the memory context route, it has to be TopMemoryContext since
nothing else lives long enough (i.e., entire connection).

On the other hand, if we go with calloc/free here, I can't use
StringInfo for these buffers because StringInfo uses palloc. Using
StringInfo is really handy; if I don't use StringInfo, I'd need to
reimplement enlargeStringInfo() for the ad-hoc buffer structure. What
I'm trying to articulate is that it'd simplify the initialization code
since it removes MemoryContext management, but it makes everything else
more complicated.

I prefer the StringInfo approach since I think the abstraction is a good
one, but if you've got a case for explicit calloc()/free() in light of
that, I'd be interested to hear it.

> +#ifdef ENABLE_GSS
> + if (conn->gss->buf.data)
> + pfree(conn->gss->buf.data);
> + if (conn->gss->writebuf.data)
> + pfree(conn->gss->writebuf.data);
> +#endif
>
> This should happen in its own memory context, no

It turns out that's not actually required, but could easily be made
explicit here. According to the README for the memory context system,
pfree() and repalloc() do not require setting CurrentMemoryContext
(since 7.1).

I'm not opposed to making this one explicit if you think it adds
clarity. I would not want to make all the calls to StringInfo functions
explicit just because they can call repalloc, though.

> @@ -775,6 +775,7 @@ infodir
> docdir
> oldincludedir
> includedir
> +runstatedir
> localstatedir
> sharedstatedir
> sysconfdir
> @@ -896,6 +897,7 @@ datadir='${datarootdir}'
> sysconfdir='${prefix}/etc'
> sharedstatedir='${prefix}/com'
> localstatedir='${prefix}/var'
> +runstatedir='${localstatedir}/run'
> What's that? I would recommend re-running autoconf to remove this
> portion (committers do it at the end btw, so that's actually no bug
> deal).

Well, I guess /that/ mistake finally happened. This is what happens
when I run autoreconf on my VMs. Because configure is checked in to
version control, I accidentally committed the whole thing instead of
just the GSSAPI changes. I can back that out.

Side note: it's really irritating to work with having this file under
version control because of how different it ends up being when I
autoreconf (which I need to do because I'm changing the build system).
(I'm also idly curious what system/autotools version is generating this
file because it doesn't match any that I tried.)

> -#if defined(WIN32) && !defined(WIN32_ONLY_COMPILER)
> -/*
> - * MIT Kerberos GSSAPI DLL doesn't properly export the symbols for MingW
> - * that contain the OIDs required. Redefine here, values copied
> - * from src/athena/auth/krb5/src/lib/gssapi/generic/gssapi_generic.c
> - */
> -static const gss_OID_desc GSS_C_NT_USER_NAME_desc =
> -{10, (void *) "\x2a\x86\x48\x86\xf7\x12\x01\x02\x01\x02"};
> -static GSS_DLLIMP gss_OID GSS_C_NT_USER_NAME = &GSS_C_NT_USER_NAME_desc;
> -#endif
> Regarding patch 0003 it may be fine to remove that... Robbie, do you
> know how long ago this has been fixed upstream? I'd rather not have
> this bit removed if this could impact some users.

I double-checked with MIT, and we think it was fixed in 2003 in commit
4ce1f7c3a46485e342d3a68b4c60b76c196d1851 which can be viewed at
https://github.com/krb5/krb5/commit/4ce1f7c3a46485e342d3a68b4c60b76c196d1851
and the corresponding bug on their bugtracker was
http://krbdev.mit.edu/rt/Ticket/Display.html?id=1666

In terms of release versions, this was first included in krb5-1.4. For
reference, the oldest Kerberos that MIT upstream supports is 1.12; the
oldest Kerberos that I (maintainer for Red Hat) support in production
(i.e., RHEL-5) is 1.6.1, and even that is slated to go away early 2017.
To my knowledge no one is supporting an older version than that.

In the interest of completion, MIT did express concern that this might
not have been a bug in MIT at all, but rather a toolchain issue. This
workaround has been present since the introduction of GSSAPI auth
support back in 2007. I pinged Magnus on IRC, but I think I missed the
awake window for Sweden, so he's on CC as well.

> On 0003, +1 for reading the whole stack of messages. That's definitely
> worth a separate patch.

Okay! It makes sense to me to move that over.

> Btw, those seem like small things to me, and my comments have been
> addressed, so I have switched the patch as ready for committer. I
> guess that Stephen would be the one to look at it.

Thanks for your comments! This patchset has definitely been much
improved by your input, and I really appreciate that.