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

[Martin Basti]

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