Re: [Freeipa-devel] [PATCH] 0081 Add --ca option to cert-revoke and cert-remove-hold

2016-06-30 Thread Jan Cholasta 

On 29.6.2016 12:18, Jan Cholasta wrote:

On 29.6.2016 10:47, Fraser Tweedale wrote:

On Wed, Jun 29, 2016 at 10:04:05AM +0200, Jan Cholasta wrote:

Hi,

On 29.6.2016 06:11, Fraser Tweedale wrote:

Dear team,

The attached patch implements the --ca option for the rest of the
cert-blah commands (https://fedorahosted.org/freeipa/ticket/5999).


1) I don't think cert-status should be treated specially. The
operation to
check status of a certificate request is not specific to Dogtag.


I'm happy to add the option, with the caveat that because (of top of
head) there is not (yet) a way in Dogtag to distinguish/filter
requests by target CA, value may go unused.


IMO that's OK, since it's a safe non-descructive operation.





2) cert-show is called twice in cert-revoke. Can we call it just once?


I'll address this in next patchset.


OK.


ACK on the first version of the patch, since it's better than nothing. 
The ticket remains open, please fix the rest ASAP.


Added VERSION bump and pushed to master: 
ffb1f5b1f24f0de30529d50f8c8dfb9a896c149e


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] 0081 Add --ca option to cert-revoke and cert-remove-hold

2016-06-29 Thread Jan Cholasta 

On 29.6.2016 10:47, Fraser Tweedale wrote:

On Wed, Jun 29, 2016 at 10:04:05AM +0200, Jan Cholasta wrote:

Hi,

On 29.6.2016 06:11, Fraser Tweedale wrote:

Dear team,

The attached patch implements the --ca option for the rest of the
cert-blah commands (https://fedorahosted.org/freeipa/ticket/5999).


1) I don't think cert-status should be treated specially. The operation to
check status of a certificate request is not specific to Dogtag.


I'm happy to add the option, with the caveat that because (of top of
head) there is not (yet) a way in Dogtag to distinguish/filter
requests by target CA, value may go unused.


IMO that's OK, since it's a safe non-descructive operation.





2) cert-show is called twice in cert-revoke. Can we call it just once?


I'll address this in next patchset.


OK.



Thanks for reviewing!
Fraser




--
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] 0081 Add --ca option to cert-revoke and cert-remove-hold

2016-06-29 Thread Fraser Tweedale 
On Wed, Jun 29, 2016 at 10:04:05AM +0200, Jan Cholasta wrote:
> Hi,
> 
> On 29.6.2016 06:11, Fraser Tweedale wrote:
> > Dear team,
> > 
> > The attached patch implements the --ca option for the rest of the
> > cert-blah commands (https://fedorahosted.org/freeipa/ticket/5999).
> 
> 1) I don't think cert-status should be treated specially. The operation to
> check status of a certificate request is not specific to Dogtag.
> 
I'm happy to add the option, with the caveat that because (of top of
head) there is not (yet) a way in Dogtag to distinguish/filter
requests by target CA, value may go unused.

> 
> 2) cert-show is called twice in cert-revoke. Can we call it just once?
> 
I'll address this in next patchset.

Thanks for reviewing!
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] 0081 Add --ca option to cert-revoke and cert-remove-hold

2016-06-29 Thread Jan Cholasta 

Hi,

On 29.6.2016 06:11, Fraser Tweedale wrote:

Dear team,

The attached patch implements the --ca option for the rest of the
cert-blah commands (https://fedorahosted.org/freeipa/ticket/5999).


1) I don't think cert-status should be treated specially. The operation 
to check status of a certificate request is not specific to Dogtag.



2) cert-show is called twice in cert-revoke. Can we call it just 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

[Freeipa-devel] [PATCH] 0081 Add --ca option to cert-revoke and cert-remove-hold

2016-06-28 Thread Fraser Tweedale 
Dear team,

