Re: [PATCH v1] GSSAPI encryption support

On Thu, Mar 31, 2016 at 2:14 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Mar 30, 2016 at 1:01 PM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
>> A new version of my GSSAPI encryption patchset is available, both in
>> this email and on my github:
>> https://github.com/frozencemetery/postgres/tree/feature/gssencrypt9
>>
>> This version is intended to address David's review suggestions:
>>
>> - The only code change (not counting SGML and comments) is that I've
>> resolved the pre-existing "XXX: Should we loop and read all messages?"
>> comment in the affirmative by... looping and reading all messages in
>> pg_GSS_error. Though commented on as part of the first patch, this is
>> bundled with the rest of the auth cleanup since the first patch is
>> code motion only.
>>
>> - Several changes to comments, documentation rewordings, and whitespace
>> additions. I look forward to finding out what needs even more of the
>> same treatment. Of all the changesets I've posted thus far, this
>> might be the one for which it makes the most sense to see what changed
>> by diffing from the previous changeset.
>
> Thanks for the new versions. For now I have been having a look at only 0001.
>
> The first thing I have noticed is that 0001 breaks the Windows build
> using Visual Studio. When building without GSSAPI, fe-gssapi-common.c
> and be-gssapi-common.c should be removed from the list of files used
> so that's simple enough to fix. Now when GSSAPI build is enabled,
> there are some declaration conflicts with your patch, leading to many
> compilation errors. This took me some time to figure out but the cause
> is this diff in be-gssapi-common.c:
> +#include "libpq/be-gssapi-common.h"
> +
> +#include "postgres.h"
>
> postgres.h should be declared *before* be-gssapi-common.h. As I
> suggested the refactoring and I guess you don't have a Windows
> environment at hand, attached is a patch that fixes the build with and
> without gssapi for Visual Studio, that can be applied on top of your
> 0001. Feel free to rebase using it.
>
> Note for others: I had as well to patch the MSVC scripts because in
> the newest installations of Kerberos:
> - inc/ does not exist anymore, include/ does
> - The current VS scripts ignore completely x64 builds, the libraries
> linked to are for x86 unconditionally.
> I am not sure if there are *any* people building the code with
> Kerberos on Windows except I, but as things stand those scripts are
> rather broken in my view.

(Lonely feeling after typing that)

> Now looking at 0002 and 0003...

(Thanks for the split btw, this is far easier to look at)

Patch 0002 has the same problems, because it introduces the
gssapi-secure stuff (see attached patch that can be applied on top of
0002 fixing the windows build). When gss build is not enabled,
compilation is able to work, now when gss is enabled... See below.

+ <varlistentry id="libpq-connect-gss-enc-require"
xreflabel="gss_enc_require">
+ <term><literal>gss_enc_require</literal></term>
+ <listitem>
Perhaps renaming that gss_require_encrypt?

+++ b/src/backend/libpq/be-secure-gssapi.c
[...]
+#include "libpq/be-gssapi-common.h"
+
+#include "postgres.h"
Those should be reversed.

+ Require GSSAPI encryption. Defaults to off, which enables GSSAPI
+ encryption only when both available and requested to maintain
+ compatability with old clients. This setting should be enabled unless
+ old clients are present.
s/compatability/compatibility/

+ <para>
+ Require GSSAPI encryption support from the remote server when set. By
+ default, clients will fall back to not using GSSAPI encryption if the
+ server does not support encryption through GSSAPI.
+ </para>
Nitpick: the use of the <acronym> markup for GSSAPI may be more
adapted (I know, the docs are a bit lazy on that).

+ iov[0].iov_base = lenbuf;
+ iov[0].iov_len = 4;
+ iov[1].iov_base = output.value;
+ iov[1].iov_len = output.length;
+
+ ret = writev(port->sock, iov, 2);
writev and iovec are not present on Windows, so this code would never
compile there, and it does not sound that this patch is a reason
sufficient enough to drop support of GSSAPI on Windows.

+static ssize_t
+be_gssapi_should_crypto(Port *port)
+{
+ if (port->gss->ctx == GSS_C_NO_CONTEXT)
+ return 0;
+ return gss_encrypt;
+}
This should return a boolean, gss_encrypt being one.

+ if (!gss_encrypt && port->hba->require_encrypt)
+ {
+ ereport(FATAL, (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
+ errmsg("GSSAPI encryption required from user \"%s\"",
+ port->user_name)));
+ }
Could be clearer here, with some error detail message, like:
"User has requested GSSAPI encryption, but server disallows it".

+static void
+assign_gss_encrypt(bool newval, void *extra)
+{
+ gss_encrypt = newval;
+}
Assigning a variable is done by the GUC machinery, you don't need that.

