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

[Robbie Harwood]

Michael Paquier <michael.paqu...@gmail.com> writes:

> On Wed, Jul 27, 2016 at 12:22 AM, Robbie Harwood <rharw...@redhat.com> wrote:
>> Michael Paquier <michael.paqu...@gmail.com> writes:
>>
>> So there's a connection setting `sslmode` that we'll want something
>> similar to here (`gssapimode` or so).  `sslmode` has six settings, but I
>> think we only need three for GSSAPI: "disable", "allow", and "prefer"
>> (which presumably would be the default).
>
> Seeing the debate regarding sslmode these days, I would not say that
> "prefer" would be the default, but that's an implementation detail.
>
>> Lets suppose we're working with "prefer".  GSSAPI will itself check two
>> places for credentials: client keytab and ccache.  But if we don't find
>> credentials there, we as the client have two options on how to proceed.
>>
>> - First, we could prompt for a password (and then call
>>   gss_acquire_cred_with_password() to get credentials), presumably with
>>   an empty password meaning to skip GSSAPI.  My memory is that the
>>   current behavior for GSSAPI auth-only is to prompt for password if we
>>   don't find credentials (and if it isn't, there's no reason not to
>>   unless we're opposed to handling the password).
>>
>> - Second, we could skip GSSAPI and proceed with the next connection
>>   method.  This might be confusing if the user is then prompted for a
>>   password and expects it to be for GSSAPI, but we could probably make
>>   it sensible.  I think I prefer the first option.
>
> Ah, right. I completely forgot that GSSAPI had its own handling of
> passwords for users registered to it...
>
> Isn't this distinction a good point for not implementing "prefer",
> "allow" or any equivalents? By that I mean that we should not have any
> GSS connection mode that fallbacks to something else if the first one
> fails. So we would live with the two following modes:
> - "disable", to only try a non-GSS connection
> - "enable", or "require", to only try a GSS connection.
> That seems quite acceptable to me as a first implementation to just
> have that.

If it is the password management that is scary here, we could have a
prefer-type mode which does not prompt, but only uses existing
credentials.  Or we could opt to never prompt, which is totally valid.


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Tom Lane <t...@sss.pgh.pa.us> writes:

> Robbie Harwood <rharw...@redhat.com> writes:
>> So there's a connection setting `sslmode` that we'll want something
>> similar to here (`gssapimode` or so).  `sslmode` has six settings, but I
>> think we only need three for GSSAPI: "disable", "allow", and "prefer"
>> (which presumably would be the default).
>
> FWIW, there is quite a bit of unhappiness around sslmode=prefer, see
> for example this thread:
> https://www.postgresql.org/message-id/flat/2A5EFBDC-41C6-42A8-8B6D-E69DA60E9962%40eggerapps.at
>
> I do not know if we can come up with a better answer, but I'd caution
> you against thinking that that's a problem-free model to emulate.

Understood.  We have the slight simplification for GSSAPI of having
mutual authentication always (i.e., no need to worry about
unauthenticated-but-encrypted connections).

My personal view is that we want to try for as much security as we can
without breaking anything [0].  If a user knows that they want a specific
security, they can set "require"; if they don't want it, they can set
"disable".  Setting "require" as the default breaks one class of users;
setting "disable" another.  And I don't think we can punt the problem to
the user and make it a mandatory parameter, either.

I'm absolutely open to suggestions for how we could do better here,
especially since we're adding support for something new, but having read
the thread you mention I don't immediately see a superior design.

0: For what it's worth, I also don't agree with the assertion that
   having the ability to fallback to plaintext from tampering makes the
   attempt at encryption useless; rather, it still foils a passive
   adversary, even if it doesn't do anything against an active one.


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Robbie Harwood <rharw...@redhat.com> writes:

> So there's a connection setting `sslmode` that we'll want something
> similar to here (`gssapimode` or so).  `sslmode` has six settings, but I
> think we only need three for GSSAPI: "disable", "allow", and "prefer"
> (which presumably would be the default).

Apologies, this should say four; I neglected "require".


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Michael Paquier <michael.paqu...@gmail.com> writes:

> On Tue, Jul 26, 2016 at 5:58 AM, Robbie Harwood <rharw...@redhat.com> wrote:
>> Robbie Harwood <rharw...@redhat.com> writes:
>
> Sorry for my late reply.

Thanks for the feedback!

>>> If I were to continue as I have been - using the plaintext connection
>>> and auth negotiation path - then at the time of startup the client has
>>> no way of knowing whether to send connection parameters or not.
>>> Personally, I would be in favor of not frontloading these connection
>>> parameters over insecure connections, but it is my impression that the
>>> project does not want to go this way (which is fine).
>
> Per the discussion upthread I got the opposite impression, the startup
> packet should be sent after the connection has been established. SSL
> does so: the SSL negotiation goes first, and then the startup packet
> is sent. That's the flow with the status changing from
> CONNECTION_SSL_START -> CONNECTION_MADE.

We are in agreement, I think.  I have structured the referenced
paragraph badly: for this design, I'm suggesting separate GSS startup
path (like SSL) and once a tunnel is established we send the startup
packet.  I probably should have just left this paragraph out.

>>> On the server, I'll need to implement `hostgss` (by analogy to
>>> `hostssl`), and we'll want to lock authentication on those connections
>>> to GSSAPI-only.
>
> As well as hostnogss, but I guess that's part of the plan.

Sure, `hostnogss` should be fine.  This isn't quadratic, right?  We don't
need hostnogssnossl (or thereabouts)?

>>> Clients will explicitly probe for GSSAPI support as they do for TLS
>>> support (I look forward to the bikeshed on the order of these) and
>>> should have a parameter to require said support.  One thing I'm not
>>> clear on is what our behavior should be when the user doesn't
>>> explicitly request GSSAPI and doesn't have a ccache - do we prompt?
>>> Skip probing?  I'm not sure what the best option there is.
>
> I am not sure I get what you mean here.

Okay.  Let me try again:

So there's a connection setting `sslmode` that we'll want something
similar to here (`gssapimode` or so).  `sslmode` has six settings, but I
think we only need three for GSSAPI: "disable", "allow", and "prefer"
(which presumably would be the default).

Lets suppose we're working with "prefer".  GSSAPI will itself check two
places for credentials: client keytab and ccache.  But if we don't find
credentials there, we as the client have two options on how to proceed.

- First, we could prompt for a password (and then call
  gss_acquire_cred_with_password() to get credentials), presumably with
  an empty password meaning to skip GSSAPI.  My memory is that the
  current behavior for GSSAPI auth-only is to prompt for password if we
  don't find credentials (and if it isn't, there's no reason not to
  unless we're opposed to handling the password).

- Second, we could skip GSSAPI and proceed with the next connection
  method.  This might be confusing if the user is then prompted for a
  password and expects it to be for GSSAPI, but we could probably make
  it sensible.  I think I prefer the first option.

Thanks,
--Robbie


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Robbie Harwood <rharw...@redhat.com> writes:

> Michael Paquier <michael.paqu...@gmail.com> writes:
>
>> On Thu, Apr 7, 2016 at 8:20 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> Robbie Harwood <rharw...@redhat.com> writes:
>>>> Tom Lane <t...@sss.pgh.pa.us> writes:
>>>>
>>>>> Wait a second.  So the initial connection-request packet is
>>>>> necessarily unencrypted under this scheme?
>>>
>>>> Yes, by necessity.  The username must be sent in the clear, even if
>>>> only as part of the GSSAPI handshake (i.e., the GSSAPI username will
>>>> appear in plantext in the GSSAPI blobs which are otherwise
>>>> encrypted).  GSSAPI performs authentication before it can start
>>>> encryption.
>>>
>>> Ugh.  I had thought we were putting work into this because it
>>> represented something we could recommend as best practice, but now
>>> you're telling me that it's always going to be inferior to what we
>>> have already.
>>
>> It does not seem necessary to have an equivalent of
>> pqsecure_open_client, just some extra handling in fe-connect.c to set
>> up the initial context with a proper message handling... Not that
>> direct anyway. So should the patch be marked as returned with feedback
>> at this stage?
>
> I think in order to satisfy Tom's (valid) concern, there does need to be
> a separate handshake - i.e., GSSAPI support in pqsecure_open_client().
>
> If I were to continue as I have been - using the plaintext connection
> and auth negotiation path - then at the time of startup the client has
> no way of knowing whether to send connection parameters or not.
> Personally, I would be in favor of not frontloading these connection
> parameters over insecure connections, but it is my impression that the
> project does not want to go this way (which is fine).
>
> The way I'm seeing this, when a connection comes in, we take the 'G'
> character for GSSAPI much as for SSL.  At that time, we need to perform
> an *authentication* handshake (because GSSAPI will not do encryption
> before authenticating).  I expect to use a consistent format for all
> GSSAPI packets - four bytes for length, and a payload.  (I would prefer
> tagging them, but previously preference for not doing this has been
> expressed.)
>
> Once GSSAPI authentication is complete, the normal handshake process can
> be tunneled through a GSSAPI encryption layer, as is done with TLS.  The
> server will need to retain some of the earlier authentication data
> (e.g., to check that the presented user-name matches GSSAPI
> credentials), but there will be no authentication packets exchanged
> (more specifically, it will resemble the anonymous case).  Authorization
> will be checked as normal, and we then proceed in the usual fashion, all
> over the GSSAPI tunnel.
>
> On the server, I'll need to implement `hostgss` (by analogy to
> `hostssl`), and we'll want to lock authentication on those connections
> to GSSAPI-only.  Clients will explicitly probe for GSSAPI support as
> they do for TLS support (I look forward to the bikeshed on the order of
> these) and should have a parameter to require said support.  One thing
> I'm not clear on is what our behavior should be when the user doesn't
> explicitly request GSSAPI and doesn't have a ccache - do we prompt?
> Skip probing?  I'm not sure what the best option there is.
>
> Before I implement this design, does anyone have any additional concerns
> or feedback on it?

Does this look reasonable to folks?


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Michael Paquier <michael.paqu...@gmail.com> writes:

> On Thu, Apr 7, 2016 at 8:20 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> Robbie Harwood <rharw...@redhat.com> writes:
>>> Tom Lane <t...@sss.pgh.pa.us> writes:
>>>
>>>> Wait a second.  So the initial connection-request packet is
>>>> necessarily unencrypted under this scheme?
>>
>>> Yes, by necessity.  The username must be sent in the clear, even if
>>> only as part of the GSSAPI handshake (i.e., the GSSAPI username will
>>> appear in plantext in the GSSAPI blobs which are otherwise
>>> encrypted).  GSSAPI performs authentication before it can start
>>> encryption.
>>
>> Ugh.  I had thought we were putting work into this because it
>> represented something we could recommend as best practice, but now
>> you're telling me that it's always going to be inferior to what we
>> have already.
>
> It does not seem necessary to have an equivalent of
> pqsecure_open_client, just some extra handling in fe-connect.c to set
> up the initial context with a proper message handling... Not that
> direct anyway. So should the patch be marked as returned with feedback
> at this stage?

I think in order to satisfy Tom's (valid) concern, there does need to be
a separate handshake - i.e., GSSAPI support in pqsecure_open_client().

If I were to continue as I have been - using the plaintext connection
and auth negotiation path - then at the time of startup the client has
no way of knowing whether to send connection parameters or not.
Personally, I would be in favor of not frontloading these connection
parameters over insecure connections, but it is my impression that the
project does not want to go this way (which is fine).

The way I'm seeing this, when a connection comes in, we take the 'G'
character for GSSAPI much as for SSL.  At that time, we need to perform
an *authentication* handshake (because GSSAPI will not do encryption
before authenticating).  I expect to use a consistent format for all
GSSAPI packets - four bytes for length, and a payload.  (I would prefer
tagging them, but previously preference for not doing this has been
expressed.)

Once GSSAPI authentication is complete, the normal handshake process can
be tunneled through a GSSAPI encryption layer, as is done with TLS.  The
server will need to retain some of the earlier authentication data
(e.g., to check that the presented user-name matches GSSAPI
credentials), but there will be no authentication packets exchanged
(more specifically, it will resemble the anonymous case).  Authorization
will be checked as normal, and we then proceed in the usual fashion, all
over the GSSAPI tunnel.

On the server, I'll need to implement `hostgss` (by analogy to
`hostssl`), and we'll want to lock authentication on those connections
to GSSAPI-only.  Clients will explicitly probe for GSSAPI support as
they do for TLS support (I look forward to the bikeshed on the order of
these) and should have a parameter to require said support.  One thing
I'm not clear on is what our behavior should be when the user doesn't
explicitly request GSSAPI and doesn't have a ccache - do we prompt?
Skip probing?  I'm not sure what the best option there is.

Before I implement this design, does anyone have any additional concerns
or feedback on it?

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [HACKERS] Lets (not) break all the things. Was: [pgsql-advocacy] 9.6 -> 10.0

[Robbie Harwood]

Justin Clift  writes:

> Moving over a conversation from the pgsql-advocacy mailing list.  In it
> Simon (CC'd) raised the issue of potentially creating a 
> backwards-compatibility
> breaking release at some point in the future, to deal with things that
> might have no other solution (my wording).
>
> Relevant part of that thread there for reference:
>
>   
> http://www.postgresql.org/message-id/CANP8+jLtk1NtaJyXc=hAqX=0k+ku4zfavgvbkfs+_sor9he...@mail.gmail.com
>
> Simon included a short starter list of potentials which might be in
> that category:
>
>   * SQL compliant identifiers
>   * Remove RULEs
>   * Change recovery.conf
>   * Change block headers
>   * Retire template0, template1
>   * Optimise FSM
>   * Add heap metapage
>   * Alter tuple headers
>   et al
>
> This still is better placed on -hackers though, so lets have the
> conversation here to figure out if a "backwards compatibility breaking"
> release really is needed or not.
>
> Hopefully we can get it all done without giving users a reason to consider
> switching. ;)

I'm sure this won't be a popular suggestion, but in the interest of
advocating for more cryptography: if we land GSSAPI auth+encryption, I'd
like the auth-only codepath to go away.


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Tom Lane <t...@sss.pgh.pa.us> writes:

> Robbie Harwood <rharw...@redhat.com> writes:
>> I need to flush this any time we might be doing encryption because it
>> needs to be in a separate request to _secure_write() from what follows
>> it.  We don't know whether we should be doing encryption until
>> connection parameters are parsed; to put it another way,
>> port->gss->encrypt will never be true here because it hasn't been parsed
>> out of port->gss->gss_encrypt yet.
>
> Wait a second.  So the initial connection-request packet is necessarily
> unencrypted under this scheme?  That seems like a pretty substantial
> step backwards from what happens with SSL.  Even granting that stuff
> like passwords won't be sent till later, the combination of user name
> and database name might already be useful info to an eavesdropper.
>
> I would think a design similar to the SSL one (special protocol version
> to cause encryption negotiation before the actual connection request
> is sent) would be better.

