Обсуждение: Direct SSL connection with ALPN and HBA rules
Hi all, (Heikki in CC.) Since 91044ae4baea (require ALPN for direct SSL connections) and d39a49c1e459 (direct hanshake), direct SSL connections are supported (yeah!), still the thread where this has been discussed does not cover the potential impact on HBA rules: https://www.postgresql.org/message-id/CAM-w4HOEAzxyY01ZKOj-iq%3DM4-VDk%3DvzQgUsuqiTFjFDZaebdg%40mail.gmail.com My point is, would there be a point in being able to enforce that ALPN is used from the server rather than just relying on the client-side sslnegotiation to decide if direct SSL connections should be forced or not? Hence, I'd imagine that we could have an HBA option for hostssl rules, like a negotiation={direct,postgres,all} that cross-checks Port->alpn_used with the option value in a hostssl entry, rejecting the use of connections using direct connections or the default protocol if these are not used, giving users a way to avoid one. As this is a new thing, there may be an argument in this option for security reasons, as well, so as it would be possible for operators to turn that off in the server. Thoughts or comments? -- Michael
Вложения
On 19/04/2024 08:06, Michael Paquier wrote: > Hi all, > (Heikki in CC.) (Adding Jacob) > Since 91044ae4baea (require ALPN for direct SSL connections) and > d39a49c1e459 (direct hanshake), direct SSL connections are supported > (yeah!), still the thread where this has been discussed does not cover > the potential impact on HBA rules: > https://www.postgresql.org/message-id/CAM-w4HOEAzxyY01ZKOj-iq%3DM4-VDk%3DvzQgUsuqiTFjFDZaebdg%40mail.gmail.com > > My point is, would there be a point in being able to enforce that ALPN > is used from the server rather than just relying on the client-side > sslnegotiation to decide if direct SSL connections should be forced or > not? > > Hence, I'd imagine that we could have an HBA option for hostssl rules, > like a negotiation={direct,postgres,all} that cross-checks > Port->alpn_used with the option value in a hostssl entry, rejecting > the use of connections using direct connections or the default > protocol if these are not used, giving users a way to avoid one. As > this is a new thing, there may be an argument in this option for > security reasons, as well, so as it would be possible for operators to > turn that off in the server. I don't think ALPN gives any meaningful security benefit, when used with the traditional 'postgres' SSL negotiation. There's little possibility of mixing that up with some other protocol, so I don't see any need to enforce it from server side. This was briefly discussed on that original thread [1]. With direct SSL negotiation, we always require ALPN. I don't see direct SSL negotiation as a security feature. Rather, the point is to reduce connection latency by saving one round-trip. For example, if gssencmode=prefer, but the server refuses GSS encryption, it seems fine to continue with negotiated SSL, instead of establishing a new connection with direct SSL. What would be the use case of requiring direct SSL in the server? What extra security do you get? Controlling these in HBA is a bit inconvenient, because you only find out after authentication if it's allowed or not. So if e.g. direct SSL connections are disabled for a user, the client would still establish a direct SSL connection, send the startup packet, and only then get rejected. The client would not know if it was rejected because of the direct SSL or for some reason, so it needs to retry with negotiated SSL. Currently, as it is master, if the TLS layer is established with direct SSL, you never need to retry with traditional negotiation, or vice versa. [1] https://www.postgresql.org/message-id/CAAWbhmjetCVgu9pHJFkQ4ejuXuaz2mD1oniXokRHft0usCa7Yg%40mail.gmail.com -- Heikki Linnakangas Neon (https://neon.tech)
On Fri, Apr 19, 2024 at 6:56 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > On 19/04/2024 08:06, Michael Paquier wrote: > > Since 91044ae4baea (require ALPN for direct SSL connections) and > > d39a49c1e459 (direct hanshake), direct SSL connections are supported > > (yeah!), still the thread where this has been discussed does not cover > > the potential impact on HBA rules: > > https://www.postgresql.org/message-id/CAM-w4HOEAzxyY01ZKOj-iq%3DM4-VDk%3DvzQgUsuqiTFjFDZaebdg%40mail.gmail.com > > > > My point is, would there be a point in being able to enforce that ALPN > > is used from the server rather than just relying on the client-side > > sslnegotiation to decide if direct SSL connections should be forced or > > not? I'm a little confused about whether we're talking about requiring ALPN or requiring direct connections. I think you mean the latter, Michael? Personally, I was hoping that we'd have a postgresql.conf option to reject every network connection that wasn't direct SSL, but I ran out of time to review the patchset for 17. I would like to see server-side enforcement of direct SSL in some way, eventually. I hadn't given much thought to HBA, though. > I don't think ALPN gives any meaningful security benefit, when used with > the traditional 'postgres' SSL negotiation. There's little possibility > of mixing that up with some other protocol, so I don't see any need to > enforce it from server side. This was briefly discussed on that original > thread [1]. Agreed. By the time you've issued a traditional SSL startup packet, and the server responds with a go-ahead, it's pretty much understood what protocol is in use. > With direct SSL negotiation, we always require ALPN. (As an aside: I haven't gotten to test the version of the patch that made it into 17 yet, but from a quick glance it looks like we're not rejecting mismatched ALPN during the handshake as noted in [1].) > I don't see direct SSL negotiation as a security feature. `direct` mode is not, since it's opportunistic, but I think people are going to use `requiredirect` as a security feature. At least, I was hoping to do that myself... > Rather, the > point is to reduce connection latency by saving one round-trip. For > example, if gssencmode=prefer, but the server refuses GSS encryption, it > seems fine to continue with negotiated SSL, instead of establishing a > new connection with direct SSL. Well, assuming the user is okay with plaintext negotiation at all. (Was that fixed before the patch went in? Is GSS negotiation still allowed even with requiredirect?) > What would be the use case of requiring > direct SSL in the server? What extra security do you get? You get protection against attacks that could have otherwise happened during the plaintext portion of the handshake. That has architectural implications for more advanced uses of SCRAM, and it should prevent any repeats of CVE-2021-23222/23214. And if the peer doesn't pass the TLS handshake, they can't send you anything that you might forget is untrusted (like, say, an error message). > Controlling these in HBA is a bit inconvenient, because you only find > out after authentication if it's allowed or not. So if e.g. direct SSL > connections are disabled for a user, Hopefully disabling direct SSL piecemeal is not a desired use case? I'm not sure it makes sense to focus on that. Forcing it to be enabled shouldn't have the same problem, should it? --Jacob [1] https://www.postgresql.org/message-id/CAOYmi%2B%3DcnV-8V8TndSkEF6Htqa7qHQUL_KnQU8-DrT0Jjnm3_Q%40mail.gmail.com
On 19/04/2024 19:48, Jacob Champion wrote: > On Fri, Apr 19, 2024 at 6:56 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> With direct SSL negotiation, we always require ALPN. > > (As an aside: I haven't gotten to test the version of the patch that > made it into 17 yet, but from a quick glance it looks like we're not > rejecting mismatched ALPN during the handshake as noted in [1].) Ah, good catch, that fell through the cracks. Agreed, the client should reject a direct SSL connection if the server didn't send ALPN. I'll add that to the Open Items so we don't forget again. >> I don't see direct SSL negotiation as a security feature. > > `direct` mode is not, since it's opportunistic, but I think people are > going to use `requiredirect` as a security feature. At least, I was > hoping to do that myself... > >> Rather, the >> point is to reduce connection latency by saving one round-trip. For >> example, if gssencmode=prefer, but the server refuses GSS encryption, it >> seems fine to continue with negotiated SSL, instead of establishing a >> new connection with direct SSL. > > Well, assuming the user is okay with plaintext negotiation at all. > (Was that fixed before the patch went in? Is GSS negotiation still > allowed even with requiredirect?) To disable sending the startup packet in plaintext, you need to use sslmode=require. Same as before the patch. GSS is still allowed, as it takes precedence over SSL if both are enabled in libpq. Per the docs: > Note that if gssencmode is set to prefer, a GSS connection is > attempted first. If the server rejects GSS encryption, SSL is > negotiated over the same TCP connection using the traditional > postgres protocol, regardless of sslnegotiation. In other words, the > direct SSL handshake is not used, if a TCP connection has already > been established and can be used for the SSL handshake. >> What would be the use case of requiring >> direct SSL in the server? What extra security do you get? > > You get protection against attacks that could have otherwise happened > during the plaintext portion of the handshake. That has architectural > implications for more advanced uses of SCRAM, and it should prevent > any repeats of CVE-2021-23222/23214. And if the peer doesn't pass the > TLS handshake, they can't send you anything that you might forget is > untrusted (like, say, an error message). Can you elaborate on the more advanced uses of SCRAM? >> Controlling these in HBA is a bit inconvenient, because you only find >> out after authentication if it's allowed or not. So if e.g. direct SSL >> connections are disabled for a user, > > Hopefully disabling direct SSL piecemeal is not a desired use case? > I'm not sure it makes sense to focus on that. Forcing it to be enabled > shouldn't have the same problem, should it? Forcing it to be enabled piecemeal based on role or database has similar problems. Forcing it enabled for all connections seems sensible, though. Forcing it enabled based on the client's source IP address, but not user/database would be somewhat sensible too, but we don't currently have the HBA code to check the source IP and accept/reject SSLRequest based on that. The HBA rejection always happens after the client has sent the startup packet. -- Heikki Linnakangas Neon (https://neon.tech)
On Sat, Apr 20, 2024 at 12:43:24AM +0300, Heikki Linnakangas wrote: > On 19/04/2024 19:48, Jacob Champion wrote: >> On Fri, Apr 19, 2024 at 6:56 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: >>> With direct SSL negotiation, we always require ALPN. >> >> (As an aside: I haven't gotten to test the version of the patch that >> made it into 17 yet, but from a quick glance it looks like we're not >> rejecting mismatched ALPN during the handshake as noted in [1].) > > Ah, good catch, that fell through the cracks. Agreed, the client should > reject a direct SSL connection if the server didn't send ALPN. I'll add that > to the Open Items so we don't forget again. Would somebody like to write a patch for that? I'm planning to look at this code more closely, as well. >> You get protection against attacks that could have otherwise happened >> during the plaintext portion of the handshake. That has architectural >> implications for more advanced uses of SCRAM, and it should prevent >> any repeats of CVE-2021-23222/23214. And if the peer doesn't pass the >> TLS handshake, they can't send you anything that you might forget is >> untrusted (like, say, an error message). > > Can you elaborate on the more advanced uses of SCRAM? I'm not sure what you mean here, either, Jacob. >>> Controlling these in HBA is a bit inconvenient, because you only find >>> out after authentication if it's allowed or not. So if e.g. direct SSL >>> connections are disabled for a user, >> >> Hopefully disabling direct SSL piecemeal is not a desired use case? >> I'm not sure it makes sense to focus on that. Forcing it to be enabled >> shouldn't have the same problem, should it? I'd get behind the case where a server rejects everything except direct SSL, yeah. Sticking that into a format similar to HBA rules would easily give the flexibility to be able to accept or reject direct or default SSL, though, while making it easy to parse. The implementation is not really complicated, and not far from the existing hostssl and nohostssl. As a whole, I can get behind a unique GUC that forces the use of direct. Or, we could extend the existing "ssl" GUC with a new "direct" value to accept only direct connections and restrict the original protocol (and a new "postgres" for the pre-16 protocol, rejecting direct?), while "on" is able to accept both. > Forcing it to be enabled piecemeal based on role or database has similar > problems. Forcing it enabled for all connections seems sensible, though. > Forcing it enabled based on the client's source IP address, but not > user/database would be somewhat sensible too, but we don't currently have > the HBA code to check the source IP and accept/reject SSLRequest based on > that. The HBA rejection always happens after the client has sent the startup > packet. Hmm. Splitting the logic checking HBA entries (around check_hba) so as we'd check for a portion of its contents depending on what the server has received or not from the client would not be that complicated. I'd question whether it makes sense to mix this information within the same configuration files as the ones holding the current HBA rules. If the same rules are used for the pre-startup-packet phase and the post-startup-packet phase, we'd want new keywords for these HBA rules, something different than the existing sslmode and no sslmode? -- Michael
Вложения
On 22/04/2024 10:19, Michael Paquier wrote: > On Sat, Apr 20, 2024 at 12:43:24AM +0300, Heikki Linnakangas wrote: >> On 19/04/2024 19:48, Jacob Champion wrote: >>> On Fri, Apr 19, 2024 at 6:56 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: >>>> With direct SSL negotiation, we always require ALPN. >>> >>> (As an aside: I haven't gotten to test the version of the patch that >>> made it into 17 yet, but from a quick glance it looks like we're not >>> rejecting mismatched ALPN during the handshake as noted in [1].) >> >> Ah, good catch, that fell through the cracks. Agreed, the client should >> reject a direct SSL connection if the server didn't send ALPN. I'll add that >> to the Open Items so we don't forget again. > > Would somebody like to write a patch for that? I'm planning to look > at this code more closely, as well. I plan to write the patch later today. >>>> Controlling these in HBA is a bit inconvenient, because you only find >>>> out after authentication if it's allowed or not. So if e.g. direct SSL >>>> connections are disabled for a user, >>> >>> Hopefully disabling direct SSL piecemeal is not a desired use case? >>> I'm not sure it makes sense to focus on that. Forcing it to be enabled >>> shouldn't have the same problem, should it? > > I'd get behind the case where a server rejects everything except > direct SSL, yeah. Sticking that into a format similar to HBA rules > would easily give the flexibility to be able to accept or reject > direct or default SSL, though, while making it easy to parse. The > implementation is not really complicated, and not far from the > existing hostssl and nohostssl. > > As a whole, I can get behind a unique GUC that forces the use of > direct. Or, we could extend the existing "ssl" GUC with a new > "direct" value to accept only direct connections and restrict the > original protocol (and a new "postgres" for the pre-16 protocol, > rejecting direct?), while "on" is able to accept both. I'd be OK with that, although I still don't really see the point of forcing this from the server side. We could also add this later. >> Forcing it to be enabled piecemeal based on role or database has similar >> problems. Forcing it enabled for all connections seems sensible, though. >> Forcing it enabled based on the client's source IP address, but not >> user/database would be somewhat sensible too, but we don't currently have >> the HBA code to check the source IP and accept/reject SSLRequest based on >> that. The HBA rejection always happens after the client has sent the startup >> packet. > > Hmm. Splitting the logic checking HBA entries (around check_hba) so > as we'd check for a portion of its contents depending on what the > server has received or not from the client would not be that > complicated. I'd question whether it makes sense to mix this > information within the same configuration files as the ones holding > the current HBA rules. If the same rules are used for the > pre-startup-packet phase and the post-startup-packet phase, we'd want > new keywords for these HBA rules, something different than the > existing sslmode and no sslmode? Sounds complicated, and I don't really see the use case for controlling the direct SSL support in such a fine-grained fashion. It would be nice if we could reject non-SSL connections before the client sends the startup packet, but that's not possible because in a plaintext connection, that's the first packet that the client sends. The reverse would be possible: reject SSLRequest or direct SSL connection immediately, if HBA doesn't allow non-SSL connections from that IP address. But that's not very interesting. HBA-based control would certainly be v18 material. -- Heikki Linnakangas Neon (https://neon.tech)
On 22/04/2024 10:47, Heikki Linnakangas wrote: > On 22/04/2024 10:19, Michael Paquier wrote: >> On Sat, Apr 20, 2024 at 12:43:24AM +0300, Heikki Linnakangas wrote: >>> On 19/04/2024 19:48, Jacob Champion wrote: >>>> On Fri, Apr 19, 2024 at 6:56 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: >>>>> With direct SSL negotiation, we always require ALPN. >>>> >>>> (As an aside: I haven't gotten to test the version of the patch that >>>> made it into 17 yet, but from a quick glance it looks like we're not >>>> rejecting mismatched ALPN during the handshake as noted in [1].) >>> >>> Ah, good catch, that fell through the cracks. Agreed, the client should >>> reject a direct SSL connection if the server didn't send ALPN. I'll add that >>> to the Open Items so we don't forget again. >> >> Would somebody like to write a patch for that? I'm planning to look >> at this code more closely, as well. > > I plan to write the patch later today. Here's the patch for that. The error message is: "direct SSL connection was established without ALPN protocol negotiation extension" That's accurate, but I wonder if we could make it more useful to a user who's wondering what went wrong. I'd imagine that if the server doesn't support ALPN, it's because you have some kind of a (not necessarily malicious) generic SSL man-in-the-middle that doesn't support it. Or you're trying to connect to an HTTPS server. Suggestions welcome. -- Heikki Linnakangas Neon (https://neon.tech)
Вложения
On Mon, Apr 22, 2024 at 10:47:51AM +0300, Heikki Linnakangas wrote: > On 22/04/2024 10:19, Michael Paquier wrote: >> As a whole, I can get behind a unique GUC that forces the use of >> direct. Or, we could extend the existing "ssl" GUC with a new >> "direct" value to accept only direct connections and restrict the >> original protocol (and a new "postgres" for the pre-16 protocol, >> rejecting direct?), while "on" is able to accept both. > > I'd be OK with that, although I still don't really see the point of forcing > this from the server side. We could also add this later. I'd be OK with doing something only in v18, if need be. Jacob, what do you think? >> Hmm. Splitting the logic checking HBA entries (around check_hba) so >> as we'd check for a portion of its contents depending on what the >> server has received or not from the client would not be that >> complicated. I'd question whether it makes sense to mix this >> information within the same configuration files as the ones holding >> the current HBA rules. If the same rules are used for the >> pre-startup-packet phase and the post-startup-packet phase, we'd want >> new keywords for these HBA rules, something different than the >> existing sslmode and no sslmode? > > Sounds complicated, and I don't really see the use case for controlling the > direct SSL support in such a fine-grained fashion. > > It would be nice if we could reject non-SSL connections before the client > sends the startup packet, but that's not possible because in a plaintext > connection, that's the first packet that the client sends. The reverse would > be possible: reject SSLRequest or direct SSL connection immediately, if HBA > doesn't allow non-SSL connections from that IP address. But that's not very > interesting. I'm not completely sure, actually. We have the APIs to do that in simple ways with existing keywords and new options. And there is some merit being able to have more complex connection policies. If both of you object to that, I won't insist. > HBA-based control would certainly be v18 material. Surely. -- Michael
Вложения
On Tue, Apr 23, 2024 at 01:48:04AM +0300, Heikki Linnakangas wrote: > Here's the patch for that. The error message is: > > "direct SSL connection was established without ALPN protocol negotiation > extension" WFM. > That's accurate, but I wonder if we could make it more useful to a user > who's wondering what went wrong. I'd imagine that if the server doesn't > support ALPN, it's because you have some kind of a (not necessarily > malicious) generic SSL man-in-the-middle that doesn't support it. Or you're > trying to connect to an HTTPS server. Suggestions welcome. Hmm. Is there any point in calling SSL_get0_alpn_selected() in open_client_SSL() to get the ALPN if current_enc_method is not ENC_DIRECT_SSL? In the documentation of PQsslAttribute(), it is mentioned that empty string is returned for "alpn" if ALPN was not used, however the code returns NULL in this case: SSL_get0_alpn_selected(conn->ssl, &data, &len); if (data == NULL || len == 0 || len > sizeof(alpn_str) - 1) return NULL; -- Michael
Вложения
On Fri, Apr 19, 2024 at 2:43 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > > On 19/04/2024 19:48, Jacob Champion wrote: > > On Fri, Apr 19, 2024 at 6:56 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > >> With direct SSL negotiation, we always require ALPN. > > > > (As an aside: I haven't gotten to test the version of the patch that > > made it into 17 yet, but from a quick glance it looks like we're not > > rejecting mismatched ALPN during the handshake as noted in [1].) > > Ah, good catch, that fell through the cracks. Agreed, the client should > reject a direct SSL connection if the server didn't send ALPN. I'll add > that to the Open Items so we don't forget again. Yes, the client should also reject, but that's not what I'm referring to above. The server needs to fail the TLS handshake itself with the proper error code (I think it's `no_application_protocol`?); otherwise a client implementing a different protocol could consume the application-level bytes coming back from the server and act on them. That's the protocol confusion attack from ALPACA we're trying to avoid. > > Well, assuming the user is okay with plaintext negotiation at all. > > (Was that fixed before the patch went in? Is GSS negotiation still > > allowed even with requiredirect?) > > To disable sending the startup packet in plaintext, you need to use > sslmode=require. Same as before the patch. GSS is still allowed, as it > takes precedence over SSL if both are enabled in libpq. Per the docs: > > > Note that if gssencmode is set to prefer, a GSS connection is > > attempted first. If the server rejects GSS encryption, SSL is > > negotiated over the same TCP connection using the traditional > > postgres protocol, regardless of sslnegotiation. In other words, the > > direct SSL handshake is not used, if a TCP connection has already > > been established and can be used for the SSL handshake. Oh. That's actually disappointing, since gssencmode=prefer is the default. A question I had in the original thread was, what's the rationale behind a "require direct ssl" option that doesn't actually require it? > >> What would be the use case of requiring > >> direct SSL in the server? What extra security do you get? > > > > You get protection against attacks that could have otherwise happened > > during the plaintext portion of the handshake. That has architectural > > implications for more advanced uses of SCRAM, and it should prevent > > any repeats of CVE-2021-23222/23214. And if the peer doesn't pass the > > TLS handshake, they can't send you anything that you might forget is > > untrusted (like, say, an error message). > > Can you elaborate on the more advanced uses of SCRAM? If you're using SCRAM to authenticate the server, as opposed to just a really strong password auth, then it really helps an analysis of the security to know that there are no plaintext bytes that have been interpreted by the client. This came up briefly in the conversations that led to commit d0f4824a. To be fair, it's a more academic concern at the moment; my imagination can only come up with problems for SCRAM-based TLS that would also be vulnerabilities for standard certificate-based TLS. But whether or not it's an advantage for the code today is also kind of orthogonal to my point. The security argument of direct SSL mode is that it reduces risk for the system as a whole, even in the face of future code changes or regressions. If you can't force its use, you're not reducing that risk very much. (If anything, a "require" option that doesn't actually require it makes the analysis more complicated, not less...) > >> Controlling these in HBA is a bit inconvenient, because you only find > >> out after authentication if it's allowed or not. So if e.g. direct SSL > >> connections are disabled for a user, > > > > Hopefully disabling direct SSL piecemeal is not a desired use case? > > I'm not sure it makes sense to focus on that. Forcing it to be enabled > > shouldn't have the same problem, should it? > > Forcing it to be enabled piecemeal based on role or database has similar > problems. Hm. For some reason I thought it was easier the other direction, but I can't remember why I thought that. I'll withdraw the comment for now :) --Jacob
On Mon, Apr 22, 2024 at 10:42 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Apr 22, 2024 at 10:47:51AM +0300, Heikki Linnakangas wrote: > > On 22/04/2024 10:19, Michael Paquier wrote: > >> As a whole, I can get behind a unique GUC that forces the use of > >> direct. Or, we could extend the existing "ssl" GUC with a new > >> "direct" value to accept only direct connections and restrict the > >> original protocol (and a new "postgres" for the pre-16 protocol, > >> rejecting direct?), while "on" is able to accept both. > > > > I'd be OK with that, although I still don't really see the point of forcing > > this from the server side. We could also add this later. > > I'd be OK with doing something only in v18, if need be. Jacob, what > do you think? I think it would be nice to have an option like that. Whether it's done now or in 18, I don't have a strong opinion about. But I do think it'd be helpful to have a consensus on whether or not this is a security improvement, or a performance enhancement only, before adding said option. As it's implemented, if the requiredirect option doesn't actually requiredirect, I think it looks like security but isn't really. (My ideal server-side option removes all plaintext negotiation and forces the use of direct SSL for every connection, paired with a new postgresqls:// scheme for the client. But I don't have any experience making a switchover like that at scale, and I'd like to avoid a StartTLS-vs-LDAPS sort of situation. That's obviously not a conversation for 17.) As for HBA control: overall, I don't see a burning need for an HBA-based configuration, honestly. I'd prefer to reduce the number of knobs and make it easier to apply the strongest security with a broad brush. --Jacob
On Tue, Apr 23, 2024 at 1:22 PM Jacob Champion <jacob.champion@enterprisedb.com> wrote: > On Mon, Apr 22, 2024 at 10:42 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Apr 22, 2024 at 10:47:51AM +0300, Heikki Linnakangas wrote: > > > On 22/04/2024 10:19, Michael Paquier wrote: > > >> As a whole, I can get behind a unique GUC that forces the use of > > >> direct. Or, we could extend the existing "ssl" GUC with a new > > >> "direct" value to accept only direct connections and restrict the > > >> original protocol (and a new "postgres" for the pre-16 protocol, > > >> rejecting direct?), while "on" is able to accept both. > > > > > > I'd be OK with that, although I still don't really see the point of forcing > > > this from the server side. We could also add this later. > > > > I'd be OK with doing something only in v18, if need be. Jacob, what > > do you think? > > I think it would be nice to have an option like that. Whether it's > done now or in 18, I don't have a strong opinion about. But I do think > it'd be helpful to have a consensus on whether or not this is a > security improvement, or a performance enhancement only, before adding > said option. As it's implemented, if the requiredirect option doesn't > actually requiredirect, I think it looks like security but isn't > really. I've not followed this thread closely enough to understand the comment about requiredirect maybe not actually requiring direct, but if that were true it seems like it might be concerning. But as far as having a GUC to force direct SSL or not, I agree that's a good idea, and that it's better than only being able to control the behavior through pg_hba.conf, because it removes room for any possible doubt about whether you're really enforcing the behavior you want to be enforcing. It might also mean that the connection can be rejected earlier in the handshaking process on the basis of the GUC value, which could conceivably prevent a client from reaching some piece of code that turns out to have a security vulnerability. For example, if we found out that direct SSL connections let you take over the Pentagon before reaching the authentication stage, but for some reason regular connections don't have the same problem, being able to categorically shut off direct SSL would be valuable. However, I don't really see why this has to be done for this release. It seems like a separate feature from direct SSL itself. If direct SSL hadn't been committed at the very last minute, then it would have been great if this had been done for this release, too. But it was. The moral we ought to take from that is "perhaps get the big features in a bit further in advance of the freeze," not "well we'll just keep hacking after the freeze." -- Robert Haas EDB: http://www.enterprisedb.com
On Tue, Apr 23, 2024 at 10:43 AM Robert Haas <robertmhaas@gmail.com> wrote: > I've not followed this thread closely enough to understand the comment > about requiredirect maybe not actually requiring direct, but if that > were true it seems like it might be concerning. It may be my misunderstanding. This seems to imply bad behavior: > If the server rejects GSS encryption, SSL is > negotiated over the same TCP connection using the traditional postgres > protocol, regardless of <literal>sslnegotiation</literal>. As does this comment: > + /* > + * If enabled, try direct SSL. Unless we have a valid TCP connection that > + * failed negotiating GSSAPI encryption or a plaintext connection in case > + * of sslmode='allow'; in that case we prefer to reuse the connection with > + * negotiated SSL, instead of reconnecting to do direct SSL. The point of > + * direct SSL is to avoid the roundtrip from the negotiation, but > + * reconnecting would also incur a roundtrip. > + */ but when I actually try those cases, I see that requiredirect does actually cause a direct SSL connection to be done, even with sslmode=allow. So maybe it's just misleading documentation (or my misreading of it) that needs to be expanded? Am I missing a different corner case where requiredirect is ignored, Heikki? I still question the utility of allowing sslmode=allow with sslnegotiation=requiredirect, because it seems like you've made both the performance and security characteristics actively worse if you choose that combination. But I want to make sure I understand the current behavior correctly before I derail the discussion too much... Thanks, --Jacob
On 23/04/2024 22:33, Jacob Champion wrote: > On Tue, Apr 23, 2024 at 10:43 AM Robert Haas <robertmhaas@gmail.com> wrote: >> I've not followed this thread closely enough to understand the comment >> about requiredirect maybe not actually requiring direct, but if that >> were true it seems like it might be concerning. > > It may be my misunderstanding. This seems to imply bad behavior: > >> If the server rejects GSS encryption, SSL is >> negotiated over the same TCP connection using the traditional postgres >> protocol, regardless of <literal>sslnegotiation</literal>. > > As does this comment: > >> + /* >> + * If enabled, try direct SSL. Unless we have a valid TCP connection that >> + * failed negotiating GSSAPI encryption or a plaintext connection in case >> + * of sslmode='allow'; in that case we prefer to reuse the connection with >> + * negotiated SSL, instead of reconnecting to do direct SSL. The point of >> + * direct SSL is to avoid the roundtrip from the negotiation, but >> + * reconnecting would also incur a roundtrip. >> + */ > > but when I actually try those cases, I see that requiredirect does > actually cause a direct SSL connection to be done, even with > sslmode=allow. So maybe it's just misleading documentation (or my > misreading of it) that needs to be expanded? Am I missing a different > corner case where requiredirect is ignored, Heikki? You're right, the comment is wrong about sslmode=allow. There is no negotiation of a plaintext connection, the client just sends the startup packet directly. The HBA rules can reject it, but the client will have to disconnect and reconnect in that case. The documentation and that comment are misleading about failed GSSAPI encryption too, and I also misremembered that. With sslnegotiation=requiredirect, libpq never uses negotiated SSL mode. It will reconnect after a rejected GSSAPI request. So that comment applies to sslnegotiation=direct, but not sslnegotiation=requiredirect. Attached patch tries to fix and clarify those. (Note that the client will only attempt GSSAPI encryption if it can find kerberos credentials in the environment.) -- Heikki Linnakangas Neon (https://neon.tech)
Вложения
On Tue, Apr 23, 2024 at 2:20 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > > Attached patch tries to fix and clarify those. s/negotiatied/negotiated/ in the attached patch, but other than that this seems like a definite improvement. Thanks! > (Note that the client will only attempt GSSAPI encryption if it can find > kerberos credentials in the environment.) Right. I don't like that it still happens with sslnegotiation=requiredirect, but I suspect that this is not the thread to complain about it in. Maybe I can propose a sslnegotiation=forcedirect or something for 18, to complement a postgresqls:// scheme. That leaves the ALPACA handshake correction, I think. (Peter had some questions on the original thread [1] that I've tried to answer.) And the overall consensus, or lack thereof, on whether or not `requiredirect` should be considered a security feature. Thanks, --Jacob [1] https://www.postgresql.org/message-id/e782e9f4-a0cd-49f5-800b-5e32a1b29183%40eisentraut.org
On Thu, Apr 25, 2024 at 12:16 PM Jacob Champion <jacob.champion@enterprisedb.com> wrote > Right. I don't like that it still happens with > sslnegotiation=requiredirect, but I suspect that this is not the > thread to complain about it in. Maybe I can propose a > sslnegotiation=forcedirect or something for 18, to complement a > postgresqls:// scheme. It is difficult to imagine a world in which we have both requiredirect and forcedirect and people are not confused. -- Robert Haas EDB: http://www.enterprisedb.com
On Thu, Apr 25, 2024 at 9:17 AM Robert Haas <robertmhaas@gmail.com> wrote: > > It is difficult to imagine a world in which we have both requiredirect > and forcedirect and people are not confused. Yeah... Any thoughts on a better scheme? require_auth was meant to lock down overly general authentication; maybe a require_proto or something could do the same for the transport? I hate that we have so many options that most people don't need but take precedence, especially when they're based on the existence of magic third-party environmental cues (e.g. Kerberos caches). And it was nice that we got sslrootcert=system to turn on strong security and reject nonsensical combinations. If someone sets `requiredirect` and leaves the default sslmode, or chooses a weaker one... Is that really useful to someone? --Jacob
On Thu, Apr 25, 2024 at 12:28 PM Jacob Champion <jacob.champion@enterprisedb.com> wrote: > On Thu, Apr 25, 2024 at 9:17 AM Robert Haas <robertmhaas@gmail.com> wrote: > > It is difficult to imagine a world in which we have both requiredirect > > and forcedirect and people are not confused. > > Yeah... Any thoughts on a better scheme? require_auth was meant to > lock down overly general authentication; maybe a require_proto or > something could do the same for the transport? I don't understand the difference between the two sets of semantics myself, so I'm not in a good position to comment. > I hate that we have so many options that most people don't need but > take precedence, especially when they're based on the existence of > magic third-party environmental cues (e.g. Kerberos caches). And it > was nice that we got sslrootcert=system to turn on strong security and > reject nonsensical combinations. If someone sets `requiredirect` and > leaves the default sslmode, or chooses a weaker one... Is that really > useful to someone? Maybe I'm missing something here, but why doesn't sslnegotiation override sslmode completely? Or alternatively, why not remove sslnegotiation entirely and just have more sslmode values? I mean maybe this shouldn't happen categorically, but if I say I want to require a direct SSL connection, to me that implies that I don't want an indirect SSL connection, and I really don't want a non-SSL connection. I think it's pretty questionable in 2024 whether sslmode=allow and sslmode=prefer make any sense at all. I don't think it would be crazy to remove them entirely. But I certainly don't think that they should be allowed to bleed into the behavior of new, higher-security configurations. Surely if I say I want direct SSL, it's that or nothing, right? -- Robert Haas EDB: http://www.enterprisedb.com
On Thu, Apr 25, 2024 at 10:35 AM Robert Haas <robertmhaas@gmail.com> wrote: > Maybe I'm missing something here, but why doesn't sslnegotiation > override sslmode completely? Or alternatively, why not remove > sslnegotiation entirely and just have more sslmode values? I mean > maybe this shouldn't happen categorically, but if I say I want to > require a direct SSL connection, to me that implies that I don't want > an indirect SSL connection, and I really don't want a non-SSL > connection. I think that comes down to the debate upthread, and whether you think it's a performance tweak or a security feature. My take on it is, `direct` mode is performance, and `requiredirect` is security. (Especially since, with the current implementation, requiredirect can slow things down?) > I think it's pretty questionable in 2024 whether sslmode=allow and > sslmode=prefer make any sense at all. I don't think it would be crazy > to remove them entirely. But I certainly don't think that they should > be allowed to bleed into the behavior of new, higher-security > configurations. Surely if I say I want direct SSL, it's that or > nothing, right? I agree, but I more or less lost the battle at [1]. Like Matthias mentioned in [2]: > I'm not sure about this either. The 'gssencmode' option is already > quite weird in that it seems to override the "require"d priority of > "sslmode=require", which it IMO really shouldn't. Thanks, --Jacob [1] https://www.postgresql.org/message-id/CAOYmi%2B%3DcnV-8V8TndSkEF6Htqa7qHQUL_KnQU8-DrT0Jjnm3_Q%40mail.gmail.com [2] https://www.postgresql.org/message-id/CAEze2Wi9j5Q3mRnuoD2Hr%3DeOFV-cMzWAUZ88YmSXSwsiJLQOWA%40mail.gmail.com
On 25/04/2024 21:13, Jacob Champion wrote: > On Thu, Apr 25, 2024 at 10:35 AM Robert Haas <robertmhaas@gmail.com> wrote: >> Maybe I'm missing something here, but why doesn't sslnegotiation >> override sslmode completely? Or alternatively, why not remove >> sslnegotiation entirely and just have more sslmode values? I mean >> maybe this shouldn't happen categorically, but if I say I want to >> require a direct SSL connection, to me that implies that I don't want >> an indirect SSL connection, and I really don't want a non-SSL >> connection. My thinking with sslnegotiation is that it controls how SSL is negotiated with the server, if SSL is to be used at all. It does not control whether SSL is used or required; that's what sslmode is for. > I think that comes down to the debate upthread, and whether you think > it's a performance tweak or a security feature. My take on it is, > `direct` mode is performance, and `requiredirect` is security. Agreed, although the the security benefits from `requiredirect` are pretty vague. It reduces the attack surface, but there are no known issues with the 'postgres' or 'direct' negotiation either. Perhaps 'requiredirect' should be renamed to 'directonly'? > (Especially since, with the current implementation, requiredirect can > slow things down?) Yes: the case is gssencmode=prefer, kerberos credentical cache present in client, and server doesn't support GSS. With sslnegotiation='postgres' or 'direct', libpq can do the SSL negotiation over the same TCP connection after the server rejected the GSSRequest. With sslnegotiation='requiredirect', it needs to open a new TCP connection. > >> I think it's pretty questionable in 2024 whether sslmode=allow and >> sslmode=prefer make any sense at all. I don't think it would be crazy >> to remove them entirely. But I certainly don't think that they should >> be allowed to bleed into the behavior of new, higher-security >> configurations. Surely if I say I want direct SSL, it's that or >> nothing, right? > > I agree, but I more or less lost the battle at [1]. Like Matthias > mentioned in [2]: > >> I'm not sure about this either. The 'gssencmode' option is already >> quite weird in that it seems to override the "require"d priority of >> "sslmode=require", which it IMO really shouldn't. Yeah, that combination is weird. I think we should forbid it. But that's separate from sslnegotiation. -- Heikki Linnakangas Neon (https://neon.tech)
On Thu, Apr 25, 2024 at 2:50 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > > I think that comes down to the debate upthread, and whether you think > > it's a performance tweak or a security feature. My take on it is, > > `direct` mode is performance, and `requiredirect` is security. > > Agreed, although the the security benefits from `requiredirect` are > pretty vague. It reduces the attack surface, but there are no known > issues with the 'postgres' or 'direct' negotiation either. I think reduction in attack surface is a concrete security benefit, not a vague one. True, I don't know of any exploits today, but that seems almost tautological -- if there were known exploits in our upgrade handshake, I assume we'd be working to fix them ASAP? > Perhaps 'requiredirect' should be renamed to 'directonly'? If it's agreed that we don't want to require a stronger sslmode for that sslnegotiation setting, then that would probably be an improvement. But who is the target user for `sslnegotiation=directonly`, in your opinion? Would they ever have a reason to use a weak sslmode? > >> I'm not sure about this either. The 'gssencmode' option is already > >> quite weird in that it seems to override the "require"d priority of > >> "sslmode=require", which it IMO really shouldn't. > > Yeah, that combination is weird. I think we should forbid it. But that's > separate from sslnegotiation. Separate but related, IMO. If we were all hypothetically okay with gssencmode ignoring `sslmode=require`, then it's hard for me to claim that `sslnegotiation=requiredirect` should behave differently. On the other hand, if we're not okay with that and we'd like to change it, it's easier for me to argue that `requiredirect` should also be stricter from the get-go. Thanks, --Jacob
On Thu, Apr 25, 2024 at 5:50 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > On 25/04/2024 21:13, Jacob Champion wrote: > > On Thu, Apr 25, 2024 at 10:35 AM Robert Haas <robertmhaas@gmail.com> wrote: > >> Maybe I'm missing something here, but why doesn't sslnegotiation > >> override sslmode completely? Or alternatively, why not remove > >> sslnegotiation entirely and just have more sslmode values? I mean > >> maybe this shouldn't happen categorically, but if I say I want to > >> require a direct SSL connection, to me that implies that I don't want > >> an indirect SSL connection, and I really don't want a non-SSL > >> connection. > > My thinking with sslnegotiation is that it controls how SSL is > negotiated with the server, if SSL is to be used at all. It does not > control whether SSL is used or required; that's what sslmode is for. I think this might boil down to the order in which someone thinks that different settings should be applied. It sounds like your mental model is that GSS settings are applied first, and then SSL settings are applied afterwards, and then within the SSL bucket you can select how you want to do SSL (direct or negotiated) and how required it is. My mental model is different: I imagine that since direct SSL happens from the first byte exchanged over the socket, direct SSL "happens first", making settings that pertain to negotiated GSS and negotiated SSL irrelevant. Because, logically, if you've decided to use direct SSL, you're not even going to get a chance to negotiate those things. I understand that the code as written works around that, by being able to open a new connection if it turns out that we need to negotiate that stuff after all, but IMHO that's rather confusing. -- Robert Haas EDB: http://www.enterprisedb.com
On 23/04/2024 20:02, Jacob Champion wrote: > On Fri, Apr 19, 2024 at 2:43 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> >> On 19/04/2024 19:48, Jacob Champion wrote: >>> On Fri, Apr 19, 2024 at 6:56 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: >>>> With direct SSL negotiation, we always require ALPN. >>> >>> (As an aside: I haven't gotten to test the version of the patch that >>> made it into 17 yet, but from a quick glance it looks like we're not >>> rejecting mismatched ALPN during the handshake as noted in [1].) >> >> Ah, good catch, that fell through the cracks. Agreed, the client should >> reject a direct SSL connection if the server didn't send ALPN. I'll add >> that to the Open Items so we don't forget again. > > Yes, the client should also reject, but that's not what I'm referring > to above. The server needs to fail the TLS handshake itself with the > proper error code (I think it's `no_application_protocol`?); otherwise > a client implementing a different protocol could consume the > application-level bytes coming back from the server and act on them. > That's the protocol confusion attack from ALPACA we're trying to > avoid. I finally understood what you mean. So if the client supports ALPN, but the list of protocols that it provides does not include 'postgresql', the server should reject the connection with 'no_applicaton_protocol' alert. Makes sense. I thought OpenSSL would do that with the alpn callback we have, but it does not. The attached patch makes that change. I used the alpn_cb() function in openssl's own s_server program as example for that. Unfortunately the error message you got in the client with that was horrible (I modified the server to not accept the 'postgresql' protocol): psql "dbname=postgres sslmode=require host=localhost" psql: error: connection to server at "localhost" (::1), port 5432 failed: SSL error: SSL error code 167773280 This is similar to the case with system errors discussed at https://postgr.es/m/b6fb018b-f05c-4afd-abd3-318c649faf18@highgo.ca, but this one is equally bad on OpenSSL 1.1.1 and 3.3.0. It seems like an OpenSSL bug to me, because there is an error string "no application protocol" in the OpenSSL sources (ssl/ssl_err.c): {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_NO_APPLICATION_PROTOCOL), "no application protocol"}, and in the server log, you get that message. But the error code seen in the client is different. There are also messages for other alerts, for example: {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_TLSV13_ALERT_MISSING_EXTENSION), "tlsv13 alert missing extension"}, The bottom line is that that seems like a bug of omission to me in OpenSSL, but I wouldn't hold my breath waiting for it to be fixed. We can easily check for that error code and print the right message ourselves however, as in the attached patch. -- Heikki Linnakangas Neon (https://neon.tech)
Вложения
On 26/04/2024 02:23, Jacob Champion wrote: > On Thu, Apr 25, 2024 at 2:50 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: >>> I think that comes down to the debate upthread, and whether you think >>> it's a performance tweak or a security feature. My take on it is, >>> `direct` mode is performance, and `requiredirect` is security. >> >> Agreed, although the the security benefits from `requiredirect` are >> pretty vague. It reduces the attack surface, but there are no known >> issues with the 'postgres' or 'direct' negotiation either. > > I think reduction in attack surface is a concrete security benefit, > not a vague one. True, I don't know of any exploits today, but that > seems almost tautological -- if there were known exploits in our > upgrade handshake, I assume we'd be working to fix them ASAP? Sure, we'd try to fix them ASAP. But we've had the SSLRequest negotiation since time immemorial. If a new vulnerability is found, it's unlikely that we'd need to disable the SSLRequest negotiation altogether to fix it. We'd be in serious trouble with back-branches in that case. There's no sudden need to have a kill-switch for it. Taking that to the extreme, you could argue for a kill-switch for every feature, just in case there's a vulnerability in them. I agree that authentication is more sensitive so reducing the surface of that is more reasonable. And but nevertheless. (This discussion is moot though, because we do have the sslnegotiation=requiredirect mode, so you can disable the SSLRequest negotiation.) >> Perhaps 'requiredirect' should be renamed to 'directonly'? > > If it's agreed that we don't want to require a stronger sslmode for > that sslnegotiation setting, then that would probably be an > improvement. But who is the target user for > `sslnegotiation=directonly`, in your opinion? Would they ever have a > reason to use a weak sslmode? It's unlikely, I agree. A user who's sophisticated enough to use sslnegotiation=directonly would probably also want sslmode=require and require_auth=scram-sha256 and channel_binding=require. Or sslmode=verify-full. But in principle they're orthogonal. If you only have v17 servers in your environment, so you know all servers support direct negotiation if they support SSL at all, but a mix of servers with and without SSL, sslnegotiation=directonly would reduce roundtrips with sslmode=prefer. Making requiredirect to imply sslmode=require, or error out unless you also set sslmode=require, feels like a cavalier way of forcing SSL. We should have a serious discussion on making sslmode=require the default instead. That would be a more direct way of nudging people to use SSL. It would cause a lot of breakage, but it would also be a big improvement to security. Consider how sslnegotiation=requiredirect/directonly would feel, if we made sslmode=require the default. If you explicitly set "sslmode=prefer" or "sslmode=disable", it would be annoying if you would also need to remove "sslnegotiation=requiredirect" from your connection string. I'm leaning towards renaming sslnegotiation=requiredirect to sslnegotiation=directonly at this point. >>>> I'm not sure about this either. The 'gssencmode' option is already >>>> quite weird in that it seems to override the "require"d priority of >>>> "sslmode=require", which it IMO really shouldn't. >> >> Yeah, that combination is weird. I think we should forbid it. But that's >> separate from sslnegotiation. > > Separate but related, IMO. If we were all hypothetically okay with > gssencmode ignoring `sslmode=require`, then it's hard for me to claim > that `sslnegotiation=requiredirect` should behave differently. On the > other hand, if we're not okay with that and we'd like to change it, > it's easier for me to argue that `requiredirect` should also be > stricter from the get-go. I think the best way forward for those is something like a new "require_proto" parameter that you suggested upthread. Perhaps call it "encryption", with options "none", "ssl", "gss" so that you can provide multiple options and libpq will try them in the order specified. For example: encryption=none encryption=ssl, none # like sslmode=prefer encryption=gss encryption=gss, ssl # try GSS first, then SSL encryption=ssl, gss # try SSL first, then GSS This would make gssencmode and sslmode=disable/allow/prefer/require settings obsolete. sslmode would stil be needed to distinguish between verify-ca/verify-full though. But sslnegotiation would be still orthogonal to that. -- Heikki Linnakangas Neon (https://neon.tech)
On 23/04/2024 10:07, Michael Paquier wrote: > In the documentation of PQsslAttribute(), it is mentioned that empty > string is returned for "alpn" if ALPN was not used, however the code > returns NULL in this case: > SSL_get0_alpn_selected(conn->ssl, &data, &len); > if (data == NULL || len == 0 || len > sizeof(alpn_str) - 1) > return NULL; Good catch. I changed the code to return an empty string, as the documentation says. I considered if NULL or empty string would be better here. The docs for PQsslAttribute also says: "Returns NULL if the connection does not use SSL or the specified attribute name is not defined for the library in use." If a caller wants to distinguish between "libpq or the SSL library doesn't support ALPN at all" from "the server didn't support ALPN", you can tell from whether PQsslAttribute returns NULL or an empty string. So I think an empty string is better. -- Heikki Linnakangas Neon (https://neon.tech)
On Mon, Apr 29, 2024 at 12:43:18PM +0300, Heikki Linnakangas wrote: > If a caller wants to distinguish between "libpq or the SSL library doesn't > support ALPN at all" from "the server didn't support ALPN", you can tell > from whether PQsslAttribute returns NULL or an empty string. So I think an > empty string is better. Thanks. I would also have used an empty string to differenciate these two cases. -- Michael
Вложения
On Mon, Apr 29, 2024 at 4:38 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > Making requiredirect to imply sslmode=require, or error out unless you > also set sslmode=require, feels like a cavalier way of forcing SSL. We > should have a serious discussion on making sslmode=require the default > instead. That would be a more direct way of nudging people to use SSL. > It would cause a lot of breakage, but it would also be a big improvement > to security. > > Consider how sslnegotiation=requiredirect/directonly would feel, if we > made sslmode=require the default. If you explicitly set "sslmode=prefer" > or "sslmode=disable", it would be annoying if you would also need to > remove "sslnegotiation=requiredirect" from your connection string. I think making sslmode=require the default is pretty unworkable, unless we also had a way of automatically setting up SSL as part of initdb or something. Otherwise, we'd have to add sslmode=disable to a million places just to get the regression tests to work, and every test cluster anyone spins up locally would break in annoying ways, too. I had been thinking we might want to change the default to sslmode=disable and remove allow and prefer, but maybe automating a basic SSL setup is better. Either way, we should move toward a world where you either ask for SSL and get it, or don't ask for it and don't get it. Being halfway in between is bad. -- Robert Haas EDB: http://www.enterprisedb.com
On Fri, Apr 26, 2024 at 3:51 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > I finally understood what you mean. So if the client supports ALPN, but > the list of protocols that it provides does not include 'postgresql', > the server should reject the connection with 'no_applicaton_protocol' > alert. Right. (And additionally, we reject clients that don't advertise ALPN over direct SSL, also during the TLS handshake.) > The attached patch makes that change. I used the alpn_cb() function in > openssl's own s_server program as example for that. This patch as written will apply the new requirement to the old negotiation style, though, won't it? My test suite sees a bunch of failures with that. > Unfortunately the error message you got in the client with that was > horrible (I modified the server to not accept the 'postgresql' protocol): > > psql "dbname=postgres sslmode=require host=localhost" > psql: error: connection to server at "localhost" (::1), port 5432 > failed: SSL error: SSL error code 167773280 <long sigh> I filed a bug upstream [1]. Thanks, --Jacob [1] https://github.com/openssl/openssl/issues/24300
On Mon, Apr 29, 2024 at 1:38 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > Sure, we'd try to fix them ASAP. But we've had the SSLRequest > negotiation since time immemorial. If a new vulnerability is found, it's > unlikely that we'd need to disable the SSLRequest negotiation altogether > to fix it. We'd be in serious trouble with back-branches in that case. > There's no sudden need to have a kill-switch for it. I'm not really arguing that you'd need the kill switch to fix a problem in the code. (At least, I'm not arguing that in this thread; I reserve the right to argue that in the future. :D) But between the point of time that a vulnerability is announced and a user has upgraded, it's really nice to have a switch as a mitigation. Even better if it's server-side, because then the DBA can protect all their clients without requiring action on their part. > Taking that to the extreme, you could argue for a kill-switch for every > feature, just in case there's a vulnerability in them. I agree that > authentication is more sensitive so reducing the surface of that is more > reasonable. And but nevertheless. I mean... that would be extreme, yeah. I don't think anyone's proposed that. > If you only > have v17 servers in your environment, so you know all servers support > direct negotiation if they support SSL at all, but a mix of servers with > and without SSL, sslnegotiation=directonly would reduce roundtrips with > sslmode=prefer. But if you're in that situation, what does the use of directonly give you over `sslnegotiation=direct`? You already know that servers support direct, so there's no additional performance penalty from the less strict mode. > Making requiredirect to imply sslmode=require, or error out unless you > also set sslmode=require, feels like a cavalier way of forcing SSL. We > should have a serious discussion on making sslmode=require the default > instead. That would be a more direct way of nudging people to use SSL. > It would cause a lot of breakage, but it would also be a big improvement > to security. > > Consider how sslnegotiation=requiredirect/directonly would feel, if we > made sslmode=require the default. If you explicitly set "sslmode=prefer" > or "sslmode=disable", it would be annoying if you would also need to > remove "sslnegotiation=requiredirect" from your connection string. That's similar to how sslrootcert=system already works. To me, it feels great, because I don't have to worry about nonsensical combinations (with the exception of GSS, which we've touched on above). libpq complains loudly if I try to shoot myself in the foot, and if I'm using sslrootcert=system then it's a pretty clear signal that I care more about security than the temporary inconvenience of editing my connection string for one weird server that doesn't use SSL for some reason. > I think the best way forward for those is something like a new > "require_proto" parameter that you suggested upthread. Perhaps call it > "encryption", with options "none", "ssl", "gss" so that you can provide > multiple options and libpq will try them in the order specified. For > example: > > encryption=none > encryption=ssl, none # like sslmode=prefer > encryption=gss > encryption=gss, ssl # try GSS first, then SSL > encryption=ssl, gss # try SSL first, then GSS > > This would make gssencmode and sslmode=disable/allow/prefer/require > settings obsolete. sslmode would stil be needed to distinguish between > verify-ca/verify-full though. But sslnegotiation would be still > orthogonal to that. I will give this some more thought. Thanks, --Jacob
On 29/04/2024 21:04, Jacob Champion wrote: > On Fri, Apr 26, 2024 at 3:51 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> I finally understood what you mean. So if the client supports ALPN, but >> the list of protocols that it provides does not include 'postgresql', >> the server should reject the connection with 'no_applicaton_protocol' >> alert. > > Right. (And additionally, we reject clients that don't advertise ALPN > over direct SSL, also during the TLS handshake.) > >> The attached patch makes that change. I used the alpn_cb() function in >> openssl's own s_server program as example for that. > > This patch as written will apply the new requirement to the old > negotiation style, though, won't it? My test suite sees a bunch of > failures with that. Yes, and that is what we want, right? If the client uses old negotiation style, and includes ALPN in its ClientHello, but requests protocol "noodles" instead of "postgresql", it seems good to reject the connection. Note that if the client does not request ALPN at all, the callback is not called, and the connection is accepted. Old clients still work because they do not request ALPN. >> Unfortunately the error message you got in the client with that was >> horrible (I modified the server to not accept the 'postgresql' protocol): >> >> psql "dbname=postgres sslmode=require host=localhost" >> psql: error: connection to server at "localhost" (::1), port 5432 >> failed: SSL error: SSL error code 167773280 > > <long sigh> > > I filed a bug upstream [1]. Thanks! -- Heikki Linnakangas Neon (https://neon.tech)
On Mon, Apr 29, 2024 at 11:43 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > Note that if the client does not request ALPN at all, the callback is > not called, and the connection is accepted. Old clients still work > because they do not request ALPN. Ugh, sorry for the noise -- I couldn't figure out why all my old clients were failing and then realized it was because I'd left some test code in place for the OpenSSL bug. I'll rebuild everything and keep reviewing. --Jacob
On 29/04/2024 21:43, Jacob Champion wrote: > On Mon, Apr 29, 2024 at 1:38 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> If you only >> have v17 servers in your environment, so you know all servers support >> direct negotiation if they support SSL at all, but a mix of servers with >> and without SSL, sslnegotiation=directonly would reduce roundtrips with >> sslmode=prefer. > > But if you're in that situation, what does the use of directonly give > you over `sslnegotiation=direct`? You already know that servers > support direct, so there's no additional performance penalty from the > less strict mode. Well, by that argument we don't need requiredirect/directonly at all. This goes back to whether it's a security feature or a performance feature. There is a small benefit with sslmode=prefer if you connect to a server that doesn't support SSL, though. With sslnegotiation=direct, if the server rejects the direct SSL connection, the client will reconnect and try SSL with SSLRequest. The server will respond with 'N', and the client will proceed without encryption. sslnegotiation=directonly removes that SSLRequest attempt, eliminating one roundtrip. >> Making requiredirect to imply sslmode=require, or error out unless you >> also set sslmode=require, feels like a cavalier way of forcing SSL. We >> should have a serious discussion on making sslmode=require the default >> instead. That would be a more direct way of nudging people to use SSL. >> It would cause a lot of breakage, but it would also be a big improvement >> to security. >> >> Consider how sslnegotiation=requiredirect/directonly would feel, if we >> made sslmode=require the default. If you explicitly set "sslmode=prefer" >> or "sslmode=disable", it would be annoying if you would also need to >> remove "sslnegotiation=requiredirect" from your connection string. > > That's similar to how sslrootcert=system already works. To me, it > feels great, because I don't have to worry about nonsensical > combinations (with the exception of GSS, which we've touched on > above). libpq complains loudly if I try to shoot myself in the foot, > and if I'm using sslrootcert=system then it's a pretty clear signal > that I care more about security than the temporary inconvenience of > editing my connection string for one weird server that doesn't use SSL > for some reason. Oh I was not aware sslrootcert=system works like that. That's a bit surprising, none of the other ssl-related settings imply or require that SSL is actually used. Did we intend to set a precedence for new settings with that? (adding Daniel in case he has an opinion) -- Heikki Linnakangas Neon (https://neon.tech)
On Mon, Apr 29, 2024 at 12:06 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > On 29/04/2024 21:43, Jacob Champion wrote: > > But if you're in that situation, what does the use of directonly give > > you over `sslnegotiation=direct`? You already know that servers > > support direct, so there's no additional performance penalty from the > > less strict mode. > > Well, by that argument we don't need requiredirect/directonly at all. > This goes back to whether it's a security feature or a performance feature. That's what I've been trying to argue, yeah. If it's not a security feature... why's it there? > There is a small benefit with sslmode=prefer if you connect to a server > that doesn't support SSL, though. With sslnegotiation=direct, if the > server rejects the direct SSL connection, the client will reconnect and > try SSL with SSLRequest. The server will respond with 'N', and the > client will proceed without encryption. sslnegotiation=directonly > removes that SSLRequest attempt, eliminating one roundtrip. Okay, agreed that in this case, there is a performance benefit. It's not enough to convince me, honestly, but are there any other cases I missed as well? > Oh I was not aware sslrootcert=system works like that. That's a bit > surprising, none of the other ssl-related settings imply or require that > SSL is actually used. For sslrootcert=system in particular, the danger of accidentally weak sslmodes is pretty high, especially for verify-ca mode. (It goes back to that other argument -- there should be, effectively, zero users who both opt in to the public CA system, and are also okay with silently falling back and not using it.) > Did we intend to set a precedence for new settings > with that? (I'll let committers answer whether they intended that or not -- I was just bringing up that we already have a setting that works like that, and I really like how it works in practice. But it's probably unsurprising that I like it.) --Jacob
On Mon, Apr 29, 2024 at 12:32 PM Jacob Champion <jacob.champion@enterprisedb.com> wrote: > > On Mon, Apr 29, 2024 at 12:06 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > > On 29/04/2024 21:43, Jacob Champion wrote: > > > But if you're in that situation, what does the use of directonly give > > > you over `sslnegotiation=direct`? You already know that servers > > > support direct, so there's no additional performance penalty from the > > > less strict mode. > > > > Well, by that argument we don't need requiredirect/directonly at all. > > This goes back to whether it's a security feature or a performance feature. > > That's what I've been trying to argue, yeah. If it's not a security > feature... why's it there? Er, I should clarify this. I _want_ requiredirect. I just want it to be a security feature. --Jacob
> On 29 Apr 2024, at 21:06, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > Oh I was not aware sslrootcert=system works like that. That's a bit surprising, none of the other ssl-related settingsimply or require that SSL is actually used. Did we intend to set a precedence for new settings with that? It was very much intentional, and documented, an sslmode other than verify-full makes little sense when combined with sslrootcert=system. It wasn't intended to set a precedence (though there is probably a fair bit of things we can do, getting this right is hard enough as it is), rather it was footgun prevention. -- Daniel Gustafsson
On 29/04/2024 22:32, Jacob Champion wrote: > On Mon, Apr 29, 2024 at 12:06 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> There is a small benefit with sslmode=prefer if you connect to a server >> that doesn't support SSL, though. With sslnegotiation=direct, if the >> server rejects the direct SSL connection, the client will reconnect and >> try SSL with SSLRequest. The server will respond with 'N', and the >> client will proceed without encryption. sslnegotiation=directonly >> removes that SSLRequest attempt, eliminating one roundtrip. > > Okay, agreed that in this case, there is a performance benefit. It's > not enough to convince me, honestly, but are there any other cases I > missed as well? I realized one case that hasn't been discussed so far: If you use the combination of "sslmode=prefer sslnegotiation=requiredirect" to connect to a pre-v17 server that has SSL enabled but does not support direct SSL connections, you will fall back to a plaintext connection instead. That's almost certainly not what you wanted. I'm coming around to your opinion that we should not allow that combination. Stepping back to summarize my thoughts, there are now three things I don't like about the status quo: 1. As noted above, the sslmode=prefer and sslnegotiation=requiredirect combination is somewhat dangerous, as you might unintentionally fall back to plaintext authentication when connecting to a pre-v17 server. 2. There is an asymmetry between "postgres" and "direct" option names. "postgres" means "try only traditional negotiation", while "direct" means "try direct first, and fall back to traditional negotiation if it fails". That is apparent only if you know that the "requiredirect" mode also exists. 3. The "require" word in "requiredirect" suggests that it's somehow more strict or more secure, similar to sslmode=require. However, I don't consider direct SSL connections to be a security feature. New proposal: - Remove the "try both" mode completely, and rename "requiredirect" to just "direct". So there would be just two modes: "postgres" and "direct". On reflection, the automatic fallback mode doesn't seem very useful. It would make sense as the default, because then you would get the benefits automatically in most cases but still be compatible with old servers. But if it's not the default, you have to fiddle with libpq settings anyway to enable it, and then you might as well use the "requiredirect" mode when you know the server supports it. There isn't anything wrong with it as such, but given how much confusion there's been on how this all works, I'd prefer to cut this back to the bare minimum now. We can add it back in the future, and perhaps make it the default at the same time. This addresses points 2. and 3. above. and: - Only allow sslnegotiation=direct with sslmode=require or higher. This is what you, Jacob, wanted to do all along, and addresses point 1. Thoughts? -- Heikki Linnakangas Neon (https://neon.tech)
On Fri, 10 May 2024 at 15:50, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > New proposal: > > - Remove the "try both" mode completely, and rename "requiredirect" to > just "direct". So there would be just two modes: "postgres" and > "direct". On reflection, the automatic fallback mode doesn't seem very > useful. It would make sense as the default, because then you would get > the benefits automatically in most cases but still be compatible with > old servers. But if it's not the default, you have to fiddle with libpq > settings anyway to enable it, and then you might as well use the > "requiredirect" mode when you know the server supports it. There isn't > anything wrong with it as such, but given how much confusion there's > been on how this all works, I'd prefer to cut this back to the bare > minimum now. We can add it back in the future, and perhaps make it the > default at the same time. This addresses points 2. and 3. above. > > and: > > - Only allow sslnegotiation=direct with sslmode=require or higher. This > is what you, Jacob, wanted to do all along, and addresses point 1. > > Thoughts? Sounds mostly good to me. But I think we'd want to automatically increase sslmode to require if it is unset, but sslnegotation is set to direct. Similar to how we bump sslmode to verify-full if sslrootcert is set to system, but sslmode is unset. i.e. it seems unnecessary/unwanted to throw an error if the connection string only contains sslnegotiation=direct
On 11/05/2024 23:45, Jelte Fennema-Nio wrote: > On Fri, 10 May 2024 at 15:50, Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> New proposal: >> >> - Remove the "try both" mode completely, and rename "requiredirect" to >> just "direct". So there would be just two modes: "postgres" and >> "direct". On reflection, the automatic fallback mode doesn't seem very >> useful. It would make sense as the default, because then you would get >> the benefits automatically in most cases but still be compatible with >> old servers. But if it's not the default, you have to fiddle with libpq >> settings anyway to enable it, and then you might as well use the >> "requiredirect" mode when you know the server supports it. There isn't >> anything wrong with it as such, but given how much confusion there's >> been on how this all works, I'd prefer to cut this back to the bare >> minimum now. We can add it back in the future, and perhaps make it the >> default at the same time. This addresses points 2. and 3. above. >> >> and: >> >> - Only allow sslnegotiation=direct with sslmode=require or higher. This >> is what you, Jacob, wanted to do all along, and addresses point 1. >> >> Thoughts? > > Sounds mostly good to me. But I think we'd want to automatically > increase sslmode to require if it is unset, but sslnegotation is set > to direct. Similar to how we bump sslmode to verify-full if > sslrootcert is set to system, but sslmode is unset. i.e. it seems > unnecessary/unwanted to throw an error if the connection string only > contains sslnegotiation=direct I find that error-prone. For example: 1. Try to connect to a server with direct negotiation: psql "host=foobar dbname=mydb sslnegotiation=direct" 2. It fails. Maybe it was an old server? Let's change it to sslnegotiation=postgres. 3. Now it succeeds. Great! You might miss that by changing sslnegotiation to 'postgres', or by removing it altogether, you not only made it compatible with older server versions, but you also allowed falling back to a plaintext connection. Maybe you're fine with that, but maybe not. I'd like to nudge people to use sslmode=require, not rely on implicit stuff like this just to make connection strings a little shorter. I'm not a fan of sslrootcert=system implying sslmode=verify-full either, for the same reasons. But at least "sslrootcert" is a clearly security-related setting, so removing it might give you a pause, whereas sslnegotition is about performance and compatibility. In v18, I'd like to make sslmode=require the default. Or maybe introduce a new setting like "encryption=ssl|gss|none", defaulting to 'ssl'. If we want to encourage encryption, that's the right way to do it. (I'd still recommend everyone to use an explicit sslmode=require in their connection strings for many years, though, because you might be using an older client without realizing it.) -- Heikki Linnakangas Neon (https://neon.tech)
On Sun, 12 May 2024 at 23:39, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > You might miss that by changing sslnegotiation to 'postgres', or by > removing it altogether, you not only made it compatible with older > server versions, but you also allowed falling back to a plaintext > connection. Maybe you're fine with that, but maybe not. I'd like to > nudge people to use sslmode=require, not rely on implicit stuff like > this just to make connection strings a little shorter. I understand your worry, but I'm not sure that this is actually much of a security issue in practice. sslmode=prefer and sslmode=require are the same amount of insecure imho (i.e. extremely insecure). The only reason sslmode=prefer would connect as non-ssl to a server that supports ssl is if an attacker has write access to the network in the middle (i.e. eavesdropping ability alone is not enough). Once an attacker has this level of network access, it's trivial for this attacker to read any data sent to Postgres by intercepting the TLS handshake and doing TLS termination with some arbitrary cert (any cert is trusted by sslmode=require). So the only actual case where this is a security issue I can think of is when an attacker has only eavesdropping ability on the network. And somehow the Postgres server that the client tries to connect to is configured incorrectly, so that no ssl is set up at all. Then a client would drop to plaintext, when connecting to this server instead of erroring, and the attacker could now read the traffic. But I don't really see this scenario end up any differently when requiring people to enter sslmode=require. The only action a user can take to connect to a server that does not have ssl support is to remove sslmode=require from the connection string. Except if they are also the server operator, in which case they could enable ssl on the server. But if ssl is not set up, then it was probably never set up, and thus providing sslnegotiation=direct would be to test if ssl works. But, if you disagree I'm fine with erroring on plain sslnegotiation=direct > In v18, I'd like to make sslmode=require the default. Or maybe introduce > a new setting like "encryption=ssl|gss|none", defaulting to 'ssl'. If we > want to encourage encryption, that's the right way to do it. (I'd still > recommend everyone to use an explicit sslmode=require in their > connection strings for many years, though, because you might be using an > older client without realizing it.) I'm definitely a huge proponent of making the connection defaults more secure. But as described above sslmode=require is still extremely insecure and I don't think we gain anything substantial by making it the default. I think the only useful secure default would be to use sslmode=verify-full (with probably some automatic fallback to sslmode=prefer when connecting to hardcoded IPs or localhost). Which probably means that sslrootcert=system should also be made the default. Which would mean that ~/.postgresql/root.crt would not be the default anymore, which I personally think is fine but others likely disagree.
On 13/05/2024 12:50, Jelte Fennema-Nio wrote: > On Sun, 12 May 2024 at 23:39, Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> In v18, I'd like to make sslmode=require the default. Or maybe introduce >> a new setting like "encryption=ssl|gss|none", defaulting to 'ssl'. If we >> want to encourage encryption, that's the right way to do it. (I'd still >> recommend everyone to use an explicit sslmode=require in their >> connection strings for many years, though, because you might be using an >> older client without realizing it.) > > I'm definitely a huge proponent of making the connection defaults more > secure. But as described above sslmode=require is still extremely > insecure and I don't think we gain anything substantial by making it > the default. I think the only useful secure default would be to use > sslmode=verify-full (with probably some automatic fallback to > sslmode=prefer when connecting to hardcoded IPs or localhost). Which > probably means that sslrootcert=system should also be made the > default. Which would mean that ~/.postgresql/root.crt would not be the > default anymore, which I personally think is fine but others likely > disagree. "channel_binding=require sslmode=require" also protects from MITM attacks. I think these options should be designed from the user's point of view, so that the user can specify the risks they're willing to accept, and the details of how that's accomplished are handled by libpq. For example, I'm OK with (tick all that apply): [ ] passive eavesdroppers seeing all the traffic [ ] MITM being able to hijack the session [ ] connecting without verifying the server's identity [ ] divulging the plaintext password to the server [ ] ... The requirements for whether SSL or GSS encryption is required, whether the server's certificate needs to signed with known CA, etc. can be derived from those. For example, if you need protection from eavesdroppers, SSL or GSS encryption must be used. If you need to verify the server's identity, it implies sslmode=verify-CA or channel_binding=true. If you don't want to divulge the password, it implies a suitable require_auth setting. I don't have a concrete proposal yet, but something like that. And the defaults for those are up for debate. psql could perhaps help by listing the above properties at the beginning of the session, something like: psql (16.2) WARNING: Connection is not encrypted. WARNING: The server's identity has not been verified Type "help" for help. postgres=# Although for the "divulge plaintext password to server" property, it's too late to print a warning after connecting, because the damage has already been done. A different line of thought is that to keep the attack surface as smal as possible, you should specify very explicitly what exactly you expect to happen in the authentication, and disallow any variance. For example, you expect SCRAM to be used, with a certificate signed by a particular CA, and if the server requests anything else, that's suspicious and the connection is aborted. We should make that possible too, but the above flexible risk-based approach seems good for the defaults. -- Heikki Linnakangas Neon (https://neon.tech)
On 10/05/2024 16:50, Heikki Linnakangas wrote: > New proposal: > > - Remove the "try both" mode completely, and rename "requiredirect" to > just "direct". So there would be just two modes: "postgres" and > "direct". On reflection, the automatic fallback mode doesn't seem very > useful. It would make sense as the default, because then you would get > the benefits automatically in most cases but still be compatible with > old servers. But if it's not the default, you have to fiddle with libpq > settings anyway to enable it, and then you might as well use the > "requiredirect" mode when you know the server supports it. There isn't > anything wrong with it as such, but given how much confusion there's > been on how this all works, I'd prefer to cut this back to the bare > minimum now. We can add it back in the future, and perhaps make it the > default at the same time. This addresses points 2. and 3. above. > > and: > > - Only allow sslnegotiation=direct with sslmode=require or higher. This > is what you, Jacob, wanted to do all along, and addresses point 1. Here's a patch to implement that. -- Heikki Linnakangas Neon (https://neon.tech)
Вложения
On Mon, 13 May 2024 at 15:38, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > Here's a patch to implement that. + if (conn->sslnegotiation[0] == 'd' && + conn->sslmode[0] != 'r' && conn->sslmode[0] != 'v') I think these checks should use strcmp instead of checking magic first characters. I see this same clever trick is used in the recently added init_allowed_encryption_methods, and I think that should be changed to use strcmp too for readability.
On Mon, 13 May 2024 at 13:07, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > "channel_binding=require sslmode=require" also protects from MITM attacks. Cool, I didn't realize we had this connection option and it could be used like this. But I think there's a few security downsides of channel_binding=require over sslmode=verify-full: If the client relies on channel_binding to validate server authenticity, a leaked server-side SCRAM hash is enough for an attacker to impersonate a server. While normally a leaked scram hash isn't really much of a security concern (assuming long enough passwords). I also don't know of many people rotating their scram hashes, even though many rotate TLS certs. > I think these options should be designed from the user's point of view, > so that the user can specify the risks they're willing to accept, and > the details of how that's accomplished are handled by libpq. For > example, I'm OK with (tick all that apply): > > [ ] passive eavesdroppers seeing all the traffic > [ ] MITM being able to hijack the session > [ ] connecting without verifying the server's identity > [ ] divulging the plaintext password to the server > [ ] ... I think that sounds like a great idea, looking forward to the proposal.
On 13/05/2024 16:55, Jelte Fennema-Nio wrote: > On Mon, 13 May 2024 at 15:38, Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> Here's a patch to implement that. > > + if (conn->sslnegotiation[0] == 'd' && > + conn->sslmode[0] != 'r' && conn->sslmode[0] != 'v') > > I think these checks should use strcmp instead of checking magic first > characters. I see this same clever trick is used in the recently added > init_allowed_encryption_methods, and I think that should be changed to > use strcmp too for readability. Oh yeah, I hate that too. These should be refactored into enums, with a clear separate stage of parsing the options from strings. But we use that pattern all over the place, so I didn't want to start reforming it with this patch. -- Heikki Linnakangas Neon (https://neon.tech)
On Mon, May 13, 2024 at 9:37 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > On 10/05/2024 16:50, Heikki Linnakangas wrote: > > New proposal: > > > > - Remove the "try both" mode completely, and rename "requiredirect" to > > just "direct". So there would be just two modes: "postgres" and > > "direct". On reflection, the automatic fallback mode doesn't seem very > > useful. It would make sense as the default, because then you would get > > the benefits automatically in most cases but still be compatible with > > old servers. But if it's not the default, you have to fiddle with libpq > > settings anyway to enable it, and then you might as well use the > > "requiredirect" mode when you know the server supports it. There isn't > > anything wrong with it as such, but given how much confusion there's > > been on how this all works, I'd prefer to cut this back to the bare > > minimum now. We can add it back in the future, and perhaps make it the > > default at the same time. This addresses points 2. and 3. above. > > > > and: > > > > - Only allow sslnegotiation=direct with sslmode=require or higher. This > > is what you, Jacob, wanted to do all along, and addresses point 1. > > Here's a patch to implement that. I find this idea to be a massive improvement over the status quo, and I didn't spot any major problems when I read through the patch, either. I'm not quite sure if the patch takes the right approach in emphasizing that weaker sslmode settings are not allowed because of unintended fallbacks. It seems to me that we could equally well say that those combinations are nonsensical. If we're making a direct SSL connection, SSL is eo ipso required. I don't have a strong opinion about whether sslnegotiation=direct should error out (as you propose here) or silently promote sslmode to require. I think either is defensible. Had I been implementing it, I think I would have done as Jacob proposes, just because once we've forced a direct SSL negotiation surely the only sensible behavior is to be using SSL, unless you think there should be a silently-reconnect-without-SSL behavior, which I sure don't. However, I disagree with Jacob's assertion that sslmode=require has no security benefits over sslmode=prefer. That seems like the kind of pessimism that makes people hate security professionals. There have got to be some attacks that are foreclosed by encrypting the connection, even if you don't stop MITM attacks or other things that are more sophisticated than running wireshark and seeing what goes by on the wire. I'm pleased to hear that you will propose to make sslmode=require the default in v18. I think we'll need to do some work to figure out how much collateral damage that will cause, and maybe it will be more than we can stomach, but Magnus has been saying for years that the current default is terrible. I'm not sure I was entirely convinced of that the first time I heard him say it, but I'm smarter now than I was then. It's really hard to believe in 2024 that anyone should ever be using a setting that may or may not encrypt the connection. There's http and https but there's no httpmaybes. -- Robert Haas EDB: http://www.enterprisedb.com
(There's, uh, a lot to respond to above and I'm trying to figure out how best to type up all of it.) On Mon, May 13, 2024 at 9:13 AM Robert Haas <robertmhaas@gmail.com> wrote: > However, > I disagree with Jacob's assertion that sslmode=require has no security > benefits over sslmode=prefer. For the record, I didn't say that... You mean Jelte's quote up above?: > sslmode=prefer and sslmode=require > are the same amount of insecure imho (i.e. extremely insecure). I agree that requiring passive security is tangibly better than allowing fallback to plaintext. I think Jelte's point might be better stated as, =prefer and =require give the same amount of protection against active attack (none). --Jacob
On Mon, 13 May 2024 at 18:14, Robert Haas <robertmhaas@gmail.com> wrote: > I disagree with Jacob's assertion that sslmode=require has no security > benefits over sslmode=prefer. That seems like the kind of pessimism > that makes people hate security professionals. There have got to be > some attacks that are foreclosed by encrypting the connection, even if > you don't stop MITM attacks or other things that are more > sophisticated than running wireshark and seeing what goes by on the > wire. Like Jacob already said, I guess you meant me here. The main point I was trying to make is that sslmode=require is extremely insecure too, so if we're changing the default then I'd rather bite the bullet and actually make the default a secure one this time. No-ones browser trusts self-signed certs by default, but currently lots of people trust self-signed certs when connecting to their production database without realizing. IMHO the only benefit that sslmode=require brings over sslmode=prefer is detecting incorrectly configured servers i.e. servers that are supposed to support ssl but somehow don't due to a misconfigured GUC/pg_hba. Such "incorrectly configured server" detection avoids sending data to such a server, which an eavesdropper on the network could see. Which is definitely a security benefit, but it's an extremely small one. In all other cases sslmode=prefer brings exactly the same protection as sslmode=require, because sslmode=prefer encrypts the connection unless postgres actively tells the client to downgrade to plaintext (which never happens when the server is configured correctly).
[soapbox thread, so I've changed the Subject] On Mon, May 13, 2024 at 4:08 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > "channel_binding=require sslmode=require" also protects from MITM attacks. This isn't true in the same way that "standard" TLS protects against MITM. I know you know that, but for the benefit of bystanders reading the threads, I think we should stop phrasing it like this. Most people who want MITM protection need to be using verify-full. Details for those bystanders: Channel binding alone will only disconnect you after the MITM is discovered, after your startup packet is leaked but before you send any queries to the server. A hash of your password will also be leaked in that situation, which starts the timer on an offline attack. And IIRC, you won't get an alert that says "someone's in the middle"; it'll just look like you mistyped your password. (Stronger passwords provide stronger protection in this situation, which is not a property that most people are used to. If I choose to sign into Google with the password "hunter2", it doesn't somehow make the TLS protection weaker. But if you rely on SCRAM by itself for server authentication, it does more or less work like that.) Use channel_binding *in addition to* sslmode=verify-full if you want enhanced authentication of the peer, as suggested in the docs [1]. Don't rely on channel binding alone for the vast majority of use cases, and if you know better for your particular use case, then you already know enough to be able to ignore my advice. [/soapbox] Thanks, --Jacob [1] https://www.postgresql.org/docs/current/preventing-server-spoofing.html
On Mon, May 13, 2024 at 12:45 PM Jacob Champion <jacob.champion@enterprisedb.com> wrote: > For the record, I didn't say that... You mean Jelte's quote up above? Yeah, sorry, I got my J-named hackers confused. Apologies. -- Robert Haas EDB: http://www.enterprisedb.com
On Mon, May 13, 2024 at 1:42 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > Like Jacob already said, I guess you meant me here. The main point I > was trying to make is that sslmode=require is extremely insecure too, > so if we're changing the default then I'd rather bite the bullet and > actually make the default a secure one this time. No-ones browser > trusts self-signed certs by default, but currently lots of people > trust self-signed certs when connecting to their production database > without realizing. > > IMHO the only benefit that sslmode=require brings over sslmode=prefer > is detecting incorrectly configured servers i.e. servers that are > supposed to support ssl but somehow don't due to a misconfigured > GUC/pg_hba. Such "incorrectly configured server" detection avoids > sending data to such a server, which an eavesdropper on the network > could see. Which is definitely a security benefit, but it's an > extremely small one. In all other cases sslmode=prefer brings exactly > the same protection as sslmode=require, because sslmode=prefer > encrypts the connection unless postgres actively tells the client to > downgrade to plaintext (which never happens when the server is > configured correctly). I think I agree with *nearly* every word of this. However: (1) I don't want to hijack this thread about a v17 open item to talk too much about a hypothetical v18 proposal. (2) While in general you need more than just SSL to ensure security, I'm not sure that there's only one way to do it, and I doubt that we should try to pick a winner. (3) I suspect that even going as far as sslmode=require by default is going to be extremely painful for hackers, the project, and users. Moving the goalposts further increases the likelihood of nothing happening at all. -- Robert Haas EDB: http://www.enterprisedb.com
On Mon, May 13, 2024 at 9:13 AM Robert Haas <robertmhaas@gmail.com> wrote: > I find this idea to be a massive improvement over the status quo, +1 > and > I didn't spot any major problems when I read through the patch, > either. Definitely not a major problem, but I think select_next_encryption_method() has gone stale, since it originally provided generality and lines of fallback that no longer exist. In other words, I think the following code is now misleading: > if (conn->sslmode[0] == 'a') > SELECT_NEXT_METHOD(ENC_PLAINTEXT); > > SELECT_NEXT_METHOD(ENC_NEGOTIATED_SSL); > SELECT_NEXT_METHOD(ENC_DIRECT_SSL); > > if (conn->sslmode[0] != 'a') > SELECT_NEXT_METHOD(ENC_PLAINTEXT); To me, that implies that negotiated mode takes precedence over direct, but the point of the patch is that it's not possible to have both. And if direct SSL is in use, then sslmode can't be "allow" anyway, and we definitely don't want ENC_PLAINTEXT. So if someone proposes a change to select_next_encryption_method(), you'll have to remember to stare at init_allowed_encryption_methods() as well, and think really hard about what's going on. And vice-versa. That worries me. > I don't have a strong opinion about whether sslnegotiation=direct > should error out (as you propose here) or silently promote sslmode to > require. I think either is defensible. I'm comforted that, since sslrootcert=system already does it, plenty of use cases will get that for free. And if you decide in the future that you really really want it to promote, it won't be a compatibility break to make that change. (That gives us more time for wider v16-17 adoption, to see how the sslrootcert=system magic promotion behavior is going in practice.) > Had I been implementing it, I > think I would have done as Jacob proposes, just because once we've > forced a direct SSL negotiation surely the only sensible behavior is > to be using SSL, unless you think there should be a > silently-reconnect-without-SSL behavior, which I sure don't. We still allow GSS to preempt SSL, though, so "forced" is probably overstating things. > It's really hard to believe in 2024 that anyone should ever be using a > setting that may or may not encrypt the connection. There's http and > https but there's no httpmaybes. +1. I think (someone hop in and correct me please) that Opportunistic Encryption for HTTP mostly fizzled, and they gave it a *lot* of thought. --Jacob
[this should probably belong to a different thread, but I'm not sure what to title it] On Mon, May 13, 2024 at 4:08 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > I think these options should be designed from the user's point of view, > so that the user can specify the risks they're willing to accept, and > the details of how that's accomplished are handled by libpq. For > example, I'm OK with (tick all that apply): > > [ ] passive eavesdroppers seeing all the traffic > [ ] MITM being able to hijack the session > [ ] connecting without verifying the server's identity > [ ] divulging the plaintext password to the server > [ ] ... I'm pessimistic about a quality-of-protection scheme for this use case (*). I don't think users need more knobs in their connection strings, and many of the goals of transport encryption are really not independent from each other in practice. As evidence I point to the absolute mess of GSSAPI wrapping, which lets you check separate boxes for things like "require the server to authenticate itself" and "require integrity" and "allow MITMs to reorder messages" and so on, as if the whole idea of "integrity" is useful if you don't know who you're talking to in the first place. I think I recall slapd having something similarly arcane (but at least it doesn't make the clients do it!). Those kinds of APIs don't evolve well, in my opinion. I think most people probably just want browser-grade security as quickly and cheaply as possible, and we don't make that very easy today. I'll try to review a QoP scheme if someone works on it, don't get me wrong, but I'd much rather spend time on a "very secure by default" mode that gets rid of most of the options (i.e. a postgresqls:// scheme). (*) I've proposed quality-of-protection in the past, for Postgres proxy authentication [1]. But I'm comfortable in my hypocrisy, because in that case, the end user doing the configuration is a DBA with a config file who is expected to understand the whole system, and it's a niche use case (IMO) without an obvious "common setup". And frankly I think my proposal is unlikely to go anywhere; the cost/benefit probably isn't good enough. > If you need to verify > the server's identity, it implies sslmode=verify-CA or > channel_binding=true. Neither of those two options provides strong authentication of the peer, and personally I wouldn't want them to be considered worthy of "verify the server's identity" mode. And -- taking a wild leap here -- if we disagree, then granularity becomes a problem: either the QoP scheme now has to have sub-checkboxes for "if the server knows my password, that's good enough" and "it's fine if the server's hostname doesn't match the cert, for some reason", or it smashes all of those different ideas into one setting and then I have to settle for the weakest common denominator during an active attack. Assuming I haven't missed a third option, will that be easier/better than the status quo of require_auth+sslmode? > A different line of thought is that to keep the attack surface as smal > as possible, you should specify very explicitly what exactly you expect > to happen in the authentication, and disallow any variance. For example, > you expect SCRAM to be used, with a certificate signed by a particular > CA, and if the server requests anything else, that's suspicious and the > connection is aborted. We should make that possible too That's 'require_auth=scram-sha-256 sslmode=verify-ca', no? --Jacob [1] https://www.postgresql.org/message-id/0768cedb-695a-8841-5f8b-da2aa64c8f3a%40timescale.com
On Mon, Apr 29, 2024 at 11:04 AM Jacob Champion <jacob.champion@enterprisedb.com> wrote: > On Fri, Apr 26, 2024 at 3:51 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > > Unfortunately the error message you got in the client with that was > > horrible (I modified the server to not accept the 'postgresql' protocol): > > > > psql "dbname=postgres sslmode=require host=localhost" > > psql: error: connection to server at "localhost" (::1), port 5432 > > failed: SSL error: SSL error code 167773280 > > <long sigh> > > I filed a bug upstream [1]. I think this is on track to be fixed in a future set of OpenSSL 3.x releases [2]. We'll still need to carry the workaround while we support 1.1.1. --Jacob [2] https://github.com/openssl/openssl/pull/24351
On 14/05/2024 01:29, Jacob Champion wrote: > Definitely not a major problem, but I think > select_next_encryption_method() has gone stale, since it originally > provided generality and lines of fallback that no longer exist. In > other words, I think the following code is now misleading: > >> if (conn->sslmode[0] == 'a') >> SELECT_NEXT_METHOD(ENC_PLAINTEXT); >> >> SELECT_NEXT_METHOD(ENC_NEGOTIATED_SSL); >> SELECT_NEXT_METHOD(ENC_DIRECT_SSL); >> >> if (conn->sslmode[0] != 'a') >> SELECT_NEXT_METHOD(ENC_PLAINTEXT); > > To me, that implies that negotiated mode takes precedence over direct, > but the point of the patch is that it's not possible to have both. And > if direct SSL is in use, then sslmode can't be "allow" anyway, and we > definitely don't want ENC_PLAINTEXT. > > So if someone proposes a change to select_next_encryption_method(), > you'll have to remember to stare at init_allowed_encryption_methods() > as well, and think really hard about what's going on. And vice-versa. > That worries me. Ok, yeah, I can see that now. Here's a new version to address that. I merged ENC_SSL_NEGOTIATED_SSL and ENC_SSL_DIRECT_SSL to a single method, ENC_SSL. The places that need to distinguish between them now check conn-sslnegotiation. That seems more clear now that there is no fallback. -- Heikki Linnakangas Neon (https://neon.tech)
Вложения
On Wed, May 15, 2024 at 6:33 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > Ok, yeah, I can see that now. Here's a new version to address that. I > merged ENC_SSL_NEGOTIATED_SSL and ENC_SSL_DIRECT_SSL to a single method, > ENC_SSL. The places that need to distinguish between them now check > conn-sslnegotiation. That seems more clear now that there is no fallback. That change and the new comment that were added seem a lot clearer to me, too; +1. And I like that this potentially preps for encryption=gss/ssl/none or similar. This assertion seems a little strange to me: > if (conn->sslnegotiation[0] == 'p') > { > ProtocolVersion pv; > > Assert(conn->sslnegotiation[0] == 'p'); But other than that nitpick, nothing else jumps out at me at the moment. Thanks, --Jacob
On Wed, May 15, 2024 at 9:33 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > Ok, yeah, I can see that now. Here's a new version to address that. I > merged ENC_SSL_NEGOTIATED_SSL and ENC_SSL_DIRECT_SSL to a single method, > ENC_SSL. The places that need to distinguish between them now check > conn-sslnegotiation. That seems more clear now that there is no fallback. Unless there is a compelling reason to do otherwise, we should expedite getting this committed so that it is included in beta1. Release freeze begins Saturday. Thanks, -- Robert Haas EDB: http://www.enterprisedb.com
> On 16 May 2024, at 15:54, Robert Haas <robertmhaas@gmail.com> wrote: > > On Wed, May 15, 2024 at 9:33 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> Ok, yeah, I can see that now. Here's a new version to address that. I >> merged ENC_SSL_NEGOTIATED_SSL and ENC_SSL_DIRECT_SSL to a single method, >> ENC_SSL. The places that need to distinguish between them now check >> conn-sslnegotiation. That seems more clear now that there is no fallback. > > Unless there is a compelling reason to do otherwise, we should > expedite getting this committed so that it is included in beta1. > Release freeze begins Saturday. +1. Having reread the thread and patch I think we should go for this one. ./daniel
On 16/05/2024 17:08, Daniel Gustafsson wrote: >> On 16 May 2024, at 15:54, Robert Haas <robertmhaas@gmail.com> wrote: >> >> On Wed, May 15, 2024 at 9:33 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: >>> Ok, yeah, I can see that now. Here's a new version to address that. I >>> merged ENC_SSL_NEGOTIATED_SSL and ENC_SSL_DIRECT_SSL to a single method, >>> ENC_SSL. The places that need to distinguish between them now check >>> conn-sslnegotiation. That seems more clear now that there is no fallback. >> >> Unless there is a compelling reason to do otherwise, we should >> expedite getting this committed so that it is included in beta1. >> Release freeze begins Saturday. > > +1. Having reread the thread and patch I think we should go for this one. Yep, committed. Thanks everyone! On 15/05/2024 21:24, Jacob Champion wrote: > This assertion seems a little strange to me: > >> if (conn->sslnegotiation[0] == 'p') >> { >> ProtocolVersion pv; >> >> Assert(conn->sslnegotiation[0] == 'p'); > > But other than that nitpick, nothing else jumps out at me at the moment. Fixed that. It was a leftover, I had the if-else conditions the other way round at one point during development. -- Heikki Linnakangas Neon (https://neon.tech)
On Thu, May 16, 2024 at 10:23 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > Yep, committed. Thanks everyone! Thanks! -- Robert Haas EDB: http://www.enterprisedb.com