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

[Stephen Frost]

Robbie, all,

* Robbie Harwood (rharw...@redhat.com) wrote:
> Michael Paquier  writes:
> > On Wed, Jul 27, 2016 at 12:22 AM, Robbie Harwood  
> > wrote:
> >> Michael Paquier  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.

For my 2c, at least, I'd like the "prefer" option when it comes to
encryption where we try to use encryption if we're doing GSSAPI
authentication.  I'm not a big fan of having the GSSAPI layer doing
password prompts, but as long as the *only* thing that does is go
through the Kerberos library to acquire the tickets and still completely
talks GSSAPI with the server, it seems reasonable.

If we end up having to fall back to a non-encrypted GSSAPI
authenticated session then we should make noise about that.  If the
authentication is handled through GSSAPI but the connection is not
encrypted, but the user is told that immediately (eg: in the psql
startup), it seems like there's relativly little sensitive information
which has been exposed at that point and the user could destroy the
session then, if they're concerned about it.

Of course, that only works for user sessions, such as with psql, and
doesn't help with application connections, but hopefully application
authors who are using GSSAPI will read the docs sufficiently to know
that they should require an encrypted connection, if their environment
demands it.

Thanks!

Stephen


signature.asc
Description: Digital signature

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

[Robbie Harwood]

Michael Paquier  writes:

> On Wed, Jul 27, 2016 at 12:22 AM, Robbie Harwood  wrote:
>> Michael Paquier  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

[Michael Paquier]

On Wed, Jul 27, 2016 at 12:22 AM, Robbie Harwood  wrote:
> Michael Paquier  writes:
>
>> On Tue, Jul 26, 2016 at 5:58 AM, Robbie Harwood  wrote:
>>> Robbie Harwood  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.

OK we're good then.

 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)?

We don't need to do that far, users could still do the same with two
different lines in pg_hba.conf.

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


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

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

[Robbie Harwood]

Tom Lane  writes:

> Robbie Harwood  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

[Tom Lane]

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

regards, tom lane


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

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

[Robbie Harwood]

Robbie Harwood  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  writes:

> On Tue, Jul 26, 2016 at 5:58 AM, Robbie Harwood  wrote:
>> Robbie Harwood  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

[Michael Paquier]

On Tue, Jul 26, 2016 at 5:58 AM, Robbie Harwood  wrote:
> Robbie Harwood  writes:

Sorry for my late reply.

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

Having the communication layer in fe-secure.c definitely makes sense.
The startup process though should not use CONNECTION_SSL_STARTUP.

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

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

OK.

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

OK, that sounds good.

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

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


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

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

[Robbie Harwood]

Robbie Harwood  writes:

> Michael Paquier  writes:
>
>> On Thu, Apr 7, 2016 at 8:20 AM, Tom Lane  wrote:
>>> Robbie Harwood  writes:
 Tom Lane  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  writes:

> On Thu, Apr 7, 2016 at 8:20 AM, Tom Lane  wrote:
>> Robbie Harwood  writes:
>>> Tom Lane  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] [PATCH v12] GSSAPI encryption support

[Robert Haas]

On Fri, Apr 8, 2016 at 2:36 PM, Stephen Frost  wrote:
> While it seems like this particular patch (with myself as committer)
> would meet the requirements stated by the RMT for an extension, having
> considered it over the past day or so, I don't think we should make it a
> policy to allow an extension when it involves a significant rework of
> the patch, as is the case here.

I agree.  To be clear, those were intended as necessary but not
necessarily sufficient reasons for extension.  I agree that patches
needing significant reworking are not good candidates for extensions.
(But that is my feeling as an RMT member, not an RMT official policy
upon which we have voted.)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

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

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Thu, Apr 7, 2016 at 10:17 PM, Michael Paquier
>  wrote:
> > On Thu, Apr 7, 2016 at 8:20 AM, Tom Lane  wrote:
> >> Robbie Harwood  writes:
> >>> Tom Lane  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?
> 
> Yeah, I think so.  It doesn't seem we have consensus on this, and it's
> too late to be trying to build one now.

Actually, I chatted with Robbie quite a bit over IRC and he's agreed on
reworking this to use the same approach that we use for SSL, but that's
expected to take the better part of a week to do.

While it seems like this particular patch (with myself as committer)
would meet the requirements stated by the RMT for an extension, having
considered it over the past day or so, I don't think we should make it a
policy to allow an extension when it involves a significant rework of
the patch, as is the case here.

Robbie, please be sure to add this to the next commitfest and please
hound me to review it, you know where to find me. :)

Thanks!

Stephen


signature.asc
Description: Digital signature

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

[Robert Haas]

On Thu, Apr 7, 2016 at 10:17 PM, Michael Paquier
 wrote:
> On Thu, Apr 7, 2016 at 8:20 AM, Tom Lane  wrote:
>> Robbie Harwood  writes:
>>> Tom Lane  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?

