Re: [Freeipa-devel] [PATCH] 0106 Make host/service cert revocation aware of lightweight CAs

2016-09-07 Thread Fraser Tweedale 
On Wed, Sep 07, 2016 at 10:39:59AM +0200, Jan Cholasta wrote:
> On 7.9.2016 10:28, Fraser Tweedale wrote:
> > On Wed, Sep 07, 2016 at 08:32:42AM +0200, Jan Cholasta wrote:
> > > On 6.9.2016 19:36, Fraser Tweedale wrote:
> > > > On Tue, Sep 06, 2016 at 10:19:14AM +0200, Jan Cholasta wrote:
> > > > > On 5.9.2016 17:30, Fraser Tweedale wrote:
> > > > > > On Mon, Sep 05, 2016 at 11:59:11PM +1000, Fraser Tweedale wrote:
> > > > > > > On Tue, Aug 30, 2016 at 10:39:16AM +0200, Jan Cholasta wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On 26.8.2016 07:42, Fraser Tweedale wrote:
> > > > > > > > > On Fri, Aug 26, 2016 at 03:37:17PM +1000, Fraser Tweedale 
> > > > > > > > > wrote:
> > > > > > > > > > Hi all,
> > > > > > > > > > 
> > > > > > > > > > Attached patch fixes 
> > > > > > > > > > https://fedorahosted.org/freeipa/ticket/6221.
> > > > > > > > > > It depends on Honza's PR #20
> > > > > > > > > > https://github.com/freeipa/freeipa/pull/20.
> > > > > > > > > > 
> > > > > > > > > > Thanks,
> > > > > > > > > > Fraser
> > > > > > > > > > 
> > > > > > > > > It does help to attach the patch :)
> > > > > > > > 
> > > > > > > > I think it would be better to call cert-find once per 
> > > > > > > > host-del/service-del
> > > > > > > > with the --host/--service option specified. That way you'll get 
> > > > > > > > all
> > > > > > > > certificates for the given host/service at once.
> > > > > > > > 
> > > > > > > > Honza
> > > > > > > > 
> > > > > > > I agree that is a nicer approach.
> > > > > > > 
> > > > > > > 'revoke_certs' is called from several other places besides just
> > > > > > > host/service_del.  If we want to land this fix Real Soon I'd 
> > > > > > > suggest
> > > > > > > we either:
> > > > > > > 
> > > > > > > A) Define function 'revoke_certs_from_cert_find', call it from
> > > > > > > host/service_del, and leave 'revoke_certs' alone; or
> > > > > > > 
> > > > > > > B) Land the patch as-is and do a bigger refactor at a later time.
> > > > > > > 
> > > > > > > What do you think?
> > > > > 
> > > > Updated patch attached; comments inline.
> > > > 
> > > > > C) Use cert-find-based revoke_certs() everywhere; use the 
> > > > > --certificate
> > > > > option of cert-find in the other places to get information about 
> > > > > specific
> > > > > certificates.
> > > > > 
> > > > As discussed on IRC, I have implemented this option.  The caveat is
> > > > that for host/service-mod, we incur call to cert_find for each
> > > > removed certificate.
> > > 
> > > It's worth noting that A) and B) suffer from the same caveat.
> > > 
> > > > 
> > > > > > > 
> > > > > > Updated patch for option (A) is attached.
> > > > > 
> > > > > 1) Instead of
> > > > > 
> > > > > if result['status'] in {'REVOKED', 'REVOKED_EXPIRED'}:
> > > > > 
> > > > > use:
> > > > > 
> > > > > if result['revoked']:
> > > > > 
> > > > Done.
> > > > 
> > > > > 
> > > > > 2)
> > > > > 
> > > > > +if 'cacn' not in cert:
> > > > > +# cert is known to Dogtag, but CA appears to have been
> > > > > +# deleted.  We cannot revoke this cert via IPA anymore.
> > > > > +# We could go directly to Dogtag to revoke it, but the
> > > > > +# issuer's cert should have been revoked so never mind.
> > > > > +continue
> > > > > 
> > > > > Or, it could be a cert issued by a 3rd party CA.
> > > > > 
> > > > I updated to comment to include this.
> > > > 
> > > > > 
> > > > > 3) host-mod/service-mod do not revoke certs:
> > > > > 
> > > > > $ ipa cert-request test.csr --principal host/test.example.com
> > > > >   Serial number: 13
> > > > > 
> > > > > $ ipa cert-show 13
> > > > >   Revoked: False
> > > > >   Owner host: test.example.com
> > > > > 
> > > > > $ ipa host-mod test.example.com --certificate=
> > > > > 
> > > > > $ ipa cert-show 13
> > > > >   Revoked: False
> > > > > 
> > > > Nice find.  This was a pre-existing bug: nothing gets revoked when
> > > > all certs are removed.  Here is the fix:
> > > > 
> > > > -if certs and self.api.Command.ca_is_enabled()['result']:
> > > > +ca_is_enabled = self.api.Command.ca_is_enabled()['result']
> > > > +if 'usercertificate' in options and ca_is_enabled:
> > > >  ... revocation code
> > > 
> > > OK. Since it is a different bug, it should be fixed in a separate patch 
> > > and
> > > have a separate ticket.
> > > 
> > > > 
> > > > Finally, host/service-remove-cert does not revoke the cert because
> > > > of (I think) a bug in cert-find.  If the cert does not exist on a
> > > > host/service the cert-find cannot find it with --certificate option.
> > > > Because host/service-remove-cert uses a post_callback to revoke the
> > > > cert, cert-find doesn't find it thus no revocation occurs.
> > > > 
> > > > Honza could you check whether this is indeed a bug/limitation of
> > > > cert-find or is it the smog in Saigon affecting me?
> > > 
> > > It's a bug - FTFY,

Re: [Freeipa-devel] [PATCH] 0106 Make host/service cert revocation aware of lightweight CAs

2016-09-07 Thread Jan Cholasta 

On 7.9.2016 10:28, Fraser Tweedale wrote:

On Wed, Sep 07, 2016 at 08:32:42AM +0200, Jan Cholasta wrote:

On 6.9.2016 19:36, Fraser Tweedale wrote:

On Tue, Sep 06, 2016 at 10:19:14AM +0200, Jan Cholasta wrote:

On 5.9.2016 17:30, Fraser Tweedale wrote:

On Mon, Sep 05, 2016 at 11:59:11PM +1000, Fraser Tweedale wrote:

On Tue, Aug 30, 2016 at 10:39:16AM +0200, Jan Cholasta wrote:

Hi,

On 26.8.2016 07:42, Fraser Tweedale wrote:

On Fri, Aug 26, 2016 at 03:37:17PM +1000, Fraser Tweedale wrote:

Hi all,

Attached patch fixes https://fedorahosted.org/freeipa/ticket/6221.
It depends on Honza's PR #20
https://github.com/freeipa/freeipa/pull/20.

Thanks,
Fraser


It does help to attach the patch :)


I think it would be better to call cert-find once per host-del/service-del
with the --host/--service option specified. That way you'll get all
certificates for the given host/service at once.

Honza


I agree that is a nicer approach.

'revoke_certs' is called from several other places besides just
host/service_del.  If we want to land this fix Real Soon I'd suggest
we either:

A) Define function 'revoke_certs_from_cert_find', call it from
host/service_del, and leave 'revoke_certs' alone; or

B) Land the patch as-is and do a bigger refactor at a later time.

What do you think?



Updated patch attached; comments inline.


C) Use cert-find-based revoke_certs() everywhere; use the --certificate
option of cert-find in the other places to get information about specific
certificates.


As discussed on IRC, I have implemented this option.  The caveat is
that for host/service-mod, we incur call to cert_find for each
removed certificate.


It's worth noting that A) and B) suffer from the same caveat.






Updated patch for option (A) is attached.


1) Instead of

if result['status'] in {'REVOKED', 'REVOKED_EXPIRED'}:

use:

if result['revoked']:


Done.



2)

+if 'cacn' not in cert:
+# cert is known to Dogtag, but CA appears to have been
+# deleted.  We cannot revoke this cert via IPA anymore.
+# We could go directly to Dogtag to revoke it, but the
+# issuer's cert should have been revoked so never mind.
+continue

Or, it could be a cert issued by a 3rd party CA.


I updated to comment to include this.



3) host-mod/service-mod do not revoke certs:

$ ipa cert-request test.csr --principal host/test.example.com
  Serial number: 13

$ ipa cert-show 13
  Revoked: False
  Owner host: test.example.com

$ ipa host-mod test.example.com --certificate=

$ ipa cert-show 13
  Revoked: False


Nice find.  This was a pre-existing bug: nothing gets revoked when
all certs are removed.  Here is the fix:

-if certs and self.api.Command.ca_is_enabled()['result']:
+ca_is_enabled = self.api.Command.ca_is_enabled()['result']
+if 'usercertificate' in options and ca_is_enabled:
 ... revocation code


OK. Since it is a different bug, it should be fixed in a separate patch and
have a separate ticket.



Finally, host/service-remove-cert does not revoke the cert because
of (I think) a bug in cert-find.  If the cert does not exist on a
host/service the cert-find cannot find it with --certificate option.
Because host/service-remove-cert uses a post_callback to revoke the
cert, cert-find doesn't find it thus no revocation occurs.

