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

[Martin Basti]

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

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 '>='?

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)