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

Jan Cholasta jcholast at redhat.com
Tue Jun 28 06:04:02 UTC 2016 

On 13.6.2016 08:59, Jan Cholasta wrote:
> On 13.6.2016 08:38, Fraser Tweedale wrote:
>> On Fri, Jun 10, 2016 at 12:48:00AM +1000, Fraser Tweedale wrote:
>>> On Thu, Jun 09, 2016 at 12:36:35PM +0200, Jan Cholasta wrote:
>>>> On 9.6.2016 11:10, Fraser Tweedale wrote:
>>>>> On Thu, Jun 09, 2016 at 10:12:40AM +0200, Jan Cholasta wrote:
>>>>>> On 9.6.2016 08:44, 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.
>>>>>>
>>>>>> Correct, but in IPA the issuer is represented by the CA object, so
>>>>>> in IPA
>>>>>> the primary key for a certificate is actually (CA name, serial).
>>>>>>
>>>>>> Certificate lookup by issuer name and serial is actually a search
>>>>>> operation,
>>>>>> analogical to how CA lookup by subject name is also a search
>>>>>> operation, so
>>>>>> it should be done by cert-find.
>>>>>>
>>>>> OK, I will remove the --issuer option for cert-find.
>>>>>
>>>>>>>>
>>>>>>>>> 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).
>>>>>>
>>>>>> That's one way to look at it, but it's true only if you assume that
>>>>>> cert-find can only search certificates in Dogtag. This will very
>>>>>> soon became
>>>>>> untrue, as we will allow cert-find to also search certificates
>>>>>> anywhere in
>>>>>> LDAP (the server part of ticket #5381). There, the difference
>>>>>> between the
>>>>>> options would be that with --ca you search for certificates issued
>>>>>> by the
>>>>>> specified managed CA, but with --issuer you search for
>>>>>> certificates with the
>>>>>> given issuer name, be it managed CA or not.
>>>>>>
>>>>> --ca is just a "shorthand" for --issuer - it merely looks up subject
>>>>> DN of the specified CA, and uses that as the issuer option.
>>>>>
>>>>>> For now, IMO the correct behavior should be that if both are
>>>>>> specified and
>>>>>> the issuer name of the specified CA does not match the specified
>>>>>> issuer
>>>>>> name, empty result is returned, otherwise carry on with the search in
>>>>>> Dogtag.
>>>>>>
>>>>> If I allow both, the behaviour will then be:
>>>>>
>>>>> specify issuer DN only)
>>>>>     search using given issuer DN
>>>>> specify CA only)
>>>>>     search using subject DN of specified CA.  If no such CA, error.
>>>>> specify issuer DN and CA)
>>>>>     search using given issuer DN, and ensure that result (if any)
>>>>>     matches subject DN of specified CA.  If no such CA, error.
>>>>>
>>>>> I'm happy to implement this if you confirm that you think it's the
>>>>> correct behaviour.
>>>>
>>>> Looks correct to me.
>>>>
>>> Thanks; updated patches attached.  Martin, this patch should also
>>> fix the upgrade issue.
>>>
>>> Cheers,
>>> Fraser
>>>
>> Another rebase to fix conflicts in VERSION file.  No other changes.
>
> Thanks.
>
> The remaining cert commands (status, revoke, remove-hold) should also
> have the cacn option consistent with cert-show. This can be added in
> further patch, though.

Ticket: <https://fedorahosted.org/freeipa/ticket/5999>.

(Note that this should be fixed in time for 4.4, as it is an backward 
incompatible change.)

>
> I think it would make sense if the cn argument in ca commands was
> optional, with the default being 'ipa'. This can also be done later.

(No ticket yet since this is a backward compatible change.)

>
> So LGTM. If Martin agrees, we can push this patch set.
>


-- 
Jan Cholasta

More information about the Freeipa-devel mailing list