Review:Patch: SSL: prefer server cipher order

Started by Adrian Klaverabout 12 years ago15 messages
#1Adrian Klaver
adrian.klaver@gmail.com

First review of the above patch as listed in current CommitFest as well
as subsequent ECDH patch in the thread below:

/messages/by-id/1383782378-7342-1-git-send-email-markokr@gmail.com

Platform OpenSuse 12.2

Both patches applied cleanly.

Configured:

./configure --with-python --with-openssl
--prefix=/home/aklaver/pgsqlTest --with-pgport=5462 --enable-cassert

make and make check ran without error.

The description of the GUCs show up in the documentation but I am not
seeing the GUCs themselves in postgresql.conf, so I could test no
further. It is entirely possible I am missing a step and would
appreciate enlightenment.

The general premise seems sound, allowing the DBA control over the type
of cipher of used.

Thanks,
--
Adrian Klaver
adrian.klaver@gmail.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#2Marko Kreen
markokr@gmail.com
In reply to: Adrian Klaver (#1)
2 attachment(s)
Re: Review:Patch: SSL: prefer server cipher order

On Fri, Nov 15, 2013 at 11:16:25AM -0800, Adrian Klaver wrote:

The description of the GUCs show up in the documentation but I am
not seeing the GUCs themselves in postgresql.conf, so I could test
no further. It is entirely possible I am missing a step and would
appreciate enlightenment.

Sorry, I forgot to update sample config.

ssl-prefer-server-cipher-order-v2.patch
- Add GUC to sample config
- Change default value to 'true', per comments from Alvaro and Magnus.

ssl-ecdh-v2.patch
- Add GUC to sample config

--
marko

Attachments:

ssl-prefer-server-cipher-order-v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 77a9303..56bfa01 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -883,6 +883,18 @@ include 'filename'
       </listitem>
      </varlistentry>
 
+     <varlistentry id="guc-ssl-prefer-server-ciphers" xreflabel="ssl_prefer_server_ciphers">
+      <term><varname>ssl_prefer_server_ciphers</varname> (<type>bool</type>)</term>
+      <indexterm>
+       <primary><varname>ssl_prefer_server_ciphers</> configuration parameter</primary>
+      </indexterm>
+      <listitem>
+       <para>
+        Specifies whether to prefer client or server ciphersuite.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="guc-password-encryption" xreflabel="password_encryption">
       <term><varname>password_encryption</varname> (<type>boolean</type>)</term>
       <indexterm>
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index 7f01a78..2094674 100644
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -112,6 +112,9 @@ static bool ssl_loaded_verify_locations = false;
 /* GUC variable controlling SSL cipher list */
 char	   *SSLCipherSuites = NULL;
 
+/* GUC variable: if false, prefer client ciphers */
+bool	   SSLPreferServerCiphers;
+
 /* ------------------------------------------------------------ */
 /*						 Hardcoded values						*/
 /* ------------------------------------------------------------ */
@@ -845,6 +848,10 @@ initialize_SSL(void)
 	if (SSL_CTX_set_cipher_list(SSL_context, SSLCipherSuites) != 1)
 		elog(FATAL, "could not set the cipher list (no valid ciphers available)");
 
+	/* Let server choose order */
+	if (SSLPreferServerCiphers)
+		SSL_CTX_set_options(SSL_context, SSL_OP_CIPHER_SERVER_PREFERENCE);
+
 	/*
 	 * Load CA store, so we can verify client certificates if needed.
 	 */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 54d8078..0ec5ddf 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -127,6 +127,7 @@ extern char *temp_tablespaces;
 extern bool ignore_checksum_failure;
 extern bool synchronize_seqscans;
 extern char *SSLCipherSuites;
+extern bool SSLPreferServerCiphers;
 
 #ifdef TRACE_SORT
 extern bool trace_sort;
@@ -801,6 +802,15 @@ static struct config_bool ConfigureNamesBool[] =
 		check_ssl, NULL, NULL
 	},
 	{
+		{"ssl_prefer_server_ciphers", PGC_POSTMASTER, CONN_AUTH_SECURITY,
+			gettext_noop("Give priority to server ciphersuite order."),
+			NULL
+		},
+		&SSLPreferServerCiphers,
+		true,
+		NULL, NULL, NULL
+	},
+	{
 		{"fsync", PGC_SIGHUP, WAL_SETTINGS,
 			gettext_noop("Forces synchronization of updates to disk."),
 			gettext_noop("The server will use the fsync() system call in several places to make "
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 34a2d05..a190e5f 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -86,6 +86,7 @@
 #ssl_key_file = 'server.key'		# (change requires restart)
 #ssl_ca_file = ''			# (change requires restart)
 #ssl_crl_file = ''			# (change requires restart)
+#ssl_prefer_server_ciphers = on		# (change requires restart)
 #password_encryption = on
 #db_user_namespace = off
 
ssl-ecdh-v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 56bfa01..3785052 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -895,6 +895,19 @@ include 'filename'
       </listitem>
      </varlistentry>
 
+     <varlistentry id="guc-ssl-ecdh-curve" xreflabel="ssl_ecdh_curve">
+      <term><varname>ssl_ecdh_curve</varname> (<type>string</type>)</term>
+      <indexterm>
+       <primary><varname>ssl_ecdh_curve</> configuration parameter</primary>
+      </indexterm>
+      <listitem>
+       <para>
+        Specifies name of EC curve which will be used in ECDH key excanges.
+        Default is <literal>prime256p1</>.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="guc-password-encryption" xreflabel="password_encryption">
       <term><varname>password_encryption</varname> (<type>boolean</type>)</term>
       <indexterm>
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index 2094674..8d688f2 100644
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -69,6 +69,9 @@
 #if SSLEAY_VERSION_NUMBER >= 0x0907000L
 #include <openssl/conf.h>
 #endif
+#if (OPENSSL_VERSION_NUMBER >= 0x0090800fL) && !defined(OPENSSL_NO_ECDH)
+#include <openssl/ec.h>
+#endif
 #endif   /* USE_SSL */
 
 #include "libpq/libpq.h"
@@ -112,6 +115,9 @@ static bool ssl_loaded_verify_locations = false;
 /* GUC variable controlling SSL cipher list */
 char	   *SSLCipherSuites = NULL;
 
+/* GUC variable for default ECHD curve. */
+char	   *SSLECDHCurve;
+
 /* GUC variable: if false, prefer client ciphers */
 bool	   SSLPreferServerCiphers;
 
@@ -765,6 +771,29 @@ info_cb(const SSL *ssl, int type, int args)
 	}
 }
 
+#if (OPENSSL_VERSION_NUMBER >= 0x0090800fL) && !defined(OPENSSL_NO_ECDH)
+static void
+initialize_ecdh(void)
+{
+	EC_KEY *ecdh;
+	int nid;
+
+	nid = OBJ_sn2nid(SSLECDHCurve);
+	if (!nid)
+		elog(FATAL, "ECDH: curve not known: %s", SSLECDHCurve);
+
+	ecdh = EC_KEY_new_by_curve_name(nid);
+	if (!ecdh)
+		elog(FATAL, "ECDH: failed to allocate key");
+
+	SSL_CTX_set_options(SSL_context, SSL_OP_SINGLE_ECDH_USE);
+	SSL_CTX_set_tmp_ecdh(SSL_context, ecdh);
+	EC_KEY_free(ecdh);
+}
+#else
+#define initialize_ecdh()
+#endif
+
 /*
  *	Initialize global SSL context.
  */
@@ -844,6 +873,9 @@ initialize_SSL(void)
 	SSL_CTX_set_tmp_dh_callback(SSL_context, tmp_dh_cb);
 	SSL_CTX_set_options(SSL_context, SSL_OP_SINGLE_DH_USE | SSL_OP_NO_SSLv2);
 
+	/* set up ephemeral ECDH keys */
+	initialize_ecdh();
+
 	/* set up the allowed cipher list */
 	if (SSL_CTX_set_cipher_list(SSL_context, SSLCipherSuites) != 1)
 		elog(FATAL, "could not set the cipher list (no valid ciphers available)");
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 0ec5ddf..7ee46bd 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -127,6 +127,7 @@ extern char *temp_tablespaces;
 extern bool ignore_checksum_failure;
 extern bool synchronize_seqscans;
 extern char *SSLCipherSuites;
+extern char *SSLECDHCurve;
 extern bool SSLPreferServerCiphers;
 
 #ifdef TRACE_SORT
@@ -3151,6 +3152,21 @@ static struct config_string ConfigureNamesString[] =
 	},
 
 	{
+		{"ssl_ecdh_curve", PGC_POSTMASTER, CONN_AUTH_SECURITY,
+			gettext_noop("Sets the list of EC curve used for ECDH."),
+			NULL,
+			GUC_SUPERUSER_ONLY
+		},
+		&SSLECDHCurve,
+#ifdef USE_SSL
+		"prime256v1",
+#else
+		"none",
+#endif
+		NULL, NULL, NULL
+	},
+
+	{
 		{"application_name", PGC_USERSET, LOGGING_WHAT,
 			gettext_noop("Sets the application name to be reported in statistics and logs."),
 			NULL,
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index a190e5f..38e158c 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -87,6 +87,7 @@
 #ssl_ca_file = ''			# (change requires restart)
 #ssl_crl_file = ''			# (change requires restart)
 #ssl_prefer_server_ciphers = on		# (change requires restart)
+#ssl_ecdh_curve = 'prime256v1'		# (change requires restart)
 #password_encryption = on
 #db_user_namespace = off
 
#3Adrian Klaver
adrian.klaver@gmail.com
In reply to: Marko Kreen (#2)
Re: Review:Patch: SSL: prefer server cipher order

On 11/15/2013 11:49 AM, Marko Kreen wrote:

On Fri, Nov 15, 2013 at 11:16:25AM -0800, Adrian Klaver wrote:

The description of the GUCs show up in the documentation but I am
not seeing the GUCs themselves in postgresql.conf, so I could test
no further. It is entirely possible I am missing a step and would
appreciate enlightenment.

Sorry, I forgot to update sample config.

ssl-prefer-server-cipher-order-v2.patch
- Add GUC to sample config
- Change default value to 'true', per comments from Alvaro and Magnus.

ssl-ecdh-v2.patch
- Add GUC to sample config

Well that worked.
I made ssl connections to the server using psql and verified it
respected the order of ssl_ciphers. I do not have a client available
with a different view of cipher order so I cannot test that.

--
Adrian Klaver
adrian.klaver@gmail.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Marko Kreen
markokr@gmail.com
In reply to: Adrian Klaver (#3)
Re: Review:Patch: SSL: prefer server cipher order

On Fri, Nov 15, 2013 at 02:16:52PM -0800, Adrian Klaver wrote:

On 11/15/2013 11:49 AM, Marko Kreen wrote:

On Fri, Nov 15, 2013 at 11:16:25AM -0800, Adrian Klaver wrote:

The description of the GUCs show up in the documentation but I am
not seeing the GUCs themselves in postgresql.conf, so I could test
no further. It is entirely possible I am missing a step and would
appreciate enlightenment.

Sorry, I forgot to update sample config.

ssl-prefer-server-cipher-order-v2.patch
- Add GUC to sample config
- Change default value to 'true', per comments from Alvaro and Magnus.

ssl-ecdh-v2.patch
- Add GUC to sample config

Well that worked.
I made ssl connections to the server using psql and verified it
respected the order of ssl_ciphers. I do not have a client available
with a different view of cipher order so I cannot test that.

Well, these are GUC patches so the thing to test is whether the GUCs work.

ssl-prefer-server-cipher-order:
Use non-standard cipher order in server, eg: RC4-SHA:DHE-RSA-AES128-SHA,
see if on/off works. You can see OpenSSL default order with
"openssl ciphers -v".

ssl-ecdh:
It should start using ECDHE-RSA immediately. Also see if adding
!ECDH to ciphers will fall back to DHE. It's kind of hard to test
the ssl_ecdh_curve as you can't see it anywhere. I tested it by
measuring if bigger curve slowed connecting down...

Bonus - test EC keys:
$ openssl ecparam -name prime256v1 -out ecparam.pem
$ openssl req -x509 -newkey ec:ecparam.pem -days 9000 -nodes \
-subj '/C=US/ST=Somewhere/L=Test/CN=localhost' \
-keyout server.key -out server.crt

ssl-better-default:
SSL should stay working, openssl ciphers -v 'value' should not contain
any weak suites (RC4, SEED, DES-CBC, EXP, NULL) and no non-authenticated
suites (ADH/AECDH).

--
marko

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Adrian Klaver
adrian.klaver@gmail.com
In reply to: Marko Kreen (#4)
Re: Review:Patch: SSL: prefer server cipher order

On 11/16/2013 06:24 AM, Marko Kreen wrote:

On Fri, Nov 15, 2013 at 02:16:52PM -0800, Adrian Klaver wrote:

On 11/15/2013 11:49 AM, Marko Kreen wrote:

On Fri, Nov 15, 2013 at 11:16:25AM -0800, Adrian Klaver wrote:

The description of the GUCs show up in the documentation but I am
not seeing the GUCs themselves in postgresql.conf, so I could test
no further. It is entirely possible I am missing a step and would
appreciate enlightenment.

Sorry, I forgot to update sample config.

ssl-prefer-server-cipher-order-v2.patch
- Add GUC to sample config
- Change default value to 'true', per comments from Alvaro and Magnus.

ssl-ecdh-v2.patch
- Add GUC to sample config

Well that worked.
I made ssl connections to the server using psql and verified it
respected the order of ssl_ciphers. I do not have a client available
with a different view of cipher order so I cannot test that.

Well, these are GUC patches so the thing to test is whether the GUCs work.

ssl-prefer-server-cipher-order:
Use non-standard cipher order in server, eg: RC4-SHA:DHE-RSA-AES128-SHA,
see if on/off works. You can see OpenSSL default order with
"openssl ciphers -v".

ssl_ciphers = 'RC4-SHA:DHE-RSA-AES128-SHA'
ssl_prefer_server_ciphers = on
#ssl_ecdh_curve = 'prime256v1'

aklaver@panda:~/pgsqlTest/data> ../bin/psql -d postgres -U aklaver -h
localhost
psql (9.4devel)
SSL connection (cipher: RC4-SHA, bits: 128)

ssl_ciphers = 'RC4-SHA:DHE-RSA-AES128-SHA'
ssl_prefer_server_ciphers = off
#ssl_ecdh_curve = 'prime256v1'

aklaver@panda:~/pgsqlTest/data> ../bin/psql -d postgres -U aklaver -h
localhost
psql (9.4devel)
SSL connection (cipher: DHE-RSA-AES128-SHA, bits: 128)

ssl-ecdh:
It should start using ECDHE-RSA immediately. Also see if adding
!ECDH to ciphers will fall back to DHE. It's kind of hard to test
the ssl_ecdh_curve as you can't see it anywhere. I tested it by
measuring if bigger curve slowed connecting down...

ssl_ciphers = 'RC4-SHA:DHE-RSA-AES128-SHA'
ssl_prefer_server_ciphers = off
ssl_ecdh_curve = 'prime256v1'

aklaver@panda:~/pgsqlTest/data> ../bin/psql -d postgres -U aklaver -h
localhost
psql (9.4devel)
SSL connection (cipher: DHE-RSA-AES128-SHA, bits: 128)

ssl_ciphers = 'RC4-SHA:DHE-RSA-AES128-SHA'
ssl_prefer_server_ciphers = on
ssl_ecdh_curve = 'prime256v1'

aklaver@panda:~/pgsqlTest/data> ../bin/psql -d postgres -U aklaver -h
localhost
psql (9.4devel)
SSL connection (cipher: RC4-SHA, bits: 128)

ssl_ciphers = 'DEFAULT:!LOW:!EXP:!MD5:@STRENGTH'
ssl_prefer_server_ciphers = on OR off
ssl_ecdh_curve = 'prime256v1'

aklaver@panda:~/pgsqlTest/data> ../bin/psql -d postgres -U aklaver -h
localhost
psql (9.4devel)
SSL connection (cipher: ECDHE-RSA-AES256-SHA, bits: 256)

ssl_ciphers = 'DEFAULT:!ECDH:!LOW:!EXP:!MD5:@STRENGTH'
ssl_prefer_server_ciphers = on
ssl_ecdh_curve = 'prime256v1'

aklaver@panda:~/pgsqlTest/data> ../bin/psql -d postgres -U aklaver -h
localhost
psql (9.4devel)
SSL connection (cipher: DHE-RSA-AES256-SHA, bits: 256)

Bonus - test EC keys:
$ openssl ecparam -name prime256v1 -out ecparam.pem
$ openssl req -x509 -newkey ec:ecparam.pem -days 9000 -nodes \
-subj '/C=US/ST=Somewhere/L=Test/CN=localhost' \
-keyout server.key -out server.crt

EC test:

ssl_ciphers = 'DEFAULT:!LOW:!EXP:!MD5:@STRENGTH'
ssl_prefer_server_ciphers = off OR on
ssl_ecdh_curve = 'prime256v1'

aklaver@panda:~/pgsqlTest/data> ../bin/psql -d postgres -U aklaver -h
localhost
psql (9.4devel)
SSL connection (cipher: ECDHE-ECDSA-AES256-SHA, bits: 256)

ssl_ciphers = 'RC4-SHA:DHE-RSA-AES128-SHA'
ssl_prefer_server_ciphers = on
#ssl_ecdh_curve = 'prime256v1'
Or
ssl_ecdh_curve = 'prime256v1'

aklaver@panda:~/pgsqlTest/data> ../bin/psql -d postgres -U aklaver -h
localhost
psql: SSL error: sslv3 alert handshake failure
FATAL: no pg_hba.conf entry for host "::1", user "aklaver", database
"postgres", SSL off

ssl-better-default:
SSL should stay working, openssl ciphers -v 'value' should not contain
any weak suites (RC4, SEED, DES-CBC, EXP, NULL) and no non-authenticated
suites (ADH/AECDH).

Not sure about the above, if it is a GUC I can't find it. If it is
something else than I will have to plead ignorance.

--
Adrian Klaver
adrian.klaver@gmail.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Marko Kreen
markokr@gmail.com
In reply to: Adrian Klaver (#5)
Re: Review:Patch: SSL: prefer server cipher order

Thanks for testing!

On Sat, Nov 16, 2013 at 12:17:40PM -0800, Adrian Klaver wrote:

On 11/16/2013 06:24 AM, Marko Kreen wrote:

ssl-better-default:
SSL should stay working, openssl ciphers -v 'value' should not contain
any weak suites (RC4, SEED, DES-CBC, EXP, NULL) and no non-authenticated
suites (ADH/AECDH).

Not sure about the above, if it is a GUC I can't find it. If it is
something else than I will have to plead ignorance.

The patch just changes the default value for 'ssl_ciphers' GUC.

The question is if the value works at all, and is good.

--
marko

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#7Adrian Klaver
adrian.klaver@gmail.com
In reply to: Marko Kreen (#6)
Re: Review:Patch: SSL: prefer server cipher order

On 11/16/2013 12:37 PM, Marko Kreen wrote:

Thanks for testing!

On Sat, Nov 16, 2013 at 12:17:40PM -0800, Adrian Klaver wrote:

On 11/16/2013 06:24 AM, Marko Kreen wrote:

ssl-better-default:
SSL should stay working, openssl ciphers -v 'value' should not contain
any weak suites (RC4, SEED, DES-CBC, EXP, NULL) and no non-authenticated
suites (ADH/AECDH).

Not sure about the above, if it is a GUC I can't find it. If it is
something else than I will have to plead ignorance.

The patch just changes the default value for 'ssl_ciphers' GUC.

I am still not sure what patch you are talking about. The two patches I
saw where for server_prefer and ECDH key exchange.

The question is if the value works at all, and is good.

What value would we be talking about?

Note: I have been working through a head cold and thought processes are
sluggish, handle accordingly:)

--
Adrian Klaver
adrian.klaver@gmail.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#8Marko Kreen
markokr@gmail.com
In reply to: Adrian Klaver (#7)
Re: Review:Patch: SSL: prefer server cipher order

On Sat, Nov 16, 2013 at 01:03:05PM -0800, Adrian Klaver wrote:

On 11/16/2013 12:37 PM, Marko Kreen wrote:

Thanks for testing!

On Sat, Nov 16, 2013 at 12:17:40PM -0800, Adrian Klaver wrote:

On 11/16/2013 06:24 AM, Marko Kreen wrote:

ssl-better-default:
SSL should stay working, openssl ciphers -v 'value' should not contain
any weak suites (RC4, SEED, DES-CBC, EXP, NULL) and no non-authenticated
suites (ADH/AECDH).

Not sure about the above, if it is a GUC I can't find it. If it is
something else than I will have to plead ignorance.

The patch just changes the default value for 'ssl_ciphers' GUC.

I am still not sure what patch you are talking about. The two
patches I saw where for server_prefer and ECDH key exchange.

The question is if the value works at all, and is good.

What value would we be talking about?

Ah, sorry. It's this one:

https://commitfest.postgresql.org/action/patch_view?id=1310

Note: I have been working through a head cold and thought processes
are sluggish, handle accordingly:)

Get better soon! :)

--
marko

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#9Adrian Klaver
adrian.klaver@gmail.com
In reply to: Marko Kreen (#8)
Re: Review:Patch: SSL: prefer server cipher order

On 11/16/2013 01:13 PM, Marko Kreen wrote:

On Sat, Nov 16, 2013 at 01:03:05PM -0800, Adrian Klaver wrote:

On 11/16/2013 12:37 PM, Marko Kreen wrote:

Thanks for testing!

On Sat, Nov 16, 2013 at 12:17:40PM -0800, Adrian Klaver wrote:

On 11/16/2013 06:24 AM, Marko Kreen wrote:

ssl-better-default:
SSL should stay working, openssl ciphers -v 'value' should not contain
any weak suites (RC4, SEED, DES-CBC, EXP, NULL) and no non-authenticated
suites (ADH/AECDH).

Not sure about the above, if it is a GUC I can't find it. If it is
something else than I will have to plead ignorance.

The patch just changes the default value for 'ssl_ciphers' GUC.

I am still not sure what patch you are talking about. The two
patches I saw where for server_prefer and ECDH key exchange.

The question is if the value works at all, and is good.

What value would we be talking about?

Ah, sorry. It's this one:

https://commitfest.postgresql.org/action/patch_view?id=1310

Got it, applied it.

Results:

openssl ciphers -v 'HIGH:!aNULL'|egrep
'(RC4|SEED|DES-CBC|EXP|NULL|ADH|AECDH)'

ECDHE-RSA-DES-CBC3-SHA SSLv3 Kx=ECDH Au=RSA Enc=3DES(168) Mac=SHA1
ECDHE-ECDSA-DES-CBC3-SHA SSLv3 Kx=ECDH Au=ECDSA Enc=3DES(168) Mac=SHA1
EDH-RSA-DES-CBC3-SHA SSLv3 Kx=DH Au=RSA Enc=3DES(168) Mac=SHA1
EDH-DSS-DES-CBC3-SHA SSLv3 Kx=DH Au=DSS Enc=3DES(168) Mac=SHA1
ECDH-RSA-DES-CBC3-SHA SSLv3 Kx=ECDH/RSA Au=ECDH Enc=3DES(168) Mac=SHA1
ECDH-ECDSA-DES-CBC3-SHA SSLv3 Kx=ECDH/ECDSA Au=ECDH Enc=3DES(168) Mac=SHA1
DES-CBC3-SHA SSLv3 Kx=RSA Au=RSA Enc=3DES(168) Mac=SHA1
DES-CBC3-MD5 SSLv2 Kx=RSA Au=RSA Enc=3DES(168) Mac=MD5

Note: I have been working through a head cold and thought processes
are sluggish, handle accordingly:)

Get better soon! :)

Thanks, the worst is over.

--
Adrian Klaver
adrian.klaver@gmail.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10Marko Kreen
markokr@gmail.com
In reply to: Adrian Klaver (#9)
Re: Review:Patch: SSL: prefer server cipher order

On Sat, Nov 16, 2013 at 02:07:57PM -0800, Adrian Klaver wrote:

On 11/16/2013 01:13 PM, Marko Kreen wrote:

https://commitfest.postgresql.org/action/patch_view?id=1310

Got it, applied it.

Results:

openssl ciphers -v 'HIGH:!aNULL'|egrep
'(RC4|SEED|DES-CBC|EXP|NULL|ADH|AECDH)'

ECDHE-RSA-DES-CBC3-SHA SSLv3 Kx=ECDH Au=RSA Enc=3DES(168) Mac=SHA1
ECDHE-ECDSA-DES-CBC3-SHA SSLv3 Kx=ECDH Au=ECDSA Enc=3DES(168) Mac=SHA1
EDH-RSA-DES-CBC3-SHA SSLv3 Kx=DH Au=RSA Enc=3DES(168) Mac=SHA1
EDH-DSS-DES-CBC3-SHA SSLv3 Kx=DH Au=DSS Enc=3DES(168) Mac=SHA1
ECDH-RSA-DES-CBC3-SHA SSLv3 Kx=ECDH/RSA Au=ECDH Enc=3DES(168) Mac=SHA1
ECDH-ECDSA-DES-CBC3-SHA SSLv3 Kx=ECDH/ECDSA Au=ECDH Enc=3DES(168) Mac=SHA1
DES-CBC3-SHA SSLv3 Kx=RSA Au=RSA Enc=3DES(168) Mac=SHA1
DES-CBC3-MD5 SSLv2 Kx=RSA Au=RSA Enc=3DES(168) Mac=MD5

DES-CBC3 is 3DES, which is fine. Plain DES-CBC would be bad.

If you don't see any other issues perhaps they are ready for committer?

--
marko

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11Adrian Klaver
adrian.klaver@gmail.com
In reply to: Marko Kreen (#10)
Re: Review:Patch: SSL: prefer server cipher order

On 11/16/2013 02:41 PM, Marko Kreen wrote:

On Sat, Nov 16, 2013 at 02:07:57PM -0800, Adrian Klaver wrote:

On 11/16/2013 01:13 PM, Marko Kreen wrote:

https://commitfest.postgresql.org/action/patch_view?id=1310

Got it, applied it.

Results:

openssl ciphers -v 'HIGH:!aNULL'|egrep
'(RC4|SEED|DES-CBC|EXP|NULL|ADH|AECDH)'

ECDHE-RSA-DES-CBC3-SHA SSLv3 Kx=ECDH Au=RSA Enc=3DES(168) Mac=SHA1
ECDHE-ECDSA-DES-CBC3-SHA SSLv3 Kx=ECDH Au=ECDSA Enc=3DES(168) Mac=SHA1
EDH-RSA-DES-CBC3-SHA SSLv3 Kx=DH Au=RSA Enc=3DES(168) Mac=SHA1
EDH-DSS-DES-CBC3-SHA SSLv3 Kx=DH Au=DSS Enc=3DES(168) Mac=SHA1
ECDH-RSA-DES-CBC3-SHA SSLv3 Kx=ECDH/RSA Au=ECDH Enc=3DES(168) Mac=SHA1
ECDH-ECDSA-DES-CBC3-SHA SSLv3 Kx=ECDH/ECDSA Au=ECDH Enc=3DES(168) Mac=SHA1
DES-CBC3-SHA SSLv3 Kx=RSA Au=RSA Enc=3DES(168) Mac=SHA1
DES-CBC3-MD5 SSLv2 Kx=RSA Au=RSA Enc=3DES(168) Mac=MD5

DES-CBC3 is 3DES, which is fine. Plain DES-CBC would be bad.

If you don't see any other issues perhaps they are ready for committer?

I do not have any other questions/issues at this point. I am new to the
review process, so I am not quite sure how it proceeds from here.

--
Adrian Klaver
adrian.klaver@gmail.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12Marko Kreen
markokr@gmail.com
In reply to: Adrian Klaver (#11)
Re: Review:Patch: SSL: prefer server cipher order

On Sat, Nov 16, 2013 at 02:54:22PM -0800, Adrian Klaver wrote:

On 11/16/2013 02:41 PM, Marko Kreen wrote:

If you don't see any other issues perhaps they are ready for committer?

I do not have any other questions/issues at this point. I am new to
the review process, so I am not quite sure how it proceeds from
here.

Simple - just click on "edit patch" on commitfest page and change
patch status to "ready for committer". Then committers will know
that they should look at the patch.

--
marko

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#13Adrian Klaver
adrian.klaver@gmail.com
In reply to: Marko Kreen (#12)
Re: Review:Patch: SSL: prefer server cipher order

On 11/16/2013 03:09 PM, Marko Kreen wrote:

On Sat, Nov 16, 2013 at 02:54:22PM -0800, Adrian Klaver wrote:

On 11/16/2013 02:41 PM, Marko Kreen wrote:

If you don't see any other issues perhaps they are ready for committer?

I do not have any other questions/issues at this point. I am new to
the review process, so I am not quite sure how it proceeds from
here.

Simple - just click on "edit patch" on commitfest page and change
patch status to "ready for committer". Then committers will know
that they should look at the patch.

Done for both:

SSL: better default ciphersuite
SSL: prefer server cipher order

Thanks for helping me through the process.

--
Adrian Klaver
adrian.klaver@gmail.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#14Marko Kreen
markokr@gmail.com
In reply to: Adrian Klaver (#13)
Re: Review:Patch: SSL: prefer server cipher order

On Sat, Nov 16, 2013 at 03:21:19PM -0800, Adrian Klaver wrote:

On 11/16/2013 03:09 PM, Marko Kreen wrote:

On Sat, Nov 16, 2013 at 02:54:22PM -0800, Adrian Klaver wrote:

On 11/16/2013 02:41 PM, Marko Kreen wrote:

If you don't see any other issues perhaps they are ready for committer?

I do not have any other questions/issues at this point. I am new to
the review process, so I am not quite sure how it proceeds from
here.

Simple - just click on "edit patch" on commitfest page and change
patch status to "ready for committer". Then committers will know
that they should look at the patch.

Done for both:

SSL: better default ciphersuite
SSL: prefer server cipher order

I think you already handled the ECDH one too:

https://commitfest.postgresql.org/action/patch_view?id=1286

Thanks for helping me through the process.

Thanks for reviewing.

--
marko

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#15Adrian Klaver
adrian.klaver@gmail.com
In reply to: Marko Kreen (#14)
Re: Review:Patch: SSL: prefer server cipher order

On 11/16/2013 03:46 PM, Marko Kreen wrote:

On Sat, Nov 16, 2013 at 03:21:19PM -0800, Adrian Klaver wrote:

On 11/16/2013 03:09 PM, Marko Kreen wrote:

On Sat, Nov 16, 2013 at 02:54:22PM -0800, Adrian Klaver wrote:

On 11/16/2013 02:41 PM, Marko Kreen wrote:

If you don't see any other issues perhaps they are ready for committer?

I do not have any other questions/issues at this point. I am new to
the review process, so I am not quite sure how it proceeds from
here.

Simple - just click on "edit patch" on commitfest page and change
patch status to "ready for committer". Then committers will know
that they should look at the patch.

Done for both:

SSL: better default ciphersuite
SSL: prefer server cipher order

I think you already handled the ECDH one too:

https://commitfest.postgresql.org/action/patch_view?id=1286

Aah, missed that one. I updated to show my review and mark as Ready for
Committer.

Thanks for helping me through the process.

Thanks for reviewing.

--
Adrian Klaver
adrian.klaver@gmail.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers