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

2015-10-30 Thread Robbie Harwood
Andreas, can you please weigh in here since your voice is important to
this process?

Robbie Harwood  writes:

> Andres Freund  writes:
>
>> On 2015-10-22 16:47:09 +0900, Michael Paquier wrote:
>>> Hm, and that's why you chose this way of going. My main concern about
>>> this patch is that it adds on top of the existing Postgres protocol a
>>> layer to encrypt and decrypt the messages between server and client
>>> based on GSSAPI. All messages transmitted between client and server
>>> are changed to 'g' messages on the fly and switched back to their
>>> original state at reception. This is symbolized by the four routines
>>> you added in the patch in this purpose, two for frontend and two for
>>> backend, each one for encryption and decryption. I may be wrong of
>>> course, but it seems to me that this approach will not survive
>>> committer-level screening because of the fact that context-level
>>> things invade higher level protocol messages.
>>
>> Agreed. At least one committer here indeed thinks this approach is not
>> acceptable (and I've said so upthread).
>
> Okay, I'll make some changes.  Before I do, though, since this is not
> the approach I came up with, can you explicitly state what you're
> looking for here?  It subjectively seems that I'm getting a lot of
> feedback of "this feels wrong" without suggestion for improvement.
>
> To be clear, what I need to know is:
>
> - What changes do you want to see in the wire protocol?  (And how will
>   fallback be supported if that's affected?)
>
> - Since this seems to be an important sticking point, what files am I
>   encouraged to change (or prohibited from changing)?  (Fallback makes
>   this complex.)
>
> - I've been assuming that we care about fallback, but I'd like to be
>   told that it's something postgres actually wants to see because it's
>   the most intricate part of these changes.  (I'm reasonably confident
>   that the code becomes simpler without it, and I myself have no use for
>   it.)
>
> If I understand what you're asking for (and the above is intended to be
> sure that I will), this will not be a trivial rework, so I want to be
> really sure before doing that because writing this code a third time is
> something I don't relish.
>
> Thanks,
> --Robbie


signature.asc
Description: PGP signature


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

2015-10-26 Thread Michael Paquier
On Thu, Oct 22, 2015 at 11:36 PM, Robbie Harwood wrote:
> To be clear, what I need to know is:
> - What changes do you want to see in the wire protocol?  (And how will
>   fallback be supported if that's affected?)

Hm. Something essential will be to send the length of the wrapped
gss_buffer_t object to be sent in the first 4 bytes of the message so
as the receiver can know how much it has to unwrap and can perform
sanity checks on what has been received.

> - Since this seems to be an important sticking point, what files am I
>   encouraged to change (or prohibited from changing)?  (Fallback makes
>   this complex.)

If we want to make that stick into Postgres, I think that we are going
to need be_gss_read and be_gss_write in be-secure.c, and pqgss_write
and pqgss_read in fe-secure.c, the use the context initialized at
authentication time to wrap and unwrap messages between the server and
client.

> - I've been assuming that we care about fallback, but I'd like to be
>   told that it's something postgres actually wants to see because it's
>   the most intricate part of these changes.  (I'm reasonably confident
>   that the code becomes simpler without it, and I myself have no use for
>   it.)

As a first shot for this patch, I would not mind if there is no
fallback at protocol level, it seems to me that it is challenging
enough to get a solid core feature first. Perhaps others have
different opinions?

> If I understand what you're asking for (and the above is intended to be
> sure that I will), this will not be a trivial rework, so I want to be
> really sure before doing that because writing this code a third time is
> something I don't relish.

This makes sense. Let's be sure that we come up with a clear picture
of what to do first.
-- 
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 v3] GSSAPI encryption support

2015-10-22 Thread Andres Freund
On 2015-10-22 16:47:09 +0900, Michael Paquier wrote:
> Hm, and that's why you chose this way of going. My main concern about
> this patch is that it adds on top of the existing Postgres protocol a
> layer to encrypt and decrypt the messages between server and client
> based on GSSAPI. All messages transmitted between client and server
> are changed to 'g' messages on the fly and switched back to their
> original state at reception. This is symbolized by the four routines
> you added in the patch in this purpose, two for frontend and two for
> backend, each one for encryption and decryption. I may be wrong of
> course, but it seems to me that this approach will not survive
> committer-level screening because of the fact that context-level
> things invade higher level protocol messages.

Agreed. At least one committer here indeed thinks this approach is not
acceptable (and I've said so upthread).

Greetings,

Andres Freund


-- 
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 v3] GSSAPI encryption support