Yeah, I think so.  It doesn't seem we have consensus on this, and it's
too late to be trying to build one now.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

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

[Michael Paquier]

On Thu, Apr 7, 2016 at 8:20 AM, Tom Lane  wrote:
> Robbie Harwood  writes:
>> Tom Lane  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?
-- 
Michael


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

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

[Tom Lane]

Robbie Harwood  writes:
> Tom Lane  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.

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

And any other GUC value that the user has decided to send via PGOPTIONS.
Whatever the merits of assuming that the username is okay to expose to
eavesdroppers, I dislike having to assume that there are not and never
will be any GUCs whose settings are potentially security-sensitive.

I really think you need to fix this so that the true startup packet
is not sent until the connection has been encrypted.  People are used
to assuming that's true with SSL encryption, and if GSSAPI is less
secure that's likely to lead to unexpected security weaknesses somewhere
down the line.

regards, tom lane


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

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

[Robbie Harwood]

Tom Lane  writes:

> Robbie Harwood  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

[Tom Lane]

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

(If I'm totally misunderstanding the context here, my apologies.  I've
not been following this thread.)

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

regards, tom lane


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

Re: [HACKERS] [PATCH 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

[Stephen Frost]

Robbie,

Just an initial pass over the patch.

* Robbie Harwood (rharw...@redhat.com) wrote:
> Here's v12, both here and on my github:
> https://github.com/frozencemetery/postgres/tree/feature/gssencrypt12

I've started taking a look at this as it's a capability I've wanted us
to support for a *long* time.

> Subject: [PATCH 1/3] Move common GSSAPI code into its own files

Didn't look too closely at this as it's mostly just moving stuff around.
I'll review it more closely once the other items are addressed though.

> Subject: [PATCH 2/3] Connection encryption support for GSSAPI

> diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
> index 73d493e..94d95bd 100644
> --- a/src/backend/libpq/auth.c
> +++ b/src/backend/libpq/auth.c
> @@ -596,10 +596,12 @@ sendAuthRequest(Port *port, AuthRequest areq)
>   pq_endmessage();
>  
>   /*
> -  * 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?

> diff --git a/src/backend/libpq/be-secure-gssapi.c 
> b/src/backend/libpq/be-secure-gssapi.c
[...]
> +/*
> + * Wrapper function indicating whether we are currently performing GSSAPI
> + * connection encryption.
> + *
> + * gss->encrypt is set when connection parameters are processed, which 
> happens
> + * immediately after AUTH_REQ_OK is sent.
> + */
> +static bool
> +be_gssapi_should_encrypt(Port *port)
> +{
> + if (port->gss->ctx == GSS_C_NO_CONTEXT)
> + return false;
> + return port->gss->encrypt;
> +}

be_gssapi_should_encrypt returns bool, which seems entirely reasonable,
but...

> +be_gssapi_write(Port *port, void *ptr, size_t len)
> +{
> + OM_uint32 major, minor;
> + gss_buffer_desc input, output;
> + ssize_t ret;
> + int conf;
> + uint32 netlen;
> + char lenbuf[4];
> +
> + ret = be_gssapi_should_encrypt(port);

Why are we storing the result into an ssize_t?

> + if (ret == -1)
> + return -1;
> + else if (ret == 0)
> + return secure_raw_write(port, ptr, len);

And then testing the result against -1...?  Or a bare 0 for that matter?

> + /* encrypt the message */
> + output.value = NULL;
> + output.length = 0;
> +
> + input.value = ptr;
> + input.length = len;
> +
> + conf = 0;
> + major = gss_wrap(, port->gss->ctx, 1, GSS_C_QOP_DEFAULT,
> +  , , );
> + if (GSS_ERROR(major))
> + {
> + pg_GSS_error(ERROR,
> +  gettext_noop("GSSAPI wrap error"),
> +  major, minor);
> + ret = -1;
> + goto cleanup;
> + }
> + else if (conf == 0)
> + {
> + ereport(FATAL, (errmsg("GSSAPI did not provide 
> confidentiality")));
> + ret = -1;
> + goto cleanup;
> + }
> +
> + /* format for on-wire: 4 network-order bytes of length, then payload */
> + netlen = htonl(output.length);
> + memcpy(lenbuf, , 4);
> +
> + appendBinaryStringInfo(>gss->writebuf, lenbuf, 4);
> + appendBinaryStringInfo(>gss->writebuf, output.value, 
> output.length);

That strikes me as a bit of overkill, we tend to just cast the pointer
to a (char *) rather than memcpy'ing the data just to get a different
pointer out of it.

> + /* 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?

> + cleanup:
> + if (output.value != NULL)
> + gss_release_buffer(, );
> +
> + return ret;
> +}

There's no need for any of this.  This goto will never be reached as
either a pg_GSS_error(ERROR) or an ereport(FATAL) isn't going to return
control to this path.  That's one of the reasons to be careful with
memory allocation and to use appropriate memory contexts, we're going to
longjmp out of this code path and clean up the memory allocations by
free'ing the contexts that we no longer need.  That is, on an ERROR
level failure, on FATAL, we're just going to exit, so we don't have to
worry about memory cleanup in that case.  Note that 

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

[Michael Paquier]

On Wed, Apr 6, 2016 at 5:58 AM, Alvaro Herrera  wrote:
> Robbie Harwood wrote:
>> Alvaro Herrera  writes:
>> > 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.
>
> Yeah, if you weren't using stringinfos that would be an option, but
> since you are I think that's out of the question.

To be honest, my first thought about that was to create a private
memory context only dedicated to those buffers, save it in pg_gssinfo
and remove those pfree calls as the memory context would clean up
itself with inheritance. Regarding the calloc/free stuff, should I be
a committer I would have given it a spin to see how the code gets
uglier or better and would have done a final decision depending on
that (you are true about the repalloc/pfree calls in memory contexts
btw, I forgot that).

>> 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?
>
> If two buffers is all, I think retail pfrees are fine.  I haven't
> actually read the patch.

Those are the only two buffers present per session.
-- 
Michael


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

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

[Michael Paquier]

On Wed, Apr 6, 2016 at 6:15 AM, Magnus Hagander  wrote:
> On Tue, Apr 5, 2016 at 7:58 PM, Robbie Harwood  wrote:
>>
>> > -#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

Thanks for the investigation! It would be interesting to see what at
least narwhal in the buildfarm thinks about that.

> That certainly looks like it fixes is. This was way too long ago for me to
> remember which versions I was using at the time though.

So, this is as well an indication that it would actually be fine :)

> It looks like it was already OK in the MSVC build back then, and only mingw
> was broken. Which makes it even more reasonable that they might've fixed it
> now - or a long time ago.
>
> If it works on reasonably modern mingw, then I suggest pushing it to the
> buildfarm and see what happens. But it definitely needs at least one round
> of building on mingw..

What about giving a spin first to patch 0003 as two different patches?
One to remove this check, and one to improve the error reporting
(which is an improvement in itself). Then we could rebase the rest on
top of it.
-- 
Michael


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

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

[Magnus Hagander]

On Tue, Apr 5, 2016 at 7:58 PM, Robbie Harwood  wrote:

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



That certainly looks like it fixes is. This was way too long ago for me to
remember which versions I was using at the time though.

It looks like it was already OK in the MSVC build back then, and only mingw
was broken. Which makes it even more reasonable that they might've fixed it
now - or a long time ago.

If it works on reasonably modern mingw, then I suggest pushing it to the
buildfarm and see what happens. But it definitely needs at least one round
of building on mingw..


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

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

[Alvaro Herrera]

Robbie Harwood wrote:
> Alvaro Herrera  writes:

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

Yeah, if you weren't using stringinfos that would be an option, but
since you are I think that's out of the question.

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

If two buffers is all, I think retail pfrees are fine.  I haven't
actually read the patch.

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


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

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

[David Steele]

On 4/5/16 1:25 AM, Michael Paquier wrote:

> Btw, those seem like small things to me, and my comments have been
> addressed, so I have switched the patch as ready for committer. I
> guess that Stephen would be the one to look at it.

I have also run this patch through my tests and didn't find any
problems.  I agree that it is ready for committer.

I don't see a problem with using the top memory context but whoever
commits it may feel differently.  I'm happy to leave that decision up to
them.

-- 
-David
da...@pgmasters.net


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

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

[Robbie Harwood]

Alvaro Herrera  writes:

> Robbie Harwood wrote:
>> Michael Paquier  writes:
>> 
>> > On Tue, Apr 5, 2016 at 9:06 AM, Robbie Harwood  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

[Alvaro Herrera]

Robbie Harwood wrote:
> Michael Paquier  writes:
> 
> > On Tue, Apr 5, 2016 at 9:06 AM, Robbie Harwood  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.)

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

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


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

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

[Robbie Harwood]

Michael Paquier  writes:

> On Tue, Apr 5, 2016 at 9:06 AM, Robbie Harwood  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 been present since the introduction of GSSAPI auth
support back in 2007.  I pinged Magnus on IRC, but I think I missed the
awake window for Sweden, so he's on CC as well.

> On 0003, +1 for reading the whole stack of messages. That's definitely
> 

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

[Michael Paquier]

On Tue, Apr 5, 2016 at 9:06 AM, Robbie Harwood  wrote:
> 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.

Affirnative, I am not seeing the failure anymore.

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

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

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

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


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

On 0003, +1 for reading the whole stack of messages. That's definitely
worth a separate patch.

Btw, those seem like small things to me, and my comments have been
addressed, so I have switched the patch as ready for committer. I
guess that Stephen would be the one to look at it.
-- 
Michael


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

Re: [HACKERS] [PATCH 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 
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_stat, OM_uint32