(Apologies for the wall of text that follows.  My GSSAPI encryption
support has gone through three major redesigns, so I've got a fair bit
to say about it at this point, and it's probably better that I say too
much than too little.  The short version is that GSSAPI works
differently than SSL and username is sent in the clear no matter what,
but this isn't a problem.)

Yes, by necessity.  The username must be sent in the clear, even if only
as part of the GSSAPI handshake (i.e., the GSSAPI username will appear
in plantext in the GSSAPI blobs which are otherwise encrypted).  GSSAPI
performs authentication before it can start encryption.

In this design, the contents of the Startup Message are the only
non-authentication related information sent in the clear.  This
contains: username (which we need anyway), database, application_name,
and I add gss_encrypt.

Why does it look this way?  Fallback support.  We already have GSSAPI
authentication code in the project, and unfortunately we can't fix the
multitude of older clients that won't have encryption support, whatever
form it takes.

What if we didn't need fallback support, though?  Doing it with a
special protocol version, as in the SSL/TLS case, would cause a separate
path for authentication to occur, during which everything would look
pretty much the same, except we wouldn't send database.  We would then
complete the auth handshake, and in a separate exchange, pass in the
database information.  Only then could we perform authorization
checking.  Authorization checking is currently coupled with the
authentication as well; we would need a way to bypass the normal auth
sequence and enter encryption.

Bottom line is that designing similarly to SSL/TLS doesn't really make
sense because the two schemes work differently.  Typically, usernames
are not considered sensitive information unless one is worried about the
security of the authentication information that goes with them.  For
instance, MIT Kerberos will reveal the difference between "username not
found" and the equivalent of "bad password".  I don't know how much
admins care about the database names being in the clear; I suspect it
doesn't matter much because just knowing their names isn't enough to
connect.  Even if it's something people care about, this is still far
better than no encryption at all (which is the current GSSAPI behavior),
and would be better addressed by supporting connecting without
specifying a database immediately anyway.

>>> I'm aware that enlargeStringInfo() does check and handle the case where
>>> the length ends up >1G, but that feels a bit grotty to me- are you sure
>>> you want the generic enlargeStringInfo() to handle that case?
>
>> This is a good point.  We definitely have to draw the line somewhere; 1G
>> is a high upper bound.  Digging around the server code I don't see a
>> precedent for what's a good size to stop at.  There's
>> PG_MAX_AUTH_TOKEN_LENGTH, which is 65k, and the password length in
>> auth.c is 1k.  Beyond that, SocketBackend() calls pq_getmessage() with
>> no maximum length, which causes the enlargeStringInfo() size
>> restrictions to be the only control (wrapped in a PG_TRY()).
>
> Note that SocketBackend() only runs *after* we've accepted the user as
> authorized.  We should be a lot charier of what we're willing to accept
> before authorization, IMO.  Note MAX_STARTUP_PACKET_LENGTH, which is
> only 10K.

Correct, we're only talking about packets that are sent after
authentication+authorization are complete.  Before authentication is
complete, the GSSAPI encryption codepaths are not taken.


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Stephen Frost  writes:

> Just an initial pass over the patch.

Thanks!  In the interest of brevity, if I haven't replied to something,
I plan to fix it.

>>  /*
>> - * Flush message so client will see it, except for AUTH_REQ_OK, which 
>> need
>> - * not be sent until we are ready for queries.
>> + * In most cases, we do not need to send AUTH_REQ_OK until we are ready
>> + * for queries.  However, if we are doing GSSAPI encryption, that 
>> request
>> + * must go out immediately to ensure that all messages which follow the
>> + * AUTH_REQ_OK are not grouped with it and can therefore be encrypted.
>>   */
>> -if (areq != AUTH_REQ_OK)
>> +if (areq != AUTH_REQ_OK || port->gss != NULL)
>>  pq_flush();
>>  
>>  CHECK_FOR_INTERRUPTS();
>
> Do we actually need to send pq_flush *whenever* port->gss is not null?
> Shouldn't this actually be port->gss->encrypt?

I need to flush this any time we might be doing encryption because it
needs to be in a separate request to _secure_write() from what follows
it.  We don't know whether we should be doing encryption until
connection parameters are parsed; to put it another way,
port->gss->encrypt will never be true here because it hasn't been parsed
out of port->gss->gss_encrypt yet.

I could parse it earlier, but then I need another variable in the struct
(i.e., to hold whether AUTH_REQ_OK has been sent yet) to check as well.
Additionally, it seemed to me that there might be some value
security-wise in delaying parsing of connection parameters until after
auth is complete, though of course for just a bool this may not be as
important.

>> +/* recur to send any buffered data */
>> +gss_release_buffer(, );
>> +return be_gssapi_write(port, ptr, len);
>
> This feels a bit odd to be doing, honestly.  We try to take a lot of
> care to consider low-memory situation and to be careufl when it comes to
> potential for infinite recursion and there's already a way to ask for
> this function to be called again, isn't there?

This call should be okay because (1) it's a tail call and (2) we know
the buffer isn't empty.

That said, the other recursion is excessively complicated and I think
it's simpler to eliminate it entirely in favor of being called again.
I'll see what I can do.

>> +/* we know the length of the packet at this point */
>> +memcpy((char *), port->gss->buf.data, 4);
>> +input.length = ntohl(input.length);
>> +enlargeStringInfo(>gss->buf, input.length - port->gss->buf.len + 
>> 4);
>
> I'm aware that enlargeStringInfo() does check and handle the case where
> the length ends up >1G, but that feels a bit grotty to me- are you sure
> you want the generic enlargeStringInfo() to handle that case?

This is a good point.  We definitely have to draw the line somewhere; 1G
is a high upper bound.  Digging around the server code I don't see a
precedent for what's a good size to stop at.  There's
PG_MAX_AUTH_TOKEN_LENGTH, which is 65k, and the password length in
auth.c is 1k.  Beyond that, SocketBackend() calls pq_getmessage() with
no maximum length, which causes the enlargeStringInfo() size
restrictions to be the only control (wrapped in a PG_TRY()).

I think what I'm trying to get at here is that I'm open to suggestion,
but don't see a clearly better way to do this.  We could use the
PG_TRY() approach here to preserve sync, though I'm not sure it's worth
it considering PQ_RECV_BUFFER_SIZE and PQ_SEND_BUFFER_SIZE are both 8k.

>> Subject: [PATCH 3/3] GSSAPI authentication cleanup
>
> Why wouldn't this be part of patch #2?

It's a cleanup of existing code, not my new code, so I thought the
separation would make it easier to understand.  I'm fine with it as part
of #2 so long as it happens, but the impression I had gotten from
earlier reviews was that it should have even more division (i.e., move
the gss_display_status() wrappers into their own patch), not less.

Thanks,
--Robbie


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Alvaro Herrera <alvhe...@2ndquadrant.com> writes:

> Robbie Harwood wrote:
>> Michael Paquier <michael.paqu...@gmail.com> writes:
>> 
>> > On Tue, Apr 5, 2016 at 9:06 AM, Robbie Harwood <rharw...@redhat.com> wrote:
>> >> Here's v12, both here and on my github:
>> >> https://github.com/frozencemetery/postgres/tree/feature/gssencrypt12
>
>> > So you are saving everything in the top memory context. I am fine to
>> > give the last word to a committer here but I would just go with
>> > calloc/free to simplify those hunks.
>> 
>> Yeah, it's definitely worth thinking/talking about; this came up in IRC
>> discussion as well.
>> 
>> If we go the memory context route, it has to be TopMemoryContext since
>> nothing else lives long enough (i.e., entire connection).
> [...]
>> It turns out that's not actually required, but could easily be made
>> explicit here.  According to the README for the memory context system,
>> pfree() and repalloc() do not require setting CurrentMemoryContext
>> (since 7.1).
>
> It seems to me that the right solution for this is to create a new
> memory context which is a direct child of TopMemoryContext, so that
> palloc can be used, and so that it can be reset separately, and that it
> doesn't suffer from resets of other contexts.  (I think Michael's point
> is that if those chunks were it a context of their own, you wouldn't
> need the pfrees but a MemCxtReset would be enough.)

Hmm, that's also an option.  I read Michael's point as arguing for
calloc()/free() rather than a new context, but I could be wrong.

A question, though: it it valuable for the context to be reset()able
separately?  If there were more than just these two buffers going into
it, I could see it being convenient - especially if it were for
different encryption types, for instance - but it seems like it would be
overkill?

This is all new to me so I may be entirely mistaken though.