Honza could you check whether this is indeed a bug/limitation of
cert-find or is it the smog in Saigon affecting me?


It's a bug - FTFY, .

Functional ACK. Full ACK once my fix is merged and the host/service-mod is
split off into a separate patch.


To clarify - you want only the fix discussed above in the separate
patch?


I want the fix for sub-CA revocation in one patch and the fix for 
host/service-mod with empty --certificate in other patch.


--
Jan Cholasta

--
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

Re: [Freeipa-devel] [PATCH] 0106 Make host/service cert revocation aware of lightweight CAs

2016-09-07 Thread Fraser Tweedale 
On Wed, Sep 07, 2016 at 08:32:42AM +0200, Jan Cholasta wrote:
> On 6.9.2016 19:36, Fraser Tweedale wrote:
> > On Tue, Sep 06, 2016 at 10:19:14AM +0200, Jan Cholasta wrote:
> > > On 5.9.2016 17:30, Fraser Tweedale wrote:
> > > > On Mon, Sep 05, 2016 at 11:59:11PM +1000, Fraser Tweedale wrote:
> > > > > On Tue, Aug 30, 2016 at 10:39:16AM +0200, Jan Cholasta wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On 26.8.2016 07:42, Fraser Tweedale wrote:
> > > > > > > On Fri, Aug 26, 2016 at 03:37:17PM +1000, Fraser Tweedale wrote:
> > > > > > > > Hi all,
> > > > > > > > 
> > > > > > > > Attached patch fixes 
> > > > > > > > https://fedorahosted.org/freeipa/ticket/6221.
> > > > > > > > It depends on Honza's PR #20
> > > > > > > > https://github.com/freeipa/freeipa/pull/20.
> > > > > > > > 
> > > > > > > > Thanks,
> > > > > > > > Fraser
> > > > > > > > 
> > > > > > > It does help to attach the patch :)
> > > > > > 
> > > > > > I think it would be better to call cert-find once per 
> > > > > > host-del/service-del
> > > > > > with the --host/--service option specified. That way you'll get all
> > > > > > certificates for the given host/service at once.
> > > > > > 
> > > > > > Honza
> > > > > > 
> > > > > I agree that is a nicer approach.
> > > > > 
> > > > > 'revoke_certs' is called from several other places besides just
> > > > > host/service_del.  If we want to land this fix Real Soon I'd suggest
> > > > > we either:
> > > > > 
> > > > > A) Define function 'revoke_certs_from_cert_find', call it from
> > > > > host/service_del, and leave 'revoke_certs' alone; or
> > > > > 
> > > > > B) Land the patch as-is and do a bigger refactor at a later time.
> > > > > 
> > > > > What do you think?
> > > 
> > Updated patch attached; comments inline.
> > 
> > > C) Use cert-find-based revoke_certs() everywhere; use the --certificate
> > > option of cert-find in the other places to get information about specific
> > > certificates.
> > > 
> > As discussed on IRC, I have implemented this option.  The caveat is
> > that for host/service-mod, we incur call to cert_find for each
> > removed certificate.
> 
> It's worth noting that A) and B) suffer from the same caveat.
> 
> > 
> > > > > 
> > > > Updated patch for option (A) is attached.
> > > 
> > > 1) Instead of
> > > 
> > > if result['status'] in {'REVOKED', 'REVOKED_EXPIRED'}:
> > > 
> > > use:
> > > 
> > > if result['revoked']:
> > > 
> > Done.
> > 
> > > 
> > > 2)
> > > 
> > > +if 'cacn' not in cert:
> > > +# cert is known to Dogtag, but CA appears to have been
> > > +# deleted.  We cannot revoke this cert via IPA anymore.
> > > +# We could go directly to Dogtag to revoke it, but the
> > > +# issuer's cert should have been revoked so never mind.
> > > +continue
> > > 
> > > Or, it could be a cert issued by a 3rd party CA.
> > > 
> > I updated to comment to include this.
> > 
> > > 
> > > 3) host-mod/service-mod do not revoke certs:
> > > 
> > > $ ipa cert-request test.csr --principal host/test.example.com
> > >   Serial number: 13
> > > 
> > > $ ipa cert-show 13
> > >   Revoked: False
> > >   Owner host: test.example.com
> > > 
> > > $ ipa host-mod test.example.com --certificate=
> > > 
> > > $ ipa cert-show 13
> > >   Revoked: False
> > > 
> > Nice find.  This was a pre-existing bug: nothing gets revoked when
> > all certs are removed.  Here is the fix:
> > 
> > -if certs and self.api.Command.ca_is_enabled()['result']:
> > +ca_is_enabled = self.api.Command.ca_is_enabled()['result']
> > +if 'usercertificate' in options and ca_is_enabled:
> >  ... revocation code
> 
> OK. Since it is a different bug, it should be fixed in a separate patch and
> have a separate ticket.
> 
> > 
> > Finally, host/service-remove-cert does not revoke the cert because
> > of (I think) a bug in cert-find.  If the cert does not exist on a
> > host/service the cert-find cannot find it with --certificate option.
> > Because host/service-remove-cert uses a post_callback to revoke the
> > cert, cert-find doesn't find it thus no revocation occurs.
> > 
> > Honza could you check whether this is indeed a bug/limitation of
> > cert-find or is it the smog in Saigon affecting me?
> 
> It's a bug - FTFY, .
> 
> Functional ACK. Full ACK once my fix is merged and the host/service-mod is
> split off into a separate patch.
> 
To clarify - you want only the fix discussed above in the separate
patch?