2015-10-22 Thread Michael Paquier
On Thu, Oct 22, 2015 at 1:28 AM, Robbie Harwood  wrote:
> Michael Paquier  writes:
>
>> Robbie,
>>
>> +#ifdef ENABLE_GSS
>> +   if (pggss_encrypt(conn) < 0)
>> +   return EOF;
>> +#endif
>>
>> @@ -1528,10 +1541,20 @@ socket_putmessage(char msgtype, const char *s,
>> size_t len)
>> if (internal_putbytes(s, len))
>> goto fail;
>> PqCommBusy = false;
>> +#ifdef ENABLE_GSS
>> +   /* if we're GSSAPI encrypting, s was allocated in be_gss_encrypt */
>> +   if (msgtype == 'g')
>> +   pfree((char *)s);
>> +#endif
>>
>> Looking at this patch in more details... Why is it necessary to wrap
>> all the encrypted messages into a dedicated message type 'g'? Cannot
>> we rely on the context on client and backend that should be set up
>> after authentication to perform the encryption and decryption
>> operations? This patch is enforcing the message type in
>> socket_putmessage for backend and pggss_encrypt/pqPutMsgEnd for
>> frontend, this just feels wrong and I think that the patch could be
>> really simplified, this includes the crafting added in fe-protocol3.c
>> that should be IMO reserved only for messages received from the
>> backend and should not be made aware of any protocol-level logic.
>
> Well, it's not strictly necessary in the general case, but it makes
> debugging a *lot* easier to have it present, and it simplifies some of
> the handling logic.  For instance, in the part quoted above, with
> socket_putmessage() and socket_putmessage_noblock(), we need some way to
> tell whether a message blob has already been encrypted.
> Some enforcement of message type *will need to be carried out* anyway to
> avoid security issues with tampering on the wire, whether that be by
> sanity-checking the stated message type and then handling accordingly,
> or trying to decrypt and detonating the connection if it fails.
> GSSAPI does not define a wire protocol for the transport of messages the
> way, e.g., TLS does, so there must be some handling of incomplete
> messages, multiple messages at once, etc.  There is already adequate
> handling of these present in postgres already, so I have structured the
> code to take advantage of it.  That said, I could absolutely reimplement
> them - but it would not be simpler, I'm reasonably sure.

Hm, and that's why you chose this way of going. My main concern about
this patch is that it adds on top of the existing Postgres protocol a
layer to encrypt and decrypt the messages between server and client
based on GSSAPI. All messages transmitted between client and server
are changed to 'g' messages on the fly and switched back to their
original state at reception. This is symbolized by the four routines
you added in the patch in this purpose, two for frontend and two for
backend, each one for encryption and decryption. I may be wrong of
course, but it seems to me that this approach will not survive
committer-level screening because of the fact that context-level
things invade higher level protocol messages.

IMO, we would live better with something at a lower level, like in
be-secure.c and fe-secure.c to exchange the encrypted and decrypted
packages using the GSSAPI context created at authentication, that's
where the context-level switches are living after all.

So I am thinking that this patch needs a rework, and should be
returned with feedback.
Stephen, others, what do you think?
-- 
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 v3] GSSAPI encryption support

2015-10-22 Thread Michael Paquier
On Thu, Oct 22, 2015 at 6:00 PM, Andres Freund  wrote:
> On 2015-10-22 16:47:09 +0900, Michael Paquier wrote:
>> Hm, and that's why you chose this way of going. My main concern about
>> this patch is that it adds on top of the existing Postgres protocol a
>> layer to encrypt and decrypt the messages between server and client
>> based on GSSAPI. All messages transmitted between client and server
>> are changed to 'g' messages on the fly and switched back to their
>> original state at reception. This is symbolized by the four routines
>> you added in the patch in this purpose, two for frontend and two for
>> backend, each one for encryption and decryption. I may be wrong of
>> course, but it seems to me that this approach will not survive
>> committer-level screening because of the fact that context-level
>> things invade higher level protocol messages.
>
> Agreed. At least one committer here indeed thinks this approach is not
> acceptable (and I've said so upthread).

OK, so marked as returned with feedback.
-- 
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 v3] GSSAPI encryption support

2015-10-22 Thread Robbie Harwood
Andres Freund  writes:

