Re: [Pki-devel] [PATCH] 0084..0086 Lightweight CA replication support

[Fraser Tweedale]

Thanks Ade.  Updated patch 0096 attached.  Comments inline.

On Wed, Apr 20, 2016 at 11:30:52AM -0400, Ade Lee wrote:
> Comments:
> 
> 95 - ack
> 
> 96 -
> 
> 1. You have made the return type of initSigUnit() to be boolean. 
>  Should you be checking the return value in init()?
> 
It is not needed to check it here; only when re-entering init from
the KeyReplicatorRunner thread.

> 2. In addInstanceToAuthorityKeyHosts(), you are still using only the
> hostname.  Should be host:port
> 
Good pickup.  Fixed in latest patch.

> 3. The logic in the KeyRetrieverRunner class looks OK to me, but I'd
> like cfu and/or jmagne to check it and make sure we are calling the
> right primitives to wrap/unwrap inside the cryptographic token.
> 
> Also I'd like them to confirm that this would wor for an HSM.
> Statements like the following make me question that:
>CryptoToken token = manager.getInternalKeyStorageToken()
> 
It won't work on HSM.  Can I get an HSM to test with? ;) I've filed
a ticket for HSM support[1].  FreeIPA does not yet support HSM[2] so
I think we can put it in 10.4 milestone (I've put it there for now).

[1] https://fedorahosted.org/pki/ticket/2292
[2] https://fedorahosted.org/freeipa/ticket/5608

> 4. Can you explain what happens if for some reason the script fails to
> retrieve the key?  Do we end up retrying later and if so, when?
> 
If the script fails to retrieve the key, it does not retry
automatically.  I filed a ticket[3] to implement retry with
backoff (this patchset is big enough already!) and put it in
10.3.1 milestone (that's up for discussion).

[3] https://fedorahosted.org/pki/ticket/2293

Right now, the following events cause authority reinitialisation,
entailing key retrieval if necessary:

- Dogtag is restarted
- LDAP disconnect-reconnect
- LDAP modification of authority replicated from another clone

> 97- ACK
> 
> 98 - ACK
>  
Thanks.  Any feedback on patch 0099?
From a256168d91c799d37e1e4f6e7af8dfb97b4340be Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Wed, 30 Mar 2016 12:38:24 +1100
Subject: [PATCH] Lightweight CAs: add key retrieval framework

Add the framework for key retrieval when a lightweight CA is missing
its signing key.  This includes all the bits for loading a
KeyRetriever implementation, initiating retrieval in a thread and
updating the record of which clones possess the key if retrieval was
successful.

It does not include a KeyRetriever implementation.  A subsequent
commit will provide this.

Part of: https://fedorahosted.org/pki/ticket/1625
---
 .../src/com/netscape/ca/CertificateAuthority.java  | 162 -
 base/ca/src/com/netscape/ca/KeyRetriever.java  |  56 +++
 .../src/netscape/security/pkcs/PKCS12Util.java |   3 +
 3 files changed, 215 insertions(+), 6 deletions(-)
 create mode 100644 base/ca/src/com/netscape/ca/KeyRetriever.java

diff --git a/base/ca/src/com/netscape/ca/CertificateAuthority.java 
b/base/ca/src/com/netscape/ca/CertificateAuthority.java
index 
37f1e95fc97f3d21ec6dc379962e27b42fb5b074..253c4bb323692b8e9fe8bd87e202d71afb810c67
 100644
--- a/base/ca/src/com/netscape/ca/CertificateAuthority.java
+++ b/base/ca/src/com/netscape/ca/CertificateAuthority.java
@@ -35,6 +35,7 @@ import java.security.cert.CertificateException;
 import java.security.cert.CertificateParsingException;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Date;
 import java.util.Enumeration;
@@ -62,8 +63,10 @@ import org.mozilla.jss.crypto.CryptoToken;
 import org.mozilla.jss.crypto.KeyPairAlgorithm;
 import org.mozilla.jss.crypto.KeyPairGenerator;
 import org.mozilla.jss.crypto.NoSuchItemOnTokenException;
+import org.mozilla.jss.crypto.PrivateKey;
 import org.mozilla.jss.crypto.SignatureAlgorithm;
 import org.mozilla.jss.crypto.TokenException;
+import org.mozilla.jss.crypto.X509Certificate;
 import org.mozilla.jss.pkix.cert.Extension;
 import org.mozilla.jss.pkix.primitive.Name;
 
@@ -205,6 +208,7 @@ public class CertificateAuthority
 protected AuthorityID authorityID = null;
 protected AuthorityID authorityParentID = null;
 protected String authorityDescription = null;
+protected Collection authorityKeyHosts = null;
 protected boolean authorityEnabled = true;
 private boolean hasKeys = false;
 private ECAException signingUnitException = null;
@@ -340,6 +344,7 @@ public class CertificateAuthority
 AuthorityID aid,
 AuthorityID parentAID,
 String signingKeyNickname,
+Collection authorityKeyHosts,
 String authorityDescription,
 boolean authorityEnabled
 ) throws EBaseException {
@@ -355,6 +360,7 @@ public class CertificateAuthority
 this.authorityDescription = authorityDescription;
 this.authorityEnabled = authorityEnabled;
 mNickname = signingKeyNickname;
+this.authorityKeyHosts = authorityKeyHosts;
  

[Pki-devel] Trac; add "Lightweight CAs" feature?

[Fraser Tweedale]

Hi all,

Could someone with the relevant permissions please add a
"Lightweight CAs" feature to the pki trac?  There's a substantial
quantity of outstanding tickets for this feature so it would be good
to have something more formal than the summary by which to group
them.

Thanks,
Fraser

___
Pki-devel mailing list
Pki-devel@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel

Re: [Pki-devel] [PATCH] 285 - 293 Patches for fine grained authz in the KRA

[Endi Sukma Dewata]

On 4/19/2016 9:47 PM, Ade Lee wrote:

Some comments inline, although most of this was discussed on #irc.

I have added two additional patches which are to be applied on top
of 258=293.

294:  This patch fixes the problems identified in this review.  In
particular:

Review comments addressed:
 1. when archiving or generating keys, realm is checked
 2. when no plugin is found for a realm, access is denied.
 3. rename mFoo to foo for new variables.
 4. add chaining of exceptions
 5. remove attributes from KeyArchivalRequest etc. when realm is
null
 6. Add more detail to denial in BasicGroupAuthz

295 - Adds the ability for authz plugins to support multiple realms.
 In particular, the authorize() command has been extended to allow
 the realm to be passed in, and the ACL plugins have been modified
 to account for the realm.

Please review,


ACK.

--
Endi S. Dewata

___
Pki-devel mailing list
Pki-devel@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel

Re: [Pki-devel] [PATCH] 0084..0086 Lightweight CA replication support

[Ade Lee]

Comments:

95 - ack

96 -

1. You have made the return type of initSigUnit() to be boolean. 
 Should you be checking the return value in init()?

2. In addInstanceToAuthorityKeyHosts(), you are still using only the
hostname.  Should be host:port

3. The logic in the KeyRetrieverRunner class looks OK to me, but I'd
like cfu and/or jmagne to check it and make sure we are calling the
right primitives to wrap/unwrap inside the cryptographic token.

Also I'd like them to confirm that this would wor for an HSM.
Statements like the following make me question that:
   CryptoToken token = manager.getInternalKeyStorageToken()

4. Can you explain what happens if for some reason the script fails to
retrieve the key?  Do we end up retrying later and if so, when?

97- ACK

98 - ACK
 

On Wed, 2016-04-20 at 16:15 +1000, Fraser Tweedale wrote:
> New version of 0097 attached (0097-4).  The only change is some
> minor improvements to the pki-ipa-retrieve-key Python program.
> 
> Cheers,
> Fraser
> 
> On Tue, Apr 19, 2016 at 07:32:16PM +1000, Fraser Tweedale wrote:
> > Both issues addressed in latest patchset.  Two new patches in the
> > mix; the order is:
> > 
> > 0095-4, 0098, 0099, 0096-4, 0097-3 (tip)
> > 
> > I also added another attribute to schema for the authority
> > certificate serial number.  It is not used in current code but I
> > have a hunch it may be needed for renewal, so I'm adding it now.
> > 
> > Thanks,
> > Fraser
> > 
> > On Thu, Apr 14, 2016 at 05:34:45PM -0400, Ade Lee wrote:
> > > Couple of points on 96/97.
> > > 
> > > 1. First off, I'm not sure you followed my concern about being
> > > able to
> > > distinguish between CA instances.
> > > 
> > > On an IPA system, this is not an issue because there is only one
> > > CA on
> > > the server.  In this case, I imagine there will be a well known
> > > directory which custodia would work with.
> > > 
> > > In general though, we have to imagine that someone could end up
> > > installing two different dogtag ca instances on the same server. 
> > >  CMS.getEEHost() would result in the same value (the hostname)
> > > for both
> > > CAs.  How does your helper program (or custodia) know which key
> > > to
> > > retrieve?
> > > 
> > > The way to distinguish Dogtag instances is host AND port.
> > > 
> > > 2.  So, we're very careful that the signing keys are never in
> > > memory in
> > > the server.  All accesses to the system certs are through JSS/NSS
> > > which
> > > essentially provides us handles to the keys.
> > > 
> > > Now, I see a case where we import PKCS12 data AND the password
> > > into
> > > memory, so that we can import it into NSS?  Say it ain't so ..
> > > 
> > > With custodia, we have a secure mechanism of transferring the
> > > keys from
> > > one server to another. It makes more sense to me to have the
> > > server
> > > kick off the custodia transfer and then have that process also
> > > import
> > > into the NSS db.  The server would then need to await status from
> > > the
> > > custodia/retriever process - and then initialize the signing unit
> > > from
> > > the NSS DB.  Or am I completely confused?
> > > 
> > > Ade
> > > 
> > > 
> > > 
> > > On Thu, 2016-04-14 at 16:35 -0400, Ade Lee wrote:
> > > > Still reviewing .. ACK on 87-95 (inclusive).
> > > > 
> > > > On Thu, 2016-04-14 at 16:18 +1000, Fraser Tweedale wrote:
> > > > > On Thu, Apr 14, 2016 at 09:04:31AM +1000, Fraser Tweedale
> > > > > wrote:
> > > > > > On Wed, Apr 13, 2016 at 05:26:44PM -0400, Ade Lee wrote:
> > > > > > > Still reviewing ..
> > > > > > > 
> > > > > > > See comment on 87.  ACK on 88,89,90,91,92,93, 94, 95.
> > > > > > > 
> > > > > > > Ade
> > > > > > > 
> > > > > > > On Mon, 2016-04-11 at 12:32 +1000, Fraser Tweedale wrote:
> > > > > > > > Thanks for review, Ade.  Comments to specific feedback
> > > > > > > > inline.
> > > > > > > > Rebased and updated patches attached.  The substantive
> > > > > > > > changes
> > > > > > > > are:
> > > > > > > > 
> > > > > > > > - KeyRetriever implementations are now required NOT to
> > > > > > > > import
> > > > > > > > the
> > > > > > > >   key themselves.  Instead the API is updated with
> > > > > > > >   KeyRetriever.retrieveKey returning a Result, which
> > > > > > > > contains
> > > > > > > > PKCS
> > > > > > > >   #12 data and password for same.
> > > > > > > > 
> > > > > > > > - KeyRetrieverRunner reads the Result and imports the
> > > > > > > > PKCS
> > > > > > > > #12
> > > > > > > > into
> > > > > > > >   NSSDB.
> > > > > > > > 
> > > > > > > > - Added new patch 0097 which provides the
> > > > > > > > IPACustodiaKeyRetriever
> > > > > > > >   and assoicated Python helper script.  It depends on
> > > > > > > > an
> > > > > > > > unmerged
> > > > > > > >   FreeIPA patch[1] as well as a particular principal
> > > > > > > > and
> > > > > > > > associated
> > > > > > > >   keytab and Custodia keys existing.  I'm working on
> > > > > > > > FreeIPA
> > > > > > > > updates
> > > > > > > >   to satisfy these requirements