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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