> On 2015-10-22 16:47:09 +0900, Michael Paquier wrote:
>> Hm, and that's why you chose this way of going. My main concern about
>> this patch is that it adds on top of the existing Postgres protocol a
>> layer to encrypt and decrypt the messages between server and client
>> based on GSSAPI. All messages transmitted between client and server
>> are changed to 'g' messages on the fly and switched back to their
>> original state at reception. This is symbolized by the four routines
>> you added in the patch in this purpose, two for frontend and two for
>> backend, each one for encryption and decryption. I may be wrong of
>> course, but it seems to me that this approach will not survive
>> committer-level screening because of the fact that context-level
>> things invade higher level protocol messages.
>
> Agreed. At least one committer here indeed thinks this approach is not
> acceptable (and I've said so upthread).

Okay, I'll make some changes.  Before I do, though, since this is not
the approach I came up with, can you explicitly state what you're
looking for here?  It subjectively seems that I'm getting a lot of
feedback of "this feels wrong" without suggestion for improvement.

To be clear, what I need to know is:

- What changes do you want to see in the wire protocol?  (And how will
  fallback be supported if that's affected?)

- Since this seems to be an important sticking point, what files am I
  encouraged to change (or prohibited from changing)?  (Fallback makes
  this complex.)

- I've been assuming that we care about fallback, but I'd like to be
  told that it's something postgres actually wants to see because it's
  the most intricate part of these changes.  (I'm reasonably confident
  that the code becomes simpler without it, and I myself have no use for
  it.)

If I understand what you're asking for (and the above is intended to be
sure that I will), this will not be a trivial rework, so I want to be
really sure before doing that because writing this code a third time is
something I don't relish.

Thanks,
--Robbie


signature.asc
Description: PGP signature


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

2015-10-21 Thread Robbie Harwood
Michael Paquier  writes:

> Robbie,
>
> +#ifdef ENABLE_GSS
> +   if (pggss_encrypt(conn) < 0)
> +   return EOF;
> +#endif
>
> @@ -1528,10 +1541,20 @@ socket_putmessage(char msgtype, const char *s,
> size_t len)
> if (internal_putbytes(s, len))
> goto fail;
> PqCommBusy = false;
> +#ifdef ENABLE_GSS
> +   /* if we're GSSAPI encrypting, s was allocated in be_gss_encrypt */
> +   if (msgtype == 'g')
> +   pfree((char *)s);
> +#endif
>
> Looking at this patch in more details... Why is it necessary to wrap
> all the encrypted messages into a dedicated message type 'g'? Cannot
> we rely on the context on client and backend that should be set up
> after authentication to perform the encryption and decryption
> operations? This patch is enforcing the message type in
> socket_putmessage for backend and pggss_encrypt/pqPutMsgEnd for
> frontend, this just feels wrong and I think that the patch could be
> really simplified, this includes the crafting added in fe-protocol3.c
> that should be IMO reserved only for messages received from the
> backend and should not be made aware of any protocol-level logic.

Well, it's not strictly necessary in the general case, but it makes
debugging a *lot* easier to have it present, and it simplifies some of
the handling logic.  For instance, in the part quoted above, with
socket_putmessage() and socket_putmessage_noblock(), we need some way to
tell whether a message blob has already been encrypted.

Some enforcement of message type *will need to be carried out* anyway to
avoid security issues with tampering on the wire, whether that be by
sanity-checking the stated message type and then handling accordingly,
or trying to decrypt and detonating the connection if it fails.

GSSAPI does not define a wire protocol for the transport of messages the
way, e.g., TLS does, so there must be some handling of incomplete
messages, multiple messages at once, etc.  There is already adequate
handling of these present in postgres already, so I have structured the
code to take advantage of it.  That said, I could absolutely reimplement
them - but it would not be simpler, I'm reasonably sure.


signature.asc
Description: PGP signature


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

2015-10-21 Thread Robbie Harwood
Michael Paquier  writes:

> On Tue, Oct 20, 2015 at 3:01 AM, Robbie Harwood wrote:
>> Stephen Frost  writes:
>>> psql: lost synchronization with server: got message type "S", length 22
>>
>> which unfortunately could be a great many things.  I've said this a
>> couple times now, but I really do need more information - a traffic
>> dump, a list of commands that were run, etc.; unfortunately, the surface
>> here is pretty large, and while I totally am willing to believe there
>> are bugs in the code I've written, I do not yet see them.
>
> --- a/src/interfaces/libpq/fe-protocol3.c
> +++ b/src/interfaces/libpq/fe-protocol3.c
> @@ -129,6 +129,58 @@ pqParseInput3(PGconn *conn)
> return;
> }
>
> +#ifdef ENABLE_GSS
> +   /* We want to be ready in both IDLE and BUSY states
> for encryption */
> +   if (id == 'g' && !conn->gss_disable_enc && conn->gctx)
> +   {
> +   ssize_t encEnd, next;
> [...]
> +   }
> +   else if (!conn->gss_disable_enc && conn->gss_auth_done &&
> +!conn->gss_decrypted_cur && id != 'E')
> +   /* This could be a sync error, so let's handle
> it as such. */
> +   handleSyncLoss(conn, id, msgLength);
> +#endif
>
> Hm. The out-of-sync error I am seeing in my environment is caused by
> this block when parsing 'g' messages coming from the backend that are
> considered as being GSSAPI-encrypted messages. I am still looking at
> that...

If you're hitting the else-block, that suggests a GSSAPI context is not
present at the time a GSSAPI message was received, I think.


signature.asc
Description: PGP signature


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

2015-10-21 Thread Michael Paquier
On Tue, Oct 20, 2015 at 3:01 AM, Robbie Harwood wrote:
> Stephen Frost  writes:
>> psql: lost synchronization with server: got message type "S", length 22
>
> which unfortunately could be a great many things.  I've said this a
> couple times now, but I really do need more information - a traffic
> dump, a list of commands that were run, etc.; unfortunately, the surface
> here is pretty large, and while I totally am willing to believe there
> are bugs in the code I've written, I do not yet see them.

--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -129,6 +129,58 @@ pqParseInput3(PGconn *conn)
return;
}

+#ifdef ENABLE_GSS
+   /* We want to be ready in both IDLE and BUSY states
for encryption */
+   if (id == 'g' && !conn->gss_disable_enc && conn->gctx)
+   {
+   ssize_t encEnd, next;
[...]
+   }
+   else if (!conn->gss_disable_enc && conn->gss_auth_done &&
+!conn->gss_decrypted_cur && id != 'E')
+   /* This could be a sync error, so let's handle
it as such. */
+   handleSyncLoss(conn, id, msgLength);
+#endif

Hm. The out-of-sync error I am seeing in my environment is caused by
this block when parsing 'g' messages coming from the backend that are
considered as being GSSAPI-encrypted messages. I am still looking at
that...
-- 
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 v3] GSSAPI encryption support

2015-10-21 Thread Michael Paquier
Robbie,

On Wed, Oct 21, 2015 at 3:54 PM, Michael Paquier
 wrote:
