Re: [PATCH v8] GSSAPI encryption support

David Steele <david(at)pgmasters(dot)net> writes:

> On 3/20/16 12:09 AM, Robbie Harwood wrote:
>
>> A new version of my GSSAPI encryption patchset is available
>
> Here's a more thorough review:

Thanks for the review! To keep this a manageable size, I'm going to
trim pretty heavily. If I haven't replied to something, please
interpret it to mean that I think it's a good suggestion and will
fix/change in v9.

> +++ b/doc/src/sgml/runtime.sgml
>
> I think you mean postgresql.conf?

Sorry, what does this review comment go with?

> +++ b/src/backend/libpq/auth.c
>
> + * In most cases, we do not need to send AUTH_REQ_OK until we are ready
> + * for queries, but if we are doing GSSAPI encryption that request must go
> + * out now.
>
> Why?

Because otherwise we will send the connection spew (connection
parameters and such) unencrypted, since it will get grouped with the
AUTH_REQ_OK packet. I'll note this in the comment.

> + ret = be_gssapi_should_crypto(port);
>
> Add LF here.
>
>
> + port->gss->writebuf.cursor += ret;
>
> And here.
>
> + /* need to be called again */
>
> And here. Well, you get the idea.

Sorry, what is LF? ASCII newline?

> * PATCH 3 - GSSAPI authentication cleanup
>
> +++ b/src/backend/libpq/auth.c
>
> + * GSS_C_REPLAY_FLAG and GSS_C_SEQUENCE_FLAG are missing for compatability
> + * with older clients and should be added in as soon as possible.
>
> Please elaborate here. Why should they be added and what functionality
> is missing before they are.

It's request reply detection and out-of-sequence detection. We can't
require them because old clients don't request them, and so would be
unable to connect. (There's some history of this in the last couple
versions I've posted, but it's not really interesting beyond "it doesn't
work".) I will clarify this comment.

> +++ b/src/backend/libpq/be-gssapi-common.c
>
> -#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
>
> Can you explain why it's OK to remove this now? I can see you've
> replaced it in gss_init_sec_context() with GSS_C_MUTUAL_FLAG. Have you
> tested this on Win32?

This comment is old enough that it references sources from Athena. If
this is in fact a current krb5 bug (which I doubt, since MIT tests
windows rather heavily), then it should be filed against krb5 instead of
kludged around here. I do not however have win32 machines to test this
with. (GSS_C_MUTUAL_FLAG is unrelated; it just requests that the client
and server are both authenticated to each other.)

Thanks,
--Robbie