Re: [HACKERS] [PATCH v12] GSSAPI encryption support
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
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
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
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
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 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
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
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 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
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
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
* 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
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
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
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
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
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
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(&minor, &output); >> +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 *)&input.length, port->gss->buf.data, 4); >> +input.length = ntohl(input.length); >> +enlargeStringInfo(&port->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, 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(&buf); > > /* > - * 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(&minor, port->gss->ctx, 1, GSS_C_QOP_DEFAULT, > + &input, &conf, &output); > + 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, &netlen, 4); > + > + appendBinaryStringInfo(&port->gss->writebuf, lenbuf, 4); > + appendBinaryStringInfo(&port->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(&minor, &output); > + 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(&minor, &output); > + > + 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,
Re: [HACKERS] [PATCH v12] GSSAPI encryption support
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
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 = >> > &GSS_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
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 = &GSS_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
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
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
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
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
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(&port->gss->buf); > + initStringInfo(&port->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 = &GSS_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 > worth a separate patch. Okay! It
Re: [HACKERS] [PATCH v12] GSSAPI encryption support
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(&port->gss->buf); + initStringInfo(&port->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 = &GSS_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
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 = &GSS_C_NT_USER_NAME_desc; -#endif - - -static void -pg_GSS_error(int severity, char *errmsg, OM_uint32 maj_stat, OM_uint32 min_stat) -{ - gss_buf