[Freeipa-devel] [PATCH 0030] Modernize mod_nss's cipher suites

[Martin Kosek]

On 02/11/2016 10:45 AM, Martin Basti wrote:
> 
> 
> On 03.02.2016 15:35, Christian Heimes wrote:
>> On 2016-01-29 15:05, Martin Basti wrote:
>>>
>>> On 29.01.2016 14:42, Christian Heimes wrote:
>>>> On 2016-01-28 09:47, Martin Basti wrote:
>>>>> On 22.01.2016 12:32, Martin Kosek wrote:
>>>>>> On 01/21/2016 04:21 PM, Christian Heimes wrote:
>>>>>>> The list of supported TLS cipher suites in /etc/httpd/conf.d/nss.conf
>>>>>>> has been modernized. Insecure or less secure algorithms such as RC4,
>>>>>>> DES and 3DES are removed. Perfect forward secrecy suites with
>>>>>>> ephemeral
>>>>>>> ECDH key exchange have been added. IE 8 on Windows XP is no longer
>>>>>>> supported.
>>>>>>>
>>>>>>> The list of enabled cipher suites has been generated with the script
>>>>>>> contrib/nssciphersuite/nssciphersuite.py.
>>>>>>>
>>>>>>> The supported suites are currently:
>>>>>>>
>>>>>>> TLS_RSA_WITH_AES_128_CBC_SHA256
>>>>>>> TLS_RSA_WITH_AES_256_CBC_SHA256
>>>>>>> TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
>>>>>>> TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA
>>>>>>> TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
>>>>>>> TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
>>>>>>> TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
>>>>>>> TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA
>>>>>>> TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
>>>>>>> TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
>>>>>>> TLS_RSA_WITH_AES_128_GCM_SHA256
>>>>>>> TLS_RSA_WITH_AES_128_CBC_SHA
>>>>>>> TLS_RSA_WITH_AES_256_GCM_SHA384
>>>>>>> TLS_RSA_WITH_AES_256_CBC_SHA
>>>>>>>
>>>>>>> https://fedorahosted.org/freeipa/ticket/5589
>>>>>> Thanks for the patch! I updated the ticket to make sure this change is
>>>>>> release notes.
>>>>>>
>>>>> Hello,
>>>>>
>>>>> I'm not sure if I'm the right person to do review on this, but I will
>>>>> try :-)
>>>>>
>>>>> 1)
>>>>> Your patch adds whitespace error
>>>>>
>>>>> Applying: Modernize mod_nss's cipher suites
>>>>> /home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:52: new blank
>>>>> line at EOF.
>>>>> +
>>>>> warning: 1 line adds whitespace errors.
>>>>>
>>>>>
>>>>> 2)
>>>>> +import urllib.request  # pylint: disable=E0611
>>>>>
>>>>> Please specify pylint disabled check by name
>>>>>
>>>>> 3)
>>>>> +def update_mod_nss_cipher_suite(http):
>>>>>
>>>>> in this upgrade, is there any possibility that ciphers might be upgraded
>>>>> again in future? (IMO yes).
>>>>>
>>>>> I think, it can be better to store revision of change instead of boolean
>>>>>
>>>>> LAST_REVISION =  1
>>>>>
>>>>> if revision >= LAST_REVISION:
>>>>>       return
>>>>>
>>>>> sysupgrade.set_upgrade_state('nss.conf', 'cipher_suite_revision',
>>>>> LAST_REVISION)
>>>> Thanks for the review. I have addressed the problems. Instead of a
>>>> revision number I'm using a date string. The sysupgrade module only
>>>> stores str and bool. With a date-based revision it's easy to see when
>>>> the cipher suite was checked last time.
>>>>
>>>> Christian
>>>>
>>> Thanks
>>>
>>> 1) Pylint :-)
>>> +    with urllib.request.urlopen(SOURCE) as r:  # pylint: disable=E1101
>> Thanks! It was easier to change the import to get rid of the second
>> pylint stanza.
>>
>>> 2)
>>> +    if revision == httpinstance.NSS_CIPHER_REVISION:
>>>
>>> may happen a case where just comparation with '==' can cause a issues
>>> (docker world)? Should not be there rather '>='?
>> Makes sense, I've changed the comparison operator to >=. This may still
>> override user settings, though.
>>
>>> 3)
>>> +        root_logger.info("Cipher suite already updated")
>>>
>>> Sorry that I did not noticed earlier, this should be just debug level,
>>> IMO this message is not so important, it will cause only mess on output
>>> (we already have plenty of unneeded info messages in upgrade, they will
>>> be fixed once)
>> Fine with me :)
>>
>> Christian
> ACK
> 
> Pushed to:
> master: 5ac3a3cee534a16db86c541b9beff4939f03410e
> ipa-4-3: c3496a4a4893c75789bdf0c617e46923361fb43b
> 

Very cool! Thanks guys! Looking forward to deploying FreeIPA 4.3.1 on the
FreeIPA public demo :-)