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.

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