On Thu, Sep 19, 2019 at 05:40:15PM -0700, Jeff Davis wrote: > On Tue, 2019-09-17 at 16:04 +0900, Michael Paquier wrote: >> What do you think? There is no need to save down the connection >> parameter value into fe_scram_state. > > I'm not sure I understand this comment, but I removed the extra boolean > flags.
Thanks for considering it. I was just asking about removing those
flags and your thoughts about my thoughts from upthread.
>> is required". Or you have in mind that this error message is better?
>
> I felt it would be a more useful error message.
Okay, fine by me.
>> I think that pgindent would complain with the comment block in
>> check_expected_areq().
>
> Changed.
A trick to make pgindent to ignore some comment blocks is to use
/*--------- at its top and bottom, FWIW.
+$ENV{PGUSER} = "ssltestuser";
$ENV{PGPASSWORD} = "pass";
test_connect_ok() can use a complementary string, so I would use that
in the SSL test part instead of relying too much on the environment
for readability, particularly for the last test added with md5testuser.
Using the environment variable in src/test/authentication/ makes
sense. Maybe that's just a matter of taste :)
+ return (state != NULL && state->state == FE_SCRAM_FINISHED &&
+ strcmp(state->sasl_mechanism, SCRAM_SHA_256_PLUS_NAME) == 0);
I think that we should document in the code why those reasons are
chosen.
I would also add a test for an invalid value of channel_binding.
A comment update is forgotten in libpq-int.h.
+# using the password authentication method; channel binding can't
work
+reset_pg_hba($node, 'password');
+$ENV{"PGCHANNELBINDING"} = 'require';
+test_login($node, 'saslpreptest4a_role', "a", 2);
+
+# SSL not in use; channel binding still can't work
+reset_pg_hba($node, 'scram-sha-256');
+$ENV{"PGCHANNELBINDING"} = 'require';
+test_login($node, 'saslpreptest4a_role', "a", 2);
Those two tests are in the test suite dedicated to SASLprep. I think
that it would be more consistent to just move them to
001_password.pl. And this does not impact the error coverage.
Missing some indentation in the perl scripts (per pgperltidy).
Those are mainly nits, and attached are the changes I would do to your
patch. Please feel free to consider those changes as you see fit.
Anyway, the base logic looks good to me, so I am switching the patch
as ready for committer.
--
Michael
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index 3b40c4b083..61f9512544 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -127,8 +127,20 @@ pg_fe_scram_channel_bound(void *opaq)
{
fe_scram_state *state = (fe_scram_state *) opaq;
- return (state != NULL && state->state == FE_SCRAM_FINISHED &&
- strcmp(state->sasl_mechanism, SCRAM_SHA_256_PLUS_NAME) == 0);
+ /* no SCRAM exchange done */
+ if (state == NULL)
+ return false;
+
+ /* SCRAM exchange not completed */
+ if (state->state != FE_SCRAM_FINISHED)
+ return false;
+
+ /* channel binding mechanism not used */
+ if (strcmp(state->sasl_mechanism, SCRAM_SHA_256_PLUS_NAME) != 0)
+ return false;
+
+ /* all clear! */
+ return true;
}
/*
@@ -368,7 +380,8 @@ build_client_first_message(fe_scram_state *state)
appendPQExpBufferStr(&buf, "p=tls-server-end-point");
}
#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
- else if (conn->ssl_in_use && conn->channel_binding[0] != 'd')
+ else if (conn->channel_binding[0] != 'd' && /* disable */
+ conn->ssl_in_use)
{
/*
* Client supports channel binding, but thinks the server does not.
@@ -508,7 +521,8 @@ build_client_final_message(fe_scram_state *state)
#endif /* HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH */
}
#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
- else if (conn->ssl_in_use && conn->channel_binding[0] != 'd')
+ else if (conn->channel_binding[0] != 'd' && /* disable */
+ conn->ssl_in_use)
appendPQExpBufferStr(&buf, "c=eSws"); /* base64 of "y,," */
#endif
else
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 0ef8fa89d0..0a70e0ea4a 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -423,7 +423,7 @@ pg_SASL_init(PGconn *conn, int payloadlen)
initPQExpBuffer(&mechanism_buf);
- if (conn->channel_binding[0] == 'r' && /* require */
+ if (conn->channel_binding[0] == 'r' && /* require */
!conn->ssl_in_use)
{
printfPQExpBuffer(&conn->errorMessage,
@@ -462,8 +462,8 @@ pg_SASL_init(PGconn *conn, int payloadlen)
/*
* Select the mechanism to use. Pick SCRAM-SHA-256-PLUS over anything
- * else if a channel binding type is set and if the client supports
- * it (and did not set channel_binding=disable). Pick SCRAM-SHA-256 if
+ * else if a channel binding type is set and if the client supports it
+ * (and did not set channel_binding=disable). Pick SCRAM-SHA-256 if
* nothing else has already been picked. If we add more mechanisms, a
* more refined priority mechanism might become necessary.
*/
@@ -477,7 +477,7 @@ pg_SASL_init(PGconn *conn, int payloadlen)
* can be created, and if channel_binding is not disabled.
*/
#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
- if (conn->channel_binding[0] != 'd') /* disable */
+ if (conn->channel_binding[0] != 'd') /* disable */
selected_mechanism = SCRAM_SHA_256_PLUS_NAME;
#endif
}
@@ -502,7 +502,7 @@ pg_SASL_init(PGconn *conn, int payloadlen)
selected_mechanism = SCRAM_SHA_256_NAME;
}
- if (conn->channel_binding[0] == 'r' && /* require */
+ if (conn->channel_binding[0] == 'r' && /* require */
strcmp(selected_mechanism, SCRAM_SHA_256_PLUS_NAME) != 0)
{
printfPQExpBuffer(&conn->errorMessage,
@@ -799,7 +799,7 @@ pg_password_sendauth(PGconn *conn, const char *password, AuthRequest areq)
static bool
check_expected_areq(AuthRequest areq, PGconn *conn)
{
- bool result = true;
+ bool result = true;
/*
* When channel_binding=require, we must protect against two cases: (1) we
@@ -808,7 +808,7 @@ check_expected_areq(AuthRequest areq, PGconn *conn)
* AUTH_REQ_OK, we still must ensure that channel binding has happened in
* order to authenticate the server.
*/
- if (conn->channel_binding[0] == 'r' /* require */)
+ if (conn->channel_binding[0] == 'r' /* require */ )
{
switch (areq)
{
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index a7db473598..f91f0f2efe 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -217,7 +217,7 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
offsetof(struct pg_conn, pgpassfile)},
{"channel_binding", "PGCHANNELBINDING", NULL, NULL,
- "Channel-Binding", "", 7, /* sizeof("require") */
+ "Channel-Binding", "", 7, /* sizeof("require") */
offsetof(struct pg_conn, channel_binding)},
{"connect_timeout", "PGCONNECT_TIMEOUT", NULL, NULL,
@@ -3517,10 +3517,11 @@ keep_going: /* We will come back to here until there is
case CONNECTION_SETENV:
{
/*
- * Do post-connection housekeeping (only needed in protocol 2.0).
+ * Do post-connection housekeeping (only needed in protocol
+ * 2.0).
*
- * We pretend that the connection is OK for the duration of these
- * queries.
+ * We pretend that the connection is OK for the duration of
+ * these queries.
*/
conn->status = CONNECTION_OK;
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 0eb4132767..64468ab4da 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -347,7 +347,8 @@ struct pg_conn
char *pguser; /* Postgres username and password, if any */
char *pgpass;
char *pgpassfile; /* path to a file containing password(s) */
- char *channel_binding; /* channel binding mode (require,prefer) */
+ char *channel_binding; /* channel binding mode
+ * (require,prefer,disable) */
char *keepalives; /* use TCP keepalives? */
char *keepalives_idle; /* time between TCP keepalives */
char *keepalives_interval; /* time between TCP keepalive
diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index 3a3b0eb7e8..aae6de8b34 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -17,7 +17,7 @@ if ($windows_os)
}
else
{
- plan tests => 8;
+ plan tests => 10;
}
@@ -86,3 +86,13 @@ test_role($node, 'md5_role', 'scram-sha-256', 2);
reset_pg_hba($node, 'md5');
test_role($node, 'scram_role', 'md5', 0);
test_role($node, 'md5_role', 'md5', 0);
+
+# Tests for channel binding without SSL.
+# Using the password authentication method; channel binding can't work
+reset_pg_hba($node, 'password');
+$ENV{"PGCHANNELBINDING"} = 'require';
+test_role($node, 'scram_role', 'scram-sha-256', 2);
+# SSL not in use; channel binding still can't work
+reset_pg_hba($node, 'scram-sha-256');
+$ENV{"PGCHANNELBINDING"} = 'require';
+test_role($node, 'scram_role', 'scram-sha-256', 2);
diff --git a/src/test/authentication/t/002_saslprep.pl b/src/test/authentication/t/002_saslprep.pl
index 37f6d19c3a..c4b335c45f 100644
--- a/src/test/authentication/t/002_saslprep.pl
+++ b/src/test/authentication/t/002_saslprep.pl
@@ -14,7 +14,7 @@ if ($windows_os)
}
else
{
- plan tests => 14;
+ plan tests => 12;
}
# Delete pg_hba.conf from the given node, add a new entry to it
@@ -104,13 +104,3 @@ test_login($node, 'saslpreptest6_role', "foobar", 2);
test_login($node, 'saslpreptest7_role', "foo\xd8\xa71bar", 0);
test_login($node, 'saslpreptest7_role', "foo1\xd8\xa7bar", 2);
test_login($node, 'saslpreptest7_role', "foobar", 2);
-
-# using the password authentication method; channel binding can't work
-reset_pg_hba($node, 'password');
-$ENV{"PGCHANNELBINDING"} = 'require';
-test_login($node, 'saslpreptest4a_role', "a", 2);
-
-# SSL not in use; channel binding still can't work
-reset_pg_hba($node, 'scram-sha-256');
-$ENV{"PGCHANNELBINDING"} = 'require';
-test_login($node, 'saslpreptest4a_role', "a", 2);
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index af508e74f5..71e5d771aa 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -18,7 +18,7 @@ if ($ENV{with_openssl} ne 'yes')
plan skip_all => 'SSL not supported by this build';
}
-my $number_of_tests = 5;
+my $number_of_tests = 7;
# This is the hostname used to connect to the server.
my $SERVERHOSTADDR = '127.0.0.1';
@@ -42,22 +42,34 @@ $node->start;
configure_test_server_for_ssl($node, $SERVERHOSTADDR, "scram-sha-256",
"pass", "scram-sha-256");
switch_server_cert($node, 'server-cn-only');
-$ENV{PGUSER} = "ssltestuser";
$ENV{PGPASSWORD} = "pass";
$common_connstr =
"dbname=trustdb sslmode=require sslcert=invalid sslrootcert=invalid hostaddr=$SERVERHOSTADDR";
# Default settings
-test_connect_ok($common_connstr, '', "Basic SCRAM authentication with SSL");
+test_connect_ok($common_connstr, "user=ssltestuser",
+ "Basic SCRAM authentication with SSL");
# Test channel_binding
-$ENV{PGCHANNELBINDING} = "disable";
-test_connect_ok($common_connstr, '', "SCRAM with SSL and channel_binding=disable");
-$ENV{PGCHANNELBINDING} = "require";
-test_connect_ok($common_connstr, '', "SCRAM with SSL and channel_binding=require");
+test_connect_fails(
+ $common_connstr,
+ "user=ssltestuser channel_binding=invalid_value",
+ qr/invalid channel_binding value: "invalid_value"/,
+ "SCRAM with SSL and channel_binding=invalid_value");
+test_connect_ok(
+ $common_connstr,
+ "user=ssltestuser channel_binding=disable",
+ "SCRAM with SSL and channel_binding=disable");
+test_connect_ok(
+ $common_connstr,
+ "user=ssltestuser channel_binding=require",
+ "SCRAM with SSL and channel_binding=require");
# Now test when the user has an MD5-encrypted password; should fail
-$ENV{PGUSER} = "md5testuser";
-test_connect_fails($common_connstr, '', qr/Channel binding required but not supported by server's authentication request/, "MD5 with SSL and channel_binding=require");
+test_connect_fails(
+ $common_connstr,
+ "user=md5testuser channel_binding=require",
+ qr/Channel binding required but not supported by server's authentication request/,
+ "MD5 with SSL and channel_binding=require");
done_testing($number_of_tests);
diff --git a/src/test/ssl/t/SSLServer.pm b/src/test/ssl/t/SSLServer.pm
index 9288b42811..005955a2ff 100644
--- a/src/test/ssl/t/SSLServer.pm
+++ b/src/test/ssl/t/SSLServer.pm
@@ -114,7 +114,7 @@ sub configure_test_server_for_ssl
{
$node->psql('postgres',
"SET password_encryption='$password_enc'; ALTER USER ssltestuser PASSWORD '$password';"
- );
+ );
# A special user that always has an md5-encrypted password
$node->psql('postgres',
"SET password_encryption='md5'; ALTER USER md5testuser PASSWORD '$password';"
signature.asc
Description: PGP signature