Thanks,
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

Re: [Freeipa-devel] [PATCH] 0106 Make host/service cert revocation aware of lightweight CAs

2016-09-07 Thread Jan Cholasta 

On 6.9.2016 19:36, Fraser Tweedale wrote:

On Tue, Sep 06, 2016 at 10:19:14AM +0200, Jan Cholasta wrote:

On 5.9.2016 17:30, Fraser Tweedale wrote:

On Mon, Sep 05, 2016 at 11:59:11PM +1000, Fraser Tweedale wrote:

On Tue, Aug 30, 2016 at 10:39:16AM +0200, Jan Cholasta wrote:

Hi,

On 26.8.2016 07:42, Fraser Tweedale wrote:

On Fri, Aug 26, 2016 at 03:37:17PM +1000, Fraser Tweedale wrote:

Hi all,

Attached patch fixes https://fedorahosted.org/freeipa/ticket/6221.
It depends on Honza's PR #20
https://github.com/freeipa/freeipa/pull/20.

Thanks,
Fraser


It does help to attach the patch :)


I think it would be better to call cert-find once per host-del/service-del
with the --host/--service option specified. That way you'll get all
certificates for the given host/service at once.

Honza


I agree that is a nicer approach.

'revoke_certs' is called from several other places besides just
host/service_del.  If we want to land this fix Real Soon I'd suggest
we either:

A) Define function 'revoke_certs_from_cert_find', call it from
host/service_del, and leave 'revoke_certs' alone; or

B) Land the patch as-is and do a bigger refactor at a later time.

What do you think?



Updated patch attached; comments inline.


C) Use cert-find-based revoke_certs() everywhere; use the --certificate
option of cert-find in the other places to get information about specific
certificates.


As discussed on IRC, I have implemented this option.  The caveat is
that for host/service-mod, we incur call to cert_find for each
removed certificate.


It's worth noting that A) and B) suffer from the same caveat.






Updated patch for option (A) is attached.


1) Instead of

if result['status'] in {'REVOKED', 'REVOKED_EXPIRED'}:

use:

if result['revoked']:


Done.



2)

+if 'cacn' not in cert:
+# cert is known to Dogtag, but CA appears to have been
+# deleted.  We cannot revoke this cert via IPA anymore.
+# We could go directly to Dogtag to revoke it, but the
+# issuer's cert should have been revoked so never mind.
+continue

Or, it could be a cert issued by a 3rd party CA.


I updated to comment to include this.



3) host-mod/service-mod do not revoke certs:

$ ipa cert-request test.csr --principal host/test.example.com
  Serial number: 13

$ ipa cert-show 13
  Revoked: False
  Owner host: test.example.com

$ ipa host-mod test.example.com --certificate=

$ ipa cert-show 13
  Revoked: False


Nice find.  This was a pre-existing bug: nothing gets revoked when
all certs are removed.  Here is the fix:

-if certs and self.api.Command.ca_is_enabled()['result']:
+ca_is_enabled = self.api.Command.ca_is_enabled()['result']
+if 'usercertificate' in options and ca_is_enabled:
 ... revocation code


OK. Since it is a different bug, it should be fixed in a separate patch 
and have a separate ticket.




Finally, host/service-remove-cert does not revoke the cert because
of (I think) a bug in cert-find.  If the cert does not exist on a
host/service the cert-find cannot find it with --certificate option.
Because host/service-remove-cert uses a post_callback to revoke the
cert, cert-find doesn't find it thus no revocation occurs.

Honza could you check whether this is indeed a bug/limitation of
cert-find or is it the smog in Saigon affecting me?


It's a bug - FTFY, .

Functional ACK. Full ACK once my fix is merged and the host/service-mod 
is split off into a separate patch.


--
Jan Cholasta

