On 06/09/2016 08:44 AM, Fraser Tweedale wrote:
On Thu, Jun 09, 2016 at 01:21:29AM +1000, Fraser Tweedale wrote:
On Wed, Jun 08, 2016 at 01:00:36PM +0200, Jan Cholasta wrote:
On 8.6.2016 05:15, Fraser Tweedale wrote:
On Tue, Jun 07, 2016 at 03:42:22PM +1000, Fraser Tweedale wrote:
On Wed, Jun 01, 2016 at 02:51:04PM +1000, Fraser Tweedale wrote:
Hi team,

This patchset implements the 'ca' plugin for creating and managing
lightweight sub-CAs, and updates the 'caacl' plugin and
'cert-request' command to support multiple CAs.

A brief overview of the patches:

0059
  'ca' plugin, associated schema changes and container objects,
  Dogtag REST API wrapper
0060
  Add CA entry for the IPA CA on install/upgrade
0061
  Update 'caacl' plugin with CA support (including enforcement)
0062
  Update ra.request_certificate() to support specifying target CA
0063
  Add '--ca' option to 'cert-request' command
0064
  Add '--issuer' option to 'cert-find' command

These patches depend on other pending patches:

    0051, 0052, 0053, 0054, 0055, 0056

Signing key replication depends on unmerged Dogtag patches.  Builds
of Dogtag with the required patches, and of FreeIPA with all
completed sub-CAs work, should be available from my COPR soon:
https://copr.fedorainfracloud.org/coprs/ftweedal/freeipa/

Some parts of the design are not implemented in the current
patchset, including:

- local parent CA (ipaca object) references
- sub-CA certificate renewal
- 'cert-show' command '--ca=NAME' option
- certmonger support for specifying CA
- revocation of deleted CAs

I look forward to your reviews!

Thanks,
Fraser

Rebased and updated patches attached.

Substantive changes:

- add required attributes for issuer DN and subject DN
- prevent rename of IPA CA
- when adding IPA CA entry, contact Dogtag to learn authority id,
  issuer DN and subject DN
- add 'read_ca' method to Dogtag interface
- tighten ACIs to prevent modification of ipacaid attribute

Updated patch 0064-3; adds --issuer option to cert-show and --ca
option to cert-show and cert-find.

Patch 0059:

1) On upgrade, why is the lightweight CA container created twice - once in
41-subca.update, once using ensure_entry() call? It should be done only
once.

I'll remove 41-subca.update; the routine in cainstance is the one
that's needed.

2) In ca_del, every CA specified in args[0] should be deleted, not just the
first one.

3) Do not use NonFatalError, issue a warning instead:

    self.add_message(MyNewWarningClass(name=...))

4) Can it actually happen that ca_show does not return ipacaid? I guess not,
so you should be able to remove the check altogether and don't bother with
the warning.

ipacaid is mandatory now, so I'll remove the check.


Patch 0060-0062: LGTM

Yippee \o/


Patch 0063:

Could you please define the CA param as follows:

    Str('cacn?',
        cli_name='ca',
        query=True,
        label=_("CA"),
        doc=_("CA to use"),
    ),

?

This is for consitency with framework-generated parent key params, which
unfortunately we cannot leverage in cert_request currently.

No problemo.


Patch 0064:

1) See my comment for patch 0063, it applies here as well.

2) The --issuer option should not be included in cert_show - show commands
are supposed to retrieve an object given primary key(s), and the primary key
of CA objects is just their cn.

The --issuer argument is because primary key for a cert is really
(issuer, serial).  So it show the cert _with_ that issuer (and
serial), not the cert _for_ that issuer.

3) In find commands, the options form a filter, so instead of raising
MutuallyExclusiveError in cert-find, return an empty result, as with any
other unmatched filter.

Here, --issuer and --ca are two different ways to specify the
issuer.  --issuer lets you give the issuer DN straight up; --ca
takes the name of an IPA CA object and looks up its issuer DN.
(Thus it makes no sense to give both options at once).

Do you think it would be clearer to provide a single option that
takes issuer DN or the name of CA?  I considered this but decided
against it because collisions are possible (one could name an IPA CA
something that passes for a stingified DN).

Thanks for reviewing!
Fraser

Updated patches attached (0059-3..0063-3, 0064-4).  No substantive
changes to 0060..0062.



Hi Fraser,

I'm doing functional review of your sub-CA patches and your first patch breaks upgrade [1].

Bear in mind that all the system configuration is upgraded after LDAP upgrade, so you actually have to use update file to add "cn=cas,cn=ca,$SUFFIX$" subtree. Otherwise the code tries to add ACIs to entries that are not yet created resulting in the pasted error.

[1] https://paste.fedoraproject.org/376600/74804146/

--
Martin^3 Babinsky

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