Re: Review:Patch: SSL: prefer server cipher order

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

http://www.postgresql.org/message-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(dot)klaver(at)gmail(dot)com

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

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(dot)klaver(at)gmail(dot)com

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

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(at)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(at)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(at)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(at)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(at)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(at)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(at)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(at)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(dot)klaver(at)gmail(dot)com

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

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(dot)klaver(at)gmail(dot)com

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

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(dot)klaver(at)gmail(dot)com

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

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(dot)klaver(at)gmail(dot)com

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

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(dot)klaver(at)gmail(dot)com

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

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(dot)klaver(at)gmail(dot)com