Re: [PATCH v9] GSSAPI encryption support

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

> 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
>>>
>> 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.

Thank you for the patch to fix 0001. There is basically no way I would
have been able to make it work on Windows myself.

> + 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.

Um. Okay. I guess on Windows I'll make two write calls then, since the
only other option I see is to hit alloc again here.

> + {
> + {"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.

Negative, setting PGC_POSTMASTER breaks the fallback support; I get

"psql: FATAL: parameter "gss_encrypt" cannot be changed without
restarting the server"

when trying to connect a new client to a new server.

I will add documentation.

> + /*
> + * 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'm confused by what you're saying here.

1. If you have a 9.6 client and a 9.5 server, with no additional
connection parameters the client will reconnect and get an
unencrypted connection. This is the "fallback" support.

2. If the client passes gss_enc_require=1, then it will just fail
because the server cannot provide encryption.

> 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.

This crash is not deterministic. I do observe it though and will try to
debug. Any tips on debugging this are appreciated.

> 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..

This is really late in the process to bring this up, but alright, let's
talk about it. I think about this in a manner similar to HTTPS: if
support for it is there, you don't want to have to have people think
about using it, you want that security by default. Wherever possible,
it should just happen.

The overhead of reasonable encryption is in general quite low, even more
so since Kerberos will be using symmetric cryptography (typically AES at
the time of writing, which specifically has CPU support in many cases).
Even running on something on lower-power hardware, you will typically
notice other problems (especially since we're working on a performant
database right now) before you notice performance impact due to
encryption.

To back that up, here's an article from a Google employee:
https://www.imperialviolet.org/2010/06/25/overclocking-ssl.html
To summarize, when they turned HTTPS on by default for GMail in 2010, no
new hardware was required and the performance impact was strongly
negligible.

> -#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.

Please see earlier comments on David's review. I feel that part of a
"GSSAPI auth cleanup" patch it is in the right place.