> On Tue, Oct 20, 2015 at 3:01 AM, Robbie Harwood wrote:
>> Stephen Frost  writes:
>>> psql: lost synchronization with server: got message type "S", length 22
>>
>> which unfortunately could be a great many things.  I've said this a
>> couple times now, but I really do need more information - a traffic
>> dump, a list of commands that were run, etc.; unfortunately, the surface
>> here is pretty large, and while I totally am willing to believe there
>> are bugs in the code I've written, I do not yet see them.
>
> --- a/src/interfaces/libpq/fe-protocol3.c
> +++ b/src/interfaces/libpq/fe-protocol3.c
> @@ -129,6 +129,58 @@ pqParseInput3(PGconn *conn)
> return;
> }
>
> +#ifdef ENABLE_GSS
> +   /* We want to be ready in both IDLE and BUSY states
> for encryption */
> +   if (id == 'g' && !conn->gss_disable_enc && conn->gctx)
> +   {
> +   ssize_t encEnd, next;
> [...]
> +   }
> +   else if (!conn->gss_disable_enc && conn->gss_auth_done &&
> +!conn->gss_decrypted_cur && id != 'E')
> +   /* This could be a sync error, so let's handle
> it as such. */
> +   handleSyncLoss(conn, id, msgLength);
> +#endif
>
> Hm. The out-of-sync error I am seeing in my environment is caused by
> this block when parsing 'g' messages coming from the backend that are
> considered as being GSSAPI-encrypted messages. I am still looking at
> that...

@@ -604,6 +604,11 @@ pqPutMsgEnd(PGconn *conn)
memcpy(conn->outBuffer + conn->outMsgStart, , 4);
}

+#ifdef ENABLE_GSS
+   if (pggss_encrypt(conn) < 0)
+   return EOF;
+#endif

@@ -1528,10 +1541,20 @@ socket_putmessage(char msgtype, const char *s,
size_t len)
if (internal_putbytes(s, len))
goto fail;
PqCommBusy = false;
+#ifdef ENABLE_GSS
+   /* if we're GSSAPI encrypting, s was allocated in be_gss_encrypt */
+   if (msgtype == 'g')
+   pfree((char *)s);
+#endif

Looking at this patch in more details... Why is it necessary to wrap
all the encrypted messages into a dedicated message type 'g'? Cannot
we rely on the context on client and backend that should be set up
after authentication to perform the encryption and decryption
operations? This patch is enforcing the message type in
socket_putmessage for backend and pggss_encrypt/pqPutMsgEnd for
frontend, this just feels wrong and I think that the patch could be
really simplified, this includes the crafting added in fe-protocol3.c
that should be IMO reserved only for messages received from the
backend and should not be made aware of any protocol-level logic.
-- 
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 v3] GSSAPI encryption support

2015-10-19 Thread Robbie Harwood
Stephen Frost  writes:

> As for this patch, the reason I've not been as involved (beyond being
> ridiculously busy) is that Michael's environment, which at least appears
> perfectly reasonable (and works with PG unpatched) isn't working.  If we
> can get that working (and I've not looked at what's happening, so I have
> no idea how easy or hard that would be), then I'd be a lot more excited
> to spend time doing review of the patch.

I would also really like to see this fixed.  Unfortunately, I haven't
been able to replicate; I followed Michael's (very nicely written)
writeup and didn't see the issues that Michael did.  The only thing I
know about the problem is that it logs:

> psql: lost synchronization with server: got message type "S", length 22

which unfortunately could be a great many things.  I've said this a
couple times now, but I really do need more information - a traffic
dump, a list of commands that were run, etc.; unfortunately, the surface
here is pretty large, and while I totally am willing to believe there
are bugs in the code I've written, I do not yet see them.


signature.asc
Description: PGP signature


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

2015-10-16 Thread Craig Ringer
On 16 October 2015 at 01:07, Robbie Harwood  wrote:

> The short - and probably most important - answer is that no, I haven't
> tested it, and it would be difficult for me to do so quickly.

IIRC it's pretty easy to fire up AWS instances that're primary domain
controllers, and then join a Pg box to them with Kerberos. I realise
that's different to having the time and desire to do it, though.

> Looking at
> http://www.postgresql.org/docs/9.4/static/protocol-message-formats.html
> suggests that SSPI follows a separate codepath from the GSS code;
> certainly it's a different auth request, which means it shouldn't
> trigger the encryption path.

It's a different auth request, but the handling in be-auth.c is
co-mingled to handle the cases:

* We're compiled with Kerberos, handling SSPI from Windows
* We're compiled with Kerberos, handling a GSSAPI request
* We're compiled with SSPI, and we're using Windows SSPI to handle a
GSSAPI auth request
* We're compiled with SSPI, and we're using it to handle a SSP client request

SSPI is a wrapper. For Windows Domain members it carries
GSSAPI/Kerberos data with the spnego extension. (For local loopback it
does NTLM, but you don't need to care about that).

> There is no reason that using GSSAPI from, e.g., MIT Kerberos for
> Windows, should not work here.

Except that *nobody* does it. The EDB installer builds Pg with SSPI,
and that's what the overwhelming majority of Windows users use.

Personally I don't care if this patch doesn't add support for GSSAPI
encryption for sspi connections, only that it doesn't break them.

>  Even more broadly than that, GSSAPI is a
> RFC-standardized protocol with rigorously tested interop etc. etc.,
> though whether anyone outside of MIT cares about MIT Kerberos for
> Windows I have no idea.

It's maintained enough that AFAIK it works, and Pg supports compiling
with it. No idea if anyone tests it with Pg anymore.

> I think the important takeaway here is that I haven't actually changed
> how *authentication* works; these changes only affect GSSAPI
> post-authentication add encryption functions as defined by that
> specification.  So if the authentication was working before, and the
> GSSAPI implementation is following RFC, I would hope that this would
> work still.

Hope so.

I'll put this on my "hope to test it at some stage" list, but I don't
have a prebuilt environment for it and have a lot to get ready for the
next CF, so it'll be a while...


-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, 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 v3] GSSAPI encryption support