--
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

Re: [Freeipa-devel] [PATCH] 0106 Make host/service cert revocation aware of lightweight CAs

2016-09-06 Thread Fraser Tweedale 
On Tue, Sep 06, 2016 at 10:19:14AM +0200, Jan Cholasta wrote:
> On 5.9.2016 17:30, Fraser Tweedale wrote:
> > On Mon, Sep 05, 2016 at 11:59:11PM +1000, Fraser Tweedale wrote:
> > > On Tue, Aug 30, 2016 at 10:39:16AM +0200, Jan Cholasta wrote:
> > > > Hi,
> > > > 
> > > > On 26.8.2016 07:42, Fraser Tweedale wrote:
> > > > > On Fri, Aug 26, 2016 at 03:37:17PM +1000, Fraser Tweedale wrote:
> > > > > > Hi all,
> > > > > > 
> > > > > > Attached patch fixes https://fedorahosted.org/freeipa/ticket/6221.
> > > > > > It depends on Honza's PR #20
> > > > > > https://github.com/freeipa/freeipa/pull/20.
> > > > > > 
> > > > > > Thanks,
> > > > > > Fraser
> > > > > > 
> > > > > It does help to attach the patch :)
> > > > 
> > > > I think it would be better to call cert-find once per 
> > > > host-del/service-del
> > > > with the --host/--service option specified. That way you'll get all
> > > > certificates for the given host/service at once.
> > > > 
> > > > Honza
> > > > 
> > > I agree that is a nicer approach.
> > > 
> > > 'revoke_certs' is called from several other places besides just
> > > host/service_del.  If we want to land this fix Real Soon I'd suggest
> > > we either:
> > > 
> > > A) Define function 'revoke_certs_from_cert_find', call it from
> > > host/service_del, and leave 'revoke_certs' alone; or
> > > 
> > > B) Land the patch as-is and do a bigger refactor at a later time.
> > > 
> > > What do you think?
> 
Updated patch attached; comments inline.

> C) Use cert-find-based revoke_certs() everywhere; use the --certificate
> option of cert-find in the other places to get information about specific
> certificates.
> 
As discussed on IRC, I have implemented this option.  The caveat is
that for host/service-mod, we incur call to cert_find for each
removed certificate.

> > > 
> > Updated patch for option (A) is attached.
> 
> 1) Instead of
> 
> if result['status'] in {'REVOKED', 'REVOKED_EXPIRED'}:
> 
> use:
> 
> if result['revoked']:
> 
Done.

> 
> 2)
> 
> +if 'cacn' not in cert:
> +# cert is known to Dogtag, but CA appears to have been
> +# deleted.  We cannot revoke this cert via IPA anymore.
> +# We could go directly to Dogtag to revoke it, but the
> +# issuer's cert should have been revoked so never mind.
> +continue
> 
> Or, it could be a cert issued by a 3rd party CA.
> 
I updated to comment to include this.

> 
> 3) host-mod/service-mod do not revoke certs:
> 
> $ ipa cert-request test.csr --principal host/test.example.com
>   Serial number: 13
> 
> $ ipa cert-show 13
>   Revoked: False
>   Owner host: test.example.com
> 
> $ ipa host-mod test.example.com --certificate=
> 
> $ ipa cert-show 13
>   Revoked: False
> 
Nice find.  This was a pre-existing bug: nothing gets revoked when
all certs are removed.  Here is the fix:

-if certs and self.api.Command.ca_is_enabled()['result']:
+ca_is_enabled = self.api.Command.ca_is_enabled()['result']
+if 'usercertificate' in options and ca_is_enabled:
 ... revocation code

Finally, host/service-remove-cert does not revoke the cert because
of (I think) a bug in cert-find.  If the cert does not exist on a
host/service the cert-find cannot find it with --certificate option.
Because host/service-remove-cert uses a post_callback to revoke the
cert, cert-find doesn't find it thus no revocation occurs.

Honza could you check whether this is indeed a bug/limitation of
cert-find or is it the smog in Saigon affecting me?

Thanks,
Fraser
From 9c829d7ec8ff67dcf814c468c406772bf311c9f8 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Fri, 26 Aug 2016 15:31:13 +1000
Subject: [PATCH] Make host/service cert revocation aware of lightweight CAs

Revocation of host/service certs on host/service deletion or other
operations is broken when cert is issued by a lightweight (sub)CA,
causing the delete operation to be aborted.  Look up the issuing CA
and pass it to 'cert_revoke' to fix the issue.

Fixes: https://fedorahosted.org/freeipa/ticket/6221
---
 ipaserver/plugins/host.py| 23 +
 ipaserver/plugins/service.py | 59 ++--
 2 files changed, 41 insertions(+), 41 deletions(-)

