On Mon, Jun 13, 2016 at 11:17:21PM +1000, Fraser Tweedale wrote: > On Mon, Jun 13, 2016 at 12:48:18PM +0200, Martin Babinsky wrote: > > On 06/13/2016 08:59 AM, 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. > > > > > > 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. > > > > > > So LGTM. If Martin agrees, we can push this patch set. > > > > > > > Hi Fraser, > > > > during functional review I found the following issues: > > > > 1.) > > > > If I create a CAACL rule tied to a specific sub-CA let's say for user > > certificate issuance: > > > > """ > > ipa caacl-show user_cert_issuance > > Enabled: TRUE > > User category: all > > CAs: user_sub_ca > > Profiles: caIPAuserCert > > ACL name: user_cert_issuance > > > > """ > > > > I can still happily request certificate for a user using root-CA: > > > > """ > > ipa cert-request cert.csr --principal jdoe --ca ipa > > Certificate: MIID9j.../Ov8mkjFA== > > Subject: CN=jdoe,O=IPA.TEST > > Issuer: CN=Certificate Authority,O=IPA.TEST > > ... > > """ > > > > should not this be denied by CA-ACL rule? > > > > The default IPA CAACL rule is like this: > > > > """ > > ipa caacl-show hosts_services_caIPAserviceCert > > Enabled: TRUE > > Host category: all > > Service category: all > > Profiles: caIPAserviceCert > > ACL name: hosts_services_caIPAserviceCert > > > > """ > > > > so the default rule should not allow users to request certs at all. > > > Yes, these should be denied. Looking into it. > Were you using 'admin' account to request the cert? admin has permission 'Request Certificate ignoring CA ACLs' via the 'Certificate Manager' privilege.
If so, please try again with less privileges (e.g. self-service as jdoe). -- 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
