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?

*) DNSSEC: Improve error reporting from ipa-ods-exporter
IMO log.exception(ex)  is enough, do we need to add traceback to msg?

*) 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?

*) 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 defaul

B)
for attr in ['ipaPrivateKey', 'ipaPublicKey', 'ipk11publickeyinfo']:
Do we need to sanitize *public*Key and publicKeyinfo?

C)
in odsmgr.py is used ipa_log_manager, can we use the same for consistency?

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?

*) DNSSEC: logging improvements in ldapkeydb.py
IMO commit message should be: ".... in ipa-ods-exporter"

Otherwise LGTM

*) 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))
    )

B) coding style
                assert not matched_already, (
                    "key %s is in more than one keyset" % hexlify(keyid)
                )

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

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.

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.

*) 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?

You may want to use DNSName from ipapython.dnsutil instead of pure dns.name

*) DNSSEC: ipa-ods-exporter: add ldap-cleanup command
LGTM

Martin^2



--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to