Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit fb5718f

Browse filesBrowse files
hlinnakahlinnaka
committed
Remove option to fall back from direct to postgres SSL negotiation
There were three problems with the sslnegotiation options: 1. The sslmode=prefer and sslnegotiation=requiredirect combination was somewhat dangerous, as you might unintentionally fall back to plaintext authentication when connecting to a pre-v17 server. 2. There was an asymmetry between 'postgres' and 'direct' options. 'postgres' meant "try only traditional negotiation", while 'direct' meant "try direct first, and fall back to traditional negotiation if it fails". That was apparent only if you knew 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. However, I don't consider direct SSL connections to be a security feature. To address these problems: - Only allow sslnegotiation='direct' if sslmode='require' or stronger. And for the record, Jacob and Robert felt that we should do that (or have sslnegotiation='direct' imply sslmode='require') anyway, regardless of the first issue. - Remove the 'direct' mode that falls back to traditional negotiation, and rename what was called 'requiredirect' to 'direct' instead. In other words, there is no "try both methods" option anymore, 'postgres' now means the traditional negotiation and 'direct' means a direct SSL connection. Reviewed-by: Jelte Fennema-Nio, Robert Haas, Jacob Champion Discussion: https://www.postgresql.org/message-id/d3b1608a-a1b6-4eda-9ec5-ddb3e4375808%40iki.fi
1 parent 8ba3462 commit fb5718f
Copy full SHA for fb5718f

File tree

Expand file treeCollapse file tree

5 files changed

+200
-253
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

5 files changed

+200
-253
lines changed
Open diff view settings
Collapse file

‎doc/src/sgml/libpq.sgml‎

Copy file name to clipboardExpand all lines: doc/src/sgml/libpq.sgml
+17-32Lines changed: 17 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1773,15 +1773,18 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
17731773
<term><literal>sslnegotiation</literal></term>
17741774
<listitem>
17751775
<para>
1776-
This option controls whether <productname>PostgreSQL</productname>
1777-
will perform its protocol negotiation to request encryption from the
1778-
server or will just directly make a standard <acronym>SSL</acronym>
1779-
connection. Traditional <productname>PostgreSQL</productname>
1780-
protocol negotiation is the default and the most flexible with
1781-
different server configurations. If the server is known to support
1782-
direct <acronym>SSL</acronym> connections then the latter requires one
1783-
fewer round trip reducing connection latency and also allows the use
1784-
of protocol agnostic SSL network tools.
1776+
This option controls how SSL encryption is negotiated with the server,
1777+
if SSL is used. In the default <literal>postgres</literal> mode, the
1778+
client first asks the server if SSL is supported. In
1779+
<literal>direct</literal> mode, the client starts the standard SSL
1780+
handshake directly after establishing the TCP/IP connection. Traditional
1781+
<productname>PostgreSQL</productname> protocol negotiation is the most
1782+
flexible with different server configurations. If the server is known
1783+
to support direct <acronym>SSL</acronym> connections then the latter
1784+
requires one fewer round trip reducing connection latency and also
1785+
allows the use of protocol agnostic SSL network tools. The direct SSL
1786+
option was introduced in <productname>PostgreSQL</productname> version
1787+
17.
17851788
</para>
17861789

17871790
<variablelist>
@@ -1799,32 +1802,14 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
17991802
<term><literal>direct</literal></term>
18001803
<listitem>
18011804
<para>
1802-
first attempt to establish a standard SSL connection and if that
1803-
fails reconnect and perform the negotiation. This fallback
1804-
process adds significant latency if the initial SSL connection
1805-
fails.
1806-
</para>
1807-
<para>
1808-
An exception is if <literal>gssencmode</literal> is set
1809-
to <literal>prefer</literal>, but the server rejects GSS encryption.
1810-
In that case, SSL is negotiated over the same TCP connection using
1811-
<productname>PostgreSQL</productname> protocol negotiation. In
1812-
other words, the direct SSL handshake is not used, if a TCP
1813-
connection has already been established and can be used for the
1805+
start SSL handshake directly after establishing the TCP/IP
1806+
connection. This is only allowed with sslmode=require or higher,
1807+
because the weaker settings could lead to unintended fallback to
1808+
plaintext authentication when the server does not support direct
18141809
SSL handshake.
18151810
</para>
18161811
</listitem>
18171812
</varlistentry>
1818-
1819-
<varlistentry>
1820-
<term><literal>requiredirect</literal></term>
1821-
<listitem>
1822-
<para>
1823-
attempt to establish a standard SSL connection and if that fails
1824-
return a connection failure immediately.
1825-
</para>
1826-
</listitem>
1827-
</varlistentry>
18281813
</variablelist>
18291814
</listitem>
18301815
</varlistentry>
@@ -2065,7 +2050,7 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
20652050
connections without having to decrypt the SSL stream. (Note that
20662051
unless the proxy is aware of the PostgreSQL protocol handshake this
20672052
would require setting <literal>sslnegotiation</literal>
2068-
to <literal>direct</literal> or <literal>requiredirect</literal>.)
2053+
to <literal>direct</literal>.)
20692054
However, <acronym>SNI</acronym> makes the destination host name appear
20702055
in cleartext in the network traffic, so it might be undesirable in
20712056
some cases.
Collapse file