diff --git a/ipaserver/plugins/host.py b/ipaserver/plugins/host.py
index 
03c64c637cbba0aee1b6569f3b5dbe200953bff8..7f63e94849b4a6f2ce871ec77b188c54d640ba94
 100644
--- a/ipaserver/plugins/host.py
+++ b/ipaserver/plugins/host.py
@@ -843,12 +843,8 @@ class host_del(LDAPDelete):
 )
 
 if self.api.Command.ca_is_enabled()['result']:
-try:
-entry_attrs = ldap.get_entry(dn, ['usercertificate'])
-except errors.NotFound:
-self.obj.handle_not_found(*keys)
-
-revoke_certs(entry_attrs.get('usercertificate', []), self.log)
+certs = self.api.Command.cert_find(host=keys)['result']
+

Re: [Freeipa-devel] [PATCH] 0106 Make host/service cert revocation aware of lightweight CAs

2016-09-06 Thread Jan Cholasta 

On 5.9.2016 17:30, Fraser Tweedale wrote:

On Mon, Sep 05, 2016 at 11:59:11PM +1000, Fraser Tweedale wrote:

On Tue, Aug 30, 2016 at 10:39:16AM +0200, Jan Cholasta wrote:

Hi,

On 26.8.2016 07:42, Fraser Tweedale wrote:

On Fri, Aug 26, 2016 at 03:37:17PM +1000, Fraser Tweedale wrote:

Hi all,

Attached patch fixes https://fedorahosted.org/freeipa/ticket/6221.
It depends on Honza's PR #20
https://github.com/freeipa/freeipa/pull/20.

Thanks,
Fraser


It does help to attach the patch :)


I think it would be better to call cert-find once per host-del/service-del
with the --host/--service option specified. That way you'll get all
certificates for the given host/service at once.

Honza


I agree that is a nicer approach.

'revoke_certs' is called from several other places besides just
host/service_del.  If we want to land this fix Real Soon I'd suggest
we either:

A) Define function 'revoke_certs_from_cert_find', call it from
host/service_del, and leave 'revoke_certs' alone; or

B) Land the patch as-is and do a bigger refactor at a later time.

What do you think?


C) Use cert-find-based revoke_certs() everywhere; use the --certificate 
option of cert-find in the other places to get information about 
specific certificates.





Updated patch for option (A) is attached.


1) Instead of

if result['status'] in {'REVOKED', 'REVOKED_EXPIRED'}:

use:

if result['revoked']:


2)

+if 'cacn' not in cert:
+# cert is known to Dogtag, but CA appears to have been
+# deleted.  We cannot revoke this cert via IPA anymore.
+# We could go directly to Dogtag to revoke it, but the
+# issuer's cert should have been revoked so never mind.
+continue

Or, it could be a cert issued by a 3rd party CA.


3) host-mod/service-mod do not revoke certs:

$ ipa cert-request test.csr --principal host/test.example.com
  Serial number: 13

$ ipa cert-show 13
  Revoked: False
  Owner host: test.example.com

$ ipa host-mod test.example.com --certificate=

$ ipa cert-show 13
  Revoked: False


--
Jan Cholasta

--
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

Re: [Freeipa-devel] [PATCH] 0106 Make host/service cert revocation aware of lightweight CAs

2016-09-05 Thread Fraser Tweedale 
On Mon, Sep 05, 2016 at 11:59:11PM +1000, Fraser Tweedale wrote:
> On Tue, Aug 30, 2016 at 10:39:16AM +0200, Jan Cholasta wrote:
> > Hi,
> > 
> > On 26.8.2016 07:42, Fraser Tweedale wrote:
> > > On Fri, Aug 26, 2016 at 03:37:17PM +1000, Fraser Tweedale wrote:
> > > > Hi all,
> > > > 
> > > > Attached patch fixes https://fedorahosted.org/freeipa/ticket/6221.
> > > > It depends on Honza's PR #20
> > > > https://github.com/freeipa/freeipa/pull/20.
> > > > 
> > > > Thanks,
> > > > Fraser
> > > > 
> > > It does help to attach the patch :)
> > 
> > I think it would be better to call cert-find once per host-del/service-del
> > with the --host/--service option specified. That way you'll get all
> > certificates for the given host/service at once.
> > 
> > Honza
> >
> I agree that is a nicer approach.
> 
> 'revoke_certs' is called from several other places besides just
> host/service_del.  If we want to land this fix Real Soon I'd suggest
> we either:
> 
> A) Define function 'revoke_certs_from_cert_find', call it from
> host/service_del, and leave 'revoke_certs' alone; or
> 
> B) Land the patch as-is and do a bigger refactor at a later time.
> 
> What do you think?
>
Updated patch for option (A) is attached.

