[Freeipa-devel] [PATCH] - Add DRM to IPA

Wed Aug 6 20:32:05 UTC 2014

Attached is a new patch.  I believe I have addressed all the issues
raided by pviktori, edewata and rcrit.

Please let me know if I missed something!

Incidentally, to get all this to work, you should use the latest Dogtag
10.2 build, which also contains a fix for pkidestroy that is not yet
merged in.  A COPR build is currently underway at: 

http://copr.fedoraproject.org/coprs/vakwetu/dogtag/build/24804/

Thanks, 
Ade

On Thu, 2014-07-17 at 21:08 +0200, Petr Viktorin wrote:
> On 07/16/2014 02:55 PM, Petr Viktorin wrote:
> > On 07/14/2014 11:45 AM, Ade Lee wrote:
> >> Hi all,
> >>
> >> I have rebased all the previous patches against master, and have
> >> squashed them all into a single patch.
> >> Its a large patch, but as many folks have already reviewed the
> >> constituent precursor patches, most if it
> >> should be familiar and easier to review.
> >
> > I think it would be nice to have the fixes that aren't related to DRM
> > (e.g. style issues) separate, so the patch can be reverted easily. But,
> > what's done is done. Thanks for the cleanup!
> >
> >> The main difference with what was specified before is that the DRM
> >> database is installed as a subtree
> >> to o=ipaca.  This means that no new replication agreements will be
> >> needed to replicate DRM data.
> >> Replication agreements set up for the Dogtag CA will automatically
> >> replicate DRM data.
> >>
> >> In order for this patch to work, a new 10.2 build of Dogtag 10.2 is
> >> needed - with specific changes to
> >> allow the ability to install a database as a subtree of an existing
> >> tree.  At this time, these
> >> changes have not yet been checked into the dogtag source.   You can
> >> obtain such a build from:
> >>
> >> http://copr.fedoraproject.org/coprs/vakwetu/dogtag/build/21936/
> >>
> >> Please review,
> >
> >
> > I've started reading the code; here are my thoughts on the first part:
> >
> > In ipa-ca-install and ipa-replica-install, you only ever set
> > "REPLICA_INFO_TOP_DIR" to None. In Python, "global" has module scope,
> > it's not for the entire process. (And it's bad practice anyway.)
> > You'll need to pass it around explicitly, perhaps store it in
> > ReplicaConfig.
> >
> > It would be nice to use the admintool framework for the new script,
> > instead of copying the old boilerplate code again. See e.g.
> > ipa-server-certinstall for an example.
> >
> > Logging functions interpolate their arguments, so you should use e.g.
> >      root_logger.critical("failed to configure %s instance %s",
> > subsystem, e)
> > instead of using the % operator.
> 
> Also, in e.g.
>      root_logger.debug(
>          'Unable to determine PIN for the Dogtag instance: %s' % str(e))
> the "s" in "%s" means "convert to string", so explicit str() isn't 
> necessary.
> 
> > installutils.create_replica_config: Any time you call sys.exit(), please
> > log a message so we know why the log file ends.
> >
> > In ipa-drm-install, why do you read /etc/ipa/default.conf manually? The
> > information is available in api.env once api is finalized.
> >
> > In ipa-server-install, do we want to display the message about backing
> > up /root/drmcert.p12 even if DRM is not installed?
> >
> > Please use path constants from ipaplatform.base.paths, or add new ones
> > if necessary, instead of DogtagInstance.{AGENT_P12_PATH,AGENT_P12_PATH}.
> > In ipa-upgradeconfig, you hardcoded '/etc/named.keytab'. Please change
> > that back to paths.NAMED_KEYTAB.
> > Same in CAInstance.uninstall with "/usr/bin/pkiremove".
> >
> > In ipa-upgradeconfig.find_subject_base, the docstring should mention to
> > DsInstance.find_subject_base's docstring rather than being a copy; we
> > dont' want them getting out of sync.
> >
> > In DsInstance.find_subject_base, don't use a bare `except:`, at least do
> > `except Exception:`, so you don't e.g. disable Ctrl+C.
> >
> > In CADSInstance.uninstall, you don't need the _running and _user_exists
> > variables at all, just call the functions.
> > Same in CAInstance.__create_ra_agent_db for _stdout, _stderr, _returncode.
> > Same in CAInstance.uninstall for _user_exists.
> >
> > In DogtagInstance, stop_tracking_certificates was moved here from
> > cainstance, but it no longer stops tracking ipaCert in HTTPD_ALIAS_DIR.
> > Is that intended?
> >
> > In CAInstance.__init__, when calling DogtagInstance.__init__, consider
> > using super, and naming the arguments for clarity:
> >      super(CAInstance, self).__init__(
> >          realm=realm,
> >          subsystem="CA",
> >          service_desc="certificate server",
> >          dogtag_constants=dogtag_constants,
> >      )
> Same in DogtagInstance, DRMInstance.
> 
> > Since CAInstance inherits from DogtagInstance, the `enable`,
> > `start_instance`, `stop_instance`, `http_proxy` methods that just call
> > the superclass are unnecessary.
> > IMO the comment from CAInstance.__enable was important, it should be in
> > DogtagInstance.enable.
> >
> > In DogtagInstance.spawn_instance, it's not nice to modify lists the
> > caller pased in. Take a copy of the nolog first. Ideally combine that
> > with the conversion to tuple, and allow any sequence to be passed in:
> >      nolog_list = tuple(nolog_list) + (self.admin_password,
> > self.dm_password)
> >
> > In CAInstance.__create_ca_user, it's better to use functions instead of
> > staticmethods, unless you'll want to override them in subclasses (which
> > you're explicitly discouraging here, by using the double underscore).
> > Anyway I don't get why you want these to be staticmethods.
> >
> 
> dogtaginstance.py, drminstance.py: Avoid star imports, especially in new 
> code. AFAICS you're only using root_logger from ipapython.ipa_log_manager.
> Also consider using an class-specific logger:
>      class DogtagInstance(...):
>          def __init__(...):
>              ...
>              self.log = ipa_log_manager.log_mgr.get_logger(self)
> 
>          later:
>              self.log.error("Unable to determine PIN for the Dogtag 
> instance:", e)
> 
> About the "# noinspection PyBroadException" comments -- we already use 
> pylint, I don't think we want instructions for other linters in the code.
> 
> In DogtagInstance.get_admin_cert:
>      admin_cert = entry_attrs.get('usercertificate')[0]
> This relies on the order of values returned by LDAP. If the choice of 
> certificate doesn't matter here, leave a comment.
> 
> drminstance.py: Please remove the `if __name__ == '__main__':` section 
> at the end. If this needs to be tested, provide a real test; if it's 
> example usage put it in the docs/docstring; if it's a personal tool just 
> keep it to yourself.
> 
> In DRMInstance.__init__, the docstring is wrong; __init__ doesn't return 
> anything.
> 
> In dogtag.py, this change breaks the formatting:
> > -    |staus                    |string         |cert_request_status|unicode [1]_     |
> > +    |status                    |string         |cert_request_status|unicode [1]_     |
> 
> In drm.__init__, use `os.path.join(api.env.dot_ipa, 'alias')` instead of 
> joining manually. There are also hardcoded paths, put them in the 
> platform paths module.
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-1-Add-a-DRM-to-IPA.patch
Type: text/x-patch
Size: 155623 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140806/a992f338/attachment.bin>