2015-10-16 Thread Stephen Frost
* Craig Ringer (cr...@2ndquadrant.com) wrote:
> On 16 October 2015 at 01:07, Robbie Harwood  wrote:
> > Looking at
> > http://www.postgresql.org/docs/9.4/static/protocol-message-formats.html
> > suggests that SSPI follows a separate codepath from the GSS code;
> > certainly it's a different auth request, which means it shouldn't
> > trigger the encryption path.
> 
> It's a different auth request, but the handling in be-auth.c is
> co-mingled to handle the cases:

be-auth.c?  You mean src/backend/libpq/auth.c?

> * We're compiled with Kerberos, handling SSPI from Windows
> * We're compiled with Kerberos, handling a GSSAPI request

Eh, *kerberos* was removed with 9.4.0.  I'm guessing you mean GSSAPI
above.

> * We're compiled with SSPI, and we're using Windows SSPI to handle a
> GSSAPI auth request
> * We're compiled with SSPI, and we're using it to handle a SSP client request
> 
> SSPI is a wrapper. For Windows Domain members it carries
> GSSAPI/Kerberos data with the spnego extension. (For local loopback it
> does NTLM, but you don't need to care about that).

I have to admit that I'm not following what you're getting at here.

src/backend/libpq/auth.c and src/interfaces/libpq/fe-auth.c have pretty
clearly independent paths for SSPI and GSSAPI and which is used
(server-side) is based on what the auth method in pg_hba.conf is.  In a
properly set up environment, the client could be using either, but it's
not like we try to figure out (or care) which the client is using from
the server's perspective.

Now, if you set up GSSAPI on the client side and tell it to require
encryption and you're using SSPI on the server side (or the other way
around) it's not likely to work without changes to the SSPI code paths,
which we should be looking at doing.  I've not looked at the patch in
much depth yet, but hopefully there's a straight-forward way to say
"we're using SSPI on (client/server) and that doesn't support
encryption yet, so don't try to require it" and throw an error if
someone does.  I don't think we necessairly have to have encryption with
SSPI working initially (and that's assuming SSPI can actually do
encryption the way GSSAPI can, I don't know that it can).

> > There is no reason that using GSSAPI from, e.g., MIT Kerberos for
> > Windows, should not work here.
> 
> Except that *nobody* does it. The EDB installer builds Pg with SSPI,
> and that's what the overwhelming majority of Windows users use.

Apparently *somebody* uses MIT KfW; I doubt MIT would have put out
KfW 4.0 if no one wanted it.  I agree that it's clearly a small number
of environments (I don't know of any currently) and it's entirely
possible that none of them use PG with GSSAPI on Windows for
authentication.

> Personally I don't care if this patch doesn't add support for GSSAPI
> encryption for sspi connections, only that it doesn't break them.

Agreed and we should certainly test it, but I'm not seeing what you're
getting at as the concern.

> >  Even more broadly than that, GSSAPI is a
> > RFC-standardized protocol with rigorously tested interop etc. etc.,
> > though whether anyone outside of MIT cares about MIT Kerberos for
> > Windows I have no idea.
> 
> It's maintained enough that AFAIK it works, and Pg supports compiling
> with it. No idea if anyone tests it with Pg anymore.

The EDB installers were changed to use SSPI instead of MIT KfW (or
perhaps just dropped building with GSSAPI, Dave would probably remember
better than I do) because KfW wasn't being maintained or updated any
more and the latest version had known security issues.  That was quite a
while ago and it looks like MIT has decided to revive KfW (note the
drought between about 2007 and 2012 or so though..) with KfW 4.0.

I doubt anyone's tried building or running PG with GSSAPI under Windows
though.  I agree it'd be good to test and see and perhaps even the EDB
installers could be changed to add support for it back in (not sure if
Dave really wants to though as it was a bit of a pain..).  However,
presumably MIT has updated KfW and contains to maintain it because
someone is using it.

Really, though, assuming that GSSAPI as provided by KfW works per the
spec-defined API, I don't see why it wouldn't work under Windows just
the same as it does under *nix, it's not like we have any
Windows-specific code in backend/libpq/auth.c or
interfaces/libpq/fe-auth.c for GSSAPI except a MingW-specific symbol
definition.

> > I think the important takeaway here is that I haven't actually changed
> > how *authentication* works; these changes only affect GSSAPI
> > post-authentication add encryption functions as defined by that
> > specification.  So if the authentication was working before, and the
> > GSSAPI implementation is following RFC, I would hope that this would
> > work still.
> 
> Hope so.
> 
> I'll put this on my "hope to test it at some stage" list, but I don't
> have a prebuilt environment for it and have a lot to get ready for the
> next CF, so 

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

2015-10-16 Thread Craig Ringer
On 16 October 2015 at 21:34, Stephen Frost  wrote:
> * Craig Ringer (cr...@2ndquadrant.com) wrote:
>> On 16 October 2015 at 01:07, Robbie Harwood  wrote:
>> > Looking at
>> > http://www.postgresql.org/docs/9.4/static/protocol-message-formats.html
>> > suggests that SSPI follows a separate codepath from the GSS code;
>> > certainly it's a different auth request, which means it shouldn't
>> > trigger the encryption path.
>>
>> It's a different auth request, but the handling in be-auth.c is
>> co-mingled to handle the cases:
>
> be-auth.c?  You mean src/backend/libpq/auth.c?

Ahem. Yes.

Also, I was actually thinking of the libpq front-end code, as it turns
out. The backend's handling of each method is much better separated.

>> * We're compiled with Kerberos, handling SSPI from Windows
>> * We're compiled with Kerberos, handling a GSSAPI request
>
> Eh, *kerberos* was removed with 9.4.0.  I'm guessing you mean GSSAPI
> above.

Yes, being sloppy.

> src/backend/libpq/auth.c and src/interfaces/libpq/fe-auth.c have pretty
> clearly independent paths for SSPI and GSSAPI

fe-auth.c doesn't. See pg_fe_sendauth(...)'s case AUTH_REQ_GSS: and
AUTH_REQ_SSPI:

Not what I call clearly independent. I've had to deal with that tangle
of joy a few times...

> and which is used
> (server-side) is based on what the auth method in pg_hba.conf is.

Yep, agreed there. I was thinking that the backend overlapped more,
like the frontend does.

>  In a
> properly set up environment, the client could be using either, but it's
> not like we try to figure out (or care) which the client is using from
> the server's perspective.

Exactly. I want to make sure we still don't have to care.

> Now, if you set up GSSAPI on the client side and tell it to require
> encryption and you're using SSPI on the server side (or the other way
> around) it's not likely to work without changes to the SSPI code paths,
> which we should be looking at doing.

So long as setting up gssapi auth on the backend without requiring
encryption still works with sspi clients, like it did before, I'm
happy. My concern is avoiding a regression in what works, not
requiring that the new functionality be supported everywhere.

>> > There is no reason that using GSSAPI from, e.g., MIT Kerberos for
>> > Windows, should not work here.
>>
>> Except that *nobody* does it. The EDB installer builds Pg with SSPI,
>> and that's what the overwhelming majority of Windows users use.
>
> Apparently *somebody* uses MIT KfW

Yeah, I meant nobody uses it with Pg on Windows.

> I doubt anyone's tried building or running PG with GSSAPI under Windows
> though.  I agree it'd be good to test and see and perhaps even the EDB
> installers could be changed to add support for it back in (not sure if
> Dave really wants to though as it was a bit of a pain..).

It's a lot of a pain, and not just because of maintenance. Kerberos on
Windows needs access to various innards that you don't need when just
using SSPI to delegate SSO to the OS. It's less secure, fiddly, and
annoying. At least it was last time I had to deal with it, when I was
working around some SSPI bugs in psqlODBC before landing up patching
them in the driver instead.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, 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 v3] GSSAPI encryption support

