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';"