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

[Martin Babinsky]

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