2015-10-16 Thread Stephen Frost
* Craig Ringer (cr...@2ndquadrant.com) wrote:
> On 16 October 2015 at 21:34, Stephen Frost  wrote:
> >> It's a different auth request, but the handling in be-auth.c is
> >> co-mingled to handle the cases:
> >
> > be-auth.c?  You mean src/backend/libpq/auth.c?
> 
> Ahem. Yes.

No worries. :)

> Also, I was actually thinking of the libpq front-end code, as it turns
> out. The backend's handling of each method is much better separated.

Ok.

> > src/backend/libpq/auth.c and src/interfaces/libpq/fe-auth.c have pretty
> > clearly independent paths for SSPI and GSSAPI
> 
> fe-auth.c doesn't. See pg_fe_sendauth(...)'s case AUTH_REQ_GSS: and
> AUTH_REQ_SSPI:
> 
> Not what I call clearly independent. I've had to deal with that tangle
> of joy a few times...

Hmm, yea, the actual choice of how to respond to the auth method the
server wants us to use is kind of tricky, but once we've settled on one
or the other, the rest of the code is pretty independent.

Still, I agree with your point that we need to be careful to not break
what we have there by, say, trying to move to encrypted GSSAPI with an
SSPI server.

I'm not sure that I want to really change that fe-auth.c code but I
really think there may be a simpler way to have the same behavior.  That
mishmash of #ifdef's and swith/case statements strikes me as possibly
being overly cute.  Clear blocks of "we got this message, this is what
we do when we're compiled with X, or Y, or X+Y" would probably be a lot
easier to follow.

> >  In a
> > properly set up environment, the client could be using either, but it's
> > not like we try to figure out (or care) which the client is using from
> > the server's perspective.
> 
> Exactly. I want to make sure we still don't have to care.

Agreed.

> > Now, if you set up GSSAPI on the client side and tell it to require
> > encryption and you're using SSPI on the server side (or the other way
> > around) it's not likely to work without changes to the SSPI code paths,
> > which we should be looking at doing.
> 
> So long as setting up gssapi auth on the backend without requiring
> encryption still works with sspi clients, like it did before, I'm
> happy. My concern is avoiding a regression in what works, not
> requiring that the new functionality be supported everywhere.

Great, we're definitely agreed on that.

> >> > There is no reason that using GSSAPI from, e.g., MIT Kerberos for
> >> > Windows, should not work here.
> >>
> >> Except that *nobody* does it. The EDB installer builds Pg with SSPI,
> >> and that's what the overwhelming majority of Windows users use.
> >
> > Apparently *somebody* uses MIT KfW
> 
> Yeah, I meant nobody uses it with Pg on Windows.

It'd be nice if it did, in case any of those users *do* end up wanting
to use PG.