Thanks,
Fraser
From dacb091292b57608af8adb97adf9a96f1cb34e54 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Fri, 26 Aug 2016 15:31:13 +1000
Subject: [PATCH] Make host/service cert revocation aware of lightweight CAs

Revocation of host/service certs on host/service deletion or other
operations is broken when cert is issued by a lightweight (sub)CA,
causing the delete operation to be aborted.  Look up the issuing CA
and pass it to 'cert_revoke' to fix the issue.

For host/service deletion, look up all certs at once and pass to an
alternative revocation function that avoids addition calls to
cert_find or cert_show.

Fixes: https://fedorahosted.org/freeipa/ticket/6221
---
 ipaserver/plugins/host.py| 11 +++-
 ipaserver/plugins/service.py | 66 +---
 2 files changed, 60 insertions(+), 17 deletions(-)

diff --git a/ipaserver/plugins/host.py b/ipaserver/plugins/host.py
index 
03c64c637cbba0aee1b6569f3b5dbe200953bff8..891dacd762a57c06ff032e6f262813158c94f9f2
 100644
--- a/ipaserver/plugins/host.py
+++ b/ipaserver/plugins/host.py
@@ -42,7 +42,8 @@ from .service import (
 validate_realm, normalize_principal, validate_certificate,
 set_certificate_attrs, ticket_flags_params, update_krbticketflags,
 set_kerberos_attrs, rename_ipaallowedtoperform_from_ldap,
-rename_ipaallowedtoperform_to_ldap, revoke_certs)
+rename_ipaallowedtoperform_to_ldap, revoke_certs,
+revoke_certs_from_cert_find)
 from .dns import (dns_container_exists,
 add_records_for_host_validation, add_records_for_host,
 get_reverse_zone)
@@ -843,12 +844,8 @@ class host_del(LDAPDelete):
 )
 
 if self.api.Command.ca_is_enabled()['result']:
-try:
-entry_attrs = ldap.get_entry(dn, ['usercertificate'])
-except errors.NotFound:
-self.obj.handle_not_found(*keys)
-
-revoke_certs(entry_attrs.get('usercertificate', []), self.log)
+certs = self.api.Command.cert_find(host=keys)['result']
+revoke_certs_from_cert_find(certs)
 
 return dn
 
diff --git a/ipaserver/plugins/service.py b/ipaserver/plugins/service.py
index 
04d1916fe989a8651bcc4d44f1914c460be1081c..407665fb450e7a8513b42815691d64665b6b2282
 100644
--- a/ipaserver/plugins/service.py
+++ b/ipaserver/plugins/service.py
@@ -232,24 +232,73 @@ def revoke_certs(certs, logger=None):
 logger.info("Problem decoding certificate: %s" % e)
 
 serial = unicode(x509.get_serial_number(cert, x509.DER))
+issuer = unicode(x509.get_issuer(cert, x509.DER))
 
 try:
-result = api.Command['cert_show'](unicode(serial))['result']
+# search by serial+issuer, not full cert match
+results = api.Command['cert_find'](
+min_serial_number=serial,
+max_serial_number=serial,
+issuer=issuer
+)['result']
+if len(results) == 0:
+# Dogtag doesn't know about the cert therefore
+# we cannot revoke it.  Perhaps it was issued by
+# a 3rd-party CA.
+continue
+result = results[0]
 except errors.CertificateOperationError:
 continue
-if 'revocation_reason' in result:
+if result['status'] in {'REVOKED', 'REVOKED_EXPIRED'}:
 continue
-if x509.normalize_certificate(result['certificate']) != cert:
+if 'cacn' not in result:
+# cert is known to Dogtag, but CA appears to have been
+# deleted.  We cannot revoke this cert via IPA anymore.
+# We could go directly to Dogtag to revoke it, but the
+#

Re: [Freeipa-devel] [PATCH] 0106 Make host/service cert revocation aware of lightweight CAs

2016-09-05 Thread Fraser Tweedale 
On Tue, Aug 30, 2016 at 10:39:16AM +0200, Jan Cholasta wrote:
> Hi,
> 
> On 26.8.2016 07:42, Fraser Tweedale wrote:
> > On Fri, Aug 26, 2016 at 03:37:17PM +1000, Fraser Tweedale wrote:
> > > Hi all,
> > > 
> > > Attached patch fixes https://fedorahosted.org/freeipa/ticket/6221.
> > > It depends on Honza's PR #20
> > > https://github.com/freeipa/freeipa/pull/20.
> > > 
> > > Thanks,
> > > Fraser
> > > 
> > It does help to attach the patch :)
> 
> I think it would be better to call cert-find once per host-del/service-del
> with the --host/--service option specified. That way you'll get all
> certificates for the given host/service at once.
> 
> Honza
>
I agree that is a nicer approach.

