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.
> 2.) > > The '--chain' option in cert-show/cert-request is not implemented, but this > was probably a deliberate decision. We should however at least open a ticket > about this and add it later. > Yes, it will be added in a later patch. > 3.) > > I cannot issue a user certificate on a replica with configured CA: > > """ > [root@replica1 ~]# ipa cert-request cert.csr --ca user_sub_ca --principal > jrogan > ipa: ERROR: Certificate operation cannot be completed: FAILURE (CA not > found: bc953851-1f24-4e16-9847-9ee5bbe6dd0e) > [root@replica1 ~]# ipa ca-find user_sub_ca > ------------ > 1 CA matched > ------------ > Name: user_sub_ca > Description: CA for issuing user certificates > Authority ID: bc953851-1f24-4e16-9847-9ee5bbe6dd0e > Subject DN: CN=User Cert Sub-CA,O=IPA.TEST > Issuer DN: CN=Certificate Authority,O=IPA.TEST > ---------------------------- > Number of entries returned 1 > ---------------------------- > [root@replica1 ~]# ipa cert-request cert.csr --ca user_sub_ca --principal > jrogan > ipa: ERROR: Certificate operation cannot be completed: FAILURE (CA not > found: bc953851-1f24-4e16-9847-9ee5bbe6dd0e) > """ > > The LDAP entry is there, but it seems like the sub-CA was not replicated at > the dogtag backend. This occurs regardless of whether the master was freshly > installed or upgraded from 4.3.1 release. > Can you please tell me the version of Dogtag packages on each of the machines? Could you please also provide /var/log/pki/pki-tomcat/ca/debug from the replica? Cheers, Fraser -- 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
