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

[Christian Heimes]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-cheimes-0030-3-Modernize-mod_nss-s-cipher-suites.patch
Type: text/x-patch
Size: 12022 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160203/0ae80a40/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160203/0ae80a40/attachment.sig>