Обсуждение: 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.klaver@gmail.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.klaver@gmail.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'skind of hard to test the ssl_ecdh_curve as you can't see it anywhere. I tested it by measuring if bigger curve slowedconnecting 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@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
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.klaver@gmail.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.klaver@gmail.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.klaver@gmail.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.klaver@gmail.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.klaver@gmail.com