[Freeipa-devel] [PATCH] 0059..0064 Lightweight sub-CAs

[Jan Cholasta]

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.

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.


Patch 0060-0062: LGTM


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.


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.

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.


-- 
Jan Cholasta