'revoke_certs' is called from several other places besides just
host/service_del.  If we want to land this fix Real Soon I'd suggest
we either:

A) Define function 'revoke_certs_from_cert_find', call it from
host/service_del, and leave 'revoke_certs' alone; or

B) Land the patch as-is and do a bigger refactor at a later time.

What do you think?

-- 
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

Re: [Freeipa-devel] [PATCH] 0106 Make host/service cert revocation aware of lightweight CAs

2016-08-30 Thread Jan Cholasta 

Hi,

On 26.8.2016 07:42, Fraser Tweedale wrote:

On Fri, Aug 26, 2016 at 03:37:17PM +1000, Fraser Tweedale wrote:

Hi all,

Attached patch fixes https://fedorahosted.org/freeipa/ticket/6221.
It depends on Honza's PR #20
https://github.com/freeipa/freeipa/pull/20.

Thanks,
Fraser


It does help to attach the patch :)


I think it would be better to call cert-find once per 
host-del/service-del with the --host/--service option specified. That 
way you'll get all certificates for the given host/service at once.


Honza

--
Jan Cholasta

--
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

Re: [Freeipa-devel] [PATCH] 0106 Make host/service cert revocation aware of lightweight CAs

2016-08-25 Thread Fraser Tweedale 
On Fri, Aug 26, 2016 at 03:37:17PM +1000, Fraser Tweedale wrote:
> Hi all,
> 
> Attached patch fixes https://fedorahosted.org/freeipa/ticket/6221.
> It depends on Honza's PR #20
> https://github.com/freeipa/freeipa/pull/20.
> 
> Thanks,
> Fraser
> 
It does help to attach the patch :)
From 35ab316731d49d503a66d8621b1812a2eb50d180 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Fri, 26 Aug 2016 15:31:13 +1000
Subject: [PATCH] Make host/service cert revocation aware of lightweight CAs

Revocation of host/service certs on host/service deletion is broken
when cert is issued by a lightweight (sub)CA, causing the delete
operation to be aborted.  Look up the issuing CA and pass it to
'cert_revoke' to fix the issue.

Fixes: https://fedorahosted.org/freeipa/ticket/6221
---
 ipaserver/plugins/service.py | 29 -
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/ipaserver/plugins/service.py b/ipaserver/plugins/service.py
index 
04d1916fe989a8651bcc4d44f1914c460be1081c..ada5cd1e6f0d289332d77ec651732ba70843ff65
 100644
--- a/ipaserver/plugins/service.py
+++ b/ipaserver/plugins/service.py
@@ -232,19 +232,38 @@ def revoke_certs(certs, logger=None):
 logger.info("Problem decoding certificate: %s" % e)
 
 serial = unicode(x509.get_serial_number(cert, x509.DER))
+issuer = unicode(x509.get_issuer(cert, x509.DER))
 
 try:
-result = api.Command['cert_show'](unicode(serial))['result']
+# search by serial+issuer, not full cert match
+results = api.Command['cert_find'](
+min_serial_number=serial,
+max_serial_number=serial,
+issuer=issuer
+)['result']
+if len(results) == 0:
+# Dogtag doesn't know about the cert therefore
+# we cannot revoke it.  Perhaps it was issued by
+# a 3rd-party CA.
+continue
+result = results[0]
 except errors.CertificateOperationError:
 continue
-if 'revocation_reason' in result:
+if result['status'] in {'REVOKED', 'REVOKED_EXPIRED'}:
 continue
-if x509.normalize_certificate(result['certificate']) != cert:
+if 'cacn' not in result:
+# cert is known to Dogtag, but CA appears to have been
+# deleted.  We cannot revoke this cert via IPA anymore.
+# We could go directly to Dogtag to revoke it, but the
+# issuer's cert should have been revoked so never mind.
 continue
 
 try:
-api.Command['cert_revoke'](unicode(serial),
-   revocation_reason=4)
+api.Command['cert_revoke'](
+serial,
+cacn=result['cacn'],
+revocation_reason=4,
+)
 except errors.NotImplementedError:
 # some CA's might not implement revoke
 pass
-- 
2.5.5

-- 
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

[Freeipa-devel] [PATCH] 0106 Make host/service cert revocation aware of lightweight CAs

2016-08-25 Thread Fraser Tweedale 
Hi all,

Attached patch fixes https://fedorahosted.org/freeipa/ticket/6221.
It depends on Honza's PR #20
https://github.com/freeipa/freeipa/pull/20.

Thanks,
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