> > I doubt anyone's tried building or running PG with GSSAPI under Windows
> > though.  I agree it'd be good to test and see and perhaps even the EDB
> > installers could be changed to add support for it back in (not sure if
> > Dave really wants to though as it was a bit of a pain..).
> 
> It's a lot of a pain, and not just because of maintenance. Kerberos on
> Windows needs access to various innards that you don't need when just
> using SSPI to delegate SSO to the OS. It's less secure, fiddly, and
> annoying. At least it was last time I had to deal with it, when I was
> working around some SSPI bugs in psqlODBC before landing up patching
> them in the driver instead.

Yeah, I remember working with KfW back-in-the-day.  I never played with
the new 4.0 version, so perhaps it's better, but I'm not particularly
anxious to get into that mess again..

As for this patch, the reason I've not been as involved (beyond being
ridiculously busy) is that Michael's environment, which at least appears
perfectly reasonable (and works with PG unpatched) isn't working.  If we
can get that working (and I've not looked at what's happening, so I have
no idea how easy or hard that would be), then I'd be a lot more excited
to spend time doing review of the patch.

Thanks!

Stephen


signature.asc
Description: Digital signature


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

2015-10-15 Thread Craig Ringer
On 14 October 2015 at 06:34, Robbie Harwood  wrote:
> Alright, here's v3.  As requested, it's one patch now.

I hate to ask, but have you looked at how this interacts with Windows?

We support Windows SSPI (on a domain-member host) authenticating to a
PostgreSQL server using gssapi with spnego.

We also support a PostgreSQL client on *nix authenticating using
gssapi with spnego to a PostgreSQL server that's requesting sspi mode.

The relevant code is all a bit tangled, since there's support in there
for using Kerberos libraries on Windows instead of SSPI too. I doubt
anybody uses that last one, tests it, or cares about it, though, given
the painful hoop-jumping, registry key permission changes, etc
required to make it work.

For bonus fun, RC4, DES, AES128 or AES256 are available/used for
Kerberos encryption on Windows. See
http://blogs.msdn.com/b/openspecification/archive/2011/05/31/windows-configurations-for-kerberos-supported-encryption-type.aspx
. Though given that Win7 defaults to AES256 it's probably reasonable
to simply not care about anything else.


-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, 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 v3] GSSAPI encryption support

2015-10-15 Thread Michael Paquier
On Wed, Oct 14, 2015 at 7:34 AM, Robbie Harwood  wrote:
> Alright, here's v3.  As requested, it's one patch now.  Other things
> addressed herein include:
> Essentially, the problem is that socket_putmessage_noblock() needs to
> know the size of the message to put in the buffer but we can't know
> that until we've encrypted the message.  socket_putmessage_noblock()
> calls socket_putmessage() after ensuring the call will not block;
> however, other code paths simply call directly into socket_putmessage()
> and so socket_putmessage() needs to have a path to encryption as well.
>
> If you have other potential solutions to this problem, I would love to
> hear them; right now though I don't see a better way.
>
> Patch follows.  Thanks!

After giving a quick shot at this patch, I am still seeing the same problem:
psql: lost synchronization with server: got message type "S", length 22
(And unpatched works, I will try to put my head into your code...)
Regards,
-- 
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 v3] GSSAPI encryption support

2015-10-15 Thread Robbie Harwood
Craig Ringer  writes:

> On 14 October 2015 at 06:34, Robbie Harwood  wrote:
>> Alright, here's v3.  As requested, it's one patch now.
>
> I hate to ask, but have you looked at how this interacts with Windows?
>
> We support Windows SSPI (on a domain-member host) authenticating to a
> PostgreSQL server using gssapi with spnego.
>
> We also support a PostgreSQL client on *nix authenticating using
> gssapi with spnego to a PostgreSQL server that's requesting sspi mode.
>
> The relevant code is all a bit tangled, since there's support in there
> for using Kerberos libraries on Windows instead of SSPI too. I doubt
> anybody uses that last one, tests it, or cares about it, though, given
> the painful hoop-jumping, registry key permission changes, etc
> required to make it work.
>
> For bonus fun, RC4, DES, AES128 or AES256 are available/used for
> Kerberos encryption on Windows. See
> http://blogs.msdn.com/b/openspecification/archive/2011/05/31/windows-configurations-for-kerberos-supported-encryption-type.aspx
> . Though given that Win7 defaults to AES256 it's probably reasonable
> to simply not care about anything else.

The short - and probably most important - answer is that no, I haven't
tested it, and it would be difficult for me to do so quickly.

In more depth:

Looking at
http://www.postgresql.org/docs/9.4/static/protocol-message-formats.html
suggests that SSPI follows a separate codepath from the GSS code;
certainly it's a different auth request, which means it shouldn't
trigger the encryption path.

There is no reason that using GSSAPI from, e.g., MIT Kerberos for
Windows, should not work here.  Even more broadly than that, GSSAPI is a
RFC-standardized protocol with rigorously tested interop etc. etc.,
though whether anyone outside of MIT cares about MIT Kerberos for
Windows I have no idea.  As for encryption types, MIT out-of-the-box
supports aes256 + aes128 (in several variants), rc4-hmac and friends,
camellia in several variants, and des3-cbc-sha1.  A much wider selection
is available on setting the appropriately named "allow_weak_crypto" in
krb5.conf, though I would hope that would not be needed.