>> Side note: it's really irritating to work with having this file under
>> version control because of how different it ends up being when I
>> autoreconf (which I need to do because I'm changing the build system).
>> (I'm also idly curious what system/autotools version is generating this
>> file because it doesn't match any that I tried.)
>
> We use stock autoconf from the GNU package and it definitely produces
> matching output for me.  Maybe your Linux distro includes a patched
> version?  I know Debian does, but I suppose you're using some Redhat
> thing, so no idea.

Hmm, that explains the Debian behavior I was seeing (it does the
above).  The Fedora one adds a couple blank lines in places but it's
much less... gratuitous... in its changes.


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Michael Paquier <michael.paqu...@gmail.com> writes:

> On Tue, Apr 5, 2016 at 9:06 AM, Robbie Harwood <rharw...@redhat.com> wrote:
>> Here's v12, both here and on my github:
>> https://github.com/frozencemetery/postgres/tree/feature/gssencrypt12
>>
> +#ifdef ENABLE_GSS
> +   {
> +   MemoryContext save = CurrentMemoryContext;
> +   CurrentMemoryContext = TopMemoryContext;
> +
> +   initStringInfo(>gss->buf);
> +   initStringInfo(>gss->writebuf);
> +
> +   CurrentMemoryContext = save;
> +   }
> +#endif
>
> So you are saving everything in the top memory context. I am fine to
> give the last word to a committer here but I would just go with
> calloc/free to simplify those hunks.

Yeah, it's definitely worth thinking/talking about; this came up in IRC
discussion as well.

If we go the memory context route, it has to be TopMemoryContext since
nothing else lives long enough (i.e., entire connection).

On the other hand, if we go with calloc/free here, I can't use
StringInfo for these buffers because StringInfo uses palloc.  Using
StringInfo is really handy; if I don't use StringInfo, I'd need to
reimplement enlargeStringInfo() for the ad-hoc buffer structure.  What
I'm trying to articulate is that it'd simplify the initialization code
since it removes MemoryContext management, but it makes everything else
more complicated.

I prefer the StringInfo approach since I think the abstraction is a good
one, but if you've got a case for explicit calloc()/free() in light of
that, I'd be interested to hear it.

> +#ifdef ENABLE_GSS
> +   if (conn->gss->buf.data)
> +   pfree(conn->gss->buf.data);
> +   if (conn->gss->writebuf.data)
> +   pfree(conn->gss->writebuf.data);
> +#endif
>
> This should happen in its own memory context, no

It turns out that's not actually required, but could easily be made
explicit here.  According to the README for the memory context system,
pfree() and repalloc() do not require setting CurrentMemoryContext
(since 7.1).

I'm not opposed to making this one explicit if you think it adds
clarity.  I would not want to make all the calls to StringInfo functions
explicit just because they can call repalloc, though.

> @@ -775,6 +775,7 @@ infodir
>  docdir
>  oldincludedir
>  includedir
> +runstatedir
>  localstatedir
>  sharedstatedir
>  sysconfdir
> @@ -896,6 +897,7 @@ datadir='${datarootdir}'
>  sysconfdir='${prefix}/etc'
>  sharedstatedir='${prefix}/com'
>  localstatedir='${prefix}/var'
> +runstatedir='${localstatedir}/run'
> What's that? I would recommend re-running autoconf to remove this
> portion (committers do it at the end btw, so that's actually no bug
> deal).

Well, I guess /that/ mistake finally happened.  This is what happens
when I run autoreconf on my VMs.  Because configure is checked in to
version control, I accidentally committed the whole thing instead of
just the GSSAPI changes.  I can back that out.

Side note: it's really irritating to work with having this file under
version control because of how different it ends up being when I
autoreconf (which I need to do because I'm changing the build system).
(I'm also idly curious what system/autotools version is generating this
file because it doesn't match any that I tried.)

> -#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 = _C_NT_USER_NAME_desc;
> -#endif
> Regarding patch 0003 it may be fine to remove that... Robbie, do you
> know how long ago this has been fixed upstream? I'd rather not have
> this bit removed if this could impact some users.

I double-checked with MIT, and we think it was fixed in 2003 in commit
4ce1f7c3a46485e342d3a68b4c60b76c196d1851 which can be viewed at
https://github.com/krb5/krb5/commit/4ce1f7c3a46485e342d3a68b4c60b76c196d1851
and the corresponding bug on their bugtracker was
http://krbdev.mit.edu/rt/Ticket/Display.html?id=1666

In terms of release versions, this was first included in krb5-1.4.  For
reference, the oldest Kerberos that MIT upstream supports is 1.12; the
oldest Kerberos that I (maintainer for Red Hat) support in production
(i.e., RHEL-5) is 1.6.1, and even that is slated to go away early 2017.
To my knowledge no one is supporting an older version than that.

In the interest of completion, MIT did express concern that this might
not have been a bug in MIT at all, but rather a toolchain issue.  This
workaround has b

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

[Robbie Harwood]

Hello friends,

Here's v12, both here and on my github:
https://github.com/frozencemetery/postgres/tree/feature/gssencrypt12

What changed:

- The code is aware of memory contexts now.  I actually really like the
  memory context stuff; just didn't see any indication of its existence
  in the code I had read.  Anyway, we allocate server buffers in the
  connection-lifetime context.  The other alternative that we discussed
  on IRC a bit was to avoid palloc()/pfree() entirely in favor of raw
  calloc()/free(), but I think if possible I prefer this approach since
  I find the StringInfo handy to work with.  This eliminates the
  traceback for me with --enable-cassert.

- Error cleanup.  I've been looking very hard at this code in order to
  try to fix the assert, and I've fixed up a couple error paths that
  hadn't been taken.  This involves replacing the double-send with a
  buffer-and-then-send, which turns out to be not only shorter but
  easier for me to reason about.

Thanks!
From 945805d45e8021f92ad73518b3a74ac6bab89525 Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharw...@redhat.com>
Date: Fri, 26 Feb 2016 16:07:05 -0500
Subject: [PATCH 1/3] Move common GSSAPI code into its own files

On both the frontend and backend, prepare for GSSAPI encryption suport
by moving common code for error handling into a common file.  Other than
build-system changes, no code changes occur in this patch.

Thanks to Michael Paquier for the Windows fixes.
---
 configure   |  2 +
 configure.in|  1 +
 src/Makefile.global.in  |  1 +
 src/backend/libpq/Makefile  |  4 ++
 src/backend/libpq/auth.c| 63 +---
 src/backend/libpq/be-gssapi-common.c| 74 +
 src/include/libpq/be-gssapi-common.h| 26 
 src/interfaces/libpq/Makefile   |  4 ++
 src/interfaces/libpq/fe-auth.c  | 48 +
 src/interfaces/libpq/fe-gssapi-common.c | 63 
 src/interfaces/libpq/fe-gssapi-common.h | 21 ++
 src/tools/msvc/Mkvcbuild.pm | 18 ++--
 12 files changed, 212 insertions(+), 113 deletions(-)
 create mode 100644 src/backend/libpq/be-gssapi-common.c
 create mode 100644 src/include/libpq/be-gssapi-common.h
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.c
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.h

diff --git a/configure b/configure
index b3f3abe..a5bd629 100755
--- a/configure
+++ b/configure
@@ -713,6 +713,7 @@ with_systemd
 with_selinux
 with_openssl
 krb_srvtab
+with_gssapi
 with_python
 with_perl
 with_tcl
@@ -5491,6 +5492,7 @@ $as_echo "$with_gssapi" >&6; }
 
 
 
+
 #
 # Kerberos configuration parameters
 #
diff --git a/configure.in b/configure.in
index 0bd90d7..4fd8f05 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/src/Makefile.global.in b/src/Makefile.global.in
index e94d6a5..3dbc5c2 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -183,6 +183,7 @@ with_perl	= @with_perl@
 with_python	= @with_python@
 with_tcl	= @with_tcl@
 with_openssl	= @with_openssl@
+with_gssapi	= @with_gssapi@
 with_selinux	= @with_selinux@
 with_systemd	= @with_systemd@
 with_libxml	= @with_libxml@
diff --git a/src/backend/libpq/Makefile b/src/backend/libpq/Makefile
index 09410c4..a8cc9a0 100644
--- a/src/backend/libpq/Makefile
+++ b/src/backend/libpq/Makefile
@@ -21,4 +21,8 @@ ifeq ($(with_openssl),yes)
 OBJS += be-secure-openssl.o
 endif
 
+ifeq ($(with_gssapi),yes)
+OBJS += be-gssapi-common.o
+endif
+
 include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 57c2f48..73d493e 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -136,11 +136,7 @@ bool		pg_krb_caseins_users;
  *
  */
 #ifdef ENABLE_GSS
-#if defined(HAVE_GSSAPI_H)
-#include 
-#else
-#include 
-#endif
+#include "libpq/be-gssapi-common.h"
 
 static int	pg_GSS_recvauth(Port *port);
 #endif   /* ENABLE_GSS */
@@ -715,63 +711,6 @@ recv_and_check_password_packet(Port *port, char **logdetail)
  */
 #ifdef ENABLE_GSS
 
-#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 = _C_NT_USER_NAME_desc;
-#endif
-
-
-static void
-pg_GSS_er

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

[Robbie Harwood]

Michael Paquier <michael.paqu...@gmail.com> writes:

> On Sat, Apr 2, 2016 at 7:34 AM, Robbie Harwood <rharw...@redhat.com> wrote:
>
>>   Since I still can't reproduce this locally (left a client machine and
>>   a process on the same machine retrying for over an hour on your test
>>   case and didn't see it), could you provide me with some more
>>   information on why repalloc is complaining?
>>   Is this a low memory situation where alloc might have failed?
>
> No, this is an assertion failure, and it seems that you are compiling
> this code without --enable-cassert, without the switch the code
> actually works.

You are right.  I now see the assertion failure.

>>   That pointer looks like it's on the heap, is that correct?
>
> appendBinaryStringInfo depends on palloc calls that allocate memory
> depending on the memory context used. It looks that what's just
> missing in your logic is a private memory context that be_gssapi_write
> and be_gssapi_read can use to handle the allocation of the
> communication buffers.

Thank you very much for the pointer!  I will work in memory context
management for the next version.


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Hello friends,

Song and dance, here's v11 both here and on my github:
https://github.com/frozencemetery/postgres/tree/feature/gssencrypt11

Changes from v10:

- Attempt to address a crash Michael is observing by switching to using
  the StringInfo/pqExpBuffer management functions over my own code as
  much as possible.  Michael, if this doesn't fix it, I'm out of ideas.
  Since I still can't reproduce this locally (left a client machine and
  a process on the same machine retrying for over an hour on your test
  case and didn't see it), could you provide me with some more
  information on why repalloc is complaining?  Is this a low memory
  situation where alloc might have failed?  What's your setup look like?
  That pointer looks like it's on the heap, is that correct?  I really
  don't have a handle on what's gone wrong here.

- Switch to using parse_bool for handling gss_encrypt.

- Remove accidental whitespace change.

Thanks!
From 945805d45e8021f92ad73518b3a74ac6bab89525 Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharw...@redhat.com>
Date: Fri, 26 Feb 2016 16:07:05 -0500
Subject: [PATCH 1/3] Move common GSSAPI code into its own files

On both the frontend and backend, prepare for GSSAPI encryption suport
by moving common code for error handling into a common file.  Other than
build-system changes, no code changes occur in this patch.

Thanks to Michael Paquier for the Windows fixes.
---
 configure   |  2 +
 configure.in|  1 +
 src/Makefile.global.in  |  1 +
 src/backend/libpq/Makefile  |  4 ++
 src/backend/libpq/auth.c| 63 +---
 src/backend/libpq/be-gssapi-common.c| 74 +
 src/include/libpq/be-gssapi-common.h| 26 
 src/interfaces/libpq/Makefile   |  4 ++
 src/interfaces/libpq/fe-auth.c  | 48 +
 src/interfaces/libpq/fe-gssapi-common.c | 63 
 src/interfaces/libpq/fe-gssapi-common.h | 21 ++
 src/tools/msvc/Mkvcbuild.pm | 18 ++--
 12 files changed, 212 insertions(+), 113 deletions(-)
 create mode 100644 src/backend/libpq/be-gssapi-common.c
 create mode 100644 src/include/libpq/be-gssapi-common.h
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.c
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.h

diff --git a/configure b/configure
index b3f3abe..a5bd629 100755
--- a/configure
+++ b/configure
@@ -713,6 +713,7 @@ with_systemd
 with_selinux
 with_openssl
 krb_srvtab
+with_gssapi
 with_python
 with_perl
 with_tcl
@@ -5491,6 +5492,7 @@ $as_echo "$with_gssapi" >&6; }
 
 
 
+
 #
 # Kerberos configuration parameters
 #
diff --git a/configure.in b/configure.in
index 0bd90d7..4fd8f05 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/src/Makefile.global.in b/src/Makefile.global.in
index e94d6a5..3dbc5c2 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -183,6 +183,7 @@ with_perl	= @with_perl@
 with_python	= @with_python@
 with_tcl	= @with_tcl@
 with_openssl	= @with_openssl@
+with_gssapi	= @with_gssapi@
 with_selinux	= @with_selinux@
 with_systemd	= @with_systemd@
 with_libxml	= @with_libxml@
diff --git a/src/backend/libpq/Makefile b/src/backend/libpq/Makefile
index 09410c4..a8cc9a0 100644
--- a/src/backend/libpq/Makefile
+++ b/src/backend/libpq/Makefile
@@ -21,4 +21,8 @@ ifeq ($(with_openssl),yes)
 OBJS += be-secure-openssl.o
 endif
 
+ifeq ($(with_gssapi),yes)
+OBJS += be-gssapi-common.o
+endif
+
 include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 57c2f48..73d493e 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -136,11 +136,7 @@ bool		pg_krb_caseins_users;
  *
  */
 #ifdef ENABLE_GSS
-#if defined(HAVE_GSSAPI_H)
-#include 
-#else
-#include 
-#endif
+#include "libpq/be-gssapi-common.h"
 
 static int	pg_GSS_recvauth(Port *port);
 #endif   /* ENABLE_GSS */
@@ -715,63 +711,6 @@ recv_and_check_password_packet(Port *port, char **logdetail)
  */
 #ifdef ENABLE_GSS
 
-#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 = _C_NT_USER_NAME_desc;
-#endif
-
-
-static void
-pg_GSS_error(int severity, char *errmsg, OM_uint32 maj_sta

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

[Robbie Harwood]

Michael Paquier <michael.paqu...@gmail.com> writes:

> On Fri, Apr 1, 2016 at 12:31 PM, Robbie Harwood <rharw...@redhat.com> wrote:
>
>> - Fixed buffering of large replies on the serverside.  This should fix
>>   the traceback that was being seen.  The issue had to do with the
>>   difference between the server and client calling conventions for the
>>   _read and _write functions.
>
> This does not fix the issue. I am still able to crash the server with
> the same trace.

Interesting.  I guess I'll keep trying to reproduce this.

>> - Move gss_encrypt out of the GUCs and into connection-specific logic.
>>   Thanks to Tom Lane for pointing me in the right direction here.
>
> +   /* delay processing until after AUTH_REQ_OK has been sent */
> +   if (port->gss->gss_encrypt != NULL)
> +   port->gss->encrypt = !strcmp(port->gss->gss_encrypt, "on");
>
> You should use parse_bool here, because contrary to libpq, clients
> should be able to use other values like "1", "0", "off", 'N', 'Y',
> etc.

Missed that function somehow.  Will fix.

>> - Replace writev() with two calls to _raw_write().  I'm not attached to
>>   this design; if someone has a preference for allocating a buffer and
>>   making a single write from that, I could be persuaded.  I don't know
>>   what the performance tradeoffs are.
>
> Relying on pqsecure_raw_write and pqsecure_raw_read is a better bet
> IMO, there is already some low-level error handling.

Indeed, it looks like it should especially lead to better behavior on
Windows.

>  static ConfigVariable *ProcessConfigFileInternal(GucContext context,
>   bool applySettings, int elevel);
>
> -
>  /*
>
> Spurious noise.

Indeed, will fix.


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Hello friends,

Here is another version of GSSAPI encryption support, both on this email
and on my github:
https://github.com/frozencemetery/postgres/tree/feature/gssencrypt10

This version is intended to address Michael's review:

- Fixed Windows build.  Thanks to Michael for patches.

- Fixed buffering of large replies on the serverside.  This should fix
  the traceback that was being seen.  The issue had to do with the
  difference between the server and client calling conventions for the
  _read and _write functions.

- gss_enc_require renamed to gss_require_encrypt.  Slightly longer, but
  half the stomach churn.

- Move gss_encrypt out of the GUCs and into connection-specific logic.
  Thanks to Tom Lane for pointing me in the right direction here.

- Replace writev() with two calls to _raw_write().  I'm not attached to
  this design; if someone has a preference for allocating a buffer and
  making a single write from that, I could be persuaded.  I don't know
  what the performance tradeoffs are.

- Typo fixes.  I need to figure out spellchecking in my editor.

- More use of .

- Change _should_crypto() functions to return bool.  Also rename them to
  be the _should_encrypt functions.

- Error message cleanup.

Thanks!
From 945805d45e8021f92ad73518b3a74ac6bab89525 Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharw...@redhat.com>
Date: Fri, 26 Feb 2016 16:07:05 -0500
Subject: [PATCH 1/3] Move common GSSAPI code into its own files

On both the frontend and backend, prepare for GSSAPI encryption suport
by moving common code for error handling into a common file.  Other than
build-system changes, no code changes occur in this patch.

Thanks to Michael Paquier for the Windows fixes.
---
 configure   |  2 +
 configure.in|  1 +
 src/Makefile.global.in  |  1 +
 src/backend/libpq/Makefile  |  4 ++
 src/backend/libpq/auth.c| 63 +---
 src/backend/libpq/be-gssapi-common.c| 74 +
 src/include/libpq/be-gssapi-common.h| 26 
 src/interfaces/libpq/Makefile   |  4 ++
 src/interfaces/libpq/fe-auth.c  | 48 +
 src/interfaces/libpq/fe-gssapi-common.c | 63 
 src/interfaces/libpq/fe-gssapi-common.h | 21 ++
 src/tools/msvc/Mkvcbuild.pm | 18 ++--
 12 files changed, 212 insertions(+), 113 deletions(-)
 create mode 100644 src/backend/libpq/be-gssapi-common.c
 create mode 100644 src/include/libpq/be-gssapi-common.h
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.c
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.h

diff --git a/configure b/configure
index b3f3abe..a5bd629 100755
--- a/configure
+++ b/configure
@@ -713,6 +713,7 @@ with_systemd
 with_selinux
 with_openssl
 krb_srvtab
+with_gssapi
 with_python
 with_perl
 with_tcl
@@ -5491,6 +5492,7 @@ $as_echo "$with_gssapi" >&6; }
 
 
 
+
 #
 # Kerberos configuration parameters
 #
diff --git a/configure.in b/configure.in
index 0bd90d7..4fd8f05 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/src/Makefile.global.in b/src/Makefile.global.in
index e94d6a5..3dbc5c2 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -183,6 +183,7 @@ with_perl	= @with_perl@
 with_python	= @with_python@
 with_tcl	= @with_tcl@
 with_openssl	= @with_openssl@
+with_gssapi	= @with_gssapi@
 with_selinux	= @with_selinux@
 with_systemd	= @with_systemd@
 with_libxml	= @with_libxml@
diff --git a/src/backend/libpq/Makefile b/src/backend/libpq/Makefile
index 09410c4..a8cc9a0 100644
--- a/src/backend/libpq/Makefile
+++ b/src/backend/libpq/Makefile
@@ -21,4 +21,8 @@ ifeq ($(with_openssl),yes)
 OBJS += be-secure-openssl.o
 endif
 
+ifeq ($(with_gssapi),yes)
+OBJS += be-gssapi-common.o
+endif
+
 include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 57c2f48..73d493e 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -136,11 +136,7 @@ bool		pg_krb_caseins_users;
  *
  */
 #ifdef ENABLE_GSS
-#if defined(HAVE_GSSAPI_H)
-#include 
-#else
-#include 
-#endif
+#include "libpq/be-gssapi-common.h"
 
 static int	pg_GSS_recvauth(Port *port);
 #endif   /* ENABLE_GSS */
@@ -715,63 +711,6 @@ recv_and_check_password_packet(Port *port, char **logdetail)
  */
 #ifdef ENABLE_GSS
 
-#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/gener

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

[Robbie Harwood]

Alvaro Herrera <alvhe...@2ndquadrant.com> writes:

> Robbie Harwood wrote:
>> Michael Paquier <michael.paqu...@gmail.com> 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

[Robbie Harwood]

Michael Paquier <michael.paqu...@gmail.com> writes:

> On Thu, Mar 31, 2016 at 2:14 PM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
>> On Wed, Mar 30, 2016 at 1:01 PM, Robbie Harwood <rharw...@redhat.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
> +   },
> +   _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_gssap

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

[Robbie Harwood]

Hello friends,

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!
From 3b62e99de16f2c4600d0bb02f3626e5157ecdc6c Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharw...@redhat.com>
Date: Fri, 26 Feb 2016 16:07:05 -0500
Subject: [PATCH 1/3] Move common GSSAPI code into its own files

On both the frontend and backend, prepare for GSSAPI encryption suport
by moving common code for error handling into a common file.  Other than
build-system changes, no code changes occur in this patch.
---
 configure   |  2 +
 configure.in|  1 +
 src/Makefile.global.in  |  1 +
 src/backend/libpq/Makefile  |  4 ++
 src/backend/libpq/auth.c| 63 +---
 src/backend/libpq/be-gssapi-common.c| 74 +
 src/include/libpq/be-gssapi-common.h| 26 
 src/interfaces/libpq/Makefile   |  4 ++
 src/interfaces/libpq/fe-auth.c  | 48 +
 src/interfaces/libpq/fe-gssapi-common.c | 63 
 src/interfaces/libpq/fe-gssapi-common.h | 21 ++
 11 files changed, 198 insertions(+), 109 deletions(-)
 create mode 100644 src/backend/libpq/be-gssapi-common.c
 create mode 100644 src/include/libpq/be-gssapi-common.h
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.c
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.h

diff --git a/configure b/configure
index b3f3abe..a5bd629 100755
--- a/configure
+++ b/configure
@@ -713,6 +713,7 @@ with_systemd
 with_selinux
 with_openssl
 krb_srvtab
+with_gssapi
 with_python
 with_perl
 with_tcl
@@ -5491,6 +5492,7 @@ $as_echo "$with_gssapi" >&6; }
 
 
 
+
 #
 # Kerberos configuration parameters
 #
diff --git a/configure.in b/configure.in
index 0bd90d7..4fd8f05 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/src/Makefile.global.in b/src/Makefile.global.in
index e94d6a5..3dbc5c2 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -183,6 +183,7 @@ with_perl	= @with_perl@
 with_python	= @with_python@
 with_tcl	= @with_tcl@
 with_openssl	= @with_openssl@
+with_gssapi	= @with_gssapi@
 with_selinux	= @with_selinux@
 with_systemd	= @with_systemd@
 with_libxml	= @with_libxml@
diff --git a/src/backend/libpq/Makefile b/src/backend/libpq/Makefile
index 09410c4..a8cc9a0 100644
--- a/src/backend/libpq/Makefile
+++ b/src/backend/libpq/Makefile
@@ -21,4 +21,8 @@ ifeq ($(with_openssl),yes)
 OBJS += be-secure-openssl.o
 endif
 
+ifeq ($(with_gssapi),yes)
+OBJS += be-gssapi-common.o
+endif
+
 include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 57c2f48..73d493e 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -136,11 +136,7 @@ bool		pg_krb_caseins_users;
  *
  */
 #ifdef ENABLE_GSS
-#if defined(HAVE_GSSAPI_H)
-#include 
-#else
-#include 
-#endif
+#include "libpq/be-gssapi-common.h"
 
 static int	pg_GSS_recvauth(Port *port);
 #endif   /* ENABLE_GSS */
@@ -715,63 +711,6 @@ recv_and_check_password_packet(Port *port, char **logdetail)
  */
 #ifdef ENABLE_GSS
 
-#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 = _C_NT_USER_NAME_desc;
-#endif
-
-
-static void
-pg_GSS_error(int severity, char *errmsg, OM_uint32 maj_stat, OM_uint32 min_stat)
-{
-	gss_buffer_desc gmsg;
-	OM_uint32	lmin_s,
-msg_ctx;

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

[Robbie Harwood]

David Steele <da...@pgmasters.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 = _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


signature.asc
Description: PGP signature

Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Robbie Harwood]

Christian Ullrich  writes:

> Updated patch attached.

Okay, I am happy now.  Thanks!


signature.asc
Description: PGP signature

Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used

[Robbie Harwood]

Christian Ullrich  writes:

> Updated patch attached.

I unfortunately don't have windows machines to test this on, but I
thought it might be helpful to review this anyway since I'm touching
code in the same general area (GSSAPI).  And as far as I can tell, you
don't break anything there; master continues to behave as expected.

Some comments inline:

>   pg_SSPI_recvauth(Port *port)
>   {
>   int mtype;
> + int status;

The section of this function for include_realm checking already uses an
int status return code (retval).  I would expect to see them share a
variable rather than have both "retval" and "status".

> + status = pg_SSPI_make_upn(accountname, sizeof(accountname),
> +   domainname, 
> sizeof(domainname),
> +   
> port->hba->upn_username);

This is the only place I see that this function is called.  That being
the case, why bother with the sizes of parameters?  Why doesn't
pg_SSPI_make_upn() just call sizeof() by itself rather than taking those
as arguments?

> + /* Build SAM name (DOMAIN\\user), then translate to UPN
> +(user@kerberos.realm). The realm name is returned in
> +lower case, but that is fine because in SSPI auth,
> +string comparisons are always case-insensitive. */

Since we're already considering changing things: this is not the comment
style for this file (though it is otherwise a good comment).

> + upname = (char*)palloc(upnamesize);

I don't believe this cast is typically included.

> + /* Replace domainname with realm name. */
> + if (upnamerealmsize > domainnamesize)
> + {
> + pfree(upname);
> + ereport(LOG,
> + (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
> +  errmsg("realm name too long")));
> +  return STATUS_ERROR;
> + }
> + 
> + /* Length is now safe. */
> + strcpy(domainname, p+1);

Is this an actual fail state or something born out of convenience?  A
naive reading of this code doesn't explain why it's forbidden for the
upn realm to be longer than the domain name.

> + /* Replace account name as well (in case UPN != SAM)? */
> + if (update_accountname)
> + {
> + if ((p - upname + 1) > accountnamesize)
> + {
> + pfree(upname);
> + ereport(LOG,
> + 
> (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
> +  errmsg("translated account name too 
> long")));
> +  return STATUS_ERROR;
> + }
> + 
> + *p = 0;
> + strcpy(accountname, upname);

Same as above.

Minus the few small things above, this looks good to me, assuming it
resolves the issue.

--Robbie


signature.asc
Description: PGP signature

[HACKERS] [PATCH v8] GSSAPI encryption support

[Robbie Harwood]

Hello friends,

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/gssencrypt8

What changed:

- Fixed fallback in the new server/old client case.  The server flag
  checking has been reduced.  In the (distant) future when all old
  clients are gone, the checking should be increased in strength once
  again.

- Fixed fallback in the old server/new client case.  This consists of
  checking whether the first message we receive after auth is done is an
  error, and raising it without decrypting if it is so that reconnection
  can happen if desired.  The quality of the fallback path has also
  generally been improved.

- Removed overzealous whitespace cleanup.  I'm a packager and have been
  bitten by upstreams doing this; I should have known better.

- Made flushing the AUTH_REQ_OK message conditional again.

- Fixed typo in server error message for insufficient GSSAPI protection.

Thanks!
From 3b62e99de16f2c4600d0bb02f3626e5157ecdc6c Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharw...@redhat.com>
Date: Fri, 26 Feb 2016 16:07:05 -0500
Subject: [PATCH 1/3] Move common GSSAPI code into its own files

On both the frontend and backend, prepare for GSSAPI encryption suport
by moving common code for error handling into a common file.  Other than
build-system changes, no code changes occur in this patch.
---
 configure   |  2 +
 configure.in|  1 +
 src/Makefile.global.in  |  1 +
 src/backend/libpq/Makefile  |  4 ++
 src/backend/libpq/auth.c| 63 +---
 src/backend/libpq/be-gssapi-common.c| 74 +
 src/include/libpq/be-gssapi-common.h| 26 
 src/interfaces/libpq/Makefile   |  4 ++
 src/interfaces/libpq/fe-auth.c  | 48 +
 src/interfaces/libpq/fe-gssapi-common.c | 63 
 src/interfaces/libpq/fe-gssapi-common.h | 21 ++
 11 files changed, 198 insertions(+), 109 deletions(-)
 create mode 100644 src/backend/libpq/be-gssapi-common.c
 create mode 100644 src/include/libpq/be-gssapi-common.h
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.c
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.h

diff --git a/configure b/configure
index b3f3abe..a5bd629 100755
--- a/configure
+++ b/configure
@@ -713,6 +713,7 @@ with_systemd
 with_selinux
 with_openssl
 krb_srvtab
+with_gssapi
 with_python
 with_perl
 with_tcl
@@ -5491,6 +5492,7 @@ $as_echo "$with_gssapi" >&6; }
 
 
 
+
 #
 # Kerberos configuration parameters
 #
diff --git a/configure.in b/configure.in
index 0bd90d7..4fd8f05 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/src/Makefile.global.in b/src/Makefile.global.in
index e94d6a5..3dbc5c2 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -183,6 +183,7 @@ with_perl	= @with_perl@
 with_python	= @with_python@
 with_tcl	= @with_tcl@
 with_openssl	= @with_openssl@
+with_gssapi	= @with_gssapi@
 with_selinux	= @with_selinux@
 with_systemd	= @with_systemd@
 with_libxml	= @with_libxml@
diff --git a/src/backend/libpq/Makefile b/src/backend/libpq/Makefile
index 09410c4..a8cc9a0 100644
--- a/src/backend/libpq/Makefile
+++ b/src/backend/libpq/Makefile
@@ -21,4 +21,8 @@ ifeq ($(with_openssl),yes)
 OBJS += be-secure-openssl.o
 endif
 
+ifeq ($(with_gssapi),yes)
+OBJS += be-gssapi-common.o
+endif
+
 include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 57c2f48..73d493e 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -136,11 +136,7 @@ bool		pg_krb_caseins_users;
  *
  */
 #ifdef ENABLE_GSS
-#if defined(HAVE_GSSAPI_H)
-#include 
-#else
-#include 
-#endif
+#include "libpq/be-gssapi-common.h"
 
 static int	pg_GSS_recvauth(Port *port);
 #endif   /* ENABLE_GSS */
@@ -715,63 +711,6 @@ recv_and_check_password_packet(Port *port, char **logdetail)
  */
 #ifdef ENABLE_GSS
 
-#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 = _C_NT_USER_NAME_desc;
-#endif
-
-
-static void
-pg_GSS_error(int severity, char *errmsg, OM_uint32 maj_stat, OM_uint32 min_stat)
-{
-	gss_buffer_desc 

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

[Robbie Harwood]

Stephen Frost <sfr...@snowman.net> writes:

> Robbie,
>
> * Robbie Harwood (rharw...@redhat.com) wrote:
>> Michael Paquier <michael.paqu...@gmail.com> writes:
>> > -   maj_stat = gss_accept_sec_context(
>> > - _stat,
>> > +   maj_stat = gss_accept_sec_context(_stat,
>> >
>> > This is just noise.
>> 
>> You're not wrong, though I do think it makes the code more readable by
>> enforcing style and this is a "cleanup" commit.  I'll take it out if it
>> bothers you.
>
> First, thanks much for working on this, I've been following along the
> discussions and hope to be able to help move it to commit, once it's
> ready.
>
> Secondly, generally, speaking, we prefer that 'cleanup' type changes
> (particularly whitespace-only ones) are in independent commits which are
> marked as just whitespace/indentation changes.  We have a number of
> organizations which follow our code changes and it makes it more
> difficult on them to include whitespace/indentation changes with code
> changes.

Thanks for the clarification.  I'll be sure to take it out!


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Michael Paquier <michael.paqu...@gmail.com> writes:

> On Tue, Mar 15, 2016 at 3:12 PM, David Steele <da...@pgmasters.net> wrote:
>> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>>> Here's yet another version of GSSAPI encryption support.
>>
>> This looks far more stable than last versions, cool to see the
>> progress. pgbench -C does not complain on my side so that's a good
>> thing. This is not yet a detailed review, there are a couple of
>> things that I find strange in what you did and are potential subject
>> to bugs, but I need a bit of time to let that mature a bit. This is
>> not something yet committable, but I really like the direction that
>> the patch is taking.

Thanks!  I must admit a preference for receiving all feedback at once
(reduces back-and-forth overhead), but if you feel there are still
design-type problems then that's very reasonable.  (I also admit to
feeling the pressure of feature freeze in less than a month.)

> For now, regarding 0002:
> /*
> -* Flush message so client will see it, except for AUTH_REQ_OK, which need
> -* not be sent until we are ready for queries.
> +* 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.
>  */
> -   if (areq != AUTH_REQ_OK)
> -   pq_flush();
> +   pq_flush();
> Er, this sends unconditionally the message without caring about the
> protocol, and so this is incorrect, no?

My impression from reading the old version of the comment above it was
that on other protocols it could go either way.  I think last time I
made it conditional; I can do so again if it is desired.  It's certainly
not /incorrect/ to send it immediately; there's just a question of
performance by minimizing the number of writes as far as I can tell.

> +#ifdef ENABLE_GSS
> +   if (conn->gss->buf.data)
> +   pfree(conn->gss->buf.data);
> +   if (conn->gss->writebuf.data)
> +   pfree(conn->gss->writebuf.data);
> +#endif
> You should use resetStringInfo here.

That will leak since resetStringInfo() doesn't free the underlying
representation.

>> OK, everything seems to be working fine with a 9.6 client and server so
>> next I tested older clients and I got this error:
>>
>> $ /usr/lib/postgresql/9.1/bin/psql -h localhost \
>>   -U vagr...@pgmasters.net postgres
>> psql: FATAL:  GSSAPI did no provide required flags
>>
>> There's a small typo in the error message, BTW.

Thanks, will fix.  I forgot that MIT doesn't provide GSS_C_REPLAY_FLAG
and GSS_C_SEQUENCE_FLAG by default.  Removing those from auth.c should
temporarily resolve the problem, which is what I'll do in the next
version.  (Tested with MIT krb5.)

On the subject of older code, I realized (one of those wake up in the
middle of the night-type moments) that the old server has a potential
problem with new clients now in that we try to decrypt the "help I don't
recognize connection parameter gss_encrypt" error message.  v8 will have
some sort of a fix, though I don't know what yet.  The only thing I've
come up with so far is to have the client decrypt check the first time
through for packets beginning with 'E'.  Pretty much anything I can do
will end up being a poor substitute for being at the protocol layer
anyway.

> And in 0003, the previous error is caused by that:
> +   target_flags = GSS_C_MUTUAL_FLAG | GSS_C_REPLAY_FLAG |
> +   GSS_C_SEQUENCE_FLAG | GSS_C_CONF_FLAG | GSS_C_INTEG_FLAG;
> +   if ((gflags & target_flags) != target_flags)
> +   {
> +   ereport(FATAL, (errmsg("GSSAPI did no provide required flags")));
> +   return STATUS_ERROR;
> +   }
> Yeah, this is a recipe for protocol incompatibility and here the
> connection context is not yet fully defined I believe. We need to be
> careful.

Nope, it's done.  This is happening immediately prior to username
checks.  By the time we exit the do/while the context is fully
complete.

> -   maj_stat = gss_accept_sec_context(
> - _stat,
> +   maj_stat = gss_accept_sec_context(_stat,
>
> This is just noise.

You're not wrong, though I do think it makes the code more readable by
enforcing style and this is a "cleanup" commit.  I'll take it out if it
bothers you.


signature.asc
Description: PGP signature

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

[Robbie Harwood]

David Steele <da...@pgmasters.net> writes:

> On 3/14/16 4:10 PM, Robbie Harwood wrote:
>
>> David Steele <da...@pgmasters.net> writes:
>>
>>> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>>>
>>>> Here's yet another version of GSSAPI encryption support.  It's also
>>>> available for viewing on my github:
>>>
>>> psql simply hangs and never returns.  I have attached a pcap of the
>>> psql/postgres session generated with:
>> 
>> Please disregard my other email.  I think I've found the issue; will
>> post a new version in a moment.
>
> Strange timing since I was just testing this.  Here's what I got:
>
> $ pg/bin/psql -h localhost -U vagr...@pgmasters.net postgres
> conn->inStart = 179, conn->inEnd = 179, conn->inCursor = 179
> psql (9.6devel)
> Type "help" for help.
>
> postgres=>

Thanks, that certainly is interesting!  I did finally manage to
reproduce the issue on my end, but the rate of incidence is much lower
than what you and Michael were seeing: I have to run connections in a
loop for about 10-20 minutes before it makes itself apparent (and no,
it's not due to entropy).  Apparently I just wasn't patient enough.

> This was after commenting out:
>
> // appendBinaryPQExpBuffer(>gwritebuf,
> //conn->inBuffer + conn->inStart,
> //conn->inEnd - conn->inStart);
> // conn->inEnd = conn->inStart;
>
> The good news I can log on every time now!

Since conn->inStart == conn->inEnd in the case you were testing, the
lines you commented out would have been a no-op anyway (that's the
normal case of operation, as far as I can tell).  That said, the chances
of hitting the race for me seemed very dependent on how much code wants
to run in that conditional: I got it up to 30-40 minutes when I added a
lot of printf()s (can't just run in gdb because it's nondeterministic
and rr has flushing bugs at the moment).

All that is to say: thank you very much for investigating that!


signature.asc
Description: PGP signature

[HACKERS] [PATCH v7] GSSAPI encryption support

[Robbie Harwood]

Hello friends,

New week, new version.  GitHub link:
https://github.com/frozencemetery/postgres/tree/feature/gssencrypt7

Changes in this version:

- Removed extra whitespace in auth code movement.

- Fixed connection desync issue.  A diff of this and v6 will reveal
  three issues:

  - First, that pg_GSS_read() didn't properly handle having a full
buffer when called because pqsecure_raw_read() doesn't handle reads of
size 0.  I've elected to change my own code here only, but it may be
desirable to change pqsecure_raw_read() as well depending on whether
other people are likely to hit that.

  - Second, that I was shunting data into the wrong buffer (don't know
how this was overlooked; it has "write" right there in the name).

  - Third, that I'm now immediately decrypting that data into
conn->inBuffer rather than deferring that step until later.  This
removes the hang because now the connection will not erroneously get
stuck polling while data is buffered.

Thanks!
From 3b62e99de16f2c4600d0bb02f3626e5157ecdc6c Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharw...@redhat.com>
Date: Fri, 26 Feb 2016 16:07:05 -0500
Subject: [PATCH 1/3] Move common GSSAPI code into its own files

On both the frontend and backend, prepare for GSSAPI encryption suport
by moving common code for error handling into a common file.  Other than
build-system changes, no code changes occur in this patch.
---
 configure   |  2 +
 configure.in|  1 +
 src/Makefile.global.in  |  1 +
 src/backend/libpq/Makefile  |  4 ++
 src/backend/libpq/auth.c| 63 +---
 src/backend/libpq/be-gssapi-common.c| 74 +
 src/include/libpq/be-gssapi-common.h| 26 
 src/interfaces/libpq/Makefile   |  4 ++
 src/interfaces/libpq/fe-auth.c  | 48 +
 src/interfaces/libpq/fe-gssapi-common.c | 63 
 src/interfaces/libpq/fe-gssapi-common.h | 21 ++
 11 files changed, 198 insertions(+), 109 deletions(-)
 create mode 100644 src/backend/libpq/be-gssapi-common.c
 create mode 100644 src/include/libpq/be-gssapi-common.h
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.c
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.h

diff --git a/configure b/configure
index b3f3abe..a5bd629 100755
--- a/configure
+++ b/configure
@@ -713,6 +713,7 @@ with_systemd
 with_selinux
 with_openssl
 krb_srvtab
+with_gssapi
 with_python
 with_perl
 with_tcl
@@ -5491,6 +5492,7 @@ $as_echo "$with_gssapi" >&6; }
 
 
 
+
 #
 # Kerberos configuration parameters
 #
diff --git a/configure.in b/configure.in
index 0bd90d7..4fd8f05 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/src/Makefile.global.in b/src/Makefile.global.in
index e94d6a5..3dbc5c2 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -183,6 +183,7 @@ with_perl	= @with_perl@
 with_python	= @with_python@
 with_tcl	= @with_tcl@
 with_openssl	= @with_openssl@
+with_gssapi	= @with_gssapi@
 with_selinux	= @with_selinux@
 with_systemd	= @with_systemd@
 with_libxml	= @with_libxml@
diff --git a/src/backend/libpq/Makefile b/src/backend/libpq/Makefile
index 09410c4..a8cc9a0 100644
--- a/src/backend/libpq/Makefile
+++ b/src/backend/libpq/Makefile
@@ -21,4 +21,8 @@ ifeq ($(with_openssl),yes)
 OBJS += be-secure-openssl.o
 endif
 
+ifeq ($(with_gssapi),yes)
+OBJS += be-gssapi-common.o
+endif
+
 include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 57c2f48..73d493e 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -136,11 +136,7 @@ bool		pg_krb_caseins_users;
  *
  */
 #ifdef ENABLE_GSS
-#if defined(HAVE_GSSAPI_H)
-#include 
-#else
-#include 
-#endif
+#include "libpq/be-gssapi-common.h"
 
 static int	pg_GSS_recvauth(Port *port);
 #endif   /* ENABLE_GSS */
@@ -715,63 +711,6 @@ recv_and_check_password_packet(Port *port, char **logdetail)
  */
 #ifdef ENABLE_GSS
 
-#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 = _C_NT_USER_NAME_desc;
-#endif
-
-
-static void
-pg_GSS_error(int severity, char *errmsg, OM_uint32 maj_stat, OM_uint32 min_stat)
-{
-

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

[Robbie Harwood]

David Steele <da...@pgmasters.net> writes:

> Hi Robbie,
>
> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>> Hello friends,
>> 
>> Here's yet another version of GSSAPI encryption support.  It's also
>> available for viewing on my github:
>
> The build went fine but when testing I was unable to logon at all.  I'm
> using the same methodology as in
> http://www.postgresql.org/message-id/56be0ff9.70...@pgmasters.net except
> that I'm running against 51c0f63 and using the v6 patch set.
>
> psql simply hangs and never returns.  I have attached a pcap of the
> psql/postgres session generated with:
>
> tcpdump -i lo -nnvvXSs 1514 port 5432 -w gssapi.pcap
>
> If you would like me to capture more information please let me know
> specifically how you would like me to capture it.

Please disregard my other email.  I think I've found the issue; will
post a new version in a moment.


signature.asc
Description: PGP signature

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

[Robbie Harwood]

David Steele <da...@pgmasters.net> writes:

> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>> 
>> Here's yet another version of GSSAPI encryption support.  It's also
>> available for viewing on my github:
>
> I got this warning when applying the first patch in the set:
>
> ../other/v6-0001-Move-common-GSSAPI-code-into-its-own-files.patch:245:
> new blank line at EOF.
> +
> warning: 1 line adds whitespace errors.

Hah, so it does.  Thanks for catching it; will fix.

> The build went fine but when testing I was unable to logon at all.  I'm
> using the same methodology as in
> http://www.postgresql.org/message-id/56be0ff9.70...@pgmasters.net except
> that I'm running against 51c0f63 and using the v6 patch set.
>
> psql simply hangs and never returns.  I have attached a pcap of the
> psql/postgres session generated with:
>
> tcpdump -i lo -nnvvXSs 1514 port 5432 -w gssapi.pcap
>
> If you would like me to capture more information please let me know
> specifically how you would like me to capture it.

Thank you for the pcap!  (I'm using wireshark so formats it can open are
greatly appreciated.)  This suggests that the hang is my client code's
fault, but just in case: I assume nothing unusual was logged on the
server?

v6-0002-Connection-encryption-support-for-GSSAPI.patch in fe-connect.c
at around line 2518 adds a call to appendBinaryPQExpBuffer and sets
conn->inEnd.  Can you try without those lines?

Can you also (e.g., with gdb or by adding printf calls) tell me what the
values of conn->inStart, conn->inEnd, and conn->inCursor any time
(should only be once) that those lines are triggered?

> I reverted to v5 and got the same behavior I was seeing with v4 and v5,
> namely that I can only logon occasionally and usually get this error:
>
> psql: expected authentication request from server, but received
>
> Using a fresh build from 51c0f63 I can logon reliably every time so I
> don't think there's an issue in my environment.

Agreed, I'm sure I've caused it somehow, though I don't know what's
wrong yet.  (And if it weren't my fault but you didn't get useful errors
out, that'd be my fault anyway for not checking enough stuff!)

I don't know if this would say anything relevant, but it might be
interesting to see what the results are of applying [1] to the v5 code.
It's the same approach to solving the problem, though it happens at a
different time due to the aforementioned protocol change between v5 and
v6.

Thanks,
--Robbie

[1] 
https://github.com/frozencemetery/postgres/commit/82c89227a6b499ac9273044f91cff747c154629f


signature.asc
Description: PGP signature

[HACKERS] [PATCH v6] GSSAPI encryption support

[Robbie Harwood]

Hello friends,

Here's yet another version of GSSAPI encryption support.  It's also
available for viewing on my github:

https://github.com/frozencemetery/postgres/tree/feature/gssencrypt6

Let me hit the highlights of this time around:

- Fallback code is back!  It's almost unchanged from early versions of
  this patchset.  Corresponding doc changes for this and the next item
  are of course included.

- Minor protocol change.  I did not realize that connection parameters
  were not read until after auth was complete, which means that in this
  version I go back to sending the AUTH_REQ_OK in the clear.  Though I
  found this initially irritating since it required re-working the
  should_crypto conditions, it ends up being a net positive since I can
  trade a library call for a couple variables.

- Client buffer flush on completion of authentication.  This should
  prevent the issue with the client getting unexpected message type of
  NUL due to encrypted data not getting decrypted.  I continue to be
  unable to replicate this issue, but since the codepath triggers in the
  "no data buffered case" all the math is sound.  (Famous last words I'm
  sure.)

- Code motion is its own patch.  This was requested and hopefully
  clarifies what's going on.

- Some GSSAPI authentication fixes have been applied.  I've been staring
  at this code too long now and writing this made me feel better.  If it
  should be a separate change that's fine and easy to do.

Thanks!
From 5674aa74effab4931bac1044f32dee83d915aa90 Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharw...@redhat.com>
Date: Fri, 26 Feb 2016 16:07:05 -0500
Subject: [PATCH 1/3] Move common GSSAPI code into its own files

On both the frontend and backend, prepare for GSSAPI encryption suport
by moving common code for error handling into a common file.  Other than
build-system changes, no code changes occur in this patch.
---
 configure   |  2 +
 configure.in|  1 +
 src/Makefile.global.in  |  1 +
 src/backend/libpq/Makefile  |  4 ++
 src/backend/libpq/auth.c| 63 +--
 src/backend/libpq/be-gssapi-common.c| 75 +
 src/include/libpq/be-gssapi-common.h| 26 
 src/interfaces/libpq/Makefile   |  4 ++
 src/interfaces/libpq/fe-auth.c  | 48 +
 src/interfaces/libpq/fe-gssapi-common.c | 63 +++
 src/interfaces/libpq/fe-gssapi-common.h | 21 +
 11 files changed, 199 insertions(+), 109 deletions(-)
 create mode 100644 src/backend/libpq/be-gssapi-common.c
 create mode 100644 src/include/libpq/be-gssapi-common.h
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.c
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.h

diff --git a/configure b/configure
index b3f3abe..a5bd629 100755
--- a/configure
+++ b/configure
@@ -713,6 +713,7 @@ with_systemd
 with_selinux
 with_openssl
 krb_srvtab
+with_gssapi
 with_python
 with_perl
 with_tcl
@@ -5491,6 +5492,7 @@ $as_echo "$with_gssapi" >&6; }
 
 
 
+
 #
 # Kerberos configuration parameters
 #
diff --git a/configure.in b/configure.in
index 0bd90d7..4fd8f05 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/src/Makefile.global.in b/src/Makefile.global.in
index e94d6a5..3dbc5c2 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -183,6 +183,7 @@ with_perl	= @with_perl@
 with_python	= @with_python@
 with_tcl	= @with_tcl@
 with_openssl	= @with_openssl@
+with_gssapi	= @with_gssapi@
 with_selinux	= @with_selinux@
 with_systemd	= @with_systemd@
 with_libxml	= @with_libxml@
diff --git a/src/backend/libpq/Makefile b/src/backend/libpq/Makefile
index 09410c4..a8cc9a0 100644
--- a/src/backend/libpq/Makefile
+++ b/src/backend/libpq/Makefile
@@ -21,4 +21,8 @@ ifeq ($(with_openssl),yes)
 OBJS += be-secure-openssl.o
 endif
 
+ifeq ($(with_gssapi),yes)
+OBJS += be-gssapi-common.o
+endif
+
 include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 57c2f48..73d493e 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -136,11 +136,7 @@ bool		pg_krb_caseins_users;
  *
  */
 #ifdef ENABLE_GSS
-#if defined(HAVE_GSSAPI_H)
-#include 
-#else
-#include 
-#endif
+#include "libpq/be-gssapi-common.h"
 
 static int	pg_GSS_recvauth(Port *port);
 #endif   /* ENABLE_GSS */
@@ -715,63 +711,6 @@ recv_and_check_password_packet(Port *port, char **logdetail)
  */
 #ifdef ENABLE_GSS
 
-#if defined(WIN32) && !defined(WIN32_ONLY_COMPILER)
-/*
- * MIT Kerberos GSSAPI DLL doesn't properly export the s

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

[Robbie Harwood]

Michael Paquier <michael.paqu...@gmail.com> writes:

> On Tue, Feb 16, 2016 at 2:45 AM, Robbie Harwood <rharw...@redhat.com> wrote:
>> David Steele <da...@pgmasters.net> writes:
>>> On 2/10/16 4:06 PM, Robbie Harwood wrote:
>>>> Hello friends,
>>>>
>>>> For your consideration, here is a new version of GSSAPI encryption
>>>> support.  For those who prefer, it's also available on my github:
>>>> https://github.com/frozencemetery/postgres/commit/c92275b6605d7929cda5551de47a4c60aab7179e
>>>
>>> It tried out this patch and ran into a few problems:
>>>
>>> 2) While I was able to apply the patch and get it compiled it seemed
>>> pretty flaky - I was only able to logon about 1 in 10 times on average.
>>>
>>> When not successful the client always output this incomplete message
>>> (without  terminating LF):
>>>
>>> psql: expected authentication request from server, but received
>>>
>>> From the logs I can see the server is reporting EOF from the client,
>>> though the client does not core dump and prints the above message before
>>> exiting.
>>>
>>> I have attached files that contain server logs at DEBUG5 and tcpdump
>>> output for both the success and failure cases.
>>>
>>> Please let me know if there's any more information you would like me to
>>> provide.
>>
>> What I can't tell from looking at your methodology is whether both the
>> client and server were running my patches or no.  There's no fallback
>> here (I'd like to talk about how that should work, with example from
>> v1-v3, if people have ideas).  This means that both the client and the
>> server need to be running my patches for the moment.  Is this your
>> setup?
>
> We need to be careful here, backward-compatibility is critical for
> both the client and the server, or to put it in other words, an
> uptables client should still be able to connect a patched server, and
> vice-versa. This is an area where it is important to not break any
> third-part tool, either using libpq or reimplementing the frontend
> protocol.

Which is why in my introduction to the v4 patch I explicitly mentioned
that this was missing.  I wanted to talk about how this should be
implemented, since I feel that I received no feedback on that design
From last time.  It's not hard to port that design over, if desired.

> So I finally began to dive into your new patch... And after testing
> this is breaking the authentication protocol for GSS. I have been able
> to connect to a server once, but at the second attempt and after
> connection is failing with the following error:
> psql: expected authentication request from server, but received ioltas

Interesting.  I will see if I can find anything.  The capture posted
earlier (thanks David!) suggests that the client doesn't expect the
encrypted data.

I suspect what has happened is a race between the client buffering data
From the socket and the client processing the completion of GSSAPI
authentication.  This kind of issue is why my v1 handled GSSAPI at the
protocol level, not at the transport level.  I think we end up with
encrypted data in the buffer that's supposed to be decrypted, and since
the GSSAPI blob starts with \x00, it doesn't know what to do.

I'll cut a v6 with most of the changes we've talked about here.  It
should address this issue, but I suspect that no one will be happy about
how, since the client essentially needs to "un-read" some data.

As a side note, this would also explain why I can't reproduce the issue,
since I'm running in very low-latency environments (three VMs on my
laptop).

> Also, something that is missing is the parametrization that has been
> discussed the last time this patch was on the mailing list. Where is
> the capacity to control if a client is connecting to a server that is
> performing encryption and downgrade to non-ecrypted connection should
> the server not be able to support it? Enforcing a client to require
> encryption support using pg_hba.conf was as well a good thing. Some
> more infrastructure is needed here, I thought that there was an
> agreement previously regarding that.

This does not match my impression of the discussion, but I would be
happy to be wrong about that since it is less work for me.

> Also, and to make the review a bit easier, it would be good to split
> the patch into smaller pieces (thanks for waiting on my input here,
> this became quite clear after looking at this code). Basically,
> pg_GSS_error is moved to a new file, bringing with it pg_GSS_recvauth
> because the error routine is being used by the new ones you are
> introducing: be_gssapi_write, etc. The

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

[Robbie Harwood]

David Steele <da...@pgmasters.net> writes:

> On 2/15/16 12:45 PM, Robbie Harwood wrote:
>> David Steele <da...@pgmasters.net> writes:
>>
>>> 1) It didn't apply cleanly to HEAD.  It did apply cleanly on a455878
>>> which I figured was recent enough for testing.  I didn't bisect to find
>>> the exact commit that broke it.
>> 
>> It applied to head of master (57c932475504d63d8f8a68fc6925d7decabc378a)
>> for me (`patch -p1 < v4-GSSAPI-encryption-support.patch`).  I rebased it
>> anyway and cut a v5 anyway, just to be sure.  It's attached, and
>> available on github as well:
>> https://github.com/frozencemetery/postgres/commit/dc10e3519f0f6c67f79abd157dc8ff1a1c293f53
>
> It could have been my mistake.  I'll give it another try when you have a
> new patch.

Please do let me know how v5 goes.  If you run into trouble, in addition
to the logs you helpfully provided before, I'd like a traffic dump (pcap
preferable; I need tcp/udp port 88 for Kerberos and tcp port 5432 or
whatever you're running postgres on) if possible.  Thanks!

>>> 2) While I was able to apply the patch and get it compiled it seemed
>>> pretty flaky - I was only able to logon about 1 in 10 times on average.
>>>  Here was my testing methodology:
>> 
>> What I can't tell from looking at your methodology is whether both the
>> client and server were running my patches or no.  There's no fallback
>> here (I'd like to talk about how that should work, with example from
>> v1-v3, if people have ideas).  This means that both the client and the
>> server need to be running my patches for the moment.  Is this your
>> setup?
>
> I was testing on a system with no version of PostgreSQL installed.  I
> applied your patch to master and then ran both server and client from
> that patched version.  Is there something I'm missing?

Not that I can immediately see.  As long as the client and server are
both patched, everything should work.  My process is the same as with
previous versions of this patchset [0], and though I'm using FreeIPA
there is no reason it shouldn't work with any other KDC (MIT, for
instance[1]) provided the IPA calls are converted.

I am curious, though - I haven't changed any of the authentication code
in v4/v5 from what's in ~master, so how often can you log in using
GSSAPI using master?

[0]: https://mivehind.net/2015/06/11/kerberized-postgresql/
[1]: http://web.mit.edu/kerberos/krb5-devel/doc/admin/install_kdc.html


signature.asc
Description: PGP signature

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

[Robbie Harwood]

David Steele <da...@pgmasters.net> writes:

> Hi Robbie,
>
> On 2/10/16 4:06 PM, Robbie Harwood wrote:
>> Hello friends,
>> 
>> For your consideration, here is a new version of GSSAPI encryption
>> support.  For those who prefer, it's also available on my github:
>> https://github.com/frozencemetery/postgres/commit/c92275b6605d7929cda5551de47a4c60aab7179e
>
> It tried out this patch and ran into a few problems:
>
> 1) It didn't apply cleanly to HEAD.  It did apply cleanly on a455878
> which I figured was recent enough for testing.  I didn't bisect to find
> the exact commit that broke it.

It applied to head of master (57c932475504d63d8f8a68fc6925d7decabc378a)
for me (`patch -p1 < v4-GSSAPI-encryption-support.patch`).  I rebased it
anyway and cut a v5 anyway, just to be sure.  It's attached, and
available on github as well:
https://github.com/frozencemetery/postgres/commit/dc10e3519f0f6c67f79abd157dc8ff1a1c293f53

> 2) While I was able to apply the patch and get it compiled it seemed
> pretty flaky - I was only able to logon about 1 in 10 times on average.
>  Here was my testing methodology:
>
> a) Build Postgres from a455878 (without your patch), install/configure
> Kerberos and get everything working.  I was able the set the auth method
> to gss in pg_hba.conf and logon successfully every time.
>
> b) On the same system rebuild Postgres from a455878 including your patch
> and attempt authentication.
>
> The problems arose after step 2b.  Sometimes I would try to logon twenty
> times without success and sometimes it only take five or six attempts.
> I was never able to logon successfully twice in a row.
>
> When not successful the client always output this incomplete message
> (without  terminating LF):
>
> psql: expected authentication request from server, but received
>
> From the logs I can see the server is reporting EOF from the client,
> though the client does not core dump and prints the above message before
> exiting.
>
> I have attached files that contain server logs at DEBUG5 and tcpdump
> output for both the success and failure cases.
>
> Please let me know if there's any more information you would like me to
> provide.

What I can't tell from looking at your methodology is whether both the
client and server were running my patches or no.  There's no fallback
here (I'd like to talk about how that should work, with example from
v1-v3, if people have ideas).  This means that both the client and the
server need to be running my patches for the moment.  Is this your
setup?

Thanks for taking it for a spin!
--Robbie

From dc10e3519f0f6c67f79abd157dc8ff1a1c293f53 Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharw...@redhat.com>
Date: Tue, 17 Nov 2015 18:34:14 -0500
Subject: [PATCH] Connect encryption support for GSSAPI

Existing GSSAPI authentication code is extended to support connection
encryption.  Connection begins as soon as possible - that is,
immediately after the client and server complete authentication.
---
 configure   |   2 +
 configure.in|   1 +
 doc/src/sgml/client-auth.sgml   |   2 +-
 doc/src/sgml/runtime.sgml   |  20 +-
 src/Makefile.global.in  |   1 +
 src/backend/libpq/Makefile  |   4 +
 src/backend/libpq/auth.c| 330 +---
 src/backend/libpq/be-gssapi.c   | 583 
 src/backend/libpq/be-secure.c   |  16 +
 src/backend/postmaster/postmaster.c |  12 +
 src/include/libpq/libpq-be.h|  31 ++
 src/interfaces/libpq/Makefile   |   4 +
 src/interfaces/libpq/fe-auth.c  | 182 ---
 src/interfaces/libpq/fe-auth.h  |   5 +
 src/interfaces/libpq/fe-connect.c   |  10 +
 src/interfaces/libpq/fe-gssapi.c| 474 +
 src/interfaces/libpq/fe-secure.c|  16 +-
 src/interfaces/libpq/libpq-int.h|  16 +-
 18 files changed, 1191 insertions(+), 518 deletions(-)
 create mode 100644 src/backend/libpq/be-gssapi.c
 create mode 100644 src/interfaces/libpq/fe-gssapi.c

diff --git a/configure b/configure
index b3f3abe..a5bd629 100755
--- a/configure
+++ b/configure
@@ -713,6 +713,7 @@ with_systemd
 with_selinux
 with_openssl
 krb_srvtab
+with_gssapi
 with_python
 with_perl
 with_tcl
@@ -5491,6 +5492,7 @@ $as_echo "$with_gssapi" >&6; }
 
 
 
+
 #
 # Kerberos configuration parameters
 #
diff --git a/configure.in b/configure.in
index 0bd90d7..4fd8f05 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..7d37223 100644
--- 

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

[Robbie Harwood]

Michael Paquier <michael.paqu...@gmail.com> writes:

> On Thu, Feb 11, 2016 at 6:06 AM, Robbie Harwood <rharw...@redhat.com> wrote:
>>
>> - The GSSAPI authentication code has been moved without modification.
>>   In doing so, the temptation to modify it (flags, error checking, that
>>   big comment at the top about things from Athena, etc.)  is very large.
>>   I do not know whether these changes are best suited to another patch
>>   in this series or should be reviewed separately.  I am also hesitant
>>   to add things beyond the core before I am told this is the right
>>   approach.
>
> I would recommend a different patch if code needs to be moved around.
> The move may make sense taken as an independent piece of the
> integration.

Sorry, are you suggesting separate patch for moving the GSS auth code,
or separate patch for changes to said code?  I am happy to move it if
so, just want to be sure.

> + * Portions Copyright (c) 2015-2016, Red Hat, Inc.
> + * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
> I think that this part may be a problem... Not sure the feeling of the
> others regarding additional copyright notices.

Good catch.  That's an accident (force of habit).  Since I'm pretty sure
this version won't be merged anyway, I'll drop it from the next one.

> It would be good to add that to the next CF, I will be happy to get a
> look at it.

Sounds good.  Thanks for looking at it!


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Hello friends,

For your consideration, here is a new version of GSSAPI encryption
support.  For those who prefer, it's also available on my github:
https://github.com/frozencemetery/postgres/commit/c92275b6605d7929cda5551de47a4c60aab7179e

Some thoughts:

- The overall design is different this time - GSS encryption sits in
  parallel construction to SSL encryption rather than at the protocol
  level - so a strict diff probably isn't useful.

- The GSSAPI authentication code has been moved without modification.
  In doing so, the temptation to modify it (flags, error checking, that
  big comment at the top about things from Athena, etc.)  is very large.
  I do not know whether these changes are best suited to another patch
  in this series or should be reviewed separately.  I am also hesitant
  to add things beyond the core before I am told this is the right
  approach.

- There's no fallback here.  I wrote fallback support for versions 1-3,
  and the same design could apply here without too much trouble, but I
  am hesitant to port it over before the encryption design is approved.
  I strongly suspect you will not want to merge this without fallback
  support, and that makes sense to me.

- The client and server code look a lot like each other.  This
  resemblance is not exact, and my understanding is that server and
  client need to compile independently, so I do not know of a way to
  rectify this.  Suggestions are welcome.

Thanks!
From c92275b6605d7929cda5551de47a4c60aab7179e Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharw...@redhat.com>
Date: Tue, 17 Nov 2015 18:34:14 -0500
Subject: [PATCH] Connect encryption support for GSSAPI

Existing GSSAPI authentication code is extended to support connection
encryption.  Connection begins as soon as possible - that is,
immediately after the client and server complete authentication.
---
 configure   |   2 +
 configure.in|   1 +
 doc/src/sgml/client-auth.sgml   |   2 +-
 doc/src/sgml/runtime.sgml   |  20 +-
 src/Makefile.global.in  |   1 +
 src/backend/libpq/Makefile  |   4 +
 src/backend/libpq/auth.c| 330 +---
 src/backend/libpq/be-gssapi.c   | 584 
 src/backend/libpq/be-secure.c   |  16 +
 src/backend/postmaster/postmaster.c |  12 +
 src/include/libpq/libpq-be.h|  31 ++
 src/interfaces/libpq/Makefile   |   4 +
 src/interfaces/libpq/fe-auth.c  | 182 ---
 src/interfaces/libpq/fe-auth.h  |   5 +
 src/interfaces/libpq/fe-connect.c   |  10 +
 src/interfaces/libpq/fe-gssapi.c| 475 +
 src/interfaces/libpq/fe-secure.c|  16 +-
 src/interfaces/libpq/libpq-int.h|  16 +-
 18 files changed, 1193 insertions(+), 518 deletions(-)
 create mode 100644 src/backend/libpq/be-gssapi.c
 create mode 100644 src/interfaces/libpq/fe-gssapi.c

diff --git a/configure b/configure
index 3dd1b15..7fd7610 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 9398482..b19932e 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..7d37223 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -915,7 +915,7 @@ omicron bryanh  guest1
 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.
+SSL or GSSAPI are used.

 

diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index cda05f5..bd8156f 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -1880,12 +1880,13 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
   
 
   
-   To prevent spoofing on TCP connections, the best solution is to use
-   SSL certificates and make sure that clients check the server's certificate.
-   To do that, the server
-   must be configured to accept only hostssl connections () and have SSL key and certificate files
-   (). The TCP client must connect using
+   To prevent spoofing on TCP connections, the best solutions are either to
+   use GSSAPI for authentication and encryption or to use SSL certificates and
+   make sure that clients check the server's certificate.  To secure using
+   SSL, the server must be configured to accept only hostssl
+ 

Re: [HACKERS] Building from git source on ubuntu with gssapi

[Robbie Harwood]

Jeff Janes  writes:

> I can't ./configure --with-gssapi from git on ubuntu 14.04.3 because:
>
> configure: error: gssapi.h header file is required for GSSAPI
>
> If I download the distribution-specific 9.3 source with apt, I find
> their secret sauce to make it work:
>
> ./debian/rules:LDFLAGS+= -Wl,--as-needed -L/usr/lib/mit-krb5
> -L/usr/lib/$(DEB_HOST_MULTIARCH)/mit-krb5
>
> ./debian/rules:CFLAGS+= -fPIC -pie -I/usr/include/mit-krb5
>
>
> Usually the packagers' secret sauce is there to change the
> installation locations and defaults and such, not to allow it to
> configure and compile at all.  It makes it a bit hard to test new code
> if you can't compile it without a bunch of messing around.
>
> Is there something we can and should do to make this compile directly
> out of git?

The preferred way to check for - and handle - GSSAPI/krb5 is with
pkg-config.  So, for instance, on my system:

rharwood@thriss:~$ pkg-config --exists krb5-gssapi
rharwood@thriss:~$ echo $?
0
rharwood@thriss:~$ pkg-config --libs krb5-gssapi
-L/usr/lib/x86_64-linux-gnu/mit-krb5 -lgssapi_krb5
rharwood@thriss:~$ pkg-config --cflags krb5-gssapi
-isystem /usr/include/mit-krb5 -isystem /usr/include/mit-krb5

In the past, this was done using krb5-config, but this has become
increasingly difficult to continue to make work (and krb5-config is not
nice code to work with).


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Andreas, can you please weigh in here since your voice is important to
this process?

Robbie Harwood <rharw...@redhat.com> writes:

> Andres Freund <and...@anarazel.de> 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 v2] GSSAPI encryption support

[Robbie Harwood]

Jeff Janes <jeff.ja...@gmail.com> writes:

> On Tue, Sep 29, 2015 at 7:53 AM, Robbie Harwood <rharw...@redhat.com> wrote:
>> Robbie Harwood <rharw...@redhat.com> writes:
>>
>>>>>> Michael Paquier <michael.paqu...@gmail.com> writes:
>>>>>>
>>>>>>> Well, the issue is still here: login through gssapi fails with
>>>>>>> your patch, not with HEAD. This patch is next on my review list by
>>>>>>> the way so I'll see what I can do about it soon even if I am in
>>>>>>> the US for Postgres Open next week. Still, how did you test it? I
>>>>>>> am just creating by myself a KDC, setting up a valid credential
>>>>>>> with kinit, and after setting up Postgres for this purpose the
>>>>>>> protocol communication just fails.
>>>
>>> I have no issues, no sync loss; nothing is amiss as far as I can see.
>>> If there is actually a problem here, I need more information from you.
>>> At the very least, as previously mentioned, I need to know what
>>> messages went over the wire to/from the server before it occurred, and
>>> what command (if it it made it to command processing) it was in the
>>> midst of sending.
>>
>> Any follow-up on this?  I'd really like my code to be bug-free.
>
> I don't know if this is worth posting as the patch is currently
> returned with feedback and you are redoing it in a different way, but
> with your patch I get this error when connecting:
>
> lost synchronization with server: got message type "T", length 27
> The connection to the server was lost. Attempting reset: Failed.
>
> I only get the error when connection to a patched server from a
> patched libpq.  If either is unpatched, then there is no problem.
>
> Let me know if this is worth looking into.

Definitely good to know, and I appreciate your testing.  It's probably
not worth looking into right now, but please do test the next version of
the code as well.

Thanks!
--Robbie


signature.asc
Description: PGP signature

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

[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

[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

[Robbie Harwood]

Michael Paquier <michael.paqu...@gmail.com> writes:

> On Tue, Oct 20, 2015 at 3:01 AM, Robbie Harwood wrote:
>> Stephen Frost <sfr...@snowman.net> 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

[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

[Robbie Harwood]

Craig Ringer <cr...@2ndquadrant.com> writes:

> On 14 October 2015 at 06:34, Robbie Harwood <rharw...@redhat.com> 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

[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)" <rharw...@redhat.com>
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.
+ 

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

[Robbie Harwood]

Michael Paquier <michael.paqu...@gmail.com> writes:

> On Sun, Oct 4, 2015 at 1:18 AM, Andres Freund <and...@anarazel.de> wrote:
>> Hi,
>>
>> I quickly read through the patch, trying to understand what exactly is
>> happening here. To me the way the patch is split doesn't make much sense
>> - I don't mind incremental patches, but right now the steps don't
>> individually make sense.
>
> I agree with Andres. While I looked a bit at this patch, I just had a
> look at them a whole block and not individually.

I'm hearing block from both of you!  Okay, if block is desired, I'll
squish for v3.  Sorry for the inconvenience.

>> On 2015-07-02 14:22:13 -0400, Robbie Harwood wrote:
>> [Andres' comments]
>
> Here are some comments on top of what Andres has mentioned.
>
> --- 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)
>
> I think that using a new configure variable like that with a dedicated
> file fe-secure-gss.c and be-secure-gss.c has little sense done this
> way, and that it would be more adapted to get everything grouped in
> fe-auth.c for the frontend and auth.c for the backend, or move all the
> GSSAPI-related stuff in its own file. I can understand the move
> though: this is to imitate OpenSSL in a way somewhat similar to what
> has been done for it with a rather generic set if routines, but with
> GSSAPI that's a bit different, we do not have such a set of routines,
> hence based on this argument moving it to its own file has little
> sense. Now, a move that would make sense though is to move all the
> GSSAPI stuff in its own file, for example pg_GSS_recvauth and
> pg_GSS_error for the backend, and you should do the same for the
> frontend with all the pg_GSS_* routines. This should be as well a
> refactoring patch on top of the actual feature.

My understanding is that frontend and backend code need to be separate
(for linking), so it's automatically in two places.  I really don't want
to put encryption-related code in files called "auth.c" and "fe-auth.c"
since those files are presumably for authentication, not encryption.

I'm not sure what you mean about "rather generic set if routines";
GSSAPI is a RFC-standardized interface.  I think I also don't understand
the last half of your above paragraph.

> diff --git a/src/interfaces/libpq/fe-secure-gss.c
> b/src/interfaces/libpq/fe-secure-gss.c
> new file mode 100644
> index 000..afea9c3
> --- /dev/null
> +++ b/src/interfaces/libpq/fe-secure-gss.c
> @@ -0,0 +1,92 @@
> +#include 
> You should add a proper header to those new files.

Sorry, what?


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Andres Freund <and...@anarazel.de> writes:

> Hi,

Hi, thanks for the review; I really appreciate your time in going
through this.  I have questions about some of your comments, so I'll
wait a bit before sending a v3.  (By the way, there is a v2 of this I've
already posted, though you seem to have replied to the v1.  The only
difference is in documentation, though.)

> I quickly read through the patch, trying to understand what exactly is
> happening here. To me the way the patch is split doesn't make much sense
> - I don't mind incremental patches, but right now the steps don't
> individually make sense.

That's fair.  Can you suggest a better organization?

> On 2015-07-02 14:22:13 -0400, Robbie Harwood wrote:
>> +#include 
>
> postgres.h should be the first header included.

Okay, will fix.

>> +size_t
>> +be_gss_encrypt(Port *port, char msgtype, const char **msgptr, size_t len)
>> +{
>> +OM_uint32 major, minor;
>> +gss_buffer_desc input, output;
>> +uint32 len_n;
>> +int conf;
>> +char *ptr = *((char **)msgptr);
>
> trivial nitpick, missing spaces...

Got it.

>> +int
>> +be_gss_inplace_decrypt(StringInfo inBuf)
>> +{
>> +OM_uint32 major, minor;
>> +gss_buffer_desc input, output;
>> +int qtype, conf;
>> +size_t msglen = 0;
>> +
>> +input.length = inBuf->len;
>> +input.value = inBuf->data;
>> +output.length = 0;
>> +output.value = NULL;
>> +
>> +major = gss_unwrap(, MyProcPort->gss->ctx, , ,
>> +   , NULL);
>> +if (GSS_ERROR(major))
>> +{
>> +pg_GSS_error(ERROR, gettext_noop("wrapping GSS message failed"),
>> + major, minor);
>> +return -1;
>> +}
>> +else if (conf == 0)
>> +{
>> +ereport(COMMERROR,
>> +(errcode(ERRCODE_PROTOCOL_VIOLATION),
>> + errmsg("Expected GSSAPI confidentiality but it 
>> was not received")));
>> +return -1;
>> +}
>
> Hm. Aren't we leaking the gss buffer here?
>
>> +qtype = ((char *)output.value)[0]; /* first byte is message type */
>> +inBuf->len = output.length - 5; /* message starts */
>> +
>> +memcpy((char *), ((char *)output.value) + 1, 4);
>> +msglen = ntohl(msglen);
>> +if (msglen - 4 != inBuf->len)
>> +{
>> +ereport(COMMERROR,
>> +(errcode(ERRCODE_PROTOCOL_VIOLATION),
>> + errmsg("Length value inside GSSAPI-encrypted 
>> packet was malformed")));
>> +return -1;
>> +}
>
> and here?
>
> Arguably it doesn't matter that much, since we'll usually disconnect
> around here, but ...

Probably better to be cautious about it.  Will fix.

>> diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
>> index a4b37ed..5a929a8 100644
>> --- a/src/backend/libpq/pqcomm.c
>> +++ b/src/backend/libpq/pqcomm.c
>> @@ -1485,6 +1485,19 @@ socket_putmessage(char msgtype, const char *s, size_t 
>> len)
>>  {
>>  if (DoingCopyOut || PqCommBusy)
>>  return 0;
>> +
>> +#ifdef ENABLE_GSS
>> +/* Do not wrap auth requests. */
>> +if (MyProcPort->hba->auth_method == uaGSS && gss_encrypt &&
>> +msgtype != 'R' && msgtype != 'g')
>> +{
>> +len = be_gss_encrypt(MyProcPort, msgtype, , len);
>> +if (len < 0)
>> +goto fail;
>> +msgtype = 'g';
>> +}
>> +#endif
>
> Putting encryption specific code here feels rather wrong to me.

Do you have a suggestion about where this code *should* go?  I need to
filter on the message type since some can't be encrypted.  I was unable
to find another place, but I may have missed it.

>> diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
>> index 6171ef3..58712fc 100644
>> --- a/src/include/libpq/libpq-be.h
>> +++ b/src/include/libpq/libpq-be.h
>> @@ -30,6 +30,8 @@
>>  #endif
>>  
>>  #ifdef ENABLE_GSS
>> +#include "lib/stringinfo.h"
>> +
>
> Conditionally including headers providing generic infrastructure strikes
> me as a recipe for build failures in different configurations.

That's fair, will fix.

>>  /* TCP keepalives configuration. These are no-ops on an AF_UNIX socket. */
>> diff --git a/src/include/libpq/libpq.h b/src/i

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

[Robbie Harwood]

Robbie Harwood <rharw...@redhat.com> writes:

>>>> Michael Paquier <michael.paqu...@gmail.com> writes:
>>>>
>>>>> Well, the issue is still here: login through gssapi fails with
>>>>> your patch, not with HEAD. This patch is next on my review list by
>>>>> the way so I'll see what I can do about it soon even if I am in
>>>>> the US for Postgres Open next week. Still, how did you test it? I
>>>>> am just creating by myself a KDC, setting up a valid credential
>>>>> with kinit, and after setting up Postgres for this purpose the
>>>>> protocol communication just fails.
>
> I have no issues, no sync loss; nothing is amiss as far as I can see.
> If there is actually a problem here, I need more information from you.
> At the very least, as previously mentioned, I need to know what
> messages went over the wire to/from the server before it occurred, and
> what command (if it it made it to command processing) it was in the
> midst of sending.

Any follow-up on this?  I'd really like my code to be bug-free.


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Michael Paquier <michael.paqu...@gmail.com> writes:

> On Thu, Sep 10, 2015 at 4:27 PM, Michael Paquier <michael.paqu...@gmail.com> 
> wrote:
>> On Thu, Sep 10, 2015 at 1:44 AM, Robbie Harwood <rharw...@redhat.com> wrote:
>>> Michael Paquier <michael.paqu...@gmail.com> writes:
>>>> On Wed, Sep 9, 2015 at 4:12 AM, Robbie Harwood wrote:
>>>>> Michael Paquier writes:
>>>>> As promised, here's a V2 to address your issues with comments.  I
>>>>> haven't heard back on the issues you found in testing, so no other
>>>>> changes are present.
>>>>
>>>> Well, the issue is still here: login through gssapi fails with your
>>>> patch, not with HEAD. This patch is next on my review list by the
>>>> way so I'll see what I can do about it soon even if I am in the US
>>>> for Postgres Open next week. Still, how did you test it? I am just
>>>> creating by myself a KDC, setting up a valid credential with kinit,
>>>> and after setting up Postgres for this purpose the protocol
>>>> communication just fails.
>>>
>>> My KDC is setup through freeIPA; I create a service for postgres,
>>> acquire a keytab, set it in the config file, and fire up the server.
>>> It should go without saying that this is working for me, which is
>>> why I asked you for more information so I could try to debug.  I
>>> wrote a post on this back in June when this was still in
>>> development:
>>> http://mivehind.net/page/view-page-slug/16/postgres-kerberos
>>
>> Hm. OK. I'll give it a try with freeipa and your patch with Fedora
>> for example. Could you as well try the configuration I have used? In
>> any case, it seems to me that we have a real problem with your patch:
>> the gss authentication protocol is broken with your patch and *not*
>> HEAD when using a custom kdc like the one I have set up manually on
>> one of my VMs.
>
> Looking more at this stuff. Your post assumes that you have an IPA
> server available (I am not really familiar with this software stack)
> already configured at hand so as you do not need to worry about any
> low-level configuration and a KDC is provided as well, among other
> things like ntpd or an apache instance. Well, the thing is that we
> just need is a KDC for this patch to have an environment suitable for
> testing, and some magic commands with kadmin.local, kinit, etc, and
> not the whole set of features that an IPA server provides (when
> kicking ipa-server-install one needs to provide a realm name, a KDC
> admin password, so that's basically just a wrapper setting up
> krb5.conf, which is handy when you want to have the full set in your
> hands actually, though just to test this patch it does not seem worth
> it). And I imagine that you do have an IPA server already set
> facilitating your work.
>
> Still, I gave it a try on a Fedora host, giving up after facing
> several failures when trying to install the server. because of several
> features.

I'm sorry to hear that FreeIPA didn't work for you.  I'd be remiss if I
didn't suggest you file bugs against the project for things that are
broken, though that's somewhat orthogonal to the patchset at hand.

I gave your setup a try; I spun up a fc22 machine (krb5v1.13.2), and
installed the KDC as per your instructions.  I only did two things
differently: I'm using the default unit file for krb5kdc, and I'm using
the postgres base dir /var/lib/pgsql/data (this is a Fedora default and
I'm sure it doesn't matter for purposes of this).

I have no issues, no sync loss; nothing is amiss as far as I can see.
If there is actually a problem here, I need more information from you.
At the very least, as previously mentioned, I need to know what messages
went over the wire to/from the server before it occurred, and what
command (if it it made it to command processing) it was in the midst of
sending.

Thanks,
--Robbie


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Michael Paquier <michael.paqu...@gmail.com> writes:

> On Wed, Sep 9, 2015 at 4:12 AM, Robbie Harwood wrote:
>> Michael Paquier writes:
>> As promised, here's a V2 to address your issues with comments.  I
>> haven't heard back on the issues you found in testing, so no other
>> changes are present.
>
> Well, the issue is still here: login through gssapi fails with your
> patch, not with HEAD. This patch is next on my review list by the way
> so I'll see what I can do about it soon even if I am in the US for
> Postgres Open next week. Still, how did you test it? I am just
> creating by myself a KDC, setting up a valid credential with kinit,
> and after setting up Postgres for this purpose the protocol
> communication just fails.

My KDC is setup through freeIPA; I create a service for postgres,
acquire a keytab, set it in the config file, and fire up the server.  It
should go without saying that this is working for me, which is why I
asked you for more information so I could try to debug.  I wrote a post
on this back in June when this was still in development:
http://mivehind.net/page/view-page-slug/16/postgres-kerberos


signature.asc
Description: PGP signature

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

[Robbie Harwood]

Michael Paquier <michael.paqu...@gmail.com> writes:

> On Fri, Jul 3, 2015 at 3:22 AM, Robbie Harwood <rharw...@redhat.com> wrote:
>
>> Hello -hackers,
>>
>> As previously discussed on this list, I have coded up GSSAPI encryption
>> support.  If it is easier for anyone, this code is also available for
>> viewing on my github:
>>
>> https://github.com/postgres/postgres/compare/master...frozencemetery:feature/gssencrypt
>>
>> Fallback support is present in both directions for talking to old client
>> and old servers; GSSAPI encryption is by default auto-upgraded to where
>> available (for compatibility), but both client and server contain
>> settings for requiring it.
>>
>> There are 8 commits in this series; I have tried to err on the side of
>> creating too much separation rather than too little.  A patch for each
>> is attached.  This is v1 of the series.
>
> I just had a quick look at this patch, and here are some comments:
> +   
> +If the client has probed GSSAPI encryption support
> and
> +the connection is GSSAPI-authenticated, then after
> the
> +server sends AuthenticationOk, all traffic between the client and
> server
> +will be GSSAPI-encrypted. Because
> +GSSAPI does not provide framing,
> +GSSAPI-encrypted messages are modeled after
> protocol-3
> +messages: the first byte is the caracter g, then four bytes of length,
> and
> +then an encrypted message.
> +   
> Message formats should be described in protocol.sgml in the section for
> message formats.
>
> +  network. In the pg_hba.conf file, the GSS authenticaion
> +  method has a parameter to require encryption; otherwise, connections
> +  will be encrypted if available and requiested by the client. On the
> s/authenticaion/authentication
> s/requiested/requested
>
> +Whether to require GSSAPI encryption.  Default is off, which causes
> +GSSAPI encryption to be enabled if available and requested for
> +compatability with old clients.  It is recommended to set this
> unless
> +old clients are present.
> s/compatability/compatibility

As promised, here's a V2 to address your issues with comments.  I
haven't heard back on the issues you found in testing, so no other
changes are present.

This means that only the last patch has changed.  For convenience, I
will therefore only provide this new patch.  I have also updated the
version available from my github.

Thanks!
From 2e9017a572a3097fecf2f7e53bf5f9aabf6ae36d Mon Sep 17 00:00:00 2001
From: "Robbie Harwood (frozencemetery)" <rharw...@redhat.com>
Date: Mon, 29 Jun 2015 15:29:36 -0400
Subject: [PATCH] Document GSSAPI encryption

---
 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 ++-
 4 files changed, 128 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index ba04bdf..0863468 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 a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 7940ef2..b80d29d 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1356,6 +1356,18 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
  
 
+ 
+  gss_enc_require
+  
+   
+If set, whether to require GSSAPI encryption support from the remote
+server. Defaults to unset, which will cause the client to fall back to
+not using GSSAPI encryption if the server does not support encryption
+through GSSAPI.
+   
+  
+ 
+
  
   service
 

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

[Robbie Harwood]

Michael Paquier michael.paqu...@gmail.com writes:

 On Fri, Jul 3, 2015 at 3:22 AM, Robbie Harwood rharw...@redhat.com wrote:

 There are 8 commits in this series; I have tried to err on the side of
 creating too much separation rather than too little.  A patch for each
 is attached.  This is v1 of the series.

 I just had a quick look at this patch, and here are some comments:

Hi!  Thanks for taking it for a spin.

 +   para
 +If the client has probed acronymGSSAPI/acronym encryption support and
 +the connection is acronymGSSAPI/acronym-authenticated, then after the
 +server sends AuthenticationOk, all traffic between the client and server
 +will be acronymGSSAPI/acronym-encrypted. Because
 +acronymGSSAPI/acronym does not provide framing,
 +acronymGSSAPI/acronym-encrypted messages are modeled after protocol-3
 +messages: the first byte is the caracter g, then four bytes of length, 
 and
 +then an encrypted message.
 +   /para

 Message formats should be described in protocol.sgml in the section for
 message formats.

ACK.  In next version of patch.

 +  network. In the filenamepg_hba.conf/ file, the GSS authenticaion
 +  method has a parameter to require encryption; otherwise, connections
 +  will be encrypted if available and requiested by the client. On the
 s/authenticaion/authentication
 s/requiested/requested

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

Thanks for catching these.  They'll be included in a new version of the
series, which I'll post once you and I have resolved the issue at the
bottom.

 Going through the docs, the overall approach taken by the patch looks neat,
 and the default values as designed for both the client and the server are
 good things to do. Now actually looking at the code I am suspecting that
 some code portions could be largely simplified in the authentication
 protocol code, though I don't have the time yet to look at that in details.

If there are ways to make it simpler without sacrificing clarity, I
welcome them.  Fresh eyes could definitely help with that!

 Also, when trying to connect with GSSAPI, I found the following problem:
 psql: lost synchronization with server: got message type S, length 22
 This happens whatever the value of require_encrypt on server-side is,
 either 0 or 1.

Well that's not good!  Since I'm not seeing this failure (even after
rebuilding my setup with patches applied to master), can you give me
more information here?  Since it's independent of require_encrypt, can
you verify it doesn't happen on master without my patches?  What
messages went over the wire to/from the server before this occurred (and
what was it trying to send at the time)?  Did you have valid
credentials?


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] libpq: Allow specifying multiple host names to try to connect to

[Robbie Harwood]

Steve Singer st...@ssinger.info writes:

 On 04/19/2015 11:18 AM, Mikko Tiihonen wrote:

 Hi,


 I would like allow specifying multiple host names for libpq to try to 
 connecting to. This is currently only supported if the host name 
 resolves to multiple addresses. Having the support for it without 
 complex dns setup would be much easier.


 Example:

 psql -h dbslave,dbmaster -p 5432 dbname

 psql 'postgresql://dbslave,dbmaster:5432/dbname'


 Here the idea is that without any added complexity of pgbouncer or 
 similar tool I can get any libpq client to try connecting to multiple 
 nodes until one answers. I have added the similar functionality to the 
 jdbc driver few years ago.


 Because libpq almost supported the feature already the patch is very 
 simple. I just split the given host name and do a dns lookup on each 
 separately, and link the results.


 If you configure a port that does not exist you can see the libpq 
 trying to connect to multiple hosts.


 psql -h 127.0.0.2,127.0.0.3, -p 

 psql: could not connect to server: Connection refused
 Is the server running on host 127.0.0.2,127.0.0.3 (127.0.0.2) 
 and accepting
 TCP/IP connections on port ?
 could not connect to server: Connection refused
 Is the server running on host 127.0.0.2,127.0.0.3 (127.0.0.3) 
 and accepting
 TCP/IP connections on port ?

 Further improvement would be to add a connection parameter to limit 
 connection only to master (writable) or to slave (read only).

 Another concern I have is that the server needs to be listening on the 
 same port against all hosts this means that in a development environment 
 we can't fully test this feature using just a single server.  I can't 
 think of anything else we have in core that couldn't be tested on a 
 single server (all the replication stuff works fine if you setup two 
 separate clusters on different ports on one server)

 You update the documentation just for  psql but your change effects any 
 libpq application if we go forward with this patch we should update the 
 documentation for libpq as well.

 This approach seems to work with the url style of conninfo

 For example
   postgres://some-down-host.info,some-other-host.org:5435/test1

 seems to work as expected but I don't like that syntax I would rather see
 postgres://some-down-host.info:5435/test1,postgres://some-other-host.org:5435/test1

 This would be a more invasive change but I think the syntax is more usable.

I agree with this; it seems to me that it's more powerful to be able to
specify complete urls for when they may differ.

For the non-url case though, I don't see a clean way of doing this.  We
could always, e.g., locally bind port specification to the closest host
specification, but that seems nasty, and is still less powerful than
passing urls (or we could just do the same for all parameters, but
that's just a mess).

Might it be reasonable to only allow the multi-host syntax in the
url-style and not otherwise?


signature.asc
Description: PGP signature

[HACKERS] [PATCH v1] GSSAPI encryption support

[Robbie Harwood]

Hello -hackers,

As previously discussed on this list, I have coded up GSSAPI encryption
support.  If it is easier for anyone, this code is also available for
viewing on my github:
https://github.com/postgres/postgres/compare/master...frozencemetery:feature/gssencrypt

Fallback support is present in both directions for talking to old client
and old servers; GSSAPI encryption is by default auto-upgraded to where
available (for compatibility), but both client and server contain
settings for requiring it.

There are 8 commits in this series; I have tried to err on the side of
creating too much separation rather than too little.  A patch for each
is attached.  This is v1 of the series.

Thanks!

From f506ba6ab6755f56c8aadba7d72a8839d5fbc0d9 Mon Sep 17 00:00:00 2001
From: Robbie Harwood (frozencemetery) rharw...@redhat.com
Date: Mon, 8 Jun 2015 19:27:45 -0400
Subject: build: Define with_gssapi for use in Makefiles

This is needed in order to control build of GSSAPI components.
---
 configure  | 2 ++
 configure.in   | 1 +
 src/Makefile.global.in | 1 +
 3 files changed, 4 insertions(+)

diff --git a/configure b/configure
index 0407c4f..b9bab06 100755
--- a/configure
+++ b/configure
@@ -711,6 +711,7 @@ with_uuid
 with_selinux
 with_openssl
 krb_srvtab
+with_gssapi
 with_python
 with_perl
 with_tcl
@@ -5452,6 +5453,7 @@ $as_echo $with_gssapi 6; }
 
 
 
+
 #
 # Kerberos configuration parameters
 #
diff --git a/configure.in b/configure.in
index 1de41a2..113bd65 100644
--- a/configure.in
+++ b/configure.in
@@ -635,6 +635,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/src/Makefile.global.in b/src/Makefile.global.in
index c583b44..e50c87d 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -167,6 +167,7 @@ with_perl	= @with_perl@
 with_python	= @with_python@
 with_tcl	= @with_tcl@
 with_openssl	= @with_openssl@
+with_gssapi 	= @with_gssapi@
 with_selinux	= @with_selinux@
 with_libxml	= @with_libxml@
 with_libxslt	= @with_libxslt@
-- 
2.1.4

From d5b973752968f87c9bb2ff9434d523657eb4ba67 Mon Sep 17 00:00:00 2001
From: Robbie Harwood (frozencemetery) rharw...@redhat.com
Date: Mon, 8 Jun 2015 20:16:42 -0400
Subject: client: Disable GSS encryption on old servers

---
 src/interfaces/libpq/fe-connect.c   | 34 --
 src/interfaces/libpq/fe-protocol3.c |  5 +
 src/interfaces/libpq/libpq-int.h|  1 +
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index a45f4cb..c6c551a 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -91,8 +91,9 @@ static int ldapServiceLookup(const char *purl, PQconninfoOption *options,
  * application_name in a startup packet.  We hard-wire the value rather
  * than looking into errcodes.h since it reflects historical behavior
  * rather than that of the current code.
+ * Servers that do not support GSS encryption will also return this error.
  */
-#define ERRCODE_APPNAME_UNKNOWN 42704
+#define ERRCODE_UNKNOWN_PARAM 42704
 
 /* This is part of the protocol so just define it */
 #define ERRCODE_INVALID_PASSWORD 28P01
@@ -2552,6 +2553,35 @@ keep_going:		/* We will come back to here until there is
 	if (res-resultStatus != PGRES_FATAL_ERROR)
 		appendPQExpBufferStr(conn-errorMessage,
 			 libpq_gettext(unexpected message from server during startup\n));
+#ifdef ENABLE_GSS
+	else if (!conn-gss_disable_enc)
+	{
+		/*
+		 * We tried to request GSS encryption, but the server
+		 * doesn't support it.  Hang up and try again.  A
+		 * connection that doesn't support appname will also
+		 * not support GSSAPI encryption, so this check goes
+		 * before that check.  See comment below.
+		 */
+		const char *sqlstate;
+
+		sqlstate = PQresultErrorField(res, PG_DIAG_SQLSTATE);
+		if (sqlstate 
+			strcmp(sqlstate, ERRCODE_UNKNOWN_PARAM) == 0)
+		{
+			OM_uint32 minor;
+
+			PQclear(res);
+			conn-gss_disable_enc = true;
+			/* Must drop the old connection */
+			pqDropConnection(conn);
+			conn-status = CONNECTION_NEEDED;
+			gss_delete_sec_context(minor, conn-gctx,
+   GSS_C_NO_BUFFER);
+			goto keep_going;
+		}
+	}
+#endif
 	else if (conn-send_appname 
 			 (conn-appname || conn-fbappname))
 	{
@@ -2569,7 +2599,7 @@ keep_going:		/* We will come back to here until there is
 
 		sqlstate = PQresultErrorField(res, PG_DIAG_SQLSTATE);
 		if (sqlstate 
-			strcmp(sqlstate, ERRCODE_APPNAME_UNKNOWN) == 0)
+			strcmp(sqlstate, ERRCODE_UNKNOWN_PARAM) == 0)
 		{
 			PQclear(res);
 			conn-send_appname = false;
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index

Re: [HACKERS] Postgres GSSAPI Encryption

[Robbie Harwood]

Robbie Harwood rharw...@redhat.com writes:

 Stephen Frost sfr...@snowman.net writes:

 Robbie,

 * Robbie Harwood (rharw...@redhat.com) wrote:

 We'd I think also want a new kind of HBA entry (probably something along
 the lines of `hostgss` to contrast with `hostssl`), but I'm not sure
 what we'd want to do for the counterpart of `hostnossl` (`hostnogss`?
 But then do we need `hostnogssnossl`?  Is this even a useful setting to
 have?), so that will probably require broader discussion.

 Are you intending to support GSSAPI encryption *without* using the
 GSSAPI authentication mechanism?  If not, maybe we can come up with a
 way to have an option to the GSSAPI auth mechanism that behaves the way
 we want for GSSAPI encryption.  Perhaps something like:

 encryption = (none | request | require)

 If you need an option for it to still be able to fall-thru to the next
 pg_hba line, that'd be more difficult, but is that really a sensible
 use-case?

 That's a good point.  I don't see GSSAPI encryption without GSSAPI
 authentication as a particularly compelling use case, so a setting like
 that (with default presumably to request for backward compatibility)
 seems perfect.

 As for fall-through on failure, I don't really know what's reasonable
 here.  My understanding of the way it works currently suggests that it
 would be *expected* to fall-through based on the way other things
 behave, though it's certainly less effort on my part if it does not.

 Finally, I think there are a couple different ways the protocol could
 look.  I'd ideally like to opportunistically encrypt all
 GSSAPI-authenticated connections and fallback to non-encrypted when the
 other end doesn't support it (possibly dropping the connection if this
 causes it to not match HBA), but I'm not sure if this will work with the
 existing code.  A (less-nice) option could be to add a new
 authentication method for gss-encryption, which feels aesthetically
 misplaced.  The approach used for SSL today could probably be adopted as
 a third approach, but I don't really see a gain from doing it this way
 since encryption happens after authentication (opposite of SSL) in
 GSSAPI.

 I'd definitely like to see us opportunistically encrypt all GSSAPI
 authenticated connections also.  The main case to consider is how to
 support migrating users who are currently using GSSAPI + SSL to GSSAPI
 auth+encryption, and how to support them if they want to continue to use
 GSSAPI just for user auth and use SSL for encryption.

 So if we go with the encryption option, we might not need a specific
 entry for GSS hosts.  For the SSL encryption/GSS auth case, we'd want it
 to work the way it does now where you specify hostssl and then gss
 as the method.  Then for the GSS for auth and encryption, one would use
 a normal host entry, then specify gss as the method, and then set
 encryption=require.

 One consequence of this would be that you can do double encryption by
 specifying hostssl, then gss with encrypt = require.  I don't
 think this is something more desirable than just one or the other, but
 unless it's actually a problem (and I don't think it is) to have around,
 I think the setup would work nicely.  We could also default encrypt to
 none when hostssl is specified if it is a problem.

I've coded up the GSSAPI encryption and is on my github[0].  It's
missing a number of things before merge, including proper error
handling, correct ACL behavior (by and large, it currently doesn't
respect hba.conf), and exposing configuration handles in hba.conf and
the client for the settings we've talked about above, as well as
documentation of all that.

What is here, importantly, is the fallback for old clients to new
servers and new clients to old servers.  A parameter is sent at the
start of normal connection (i.e., after SSL querying), and if the server
sees it, we encrypt; otherwise, the server will generate a failure
similar to the application_name case (and we will retry without it in
the same manner).

Thanks!
--Robbie

[0]: https://github.com/frozencemetery/postgres/


signature.asc
Description: PGP signature

Re: [HACKERS] Postgres GSSAPI Encryption

[Robbie Harwood]

Stephen Frost sfr...@snowman.net writes:

 Robbie,

 * Robbie Harwood (rharw...@redhat.com) wrote:

 We'd I think also want a new kind of HBA entry (probably something along
 the lines of `hostgss` to contrast with `hostssl`), but I'm not sure
 what we'd want to do for the counterpart of `hostnossl` (`hostnogss`?
 But then do we need `hostnogssnossl`?  Is this even a useful setting to
 have?), so that will probably require broader discussion.

 Are you intending to support GSSAPI encryption *without* using the
 GSSAPI authentication mechanism?  If not, maybe we can come up with a
 way to have an option to the GSSAPI auth mechanism that behaves the way
 we want for GSSAPI encryption.  Perhaps something like:

 encryption = (none | request | require)

 If you need an option for it to still be able to fall-thru to the next
 pg_hba line, that'd be more difficult, but is that really a sensible
 use-case?

That's a good point.  I don't see GSSAPI encryption without GSSAPI
authentication as a particularly compelling use case, so a setting like
that (with default presumably to request for backward compatibility)
seems perfect.

As for fall-through on failure, I don't really know what's reasonable
here.  My understanding of the way it works currently suggests that it
would be *expected* to fall-through based on the way other things
behave, though it's certainly less effort on my part if it does not.

 Finally, I think there are a couple different ways the protocol could
 look.  I'd ideally like to opportunistically encrypt all
 GSSAPI-authenticated connections and fallback to non-encrypted when the
 other end doesn't support it (possibly dropping the connection if this
 causes it to not match HBA), but I'm not sure if this will work with the
 existing code.  A (less-nice) option could be to add a new
 authentication method for gss-encryption, which feels aesthetically
 misplaced.  The approach used for SSL today could probably be adopted as
 a third approach, but I don't really see a gain from doing it this way
 since encryption happens after authentication (opposite of SSL) in
 GSSAPI.

 I'd definitely like to see us opportunistically encrypt all GSSAPI
 authenticated connections also.  The main case to consider is how to
 support migrating users who are currently using GSSAPI + SSL to GSSAPI
 auth+encryption, and how to support them if they want to continue to use
 GSSAPI just for user auth and use SSL for encryption.

So if we go with the encryption option, we might not need a specific
entry for GSS hosts.  For the SSL encryption/GSS auth case, we'd want it
to work the way it does now where you specify hostssl and then gss
as the method.  Then for the GSS for auth and encryption, one would use
a normal host entry, then specify gss as the method, and then set
encryption=require.

One consequence of this would be that you can do double encryption by
specifying hostssl, then gss with encrypt = require.  I don't
think this is something more desirable than just one or the other, but
unless it's actually a problem (and I don't think it is) to have around,
I think the setup would work nicely.  We could also default encrypt to
none when hostssl is specified if it is a problem.

Thanks!
--Robbie


signature.asc
Description: PGP signature

[HACKERS] Postgres GSSAPI Encryption

[Robbie Harwood]

Hello!

Today, there exists GSSAPI authentication support in Postgres.  I plan
to extend this work to include encryption as well, but wanted to get
your input on that first since you've probably thought about this
already.

From what I can tell, the auth/encryption layer is very nicely designed
for extensibility; adding an encryption mechanism should just involve
adding another option to the read/write functions in {f,b}e-secure.c,
and then creating {f,b}e-secure-gssapi.c (or similar) to populate from.

We'd I think also want a new kind of HBA entry (probably something along
the lines of `hostgss` to contrast with `hostssl`), but I'm not sure
what we'd want to do for the counterpart of `hostnossl` (`hostnogss`?
But then do we need `hostnogssnossl`?  Is this even a useful setting to
have?), so that will probably require broader discussion.

Finally, I think there are a couple different ways the protocol could
look.  I'd ideally like to opportunistically encrypt all
GSSAPI-authenticated connections and fallback to non-encrypted when the
other end doesn't support it (possibly dropping the connection if this
causes it to not match HBA), but I'm not sure if this will work with the
existing code.  A (less-nice) option could be to add a new
authentication method for gss-encryption, which feels aesthetically
misplaced.  The approach used for SSL today could probably be adopted as
a third approach, but I don't really see a gain from doing it this way
since encryption happens after authentication (opposite of SSL) in
GSSAPI.

I'm interested to hear your thoughts on this.

Thanks!

--Robbie


signature.asc
Description: PGP signature