The attached patch implements the --ca option for the rest of the
cert-blah commands (https://fedorahosted.org/freeipa/ticket/5999).

Thanks,
Fraser
From 668b826d94237d33e34605a5517b40c17de36780 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Wed, 29 Jun 2016 13:57:53 +1000
Subject: [PATCH] Add --ca option to cert-revoke and cert-remove-hold

Implement the --ca option for cert-revoke and cert-remove-hold.
Defaults to the IPA CA.  Raise NotFound if the cert with the given
serial was not issued by the nominated CA.

Also default the --ca option of cert-show to the IPA CA.

Add commentary to cert-status to explain why it does not use the
--ca option.

Fixes: https://fedorahosted.org/freeipa/ticket/5999
---
 API.txt   | 10 ++
 ipaserver/plugins/cert.py | 47 ---
 2 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/API.txt b/API.txt
index 
76e58aeec4301577952f919b17a58b71c06a..77a67cbc7a0533cf13404abd3a7d27972e2d65cc
 100644
--- a/API.txt
+++ b/API.txt
@@ -760,8 +760,9 @@ output: ListOfEntries('result')
 output: Output('summary', type=[, ])
 output: Output('truncated', type=[])
 command: cert_remove_hold/1
-args: 1,1,1
+args: 1,2,1
 arg: Int('serial_number')
+option: Str('cacn?', autofill=True, cli_name='ca', default=u'ipa')
 option: Str('version?')
 output: Output('result')
 command: cert_request/1
@@ -769,7 +770,7 @@ args: 1,8,3
 arg: Str('csr', cli_name='csr_file')
 option: Flag('add', autofill=True, default=False)
 option: Flag('all', autofill=True, cli_name='all', default=False)
-option: Str('cacn?', cli_name='ca')
+option: Str('cacn?', autofill=True, cli_name='ca', default=u'ipa')
 option: Str('principal')
 option: Str('profile_id?')
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
@@ -779,8 +780,9 @@ output: Entry('result')
 output: Output('summary', type=[, ])
 output: PrimaryKey('value')
 command: cert_revoke/1
-args: 1,2,1
+args: 1,3,1
 arg: Int('serial_number')
+option: Str('cacn?', autofill=True, cli_name='ca', default=u'ipa')
 option: Int('revocation_reason', autofill=True, default=0)
 option: Str('version?')
 output: Output('result')
@@ -788,7 +790,7 @@ command: cert_show/1
 args: 1,6,3
 arg: Int('serial_number')
 option: Flag('all', autofill=True, cli_name='all', default=False)
-option: Str('cacn?', cli_name='ca')
+option: Str('cacn?', autofill=True, cli_name='ca', default=u'ipa')
 option: Flag('no_members', autofill=True, default=False)
 option: Str('out?')
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
diff --git a/ipaserver/plugins/cert.py b/ipaserver/plugins/cert.py
index 
564d582c77ef63e780604fd7fc55e6cc7889a351..5137a1e1cff7554b3aca8a6f830199f220fcaa2b
 100644
--- a/ipaserver/plugins/cert.py
+++ b/ipaserver/plugins/cert.py
@@ -321,6 +321,8 @@ class BaseCertMethod(Method):
 def get_options(self):
 yield Str('cacn?',
 cli_name='ca',
+default=IPA_CA_CN,
+autofill=True,
 query=True,
 label=_('Issuing CA'),
 doc=_('Name of issuing CA'),
@@ -408,7 +410,7 @@ class cert_request(Create, BaseCertMethod, VirtualCommand):
 # enforcement so that user gets better error message if
 # referencing nonexistant CA) and look up authority ID.
 #
-ca = kw.get('cacn', IPA_CA_CN)
+ca = kw['cacn']
 ca_id = api.Command.ca_show(ca)['result']['ipacaid'][0]
 
 """
@@ -624,6 +626,8 @@ class cert_status(Retrieve, BaseCertMethod, VirtualCommand):
 def get_options(self):
 for option in super(cert_status, self).get_options():
 if option.name == 'cacn':
+# Dogtag requests are uniquely identified by their
+# number; there is no need to distinguish by CA.
 continue
 yield option
 
@@ -734,10 +738,8 @@ class cert_show(Retrieve, CertMethod, VirtualCommand):
 raise acierr
 hostname = get_host_from_principal(bind_principal)
 
-issuer_dn = None
-if 'cacn' in options:
-ca_obj = api.Command.ca_show(options['cacn'])['result']
-issuer_dn = ca_obj['ipacasubjectdn'][0]
+ca_obj = api.Command.ca_show(options['cacn'])['result']
+issuer_dn = ca_obj['ipacasubjectdn'][0]
 
 # Dogtag lightweight CAs have shared serial number domain, so
 # we don't tell Dogtag the issuer (but we check the cert after).
@@ -745,7 +747,7 @@ class cert_show(Retrieve, CertMethod, VirtualCommand):
 result = self.Backend.ra.get_certificate(str(serial_number))
 cert = x509.load_certificate(result['certificate'])
 
-if issuer_dn is not None and DN(unicode(cert.issuer)) != DN(issuer_dn):
+if DN(unicode(cert.issuer)) != DN(issuer_dn):
 # DN of cert differs from what we requested
 raise errors.NotFound(
 reason=_("Certificate