I think the important takeaway here is that I haven't actually changed
how *authentication* works; these changes only affect GSSAPI
post-authentication add encryption functions as defined by that
specification.  So if the authentication was working before, and the
GSSAPI implementation is following RFC, I would hope that this would
work still.

Thanks,
--Robbie


signature.asc
Description: PGP signature


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

2015-10-13 Thread Robbie Harwood
Alright, here's v3.  As requested, it's one patch now.  Other things
addressed herein include:

 - postgres.h/assert.h ordering fix
 - spacing around casts
 - leaking of GSS buffer in be_gss_inplace_decrypt
 - libpq-be.h not having a conditional internal include
 - always exposing guc veriable gss_encrypt
 - copyright/description headers on all new files
 - movement of GSSAPI methods from fe-auth.c and auth.c to fe-gss.c and
   be-gss.c respectively
 - renaming GSSAPI files to fe-gss.c and be-gss.c (drops -secure)

Andres, one thing you mentioned as "feels rather wrong" was the
GSSAPI-specific code in pqcomm.c; while looking at that again, I have a
slightly better explanation than what I said previously.

Essentially, the problem is that socket_putmessage_noblock() needs to
know the size of the message to put in the buffer but we can't know
that until we've encrypted the message.  socket_putmessage_noblock()
calls socket_putmessage() after ensuring the call will not block;
however, other code paths simply call directly into socket_putmessage()
and so socket_putmessage() needs to have a path to encryption as well.

If you have other potential solutions to this problem, I would love to
hear them; right now though I don't see a better way.

Patch follows.  Thanks!
From 6710d5ad0226ea3a5ea8e35d6dc54b4500f1d3e0 Mon Sep 17 00:00:00 2001
From: "Robbie Harwood (frozencemetery)" 
Date: Mon, 8 Jun 2015 19:27:45 -0400
Subject: [PATCH] GSSAPI encryption support

Encryption is opportuinistic by default for backward compatability, but can be
forced using a server HBA parameter or a client connection URI parameter.
---
 configure   |   2 +
 configure.in|   1 +
 doc/src/sgml/client-auth.sgml   |  19 +-
 doc/src/sgml/libpq.sgml |  12 ++
 doc/src/sgml/protocol.sgml  |  82 +++-
 doc/src/sgml/runtime.sgml   |  20 +-
 src/Makefile.global.in  |   1 +
 src/backend/libpq/Makefile  |   4 +
 src/backend/libpq/auth.c| 338 +-
 src/backend/libpq/be-gss.c  | 397 
 src/backend/libpq/hba.c |   9 +
 src/backend/libpq/pqcomm.c  |  39 
 src/backend/tcop/postgres.c |  30 ++-
 src/backend/utils/init/postinit.c   |   7 +-
 src/backend/utils/misc/guc.c|  30 +++
 src/include/libpq/auth.h|   2 +
 src/include/libpq/hba.h |   1 +
 src/include/libpq/libpq-be.h|  26 +++
 src/include/libpq/libpq.h   |   2 +
 src/interfaces/libpq/Makefile   |   4 +
 src/interfaces/libpq/fe-auth.c  | 182 -
 src/interfaces/libpq/fe-connect.c   |  56 -
 src/interfaces/libpq/fe-gss.c   | 280 +
 src/interfaces/libpq/fe-misc.c  |   5 +
 src/interfaces/libpq/fe-protocol3.c |  60 ++
 src/interfaces/libpq/libpq-int.h|  16 ++
 26 files changed, 1097 insertions(+), 528 deletions(-)
 create mode 100644 src/backend/libpq/be-gss.c
 create mode 100644 src/interfaces/libpq/fe-gss.c

diff --git a/configure b/configure
index b771a83..a542577 100755
--- a/configure
+++ b/configure
@@ -712,6 +712,7 @@ with_uuid
 with_selinux
 with_openssl
 krb_srvtab
+with_gssapi
 with_python
 with_perl
 with_tcl
@@ -5488,6 +5489,7 @@ $as_echo "$with_gssapi" >&6; }
 
 
 
+
 #
 # Kerberos configuration parameters
 #
diff --git a/configure.in b/configure.in
index b5868b0..fccf542 100644
--- a/configure.in
+++ b/configure.in
@@ -636,6 +636,7 @@ PGAC_ARG_BOOL(with, gssapi, no, [build with GSSAPI support],
   krb_srvtab="FILE:\$(sysconfdir)/krb5.keytab"
 ])
 AC_MSG_RESULT([$with_gssapi])
+AC_SUBST(with_gssapi)
 
 
 AC_SUBST(krb_srvtab)
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 3b2935c..e6456a1 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -913,9 +913,10 @@ omicron bryanh  guest1
 GSSAPI with Kerberos
 authentication according to RFC 1964. GSSAPI
 provides automatic authentication (single sign-on) for systems
-that support it. The authentication itself is secure, but the
-data sent over the database connection will be sent unencrypted unless
-SSL is used.
+that support it. The authentication itself is secure, and GSSAPI can be
+used for connection encryption as well (see the
+require_encrypt parameter below); SSL
+can also be used for connection security.

 

@@ -1046,6 +1047,18 @@ omicron bryanh  guest1

   
  
+
+ 
+  require_encrypt
+  
+   
+Whether to require GSSAPI encryption.  Default is off, which causes
+GSSAPI encryption to be enabled if available and requested for
+compatibility with old clients.  It is recommended to set this unless
+old clients are present.
+   
+  
+ 
 

   
diff --git