[Freeipa-devel] [PATCH 0072-0082] DNSSEC: fixes

Mon Jan 4 13:14:55 UTC 2016

On 22.12.2015 14:32, Petr Spacek wrote:
> On 21.12.2015 18:56, Martin Basti wrote:
>>
>>
>> On 21.12.2015 15:45, Martin Basti wrote:
>>>
>>>
>>> On 21.12.2015 15:33, Petr Spacek wrote:
>>>> Hello,
>>>>
>>>> this patch set fixes key rotation in DNSSEC.
>>>>
>>>> You can use attached template files for OpenDNSSEC config to shorten time
>>>> intervals between key rotations.
>>>>
>>>> Please let me know if you have any questions, I'm all ears!
>>>>
>>> Please fix whitespace error:
>>>
>>> Applying: DNSSEC: logging improvements in ldapkeydb.py
>>> /home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:14: trailing
>>> whitespace.
>>>
>>> warning: 1 line adds whitespace errors.
>>>
>> *) DNSSEC: ipa-dnskeysyncd: call ods-signer ldap-cleanup on zone removal
>>
>> Is is safe to do not use try - except with ipatuil.run()? What if ods-signer
>> command failed?
> 
> That is intentional. The call should never fail, so if it fails there is no
> way how to recover cleanly except restarting the daemon.
> 
> The unhandled exception will kill the daemon and systemd will restart it later on.
> 
> 
>> *) DNSSEC: Improve error reporting from ipa-ods-exporter
>> IMO log.exception(ex)  is enough, do we need to add traceback to msg?
> 
> msg is sent over socket to another process (see send_systemd_reply(conn, msg)
> call in finally: block). Without this the remote party would not receive the
> error information.
> 
> 
>> *) DNSSEC: Make sure that current state in OpenDNSSEC matches key state in LDAP
>> I think this is okay because we want to use KSK instantly, but just to be
>> sure, is Publish->Activate okay?
>> +            bind_times['idnsSecKeyActivate'] = ods_times['idnsSecKeyPublish']
>>
>> Just to be sure how this will be handle during KSK key rotation?
> 
> We have to copy semantics from OpenDNSSEC. Please see design page
> https://fedorahosted.org/bind-dyndb-ldap/wiki/BIND9/Design/DNSSEC/OpenDNSSEC2BINDKeyStates
> , it describes in detail why I done it this way.
> 
> 
>> *) DNSSEC: Make sure that current key state in LDAP matches key state in BIND
>> LGTM
>>
>> *) DNSSEC: remove obsolete TODO note
>> ACK
>>
>> *) DNSSEC: add debug mode to ldapkeydb.py
>> A)
>> You can remove __str__ method, python will use __repr__ as default
> 
> Done.
> 
> 
>> B)
>> for attr in ['ipaPrivateKey', 'ipaPublicKey', 'ipk11publickeyinfo']:
>> Do we need to sanitize *public*Key and publicKeyinfo?
> 
> Yes, we need it. The output with any of ['ipaPrivateKey', 'ipaPublicKey',
> 'ipk11publickeyinfo'] is huge blob and printing it does not help readability.
> Purpose of the patch is to make it easy to read and debug so printing useless
> blobs would go directly against the purpose :-)
> 
> 
>> C)
>> in odsmgr.py is used ipa_log_manager, can we use the same for consistency?
> 
> Fixed, thanks.
> 
> 
>> D)
>> Do we need logging there, everything is printed via print except debug info
>> about connecting, can you just redirect it to stderr, and usable data leave in
>> stdout?
> 
> Yes, we need it because it eases debugging. print() prints useful information
> to stdout. 'Garbage' about connecting to LDAP, IPA framework initialization
> and so on does via logger to stderr, so it can be easily separated from useful
> information using redirection in BASH.
> 
> I've added a comment right below if __name__ == '__main__': to make it clear
> why we do not use logger in there.
> 
> 
>> *) DNSSEC: logging improvements in ldapkeydb.py
>> IMO commit message should be: ".... in ipa-ods-exporter"
>>
>> Otherwise LGTM
> 
> Fixed, thanks.
> 
> 
>> *) DNSSEC: remove keys purged by OpenDNSSEC from master HSM from LDAP
>>
>> A) coding style: please use (), instead of "\"
>>     assert set(pubkeys_local) == set(privkeys_local), (
>>             "IDs of private and public keys for DNS zones in local HSM does "
>>             "not match to key pairs: %s vs. %s" %
>>             (hex_set(pubkeys_local), hex_set(privkeys_local))
>>     )
> 
> Fixed.
> 
> 
>> B) coding style
>>                 assert not matched_already, (
>>                     "key %s is in more than one keyset" % hexlify(keyid)
>>                 )
> 
> Not relevant anymore, see below.
> 
> 
>> C) schedule_key_deletion()
>> how about case when keyid is not in any keyset, then keyid will not be
>> replaced by object and it blow up somewhere else
> 
> Not relevant anymore, see below.
> 
> 
>> D) +class KeyDeleter(object):
>> I would like to have a check there which blows up nicely if _update_key() is
>> called twice on the same object. With current implementation you will get
>> NoneType has no delete_entry method.
> 
> Not relevant anymore, see below.
> 
> 
>> E)
>> I somehow does not like the placeholder object.  Could we just extend Key
>> object with attribute "to_be_deleted" or something similar, and if this
>> attribute is set to True, Key._update_key() can remove, instead of creation a
>> new object.
>> Key.prepare_deletion() can set the value "to_be_deleted" to True.
> 
> Main purpose of the KeyDeleter object was to be incompatible the Key object. I
> want to be 100 % that is not possible to call schedule_delete() and
> subsequenty modify the Key object.
> 
> I've reworked the Key object so it has schedule_deletion() method and that all
> other methods call __assert_not_deleted() to make sure that the object was not
> deleted.
> 
> Is it better?
> 
> 
>> *) DNSSEC: ipa-dnskeysyncd: Skip zones with old DNSSEC metadata in LDAP
>> How often is keysyncer initialized? Might happen the case where one of
>> dnssec_zones has been disabled and keysyncer is not aware of this change?
> 
> KeySyncer is SyncReplConsumer, so it gets constant stream of updates from
> LDAP. IMHO the answer is no, it cannot miss the update.
> 
> 
>> You may want to use DNSName from ipapython.dnsutil instead of pure dns.name
> 
> Other places in daemons are using dns.name too, so I will keep it that way.
> For this particular case there would be no advantage anyway.
> 
> 
>> *) DNSSEC: ipa-ods-exporter: add ldap-cleanup command
>> LGTM
> 
> Old and new version of patches can be found on Github:
> * old: https://github.com/pspacek/freeipa/tree/dnssec_fixes
> * new: https://github.com/pspacek/freeipa/tree/dnssec_fixes2
> 
> Fixed patched (+1 new) are attached.

Rebased patches are attached (and were also force-pushed to
https://github.com/pspacek/freeipa/tree/dnssec_fixes2)

-- 
Petr^2 Spacek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0072-3-DNSSEC-Improve-error-reporting-from-ipa-ods-exporter.patch
Type: text/x-patch
Size: 1059 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160104/d2cdf800/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0073-3-DNSSEC-Make-sure-that-current-state-in-OpenDNSSEC-ma.patch
Type: text/x-patch
Size: 8091 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160104/d2cdf800/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0074-3-DNSSEC-Make-sure-that-current-key-state-in-LDAP-matc.patch
Type: text/x-patch
Size: 1800 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160104/d2cdf800/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0075-3-DNSSEC-remove-obsolete-TODO-note.patch
Type: text/x-patch
Size: 1005 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160104/d2cdf800/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0076-3-DNSSEC-add-debug-mode-to-ldapkeydb.py.patch
Type: text/x-patch
Size: 3382 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160104/d2cdf800/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0077-3-DNSSEC-logging-improvements-in-ipa-ods-exporter.patch
Type: text/x-patch
Size: 2756 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160104/d2cdf800/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0078-3-DNSSEC-remove-keys-purged-by-OpenDNSSEC-from-master-.patch
Type: text/x-patch
Size: 9666 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160104/d2cdf800/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0079-3-DNSSEC-ipa-dnskeysyncd-Skip-zones-with-old-DNSSEC-me.patch
Type: text/x-patch
Size: 4507 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160104/d2cdf800/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0080-3-DNSSEC-ipa-ods-exporter-add-ldap-cleanup-command.patch
Type: text/x-patch
Size: 4806 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160104/d2cdf800/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0081-3-DNSSEC-ipa-dnskeysyncd-call-ods-signer-ldap-cleanup-.patch
Type: text/x-patch
Size: 1444 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160104/d2cdf800/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pspacek-0082-3-DNSSEC-Log-debug-messages-at-log-level-DEBUG.patch
Type: text/x-patch
Size: 1112 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160104/d2cdf800/attachment-0010.bin>