[Freeipa-devel] [PATCH] [WIP] DNSSEC support - preview

[Jan Cholasta]

Hi,

Dne 16.10.2014 v 17:59 Martin Basti napsal(a):
> On 10/10/14 09:17, Martin Kosek wrote:
>> On 10/09/2014 03:57 PM, Petr Spacek wrote:
>>> Hello,
>>>
>>> it would be great if people could look at current state of DNSSEC
>>> patches for
>>> FreeIPA.
>>>
>>> It consist of several relatively independent parts:
>>> - python-pkcs#11 interface written by Martin Basti:
>>> https://github.com/spacekpe/freeipa-pkcs11
>>>
>>> - DNSSEC daemons written by me:
>>> https://github.com/spacekpe/ipadnssecd
>>>
>>> - FreeIPA integration written by Martin Basti:
>>> https://github.com/bastiak/freeipa/tree/dnssec
> Here is updated repo with installers, please review:
> https://github.com/bastiak/freeipa/tree/dnssec-4
> branch dnssec-4
>
> TODO: integrate ipadnssecd daemons and pkcs11 helper, when finished

Commit "DNSSEC: schema":

1)

ipaSecretKeyRef is missing X-ORIGIN.


2)

Nitpick: rename 60ipapkcs11.ldif to 60ipapk11.ldif.


Commit "DNSSEC: DNS key synchronization daemon":

1)

Maybe we should rename uuid-ipauniqueid.ldif to uuid.ldif, now that it 
contains more than just ipauniqueid.


2)

ipaConfigString values use camel case, so I would prefer "dnssecVersion" 
instead of "dnssec-version".


3)

Use ipapython.ipautil.ipa_generate_password() to generate the PIN in 
DNSKeySyncInstance.__setup_softhsm()


4)

You probably should put a constant with a path to softhsm2-util to 
ipaplatform.paths and use that in ipautil.run().


5)

There are some PEP8 errors in ipaserver/install/dnskeysyncinstance.py.


Commit "DNSSEC: opendnssec services":

1)

There are some PEP8 errors in ipaserver/install/odsexporterinstance.py 
and ipaserver/install/opendnssecinstance.py.


2)

Nitpick: rename OdsExporterInstance to ODSExporterInstance


3)

Not something you can fix in this commit, but shouldn't ipa-ods-exporter 
be named ipa-odsexportd, so that the naming is consistent with the rest 
of our daemons?


4)

dns_exporter_principal is a bit misleading name for a variable that 
holds a DN in OdsExporterInstance.__setup_principal().


5)

To follow up on the disable/mask issue: IMO you should create a special 
service class for ods-signerd in ipaplatform which calls "systemctl 
mask" when disabled, instead of adding a mask method.


6)

IMO the KEYMASTER constant should be u'dnssecKeyMaster', again to make 
it consistent with other ipaConfigString values.


Commit "DNSSEC: platform and service variables":

1)

IMO some of the path constants should be renamed for consistency:

     NAMED = "/usr/sbin/named"
     NAMED_PKCS11 = "/usr/sbin/named-pkcs11"
     SOFTHSM_CONF = "/etc/softhsm2.conf"
     DNSSEC_TOKENS_DIR = "/var/lib/ipa/dnssec/tokens"
     DNSSEC_SOFTHSM_PIN_SO = "/etc/ipa/softhsm_pin_so"
     DNSSEC_SOFTHSM_PIN = "/var/lib/ipa/dnssec/softhsm_pin"
     VAR_OPENDNSSEC_DIR = "/var/opendnssec"
     ETC_OPENDNSSEC_DIR = "/etc/opendnssec"
     ODS_KSMUTIL = "/usr/bin/ods-ksmutil"


2)

Why do you use the default /etc/softhsm2.conf file, instead of using 
e.g. /etc/ipa/dnssec/softhsm2.conf and passing it to SoftHSM in the 
SOFTHSM2_CONF environment variable?


3)

You should provide /usr/lib/ equivalent for:

     SOFTHSM_LIB = "/usr/lib64/pkcs11/libsofthsm2.so"

and use it when necessary.


4)

I think /etc/ipa/softhsm_pin_so should be moved to 
/etc/ipa/dnssec/softhsm_pin_so.


5)

Nitpick: unnecessary whitespace change in ipaplatform/redhat/services.py


6)

Instead of making changes to the redhat platform module and reverting 
them in the rhel platform module, you should do them in the fedora 
platform module only.


7)

IMO it would be better to move the new RedHatTaskNamespace methods to 
service-specific classes (create them if necessary).


Commit "DNSSEC: validate forwarders":

1)

I'm not sure if failing on DNSSEC-disabled forwarders by default is a 
good idea. Perhaps there could be some auto-detection code? Something 
along the lines of:

    if forwarders_support_dnssec:
        if not options.no_dnssec_validation:
            enable_dnssec_in_ipa()
    else:
        print "WARNING: DNSSEC will not be enabled"


2)

Please don't use ValidationError in check_forwarders(), use ValueError 
instead and re-raise it as ValidationError only where necessary.


Commit "DNSSEC: modify named service to support dnssec":

1)

Rather than checking if paths.DNSSEC_KEYFROMLABEL is None, add a method 
with the check to named-specific service class in ipaplatform.


Commit "DNSSEC: installation":

1)

This should be handled in OpenDNSSECInstance.get_masters() itself:

+        ods.ldap_connect()  # we need connection before getting masters


More tomorrow!

Honza

-- 
Jan Cholasta