Обсуждение: pgsql: Add libpq parameter 'channel_binding'.
Add libpq parameter 'channel_binding'. Allow clients to require channel binding to enhance security against untrusted servers. Author: Jeff Davis Reviewed-by: Michael Paquier Discussion: https://postgr.es/m/227015d8417f2b4fef03f8966dbfa5cbcc4f44da.camel%40j-davis.com Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/d6e612f837e235db0411e8b67558c9a6b3e9f41f Modified Files -------------- doc/src/sgml/libpq.sgml | 32 +++++++++++++ src/interfaces/libpq/fe-auth-scram.c | 41 ++++++++++++++--- src/interfaces/libpq/fe-auth.c | 76 ++++++++++++++++++++++++++++--- src/interfaces/libpq/fe-auth.h | 1 + src/interfaces/libpq/fe-connect.c | 41 +++++++++++++++-- src/interfaces/libpq/libpq-int.h | 2 + src/test/authentication/t/001_password.pl | 12 ++++- src/test/ssl/t/002_scram.pl | 39 ++++++++++++++-- src/test/ssl/t/SSLServer.pm | 9 +++- 9 files changed, 233 insertions(+), 20 deletions(-)
Jeff Davis <jdavis@postgresql.org> writes:
> Add libpq parameter 'channel_binding'.
I found out the hard way that the added ssl tests fall over on a
platform that doesn't HAVE_X509_GET_SIGNATURE_NID:
# Running: psql -X -A -t -c SELECT $$connected with user=ssltestuser channel_bin
ding=require$$ -d dbname=trustdb sslmode=require sslcert=invalid sslrootcert=inv
alid hostaddr=127.0.0.1 user=ssltestuser channel_binding=require
psql: error: could not connect to server: channel binding is required, but serve
r did not offer an authentication method that supports channel binding
not ok 5 - SCRAM with SSL and channel_binding=require
# Failed test 'SCRAM with SSL and channel_binding=require'
# at t/002_scram.pl line 63.
I don't think that it's acceptable for the test to fail on a platform
that we're willing to compile on. Maybe just skip these tests if we
lack X509_get_signature_nid?
Another point is that this error message is misleading --- or at least
would be misleading if the server had X509_get_signature_nid and the
client didn't. Perhaps do something like
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 04118d5..4289551 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -476,9 +476,15 @@ pg_SASL_init(PGconn *conn, int payloadlen)
* supported by the client if a hash of the peer certificate
* can be created, and if channel_binding is not disabled.
*/
-#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
if (conn->channel_binding[0] != 'd') /* disable */
+ {
+#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
selected_mechanism = SCRAM_SHA_256_PLUS_NAME;
+#else
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("client does not support SCRAM-SHA-256-PLUS authentication\n"));
+ goto error;
+ }
#endif
}
else
regards, tom lane
On Sun, Sep 29, 2019 at 12:51:31PM -0400, Tom Lane wrote: > I found out the hard way that the added ssl tests fall over on a > platform that doesn't HAVE_X509_GET_SIGNATURE_NID: > > [...] > > I don't think that it's acceptable for the test to fail on a platform > that we're willing to compile on. Maybe just skip these tests if we > lack X509_get_signature_nid? Yes, that's the bug I found three days ago for which I have posted a patch here: https://www.postgresql.org/message-id/20190927024457.GA8485@paquier.xyz In short, I think that the proper way is to adapt the test if X509_get_signature_nid is not around. > Another point is that this error message is misleading --- or at least > would be misleading if the server had X509_get_signature_nid and the > client didn't. > > -#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH > if (conn->channel_binding[0] != 'd') /* disable */ > + { > +#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH > selected_mechanism = SCRAM_SHA_256_PLUS_NAME; > +#else > + printfPQExpBuffer(&conn->errorMessage, > + libpq_gettext("client does not support SCRAM-SHA-256-PLUS authentication\n")); > + goto error; > + } > #endif > } Yes, it looks sensible to do that. -- Michael
Вложения
On Mon, Sep 30, 2019 at 08:47:33AM +0900, Michael Paquier wrote: > Yes, it looks sensible to do that. Tom, would you prefer taking take of both issues by yourself? Do you mind if I handle both things? Or I could just commit my fix first, then leave it to you for the second one. I don't mind doing both FWIW. -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Sep 30, 2019 at 08:47:33AM +0900, Michael Paquier wrote:
>> Yes, it looks sensible to do that.
> Tom, would you prefer taking take of both issues by yourself? Do you
> mind if I handle both things? Or I could just commit my fix first,
> then leave it to you for the second one. I don't mind doing both
> FWIW.
Feel free to deal with both.
regards, tom lane
On Sun, Sep 29, 2019 at 11:39:12PM -0400, Tom Lane wrote: > Feel free to deal with both. Thanks. Will do so now then. -- Michael
Вложения
On Mon, Sep 30, 2019 at 08:47:33AM +0900, Michael Paquier wrote:
> On Sun, Sep 29, 2019 at 12:51:31PM -0400, Tom Lane wrote:
>> Another point is that this error message is misleading --- or at least
>> would be misleading if the server had X509_get_signature_nid and the
>> client didn't.
>>
>> -#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
>> if (conn->channel_binding[0] != 'd') /* disable */
>> + {
>> +#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
>> selected_mechanism = SCRAM_SHA_256_PLUS_NAME;
>> +#else
>> + printfPQExpBuffer(&conn->errorMessage,
>> + libpq_gettext("client does not support SCRAM-SHA-256-PLUS authentication\n"));
>> + goto error;
>> + }
>> #endif
>> }
>
> Yes, it looks sensible to do that.
If the server publishes SCRAM-SHA-256-PLUS and the server does not
support channel binding, then we get this error message:
"channel binding is required, but server did not offer an
authentication method that supports channel binding."
So that's the part which is wrong.
Now, I am not completely sure that the suggested change is completely
right either as we would get an error in this scenario when
channel_binding is "prefer" or "require". For "require", this error
message is fine. However, for "prefer", shouldn't we do what we do on
HEAD, aka *not* select SCRAM-SHA-256-PLUS and switch to SCRAM-SHA-256?
This would have the advantage to make the connection work with default
parameters.
--
Michael
Вложения
On Mon, Sep 30, 2019 at 03:45:39PM +0900, Michael Paquier wrote: > If the server publishes SCRAM-SHA-256-PLUS and the server does not > support channel binding, then we get this error message: > "channel binding is required, but server did not offer an > authentication method that supports channel binding." > So that's the part which is wrong. > > Now, I am not completely sure that the suggested change is completely > right either as we would get an error in this scenario when > channel_binding is "prefer" or "require". For "require", this error > message is fine. However, for "prefer", shouldn't we do what we do on > HEAD, aka *not* select SCRAM-SHA-256-PLUS and switch to SCRAM-SHA-256? > This would have the advantage to make the connection work with default > parameters. So, something like the attached looks better to me. Using a server which publishes SCRAM-SHA-256-PLUS, I get the following over SSL: 1) client supports channel binding: 1-1) channel_binding = disable => OK, with SCRAM-SHA-256 1-2) channel_binding = prefer => OK, with SCRAM-SHA-256-PLUS 1-3) channel_binding = require => OK, with SCRAM-SHA-256-PLUS 2) client does not support channel binding 2-1) channel_binding = disable => OK, with SCRAM-SHA-256 2-2) channel_binding = prefer => OK, with SCRAM-SHA-256 2-3) channel_binding = require => failure with new error message, instead of the confusing one. The bug is with 2-3, and Tom's suggestion would have switched 2-2 to a failure (2-2 works on HEAD). -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes:
> So, something like the attached looks better to me.
+1
regards, tom lane
On Mon, 2019-09-30 at 16:08 +0900, Michael Paquier wrote:
> So, something like the attached looks better to me. Using a server
> which publishes SCRAM-SHA-256-PLUS, I get the following over SSL:
> 1) client supports channel binding:
> 1-1) channel_binding = disable => OK, with SCRAM-SHA-256
> 1-2) channel_binding = prefer => OK, with SCRAM-SHA-256-PLUS
> 1-3) channel_binding = require => OK, with SCRAM-SHA-256-PLUS
> 2) client does not support channel binding
> 2-1) channel_binding = disable => OK, with SCRAM-SHA-256
> 2-2) channel_binding = prefer => OK, with SCRAM-SHA-256
> 2-3) channel_binding = require => failure with new error message,
> instead of the confusing one.
For 2-3, shouldn't we error at an earlier stage? The user of the client
has requested something impossible to satisfy.
Regards,
Jeff Davis
Jeff Davis <pgsql@j-davis.com> writes:
> For 2-3, shouldn't we error at an earlier stage? The user of the client
> has requested something impossible to satisfy.
Can't get excited about that. It'd require duplicating this code
somewhere else, which is a maintenance issue. And the case of building
with obsolete OpenSSL ought to be fairly infrequent and getting more so
as time goes on, so I'm not really eager to expend lots of work on it.
regards, tom lane
On Mon, 2019-09-30 at 16:08 +0900, Michael Paquier wrote:
> So, something like the attached looks better to me. Using a server
> which publishes SCRAM-SHA-256-PLUS, I get the following over SSL:
> 1) client supports channel binding:
> 1-1) channel_binding = disable => OK, with SCRAM-SHA-256
> 1-2) channel_binding = prefer => OK, with SCRAM-SHA-256-PLUS
> 1-3) channel_binding = require => OK, with SCRAM-SHA-256-PLUS
> 2) client does not support channel binding
> 2-1) channel_binding = disable => OK, with SCRAM-SHA-256
> 2-2) channel_binding = prefer => OK, with SCRAM-SHA-256
> 2-3) channel_binding = require => failure with new error message,
> instead of the confusing one.
>
> The bug is with 2-3, and Tom's suggestion would have switched 2-2 to
> a
> failure (2-2 works on HEAD).
Looks good to me, though I think you need to update the expected error
message in the test you just added.
Regards,
Jeff Davis
Jeff Davis <pgsql@j-davis.com> writes:
> Looks good to me, though I think you need to update the expected error
> message in the test you just added.
The test case did pass for me when I tried it on an old-openssl machine
a few hours ago. I don't think this test has any way to exercise the
code path where the server has support and the client doesn't (or
vice versa).
regards, tom lane
On Mon, Sep 30, 2019 at 05:41:46PM -0400, Tom Lane wrote: > Jeff Davis <pgsql@j-davis.com> writes: >> Looks good to me, though I think you need to update the expected error >> message in the test you just added. > > The test case did pass for me when I tried it on an old-openssl machine > a few hours ago. I don't think this test has any way to exercise the > code path where the server has support and the client doesn't (or > vice versa). The behaviors of "prefer" which make sense with or without channel binding support on the client-side is actually what matters here when the server sends back SCRAM-SHA-256-PLUS over SSL. We could use a compile flag and enforce it in a buildfarm animal, or have more modes within the parameter, but the gains are not really worth the code complications in my opinion, and the parameter is already complicated enough. -- Michael
Вложения
On Mon, Sep 30, 2019 at 02:20:29PM -0400, Tom Lane wrote: > Jeff Davis <pgsql@j-davis.com> writes: >> For 2-3, shouldn't we error at an earlier stage? The user of the client >> has requested something impossible to satisfy. > > Can't get excited about that. It'd require duplicating this code > somewhere else, which is a maintenance issue. And the case of building > with obsolete OpenSSL ought to be fairly infrequent and getting more so > as time goes on, so I'm not really eager to expend lots of work on it. Neither am I, and there is one extra reason on top of what Tom has mentioned: there is still value in warning the client if a rogue server sends SCRAM-SHA-256-PLUS without SSL even if channel_binding is required. I have double-checked the patch and done more tests (server publishing SCRAM-SHA-256-PLUS with various libpq clients). I have included the full description of the behavior in the commit log, and applied it. -- Michael