+ {
+ {"gss_encrypt", PGC_USERSET, CONN_AUTH_SECURITY,
+ gettext_noop("Require encryption for all GSSAPI connections."),
+ NULL,
+ GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+ },
+ &gss_encrypt, false, check_gss_encrypt, assign_gss_encrypt, NULL
+ },
Hm. I think that this would be better as PGC_POSTMASTER, the idea
behind this parameter being that the server enforces any connections
from clients to be encrypted. Clients should not have the right to
disable that at will depending on their session, and this is used in
authentication code paths. Also, this parameter is not documented, the
new parameters in pg_hba.conf and client-side are though.

A consequence of that, is that you would not need the following thingy:
+static bool
+check_gss_encrypt(bool *newval, void **extra, GucSource source)
+{
+ if (MyProcPort && MyProcPort->hba && MyProcPort->hba->require_encrypt &&
+ !*newval)
+ return false;
+ return true;
+}
So I'd suggest go with PGC_POSTMASTER first to simplify the code. We
could always relax that later on as need be, but that's not something
I think the code patch should bother much about.

+/*
+ * Only consider encryption when GSS context is complete
+ */
+ssize_t
+pg_GSS_should_crypto(PGconn *conn)
+{
s/crypto/encrypt?

+ /*
+ * We tried to request GSSAPI encryption, but the
+ * server doesn't support it. Retries are permitted
+ * here, so hang up and try again. A connection that
+ * doesn't support appname will also not support
+ * GSSAPI encryption.
+ */
+ const char *sqlstate;
Hm... I'm having second thoughts regarding gss_enc_require... This
code path would happen with a 9.6 client and a ~9.5 server, it seems a
bit treacherous to not ERROR here and fallback to an unencrypted
connection, client want to have an encrypted connection at the end.

I ran as well some tests, and I was able to crash the server. For
example take this SQL sequence:
CREATE TABLE aa (a int);
INSERT INTO aa VALUES (generate_series(1,1000000));
COPY aa TO STDOUT; -- crash
Which resulted in the following crash:
1046 AssertArg(MemoryContextIsValid(context));
(gdb) bt
#0 0x0000000000980552 in repalloc (pointer=0x2aa1bb0, size=8192) at mcxt.c:1046
#1 0x00000000006b75d5 in enlargeStringInfo (str=0x2aa6b70,
needed=7784) at stringinfo.c:286
#2 0x00000000006b7447 in appendBinaryStringInfo (str=0x2aa6b70,
data=0x2b40f25
"\337\321\261\026jy\352\334#\275l)\030\021s\235\f;\237\336\222PZsd\025>ioS\313`9C\375\a\340Z\354E\355\235\276y\307)D\261\344$D\347\323\036\177S\265\374\373\332~\264\377\317\375<\017\330\214P\a\237\321\375\002$=6\326\263\265{\237\344\214\344.W\303\216\373\206\325\257E\223N\t\324\223\030\363\252&\374\241T\322<\343,\233\203\320\252\343\344\f\036*\274\311\066\206\n\337\300\320L,>-A\016D\346\263pv+A>y\324\254k\003)\264\212zc\344\n\223\224\211\243\"\224\343\241Q\264\233\223\303\"\b\275\026%\302\352\065]8\207\244\304\353\220p\364\272\240\307\247l\216}N\325\aUO6\322\352\273"...,
datalen=7783) at stringinfo.c:213
#3 0x00000000006c8359 in be_gssapi_write (port=0x2aa31d0,
ptr=0x2aa8b48, len=8192) at be-secure-gssapi.c:158
#4 0x00000000006b9697 in secure_write (port=0x2aa31d0, ptr=0x2aa8b48,
len=8192) at be-secure.c:248
Which is actually related to your use of StringInfoData, pointing out
that the buffer handling should be reworked.

Also, to sum up with this patch, things behave like that:
1) server enforces encryption, client asks for encryption: all good,
encryption is done.
2) server enforces encryption, client does not ask for it: encryption
is enforced. Perhaps a FATAL on backend would be more adapted to let
the caller know the incompatibility? I am afraid of cases where the
client intentionally does *not* want encryption because of the
overhead that it creates at low-level to encrypt/decrypt the messages.
We'd lose control of that with this patch, and encryption can get
really a lot slower..
3) server does not enforce encryption, client asks for it: FATAL on
backend side.
4) server does not enforce encryption, client not asking for it, no encryption.

-#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
What makes you say that it is fine to remove that? If that's actually
the case, we should remove it as part of a separate patch.
--
Michael