‎src/interfaces/libpq/fe-connect.c‎

Copy file name to clipboardExpand all lines: src/interfaces/libpq/fe-connect.c
+70-72Lines changed: 70 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -274,7 +274,7 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
274274
offsetof(struct pg_conn, sslmode)},
275275

276276
{"sslnegotiation", "PGSSLNEGOTIATION", DefaultSSLNegotiation, NULL,
277-
"SSL-Negotiation", "", 14, /* sizeof("requiredirect") == 14 */
277+
"SSL-Negotiation", "", 9, /* sizeof("postgres") == 9 */
278278
offsetof(struct pg_conn, sslnegotiation)},
279279

280280
{"sslcompression", "PGSSLCOMPRESSION", "0", NULL,
@@ -1590,8 +1590,7 @@ pqConnectOptions2(PGconn *conn)
15901590
if (conn->sslnegotiation)
15911591
{
15921592
if (strcmp(conn->sslnegotiation, "postgres") != 0
1593-
&& strcmp(conn->sslnegotiation, "direct") != 0
1594-
&& strcmp(conn->sslnegotiation, "requiredirect") != 0)
1593+
&& strcmp(conn->sslnegotiation, "direct") != 0)
15951594
{
15961595
conn->status = CONNECTION_BAD;
15971596
libpq_append_conn_error(conn, "invalid %s value: \"%s\"",
@@ -1608,6 +1607,25 @@ pqConnectOptions2(PGconn *conn)
16081607
return false;
16091608
}
16101609
#endif
1610+
1611+
/*
1612+
* Don't allow direct SSL negotiation with sslmode='prefer', because
1613+
* that poses a risk of unintentional fallback to plaintext connection
1614+
* when connecting to a pre-v17 server that does not support direct
1615+
* SSL connections. To keep things simple, don't allow it with
1616+
* sslmode='allow' or sslmode='disable' either. If a user goes through
1617+
* the trouble of setting sslnegotiation='direct', they probably
1618+
* intend to use SSL, and sslmode=disable or allow is probably a user
1619+
* user mistake anyway.
1620+
*/
1621+
if (conn->sslnegotiation[0] == 'd' &&
1622+
conn->sslmode[0] != 'r' && conn->sslmode[0] != 'v')
1623+
{
1624+
conn->status = CONNECTION_BAD;
1625+
libpq_append_conn_error(conn, "weak sslmode \"%s\" may not be used with sslnegotiation=direct (use \"require\", \"verify-ca\", or \"verify-full\")",
1626+
conn->sslmode);
1627+
return false;
1628+
}
16111629
}
16121630
else
16131631
{
@@ -3347,42 +3365,45 @@ PQconnectPoll(PGconn *conn)
33473365
goto error_return;
33483366

33493367
/*
3350-
* If direct SSL is enabled, jump right into SSL handshake. We
3351-
* will come back here after SSL encryption has been
3352-
* established, with ssl_in_use set.
3368+
* If SSL is enabled, start the SSL negotiation. We will come
3369+
* back here after SSL encryption has been established, with
3370+
* ssl_in_use set.
33533371
*/
3354-
if (conn->current_enc_method == ENC_DIRECT_SSL && !conn->ssl_in_use)
3372+
if (conn->current_enc_method == ENC_SSL && !conn->ssl_in_use)
33553373
{
3356-
conn->status = CONNECTION_SSL_STARTUP;
3357-
return PGRES_POLLING_WRITING;
3358-
}
3359-
3360-
/*
3361-
* If negotiated SSL is enabled, request SSL and proceed with
3362-
* SSL handshake. We will come back here after SSL encryption
3363-
* has been established, with ssl_in_use set.
3364-
*/
3365-
if (conn->current_enc_method == ENC_NEGOTIATED_SSL && !conn->ssl_in_use)
3366-
{
3367-
ProtocolVersion pv;
3368-
33693374
/*
3370-
* Send the SSL request packet.
3371-
*
3372-
* Theoretically, this could block, but it really
3373-
* shouldn't since we only got here if the socket is
3374-
* write-ready.
3375+
* If traditional postgres SSL negotiation is used, send
3376+
* the SSL request. In direct negotiation, jump straight
3377+
* into the SSL handshake.
33753378
*/
3376-
pv = pg_hton32(NEGOTIATE_SSL_CODE);
3377-
if (pqPacketSend(conn, 0, &pv, sizeof(pv)) != STATUS_OK)
3379+
if (conn->sslnegotiation[0] == 'p')
33783380
{
3379-
libpq_append_conn_error(conn, "could not send SSL negotiation packet: %s",
3380-
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
3381-
goto error_return;
3381+
ProtocolVersion pv;
3382+
3383+
/*
3384+
* Send the SSL request packet.
3385+
*
3386+
* Theoretically, this could block, but it really
3387+
* shouldn't since we only got here if the socket is
3388+
* write-ready.
3389+
*/
3390+
pv = pg_hton32(NEGOTIATE_SSL_CODE);
3391+
if (pqPacketSend(conn, 0, &pv, sizeof(pv)) != STATUS_OK)
3392+
{
3393+
libpq_append_conn_error(conn, "could not send SSL negotiation packet: %s",
3394+
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
3395+
goto error_return;
3396+
}
3397+
/* Ok, wait for response */
3398+
conn->status = CONNECTION_SSL_STARTUP;
3399+
return PGRES_POLLING_READING;
3400+
}
3401+
else
3402+
{
3403+
Assert(conn->sslnegotiation[0] == 'd');
3404+
conn->status = CONNECTION_SSL_STARTUP;
3405+
return PGRES_POLLING_WRITING;
33823406
}
3383-
/* Ok, wait for response */
3384-
conn->status = CONNECTION_SSL_STARTUP;
3385-
return PGRES_POLLING_READING;
33863407
}
33873408
#endif /* USE_SSL */
33883409

@@ -3453,11 +3474,11 @@ PQconnectPoll(PGconn *conn)
34533474
PostgresPollingStatusType pollres;
34543475

34553476
/*
3456-
* On first time through, get the postmaster's response to our
3457-
* SSL negotiation packet. If we are trying a direct ssl
3458-
* connection, go straight to initiating ssl.
3477+
* On first time through with traditional SSL negotiation, get
3478+
* the postmaster's response to our SSLRequest packet. With
3479+
* sslnegotiation='direct', go straight to initiating SSL.
34593480
*/
3460-
if (!conn->ssl_in_use && conn->current_enc_method == ENC_NEGOTIATED_SSL)
3481+
if (!conn->ssl_in_use && conn->sslnegotiation[0] == 'p')
34613482
{
34623483
/*
34633484
* We use pqReadData here since it has the logic to
@@ -4282,7 +4303,7 @@ init_allowed_encryption_methods(PGconn *conn)
42824303
if (conn->raddr.addr.ss_family == AF_UNIX)
42834304
{
42844305
/* Don't request SSL or GSSAPI over Unix sockets */
4285-
conn->allowed_enc_methods &= ~(ENC_DIRECT_SSL | ENC_NEGOTIATED_SSL | ENC_GSSAPI);
4306+
conn->allowed_enc_methods &= ~(ENC_SSL | ENC_GSSAPI);
42864307

42874308
/*
42884309
* XXX: we probably should not do this. sslmode=require works
@@ -4309,12 +4330,7 @@ init_allowed_encryption_methods(PGconn *conn)
43094330
/* sslmode anything but 'disable', and GSSAPI not required */
43104331
if (conn->sslmode[0] != 'd' && conn->gssencmode[0] != 'r')
43114332
{
4312-
if (conn->sslnegotiation[0] == 'p')
4313-
conn->allowed_enc_methods |= ENC_NEGOTIATED_SSL;
4314-
else if (conn->sslnegotiation[0] == 'd')
4315-
conn->allowed_enc_methods |= ENC_DIRECT_SSL | ENC_NEGOTIATED_SSL;
4316-
else if (conn->sslnegotiation[0] == 'r')
4317-
conn->allowed_enc_methods |= ENC_DIRECT_SSL;
4333+
conn->allowed_enc_methods |= ENC_SSL;
43184334
}
43194335
#endif
43204336

@@ -4354,7 +4370,8 @@ encryption_negotiation_failed(PGconn *conn)
43544370

43554371
if (select_next_encryption_method(conn, true))
43564372
{
4357-
if (conn->current_enc_method == ENC_DIRECT_SSL)
4373+
/* An existing connection cannot be reused for direct SSL */
4374+
if (conn->current_enc_method == ENC_SSL && conn->sslnegotiation[0] == 'd')
43584375
return 2;
43594376
else
43604377
return 1;
@@ -4376,18 +4393,6 @@ connection_failed(PGconn *conn)
43764393
Assert((conn->failed_enc_methods & conn->current_enc_method) == 0);
43774394
conn->failed_enc_methods |= conn->current_enc_method;
43784395

4379-
/*
4380-
* If the server reported an error after the SSL handshake, no point in
4381-
* retrying with negotiated vs direct SSL.
4382-
*/
4383-
if ((conn->current_enc_method & (ENC_DIRECT_SSL | ENC_NEGOTIATED_SSL)) != 0 &&
4384-
conn->ssl_handshake_started)
4385-
{
4386-
conn->failed_enc_methods |= (ENC_DIRECT_SSL | ENC_NEGOTIATED_SSL) & conn->allowed_enc_methods;
4387-
}
4388-
else
4389-
conn->failed_enc_methods |= conn->current_enc_method;
4390-
43914396
return select_next_encryption_method(conn, false);
43924397
}
43934398

@@ -4445,24 +4450,17 @@ select_next_encryption_method(PGconn *conn, bool have_valid_connection)
44454450
SELECT_NEXT_METHOD(ENC_GSSAPI);
44464451
#endif
44474452

4448-
/* With sslmode=allow, try plaintext connection before SSL. */
4449-
if (conn->sslmode[0] == 'a')
4450-
SELECT_NEXT_METHOD(ENC_PLAINTEXT);
4451-
44524453
/*
4453-
* If enabled, try direct SSL. Unless we have a valid TCP connection that
4454-
* failed negotiating GSSAPI encryption; in that case we prefer to reuse
4455-
* the connection with negotiated SSL, instead of reconnecting to do
4456-
* direct SSL. The point of sslnegotiation=direct is to avoid the
4457-
* roundtrip from the negotiation, but reconnecting would also incur a
4458-
* roundtrip. (In sslnegotiation=requiredirect mode, negotiated SSL is not
4459-
* in the list of allowed methods and we will reconnect.)
4454+
* The order between SSL encryption and plaintext depends on sslmode. With
4455+
* sslmode=allow, try plaintext connection before SSL. With
4456+
* sslmode=prefer, it's the other way round. With other modes, we only try
4457+
* plaintext or SSL connections so the order they're listed here doesn't
4458+
* matter.
44604459
*/
4461-
if (have_valid_connection)
4462-
SELECT_NEXT_METHOD(ENC_NEGOTIATED_SSL);
4460+
if (conn->sslmode[0] == 'a')
4461+
SELECT_NEXT_METHOD(ENC_PLAINTEXT);
44634462

4464-
SELECT_NEXT_METHOD(ENC_DIRECT_SSL);
4465-
SELECT_NEXT_METHOD(ENC_NEGOTIATED_SSL);
4463+
SELECT_NEXT_METHOD(ENC_SSL);
44664464

44674465
if (conn->sslmode[0] != 'a')
44684466
SELECT_NEXT_METHOD(ENC_PLAINTEXT);
Collapse file

‎src/interfaces/libpq/fe-secure-openssl.c‎

Copy file name to clipboardExpand all lines: src/interfaces/libpq/fe-secure-openssl.c
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1586,7 +1586,7 @@ open_client_SSL(PGconn *conn)
15861586
}
15871587

15881588
/* ALPN is mandatory with direct SSL connections */
1589-
if (conn->current_enc_method == ENC_DIRECT_SSL)
1589+
if (conn->current_enc_method == ENC_SSL && conn->sslnegotiation[0] == 'd')
15901590
{
15911591
const unsigned char *selected;
15921592
unsigned int len;
Collapse file

‎src/interfaces/libpq/libpq-int.h‎

Copy file name to clipboardExpand all lines: src/interfaces/libpq/libpq-int.h
+2-4Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -235,8 +235,7 @@ typedef enum
235235
#define ENC_ERROR 0
236236
#define ENC_PLAINTEXT 0x01
237237
#define ENC_GSSAPI 0x02
238-
#define ENC_DIRECT_SSL 0x04
239-
#define ENC_NEGOTIATED_SSL 0x08
238+
#define ENC_SSL 0x04
240239

241240
/* Target server type (decoded value of target_session_attrs) */
242241
typedef enum
@@ -395,8 +394,7 @@ struct pg_conn
395394
char *keepalives_count; /* maximum number of TCP keepalive
396395
* retransmits */
397396
char *sslmode; /* SSL mode (require,prefer,allow,disable) */
398-
char *sslnegotiation; /* SSL initiation style
399-
* (postgres,direct,requiredirect) */
397+
char *sslnegotiation; /* SSL initiation style (postgres,direct) */
400398
char *sslcompression; /* SSL compression (0 or 1) */
401399
char *sslkey; /* client key filename */
402400
char *sslcert; /* client certificate filename */

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.