Re: [HACKERS] [PATCH v9] GSSAPI encryption support

[Michael Paquier]

On Fri, Apr 1, 2016 at 6:10 AM, Tom Lane  wrote:
> If that's what it is, it seems fairly broken to have it connected up to a
> GUC variable.  Especially one that's USERSET; some people will wonder why
> frobbing it with SET does nothing, and others will bitch that they think
> it should be superuser-only or some such.  I'd keep it localized to the
> connection logic, myself.  There's already logic in ProcessStartupPacket
> for connection options that aren't GUC variables, so I'd suggest adding
> another case there instead of pretending this is a settable GUC variable.

Argh, yes right. That's what we should look for.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH v9] GSSAPI encryption support

[Tom Lane]

Robbie Harwood  writes:
>>> +   {
>>> +   {"gss_encrypt", PGC_USERSET, CONN_AUTH_SECURITY,
>>> +gettext_noop("Require encryption for all GSSAPI connections."),
>>> +NULL,
>>> +GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
>>> +   },
>>> +   _encrypt, false, check_gss_encrypt, assign_gss_encrypt, NULL
>>> +   },

>> Why is this marked NOT_IN_SAMPLE?

> Well, because it's not something that's supposed to be set in the file
> (and indeed, you can't set it there, if I understand
> GUC_DISALLOW_IN_FILE).  It's used only as a connection parameter, and I
> use its presence / absence for the client and server to negotiate GSSAPI
> encryption support.

If that's what it is, it seems fairly broken to have it connected up to a
GUC variable.  Especially one that's USERSET; some people will wonder why
frobbing it with SET does nothing, and others will bitch that they think
it should be superuser-only or some such.  I'd keep it localized to the
connection logic, myself.  There's already logic in ProcessStartupPacket
for connection options that aren't GUC variables, so I'd suggest adding
another case there instead of pretending this is a settable GUC variable.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH v9] GSSAPI encryption support

[Robbie Harwood]

Alvaro Herrera  writes:

> Robbie Harwood wrote:
>> Michael Paquier  writes:
>
>> > +   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.
>
> Hmm, I wouldn't push my luck by using writev here at all.  We don't use
> writev/readv anywhere, and it's quite possible that they are not present
> on older Unixen which we still support.
> http://pubs.opengroup.org/onlinepubs/009695399/functions/writev.html
> says writev was introduced in "issue 4 version 2", which AFAICT is the
> 2004 version, but our baseline is SUSv2 (1997).  So it's definitely not
> workable.

Understood.  What do you suggest instead?  To give some context here,
writev() is being used here because I have a GSSAPI payload that is
(effectively) opaque and need to include length in it.  The only
alternatives I can see are either allocating a new buffer and reading
the payload + length into it (incurs additional memory overhead), or
calling a write/send function twice (incurs syscall overhead at
minimum).

>> > +   {
>> > +   {"gss_encrypt", PGC_USERSET, CONN_AUTH_SECURITY,
>> > +gettext_noop("Require encryption for all GSSAPI connections."),
>> > +NULL,
>> > +GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
>> > +   },
>> > +   _encrypt, false, check_gss_encrypt, assign_gss_encrypt, NULL
>> > +   },
>
> Why is this marked NOT_IN_SAMPLE?

Well, because it's not something that's supposed to be set in the file
(and indeed, you can't set it there, if I understand
GUC_DISALLOW_IN_FILE).  It's used only as a connection parameter, and I
use its presence / absence for the client and server to negotiate GSSAPI
encryption support.


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH v9] GSSAPI encryption support

[Alvaro Herrera]

Robbie Harwood wrote:
> Michael Paquier  writes:

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

Hmm, I wouldn't push my luck by using writev here at all.  We don't use
writev/readv anywhere, and it's quite possible that they are not present
on older Unixen which we still support.
http://pubs.opengroup.org/onlinepubs/009695399/functions/writev.html
says writev was introduced in "issue 4 version 2", which AFAICT is the
2004 version, but our baseline is SUSv2 (1997).  So it's definitely not
workable.

> > +   {
> > +   {"gss_encrypt", PGC_USERSET, CONN_AUTH_SECURITY,
> > +gettext_noop("Require encryption for all GSSAPI connections."),
> > +NULL,
> > +GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
> > +   },
> > +   _encrypt, false, check_gss_encrypt, assign_gss_encrypt, NULL
> > +   },

Why is this marked NOT_IN_SAMPLE?

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH v9] GSSAPI encryption support

[Robbie Harwood]

Michael Paquier  writes:

> On Thu, Mar 31, 2016 at 2:14 PM, Michael Paquier
>  wrote:
>> On Wed, Mar 30, 2016 at 1:01 PM, Robbie Harwood  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
> +   },
> +   _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,100));
> COPY aa TO STDOUT; -- crash
> Which resulted in the following crash:
> 1046AssertArg(MemoryContextIsValid(context));
> (gdb) bt
> #0  0x00980552 in repalloc (pointer=0x2aa1bb0, size=8192) at 
> mcxt.c:1046
> #1  0x006b75d5 in enlargeStringInfo (str=0x2aa6b70,
> needed=7784) at stringinfo.c:286
> #2  0x006b7447 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  0x006c8359 in be_gssapi_write (port=0x2aa31d0,
> ptr=0x2aa8b48, len=8192) at be-secure-gssapi.c:158
> #4  0x006b9697 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