pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Add libpq parameter 'channel_binding'.
15+ messages / 4 participants
[nested] [flat]
* pgsql: Add libpq parameter 'channel_binding'.
@ 2019-09-23 21:05 Jeff Davis <jdavis@postgresql.org>
2019-09-29 16:51 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 15+ messages in thread
From: Jeff Davis @ 2019-09-23 21:05 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
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(-)
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pgsql: Add libpq parameter 'channel_binding'.
2019-09-23 21:05 pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <jdavis@postgresql.org>
@ 2019-09-29 16:51 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-09-29 23:47 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2019-09-29 16:51 UTC (permalink / raw)
To: Jeff Davis <jdavis@postgresql.org>; +Cc: pgsql-committers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pgsql: Add libpq parameter 'channel_binding'.
2019-09-23 21:05 pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <jdavis@postgresql.org>
2019-09-29 16:51 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
@ 2019-09-29 23:47 ` Michael Paquier <michael@paquier.xyz>
2019-09-30 01:46 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 06:45 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
0 siblings, 2 replies; 15+ messages in thread
From: Michael Paquier @ 2019-09-29 23:47 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Davis <jdavis@postgresql.org>; pgsql-committers@lists.postgresql.org
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190929234733.GA2888@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pgsql: Add libpq parameter 'channel_binding'.
2019-09-23 21:05 pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <jdavis@postgresql.org>
2019-09-29 16:51 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
2019-09-29 23:47 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
@ 2019-09-30 01:46 ` Michael Paquier <michael@paquier.xyz>
2019-09-30 03:39 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 15+ messages in thread
From: Michael Paquier @ 2019-09-30 01:46 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Davis <jdavis@postgresql.org>; pgsql-committers@lists.postgresql.org
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190930014641.GE2888@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pgsql: Add libpq parameter 'channel_binding'.
2019-09-23 21:05 pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <jdavis@postgresql.org>
2019-09-29 16:51 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
2019-09-29 23:47 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 01:46 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
@ 2019-09-30 03:39 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-09-30 03:43 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2019-09-30 03:39 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Jeff Davis <jdavis@postgresql.org>; pgsql-committers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pgsql: Add libpq parameter 'channel_binding'.
2019-09-23 21:05 pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <jdavis@postgresql.org>
2019-09-29 16:51 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
2019-09-29 23:47 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 01:46 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 03:39 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
@ 2019-09-30 03:43 ` Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 15+ messages in thread
From: Michael Paquier @ 2019-09-30 03:43 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Davis <jdavis@postgresql.org>; pgsql-committers@lists.postgresql.org
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190930034314.GH2888@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pgsql: Add libpq parameter 'channel_binding'.
2019-09-23 21:05 pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <jdavis@postgresql.org>
2019-09-29 16:51 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
2019-09-29 23:47 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
@ 2019-09-30 06:45 ` Michael Paquier <michael@paquier.xyz>
2019-09-30 07:08 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 15+ messages in thread
From: Michael Paquier @ 2019-09-30 06:45 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Davis <jdavis@postgresql.org>; pgsql-committers@lists.postgresql.org
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190930064539.GK2888@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pgsql: Add libpq parameter 'channel_binding'.
2019-09-23 21:05 pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <jdavis@postgresql.org>
2019-09-29 16:51 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
2019-09-29 23:47 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 06:45 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
@ 2019-09-30 07:08 ` Michael Paquier <michael@paquier.xyz>
2019-09-30 13:32 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
2019-09-30 18:14 ` Re: pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <pgsql@j-davis.com>
2019-09-30 21:38 ` Re: pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <pgsql@j-davis.com>
0 siblings, 3 replies; 15+ messages in thread
From: Michael Paquier @ 2019-09-30 07:08 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Davis <jdavis@postgresql.org>; pgsql-committers@lists.postgresql.org
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
Attachments:
[text/x-diff] channel-binding-fix-v1.patch (1.3K, ../../20190930070826.GM2888@paquier.xyz/2-channel-binding-fix-v1.patch)
download | inline diff:
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 04118d54e2..d6e4036641 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -471,14 +471,28 @@ pg_SASL_init(PGconn *conn, int payloadlen)
{
if (conn->ssl_in_use)
{
- /*
- * The server has offered SCRAM-SHA-256-PLUS, which is only
- * supported by the client if a hash of the peer certificate
- * can be created, and if channel_binding is not disabled.
- */
+ /* The server has offered SCRAM-SHA-256-PLUS */
+
#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
+ /*
+ * The client supports channel binding, which is chosen if
+ * channel_binding is not disabled.
+ */
if (conn->channel_binding[0] != 'd') /* disable */
selected_mechanism = SCRAM_SHA_256_PLUS_NAME;
+#else
+ /*
+ * The client does not support channel binding. If it is
+ * required, complain immediately instead of the error below
+ * which would be confusing as the server is publishing
+ * SCRAM-SHA-256-PLUS.
+ */
+ if (conn->channel_binding[0] == 'r') /* require */
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("channel binding is required, but client does not support it\n"));
+ goto error;
+ }
#endif
}
else
[application/pgp-signature] signature.asc (832B, ../../20190930070826.GM2888@paquier.xyz/3-signature.asc)
download
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pgsql: Add libpq parameter 'channel_binding'.
2019-09-23 21:05 pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <jdavis@postgresql.org>
2019-09-29 16:51 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
2019-09-29 23:47 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 06:45 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 07:08 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
@ 2019-09-30 13:32 ` Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 0 replies; 15+ messages in thread
From: Tom Lane @ 2019-09-30 13:32 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Jeff Davis <jdavis@postgresql.org>; pgsql-committers@lists.postgresql.org
Michael Paquier <michael@paquier.xyz> writes:
> So, something like the attached looks better to me.
+1
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pgsql: Add libpq parameter 'channel_binding'.
2019-09-23 21:05 pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <jdavis@postgresql.org>
2019-09-29 16:51 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
2019-09-29 23:47 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 06:45 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 07:08 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
@ 2019-09-30 18:14 ` Jeff Davis <pgsql@j-davis.com>
2019-09-30 18:20 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 1 reply; 15+ messages in thread
From: Jeff Davis @ 2019-09-30 18:14 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Davis <jdavis@postgresql.org>; pgsql-committers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pgsql: Add libpq parameter 'channel_binding'.
2019-09-23 21:05 pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <jdavis@postgresql.org>
2019-09-29 16:51 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
2019-09-29 23:47 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 06:45 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 07:08 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 18:14 ` Re: pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <pgsql@j-davis.com>
@ 2019-09-30 18:20 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-10-01 02:04 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2019-09-30 18:20 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Jeff Davis <jdavis@postgresql.org>; pgsql-committers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pgsql: Add libpq parameter 'channel_binding'.
2019-09-23 21:05 pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <jdavis@postgresql.org>
2019-09-29 16:51 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
2019-09-29 23:47 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 06:45 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 07:08 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 18:14 ` Re: pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <pgsql@j-davis.com>
2019-09-30 18:20 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
@ 2019-10-01 02:04 ` Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 15+ messages in thread
From: Michael Paquier @ 2019-10-01 02:04 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Davis <pgsql@j-davis.com>; Jeff Davis <jdavis@postgresql.org>; pgsql-committers@lists.postgresql.org
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20191001020430.GD2781@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pgsql: Add libpq parameter 'channel_binding'.
2019-09-23 21:05 pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <jdavis@postgresql.org>
2019-09-29 16:51 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
2019-09-29 23:47 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 06:45 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 07:08 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
@ 2019-09-30 21:38 ` Jeff Davis <pgsql@j-davis.com>
2019-09-30 21:41 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 1 reply; 15+ messages in thread
From: Jeff Davis @ 2019-09-30 21:38 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Davis <jdavis@postgresql.org>; pgsql-committers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pgsql: Add libpq parameter 'channel_binding'.
2019-09-23 21:05 pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <jdavis@postgresql.org>
2019-09-29 16:51 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
2019-09-29 23:47 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 06:45 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 07:08 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 21:38 ` Re: pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <pgsql@j-davis.com>
@ 2019-09-30 21:41 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-10-01 00:43 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2019-09-30 21:41 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-committers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: pgsql: Add libpq parameter 'channel_binding'.
2019-09-23 21:05 pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <jdavis@postgresql.org>
2019-09-29 16:51 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
2019-09-29 23:47 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 06:45 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 07:08 ` Re: pgsql: Add libpq parameter 'channel_binding'. Michael Paquier <michael@paquier.xyz>
2019-09-30 21:38 ` Re: pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <pgsql@j-davis.com>
2019-09-30 21:41 ` Re: pgsql: Add libpq parameter 'channel_binding'. Tom Lane <tgl@sss.pgh.pa.us>
@ 2019-10-01 00:43 ` Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 15+ messages in thread
From: Michael Paquier @ 2019-10-01 00:43 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Jeff Davis <pgsql@j-davis.com>; pgsql-committers@lists.postgresql.org
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20191001004342.GC2781@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2019-10-01 02:04 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-09-23 21:05 pgsql: Add libpq parameter 'channel_binding'. Jeff Davis <jdavis@postgresql.org>
2019-09-29 16:51 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-09-29 23:47 ` Michael Paquier <michael@paquier.xyz>
2019-09-30 01:46 ` Michael Paquier <michael@paquier.xyz>
2019-09-30 03:39 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-09-30 03:43 ` Michael Paquier <michael@paquier.xyz>
2019-09-30 06:45 ` Michael Paquier <michael@paquier.xyz>
2019-09-30 07:08 ` Michael Paquier <michael@paquier.xyz>
2019-09-30 13:32 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-09-30 18:14 ` Jeff Davis <pgsql@j-davis.com>
2019-09-30 18:20 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-10-01 02:04 ` Michael Paquier <michael@paquier.xyz>
2019-09-30 21:38 ` Jeff Davis <pgsql@j-davis.com>
2019-09-30 21:41 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-10-01 00:43 ` Michael Paquier <michael@paquier.xyz>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox