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

Martin Basti mbasti at redhat.com
Wed Jan 6 17:49:14 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.
>
It works for me, except one case (I will try to reproduce it)

Keys were rotated on master and new KSK had not been marked as ds-seen.
When I installed replica, dig +dnssec did not show any RRSIG records 
(ipa-dnskeysyncd showed that keys were there).
After I marked KSK key as ds-seen on master, replica start to sing DNS 
records.
ZSK key rotation works on master and replica just fine.

Just note, the previous KSK has been still in active state when replica 
did not sign records.




More information about the Freeipa-devel mailing list