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

[Fraser Tweedale]

On Fri, Apr 22, 2016 at 07:50:06PM -0400, John Magne wrote:
> I took a look at the stuff alee asked for.
> 
> CFU even took a quick look when I asked her a couple of questions.
> She was unsure of something (as was I) and she would like to be able
> to take a closer look next week. I will give my quick thoughts.
> 
> 1. I agree that HSM support is not in the patch, seems fine to move that
> to a future ticket.
> 
> Here is one thing I was kind of worried about:
> This is the code that imports the archive of the desired private key.
> 
> 
> ublic static PrivateKey importPKIArchiveOptions(
> +CryptoToken token, PrivateKey unwrappingKey,
> +PublicKey pubkey, byte[] data)
> +throws InvalidBERException, Exception {
> +ByteArrayInputStream in = new ByteArrayInputStream(data);
> +PKIArchiveOptions options = (PKIArchiveOptions)
> +(new PKIArchiveOptions.Template()).decode(in);
> +EncryptedKey encKey = options.getEncryptedKey();
> +EncryptedValue encVal = encKey.getEncryptedValue();
> +AlgorithmIdentifier algId = encVal.getSymmAlg();
> +BIT_STRING encSymKey = encVal.getEncSymmKey();
> +BIT_STRING encPrivKey = encVal.getEncValue();
> 
> This the wrapper object that is build off of the caSigningUnit key gotten
> in the other patch, the RetrieverThread like this:
> 
> 
> 
>  PrivateKey unwrappingKey = hostCA.mSigningUnit.getPrivateKey();
> 
> 
> 
> The code below works fine if said key is RSA. I talked over with CFU and she 
> said there
> could be a chance this key is ECC for an ECC CA.
> 
> We both think the rest of the code in this routine is fine, except for 
> possibly that.
> She is also not even sure if JSS can support an ECC private key wrapper.
> 
> She requests you guys give her a day or two to look at it.
> 
> Except for the hsm issue, the code that calls this routine in the thread 
> seems fine too.
> 
> +
> +KeyWrapper wrapper = token.getKeyWrapper(KeyWrapAlgorithm.RSA);
> +wrapper.initUnwrap(unwrappingKey, null);
> 
> 
> 
> 
> 
> 
> +SymmetricKey sk = wrapper.unwrapSymmetric(
> +encSymKey.getBits(), SymmetricKey.Type.DES3, 0);
> +
> +ASN1Value v = algId.getParameters();
> +v = ((ANY) v).decodeWith(new OCTET_STRING.Template());
> +byte iv[] = ((OCTET_STRING) v).toByteArray();
> +IVParameterSpec ivps = new IVParameterSpec(iv);
> +
> +wrapper = token.getKeyWrapper(KeyWrapAlgorithm.DES3_CBC_PAD);
> +wrapper.initUnwrap(sk, ivps);
> +PrivateKey.Type keyType = pubkey.getAlgorithm().equals("EC")
> +? PrivateKey.Type.EC
> +: PrivateKey.Type.RSA;
> +return wrapper.unwrapPrivate(encPrivKey.getBits(), keyType, pubkey);
> +}
>
Pushed to master.

Christina, I know you were were/are very busy so thanks for spending
some time looking at these patches.  If you have any other questions
or concerns let me know ASAP.

24992c089b9b5088f4481fda3d01a907565b5121 Lightweight CAs: authority schema 
changes
dc8c21cc9a68968a2b1db87f9b21cf3afbdb966a Add method 
CryptoUtil.importPKIArchiveOptions
e21aadd5e14dbcda73c20f20e67b1bcc8d5b5bfc Add ca-authority-key-export command
94ee373d053b34e534fbb61826e586693a38c934 Lightweight CAs: add key retrieval 
framework
a2a4117dbc7e489cbb1964d6ce5f95b786a03fde Lightweight CAs: add 
IPACustodiaKeyRetriever

Cheers,
Fraser

___
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

[Fraser Tweedale]

On Fri, Apr 22, 2016 at 07:50:06PM -0400, John Magne wrote:
> I took a look at the stuff alee asked for.
> 
> CFU even took a quick look when I asked her a couple of questions.
> She was unsure of something (as was I) and she would like to be able
> to take a closer look next week. I will give my quick thoughts.
> 
> 1. I agree that HSM support is not in the patch, seems fine to move that
> to a future ticket.
> 
Thanks.  I filed ticket: https://fedorahosted.org/pki/ticket/2292

> Here is one thing I was kind of worried about:
> This is the code that imports the archive of the desired private key.
> 
> 
> ublic static PrivateKey importPKIArchiveOptions(
> +CryptoToken token, PrivateKey unwrappingKey,
> +PublicKey pubkey, byte[] data)
> +throws InvalidBERException, Exception {
> +ByteArrayInputStream in = new ByteArrayInputStream(data);
> +PKIArchiveOptions options = (PKIArchiveOptions)
> +(new PKIArchiveOptions.Template()).decode(in);
> +EncryptedKey encKey = options.getEncryptedKey();
> +EncryptedValue encVal = encKey.getEncryptedValue();
> +AlgorithmIdentifier algId = encVal.getSymmAlg();
> +BIT_STRING encSymKey = encVal.getEncSymmKey();
> +BIT_STRING encPrivKey = encVal.getEncValue();
> 
> This the wrapper object that is build off of the caSigningUnit key gotten
> in the other patch, the RetrieverThread like this:
> 
> 
> 
>  PrivateKey unwrappingKey = hostCA.mSigningUnit.getPrivateKey();
> 
> 
> 
> The code below works fine if said key is RSA. I talked over with CFU and she 
> said there
> could be a chance this key is ECC for an ECC CA.
> 
> We both think the rest of the code in this routine is fine, except for 
> possibly that.
> She is also not even sure if JSS can support an ECC private key wrapper.
> 
Yes, it is currently not supported in JSS (I'm unsure if NSS
supports it).  However, because the first release of Lightweight CAs
is to support FreeIPA sub-CAs feature, and FreeIPA does not yet
support EC CA, I don't think it's a show-stopper.  I filed a ticket
for key replication with non-RSA CA:

https://fedorahosted.org/pki/ticket/2291

> She requests you guys give her a day or two to look at it.
> 
No problem.  Thank you (and Christina) for the review.

Cheers,
Fraser

> Except for the hsm issue, the code that calls this routine in the thread 
> seems fine too.
> 
> +
> +KeyWrapper wrapper = token.getKeyWrapper(KeyWrapAlgorithm.RSA);
> +wrapper.initUnwrap(unwrappingKey, null);
> 
> 
> 
> 
> 
> 
> +SymmetricKey sk = wrapper.unwrapSymmetric(
> +encSymKey.getBits(), SymmetricKey.Type.DES3, 0);
> +
> +ASN1Value v = algId.getParameters();
> +v = ((ANY) v).decodeWith(new OCTET_STRING.Template());
> +byte iv[] = ((OCTET_STRING) v).toByteArray();
> +IVParameterSpec ivps = new IVParameterSpec(iv);
> +
> +wrapper = token.getKeyWrapper(KeyWrapAlgorithm.DES3_CBC_PAD);
> +wrapper.initUnwrap(sk, ivps);
> +PrivateKey.Type keyType = pubkey.getAlgorithm().equals("EC")
> +? PrivateKey.Type.EC
> +: PrivateKey.Type.RSA;
> +return wrapper.unwrapPrivate(encPrivKey.getBits(), keyType, pubkey);
> +    }
> 
> 
> ----- Original Message -
> > From: "Fraser Tweedale" <ftwee...@redhat.com>
> > To: "Ade Lee" <a...@redhat.com>
> > Cc: pki-devel@redhat.com
> > Sent: Wednesday, April 20, 2016 9:58:54 PM
> > Subject: Re: [Pki-devel] [PATCH] 0084..0086 Lightweight CA replication  
> > support
> > 
> > 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.
> >

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

[John Magne]

I took a look at the stuff alee asked for.

CFU even took a quick look when I asked her a couple of questions.
She was unsure of something (as was I) and she would like to be able
to take a closer look next week. I will give my quick thoughts.

1. I agree that HSM support is not in the patch, seems fine to move that
to a future ticket.

Here is one thing I was kind of worried about:
This is the code that imports the archive of the desired private key.


ublic static PrivateKey importPKIArchiveOptions(
+CryptoToken token, PrivateKey unwrappingKey,
+PublicKey pubkey, byte[] data)
+throws InvalidBERException, Exception {
+ByteArrayInputStream in = new ByteArrayInputStream(data);
+PKIArchiveOptions options = (PKIArchiveOptions)
+(new PKIArchiveOptions.Template()).decode(in);
+EncryptedKey encKey = options.getEncryptedKey();
+EncryptedValue encVal = encKey.getEncryptedValue();
+AlgorithmIdentifier algId = encVal.getSymmAlg();
+BIT_STRING encSymKey = encVal.getEncSymmKey();
+BIT_STRING encPrivKey = encVal.getEncValue();

This the wrapper object that is build off of the caSigningUnit key gotten
in the other patch, the RetrieverThread like this:



 PrivateKey unwrappingKey = hostCA.mSigningUnit.getPrivateKey();



The code below works fine if said key is RSA. I talked over with CFU and she 
said there
could be a chance this key is ECC for an ECC CA.

We both think the rest of the code in this routine is fine, except for possibly 
that.
She is also not even sure if JSS can support an ECC private key wrapper.

She requests you guys give her a day or two to look at it.

Except for the hsm issue, the code that calls this routine in the thread seems 
fine too.

+
+KeyWrapper wrapper = token.getKeyWrapper(KeyWrapAlgorithm.RSA);
+wrapper.initUnwrap(unwrappingKey, null);






+SymmetricKey sk = wrapper.unwrapSymmetric(
+encSymKey.getBits(), SymmetricKey.Type.DES3, 0);
+
+ASN1Value v = algId.getParameters();
+v = ((ANY) v).decodeWith(new OCTET_STRING.Template());
+byte iv[] = ((OCTET_STRING) v).toByteArray();
+IVParameterSpec ivps = new IVParameterSpec(iv);
+
+wrapper = token.getKeyWrapper(KeyWrapAlgorithm.DES3_CBC_PAD);
+wrapper.initUnwrap(sk, ivps);
+PrivateKey.Type keyType = pubkey.getAlgorithm().equals("EC")
+? PrivateKey.Type.EC
+: PrivateKey.Type.RSA;
+return wrapper.unwrapPrivate(encPrivKey.getBits(), keyType, pubkey);
+}


- Original Message -
> From: "Fraser Tweedale" <ftwee...@redhat.com>
> To: "Ade Lee" <a...@redhat.com>
> Cc: pki-devel@redhat.com
> Sent: Wednesday, April 20, 2016 9:58:54 PM
> Subject: Re: [Pki-devel] [PATCH] 0084..0086 Lightweight CA replication
> support
> 
> 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:
> 
> - Dog

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

[Ade Lee]

ACK on latest 96 and 99.

I will ask  cfu or jmagne to look at the KeyRetrieveRunner logic today.

Ade

On Thu, 2016-04-21 at 14:58 +1000, Fraser Tweedale wrote:
> 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?

___
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

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

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 

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

[Fraser Tweedale]

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 automatically on install or
> > > > > > upgrade
> > > > > >   but if you want to test this patch LMK and I'll provide
> > > > > > detailed
> > > > > >   instructions.
> > > > > > 
> > > > > >   [1] 
> > > > > > https://www.redhat.com/archives/freeipa-devel/2016-April/msg0
> > > > > > 00
> > > > > > 55.html
> > > > > > 
> > > > > > Other comments inline.
> > > > > > 
> > > > > > Cheers,
> > > > > > Fraser
> > > > > > 
> > > > > > On Fri, Apr 08, 2016 at 11:16:19AM -0400, Ade Lee wrote:
> > > > > > > 
> > > > > > > 0087
> > > > > > > 
> > > > > > > 1. In SigningUnit.java -- you catch an ObjectNotFound
> > > > > > > exception and
> > > > > > > rethrow that as a CAMissingKey exception.  Is that the only
> > > > > > > way the
> > > > > > > ObjectNotFound exception can be thrown?  What if the key is
> > > > > > > present
> > > > > > > but
> > > > > > > the cert is not?  Can we refactor here to ensure that the
> > > > > > > correct
> > > > > > > exception is thrown?
> > > > > > > 
> > > > > > One can't get additional info out of ObjectNotFound without
> > > > > > inspecting the String message, which I'm not comfortable
> > > > > > doing.
> > > > > >   The
> > > > > > key retrieval system should import key and cert at same time
> > > > > > so
> > > > > > I've
> > > > > > renamed the exception to CAMissingKeyOrCert for clarity.
> > > > > > 
> > > > > 
> > > > > Well, you can always nest exceptions like so :
> > > > > 
> > > > >   mToken.login(cb); // ONE_TIME by default.
> > > > > 
> > > > > try {
> > > > > mCert = 

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

[Fraser Tweedale]

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?
> 
Because it is running in IPA deployment, it contacts the Custodia
instance at https:///ipa/keys/... .  Note that this is
IPACustodiaKeyRetriever which is design for this purpose (and no
other).  Different setups will use a different KeyRetriever
implementation.  It is conceivable that IPACustodiaKeyRetriever
could be generalised in future.


> The way to distinguish Dogtag instances is host AND port.
> 
Re multiple Dogtag instances, uh yes I overlooked this.  For the IPA
use case it doesn't matter but I will update the code to write
`CMS.getEEHost() + ":" CMS.getEEPort()` into the authority entry.
Alternative KeyRetriever implementations can use this to
distinguish between instances.

(Or do you think separate attributes for host and port would be
better?  Not much work either way.)

> 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?
> 
In the original implementation, Custodia put the key directly into
the NSSDB.  Unfortunately, Dogtag could not observe the key unless
restarted (highly undesirable).  I did not deeply investigate why (I
guess some sort of caching or locking) - but I did not find a
workaround.  Even logging out and back into the Token did not help
(and caused other issues, like dropped or failed TLS connections in
other threads).

So I reluctantly redesigned it to what you see now, which works
- but I did not see the (obvious, in hindsight) problem.

Let's chat on IRC about it.  I will probably need the help of
NSS/JSS gurus to make the former approach work.

Cheers,
Fraser

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

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 automatically on install or
> > > > > upgrade
> > > > >   but if you want to test this patch LMK and I'll provide
> > > > > detailed
> > > > >   instructions.
> > > > > 
> > > > >   [1] 
> > > > > https://www.redhat.com/archives/freeipa-devel/2016-April/msg0
> > > > > 00
> > > > > 55.html
> > > > > 
> > > > > Other comments inline.
> > > > > 
> > > > > Cheers,
> > > > > Fraser
> > > > > 
> > > > > On Fri, Apr 08, 2016 at 11:16:19AM -0400, Ade Lee wrote:
> > > > > > 
> > > > > > 0087
> > > > > > 
> > > > > > 1. In SigningUnit.java -- you catch an ObjectNotFound
> > > > > > exception and
> > > > > > rethrow that as a CAMissingKey exception.  Is that the only
> > > > > > way the
> > > > > > ObjectNotFound exception can be thrown?  What if the key is
> > > > > > present
> > > > > > but
> > > > > > the cert is not?  Can we refactor here to ensure that the
> > > > > > correct
> > > > > > exception is thrown?
> > > > > > 
> > > > > One can't get additional info out of ObjectNotFound without
> > > > > inspecting the String message, which I'm not comfortable
> > > > > doing.
> > > > >   The
> > > > > key retrieval system should import key and cert at same time
> > > > > so
> > > > > I've
> > > > > renamed the exception to CAMissingKeyOrCert for clarity.
> > > > > 
> > > > 
> > > > Well, you can always nest exceptions like so :
> > > > 
> > > > mToken.login(cb); // ONE_TIME by default.
> > > > 
> > > > try {
> > > > mCert = mManager.findCertByNickname(mNickname);
> > > > CMS.debug("Found cert by nickname: '" +
> > > > mNickname
> > > > + "' with serial number: " + mCert.getSerialNumber());
> > > > 
> > > > mCertImpl = new
> > > > X509CertImpl(mCert.getEncoded());
> > > > CMS.debug("converted to x509CertImpl");
> > > > } catch (ObjectNotFoundException e) {
> > > > throw new CAMissingCertException();
> > > > }
> > > > 
> > > > try {
> > > > mPrivk = mManager.findPrivKeyByCert(mCert);
> > > > CMS.debug("Got private key from cert");
> > > >   

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

[Fraser Tweedale]

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 automatically on install or upgrade
> > >   but if you want to test this patch LMK and I'll provide detailed
> > >   instructions.
> > > 
> > >   [1] https://www.redhat.com/archives/freeipa-devel/2016-April/msg000
> > > 55.html
> > > 
> > > Other comments inline.
> > > 
> > > Cheers,
> > > Fraser
> > > 
> > > On Fri, Apr 08, 2016 at 11:16:19AM -0400, Ade Lee wrote:
> > > > 
> > > > 0087
> > > > 
> > > > 1. In SigningUnit.java -- you catch an ObjectNotFound exception and
> > > > rethrow that as a CAMissingKey exception.  Is that the only way the
> > > > ObjectNotFound exception can be thrown?  What if the key is present
> > > > but
> > > > the cert is not?  Can we refactor here to ensure that the correct
> > > > exception is thrown?
> > > > 
> > > One can't get additional info out of ObjectNotFound without
> > > inspecting the String message, which I'm not comfortable doing.  The
> > > key retrieval system should import key and cert at same time so I've
> > > renamed the exception to CAMissingKeyOrCert for clarity.
> > > 
> > 
> > Well, you can always nest exceptions like so :
> > 
> > mToken.login(cb); // ONE_TIME by default.
> > 
> > try {
> > mCert = mManager.findCertByNickname(mNickname);
> > CMS.debug("Found cert by nickname: '" + mNickname + "' with 
> > serial number: " + mCert.getSerialNumber());
> > 
> > mCertImpl = new X509CertImpl(mCert.getEncoded());
> > CMS.debug("converted to x509CertImpl");
> > } catch (ObjectNotFoundException e) {
> > throw new CAMissingCertException();
> > }
> > 
> > try {
> > mPrivk = mManager.findPrivKeyByCert(mCert);
> > CMS.debug("Got private key from cert");
> > } catch (ObjectNotFoundException e) {
> >throw new CAMissingKeyException();
> > }
> > 
> > 
> > The only reason that I suggest this is that I could imagine this kind
> > of differentiation being useful in debugging failed custodia
> > replications.  If you think otherwise, I'm prepare to be convinced
> > otherwise.
> > 
> I think a scenario where we get key but not cert, or vice versa, is
> unlikely (Custodia gives us a PKCS #12 file with both).  However,
> your suggestion should work and it is a relatively small change.
> I'll cut a new patchset with this change today, along with the
> rebase.
> 
Updated and rebased patches attached.

The suggested changes were made to 0087.  This also resulted in
changes to patch 0094 (indicate when CA does not yet have keys).

No substantive changes to any other patches.

Cheers,
Fraser
From 6d72a9c7fc067df42a3259fc5ea87b65e94f76ad Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Thu, 31 Mar 2016 12:46:03 +1100
Subject: [PATCH 87/96] Lightweight CAs: add exceptions for missing signing key
 or cert

Add the CAMissingCertException and CAMissingKeyException classes and
throw when signing unit initialisation fails due to a missing
object.  In CertificateAuthority, store the exception if it occurs
for possible re-throwing later.  Also add the private 'hasKeys'
field for internal use.

Part of: https://fedorahosted.org/pki/ticket/1625
---
 .../src/com/netscape/ca/CertificateAuthority.java  | 14 +-
 base/ca/src/com/netscape/ca/SigningUnit.java   | 22 --
 .../certsrv/ca/CAMissingCertException.java | 15 +++
 .../netscape/certsrv/ca/CAMissingKeyException.java | 15 +++
 4 files changed, 59 insertions(+), 7 deletions(-)
 create mode 100644 
base/common/src/com/netscape/certsrv/ca/CAMissingCertException.java
 create mode 100644 
base/common/src/com/netscape/certsrv/ca/CAMissingKeyException.java

diff --git a/base/ca/src/com/netscape/ca/CertificateAuthority.java 

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

[Fraser Tweedale]

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 automatically on install or upgrade
> >   but if you want to test this patch LMK and I'll provide detailed
> >   instructions.
> > 
> >   [1] https://www.redhat.com/archives/freeipa-devel/2016-April/msg000
> > 55.html
> > 
> > Other comments inline.
> > 
> > Cheers,
> > Fraser
> > 
> > On Fri, Apr 08, 2016 at 11:16:19AM -0400, Ade Lee wrote:
> > > 
> > > 0087
> > > 
> > > 1. In SigningUnit.java -- you catch an ObjectNotFound exception and
> > > rethrow that as a CAMissingKey exception.  Is that the only way the
> > > ObjectNotFound exception can be thrown?  What if the key is present
> > > but
> > > the cert is not?  Can we refactor here to ensure that the correct
> > > exception is thrown?
> > > 
> > One can't get additional info out of ObjectNotFound without
> > inspecting the String message, which I'm not comfortable doing.  The
> > key retrieval system should import key and cert at same time so I've
> > renamed the exception to CAMissingKeyOrCert for clarity.
> > 
> 
> Well, you can always nest exceptions like so :
> 
>   mToken.login(cb); // ONE_TIME by default.
> 
> try {
> mCert = mManager.findCertByNickname(mNickname);
> CMS.debug("Found cert by nickname: '" + mNickname + "' with 
> serial number: " + mCert.getSerialNumber());
> 
> mCertImpl = new X509CertImpl(mCert.getEncoded());
> CMS.debug("converted to x509CertImpl");
> } catch (ObjectNotFoundException e) {
> throw new CAMissingCertException();
> }
> 
> try {
> mPrivk = mManager.findPrivKeyByCert(mCert);
> CMS.debug("Got private key from cert");
> } catch (ObjectNotFoundException e) {
>throw new CAMissingKeyException();
> }
> 
> 
> The only reason that I suggest this is that I could imagine this kind
> of differentiation being useful in debugging failed custodia
> replications.  If you think otherwise, I'm prepare to be convinced
> otherwise.
> 
I think a scenario where we get key but not cert, or vice versa, is
unlikely (Custodia gives us a PKCS #12 file with both).  However,
your suggestion should work and it is a relatively small change.
I'll cut a new patchset with this change today, along with the
rebase.

Cheers,
Fraser

> > > 0088:
> > > 
> > > 2. What does dbFactory.reset() do and does it need to be called in
> > > a
> > > cleanup routine somewhere?  Are we leaking resources?
> > > 
> > > Answered I think on IRC.  It just terminates any current
> > > connections -
> > > but do we need to call it on CA shutdown?
> > > 
> > dbFactory.reset() is already called in the shutdown() method.  (Only
> > the host authority calls it).
> > 
> > > 0089:  ACK
> > > 
> > > 0090:  ACK
> > > 
> > > 0091: ACK (with proviso below)
> > > 
> > > 3. Not super-crazy about the names of the methods
> > > commitAuthority(),
> > > commitModifyAuthority and deleteAuthorityEntry().  They are not
> > > very
> > > consistent.  I would suggest addAuthorityEntry(),
> > > modifyAuthorityEntry() and deleteAuthorityEntry() instead.
> > > 
> > Done.
> > 
> > > 0092: ACK (with following proviso)
> > > 
> > > 4. Talking with Nathan about this, he suggested that syncrepl is
> > > then
> > > more modern and preferred method to perform persistent searches. 
> > >  In
> > > fact, I see IPA tickets to replace persistent searches with
> > > syncrepl
> > > instead.
> > > 
> > > We could replace the persistent search with a separate follow-on
> > > patch
> > > if you prefer, or just do it now.
> > > 
> > Syncrepl is not supported by ldapjdk (iirc).  If/when it is, and if
> > syncrepl provides a tangible advantage over persistent search in our
> > use case (where it can be assumed that disconnections from DS are
> > infrequent and brief, and full refresh of local view is tolerable),
> > then I am happy to change it - in a separate commit (because
> > LDAPProfileSubsystem is also 

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

[Ade Lee]

96-2 looks like it does not apply.  Please rebase .

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 automatically on install or upgrade
>   but if you want to test this patch LMK and I'll provide detailed
>   instructions.
> 
>   [1] https://www.redhat.com/archives/freeipa-devel/2016-April/msg000
> 55.html
> 
> Other comments inline.
> 
> Cheers,
> Fraser
> 
> On Fri, Apr 08, 2016 at 11:16:19AM -0400, Ade Lee wrote:
> > 
> > 0087
> > 
> > 1. In SigningUnit.java -- you catch an ObjectNotFound exception and
> > rethrow that as a CAMissingKey exception.  Is that the only way the
> > ObjectNotFound exception can be thrown?  What if the key is present
> > but
> > the cert is not?  Can we refactor here to ensure that the correct
> > exception is thrown?
> > 
> One can't get additional info out of ObjectNotFound without
> inspecting the String message, which I'm not comfortable doing.  The
> key retrieval system should import key and cert at same time so I've
> renamed the exception to CAMissingKeyOrCert for clarity.
> 
> > 0088:
> > 
> > 2. What does dbFactory.reset() do and does it need to be called in
> > a
> > cleanup routine somewhere?  Are we leaking resources?
> > 
> > Answered I think on IRC.  It just terminates any current
> > connections -
> > but do we need to call it on CA shutdown?
> > 
> dbFactory.reset() is already called in the shutdown() method.  (Only
> the host authority calls it).
> 
> > 0089:  ACK
> > 
> > 0090:  ACK
> > 
> > 0091: ACK (with proviso below)
> > 
> > 3. Not super-crazy about the names of the methods
> > commitAuthority(),
> > commitModifyAuthority and deleteAuthorityEntry().  They are not
> > very
> > consistent.  I would suggest addAuthorityEntry(),
> > modifyAuthorityEntry() and deleteAuthorityEntry() instead.
> > 
> Done.
> 
> > 0092: ACK (with following proviso)
> > 
> > 4. Talking with Nathan about this, he suggested that syncrepl is
> > then
> > more modern and preferred method to perform persistent searches. 
> >  In
> > fact, I see IPA tickets to replace persistent searches with
> > syncrepl
> > instead.
> > 
> > We could replace the persistent search with a separate follow-on
> > patch
> > if you prefer, or just do it now.
> > 
> Syncrepl is not supported by ldapjdk (iirc).  If/when it is, and if
> syncrepl provides a tangible advantage over persistent search in our
> use case (where it can be assumed that disconnections from DS are
> infrequent and brief, and full refresh of local view is tolerable),
> then I am happy to change it - in a separate commit (because
> LDAPProfileSubsystem is also affected).
> 
> > 0093 : ACK
> > 
> > 0094: ACK
> > 
> > 0095: ACK
> > 
> > 0096: Looks good in general.
> > 
> > 5. One thing to keep in mind though.  It is perfectly possible to
> > have
> > more than one dogtag instance on a host.  What determines the
> > uniqueness of the instance is the host:port.
> > 
> Noted.  KeyRetriever implementations can access instance info via
> the `CMS' class.
> 
> Cheers,
> Fraser

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

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 automatically on install or upgrade
>   but if you want to test this patch LMK and I'll provide detailed
>   instructions.
> 
>   [1] https://www.redhat.com/archives/freeipa-devel/2016-April/msg000
> 55.html
> 
> Other comments inline.
> 
> Cheers,
> Fraser
> 
> On Fri, Apr 08, 2016 at 11:16:19AM -0400, Ade Lee wrote:
> > 
> > 0087
> > 
> > 1. In SigningUnit.java -- you catch an ObjectNotFound exception and
> > rethrow that as a CAMissingKey exception.  Is that the only way the
> > ObjectNotFound exception can be thrown?  What if the key is present
> > but
> > the cert is not?  Can we refactor here to ensure that the correct
> > exception is thrown?
> > 
> One can't get additional info out of ObjectNotFound without
> inspecting the String message, which I'm not comfortable doing.  The
> key retrieval system should import key and cert at same time so I've
> renamed the exception to CAMissingKeyOrCert for clarity.
> 

Well, you can always nest exceptions like so :

mToken.login(cb); // ONE_TIME by default.

try {
mCert = mManager.findCertByNickname(mNickname);
CMS.debug("Found cert by nickname: '" + mNickname + "' with 
serial number: " + mCert.getSerialNumber());

mCertImpl = new X509CertImpl(mCert.getEncoded());
CMS.debug("converted to x509CertImpl");
} catch (ObjectNotFoundException e) {
throw new CAMissingCertException();
}

try {
mPrivk = mManager.findPrivKeyByCert(mCert);
CMS.debug("Got private key from cert");
} catch (ObjectNotFoundException e) {
   throw new CAMissingKeyException();
}


The only reason that I suggest this is that I could imagine this kind
of differentiation being useful in debugging failed custodia
replications.  If you think otherwise, I'm prepare to be convinced
otherwise.

> > 0088:
> > 
> > 2. What does dbFactory.reset() do and does it need to be called in
> > a
> > cleanup routine somewhere?  Are we leaking resources?
> > 
> > Answered I think on IRC.  It just terminates any current
> > connections -
> > but do we need to call it on CA shutdown?
> > 
> dbFactory.reset() is already called in the shutdown() method.  (Only
> the host authority calls it).
> 
> > 0089:  ACK
> > 
> > 0090:  ACK
> > 
> > 0091: ACK (with proviso below)
> > 
> > 3. Not super-crazy about the names of the methods
> > commitAuthority(),
> > commitModifyAuthority and deleteAuthorityEntry().  They are not
> > very
> > consistent.  I would suggest addAuthorityEntry(),
> > modifyAuthorityEntry() and deleteAuthorityEntry() instead.
> > 
> Done.
> 
> > 0092: ACK (with following proviso)
> > 
> > 4. Talking with Nathan about this, he suggested that syncrepl is
> > then
> > more modern and preferred method to perform persistent searches. 
> >  In
> > fact, I see IPA tickets to replace persistent searches with
> > syncrepl
> > instead.
> > 
> > We could replace the persistent search with a separate follow-on
> > patch
> > if you prefer, or just do it now.
> > 
> Syncrepl is not supported by ldapjdk (iirc).  If/when it is, and if
> syncrepl provides a tangible advantage over persistent search in our
> use case (where it can be assumed that disconnections from DS are
> infrequent and brief, and full refresh of local view is tolerable),
> then I am happy to change it - in a separate commit (because
> LDAPProfileSubsystem is also affected).
> 
> > 0093 : ACK
> > 
> > 0094: ACK
> > 
> > 0095: ACK
> > 
> > 0096: Looks good in general.
> > 
> > 5. One thing to keep in mind though.  It is perfectly possible to
> > have
> > more than one dogtag instance on a host.  What determines the
> > uniqueness of the instance is the host:port.
> > 
> Noted.  KeyRetriever implementations can access instance info via
> the `CMS' class.
> 
> Cheers,
> Fraser

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

A few comments.

1. One of the first things that struck me as odd was making
CertificateAuthority implement Runnable.  I think it would be cleaner
to have a static inner class called AuthorityMonitor or similar to
which we pass in the CertificateAuthority.

2. I do like the fact that the caMap updates are done through a static
database connection factory created by the hostCA.  How do you ensure
that the database connection factory is created before being used by
other CAs? 

3.  I'm not sure I understand how the initialLoadDone counter is
supposed to work.  Are all the CA's supposed to stop until the hostCA
has completed its initial load?  Because it looks like only the hostCA
calls await().

4. There is a lot of code in that initial patch.  It would help review
to split that off into at least two patches, say one in which you add
the functions in CertificateAuthority that handle modifications in the
caMap based on persistent search results, and one which adds the new
monitor thread.

5. Some in-code documentation would not go amiss.  For instance, I have
no idea why this code is correct --

String[] objectClasses =
entry.getAttribute("objectClass").getStringValueArray();
if 
(Arrays.asList(objectClasses).contains("organizationalUnit")) {
initialNumAuthorities = new Integer(
entry.getAttribute("numSubordinates")
.getStringValueArray()[0]);
checkInitialLoadDone();
continue;
}
organizationalUnit?

There are lots of different variables like initialNumAuthorities etc. which 
could
potentially be hidden in an inner class, making this more understandable.
 
Ade

On Tue, 2016-03-22 at 16:00 +1000, Fraser Tweedale wrote:
> On Fri, Mar 18, 2016 at 02:30:24PM +1000, Fraser Tweedale wrote:
> > Hi all,
> > 
> > The attached patches implement replication support for lightweight
> > CAs.  These patches do not implement key replication via Custodia
> > (my next task) but they do implement the persistent search thread
> > and appropriate** API behaviour when the signing keys are not yet
> > available.
> > 
> > ** In most cases, we respond 503 Service Unavailable; this is open
> >for discussion.  ca-authority-find and ca-authority-show include
> >a boolean field indicating whether the CA is ready to sign.
> >There might be (probably are) endpoints I've missed.
> > 
> > Cheers,
> > Fraser
> > 
> Updated patches attached - small change in patch 0084 to fix a race
> condition when deleting an authority that can cause NPE.
> 
> Thanks,
> Fraser
> ___
> Pki-devel mailing list
> Pki-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel

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

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

[Fraser Tweedale]

Hi all,

The attached patches implement replication support for lightweight
CAs.  These patches do not implement key replication via Custodia
(my next task) but they do implement the persistent search thread
and appropriate** API behaviour when the signing keys are not yet
available.

** In most cases, we respond 503 Service Unavailable; this is open
   for discussion.  ca-authority-find and ca-authority-show include
   a boolean field indicating whether the CA is ready to sign.
   There might be (probably are) endpoints I've missed.

Cheers,
Fraser
From fae1f14095cba4a9a14486230f9b0d353dcf7513 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Wed, 9 Mar 2016 02:18:41 -0500
Subject: [PATCH 84/86] Lightweight CAs: monitor database for changes

Implement a thread that performs an LDAP persistent search to keep a
running CA's view of lightweight CAs in sync with the database.

Signing key replication is not yet supported; this will be
implemented in a later patch and will not use the database to
propagate keys.

Part of: https://fedorahosted.org/pki/ticket/1625
---
 .../src/com/netscape/ca/CertificateAuthority.java  | 689 ++---
 base/ca/src/com/netscape/ca/SigningUnit.java   |   3 +-
 .../netscape/certsrv/ca/CAMissingKeyException.java |  15 +
 3 files changed, 486 insertions(+), 221 deletions(-)
 create mode 100644 
base/common/src/com/netscape/certsrv/ca/CAMissingKeyException.java

diff --git a/base/ca/src/com/netscape/ca/CertificateAuthority.java 
b/base/ca/src/com/netscape/ca/CertificateAuthority.java
index 
63c7ca4e4a8083dc58b54196af89cc7629e9fd97..d8177e1708dca15ae97c5c01534215a64dbe34d0
 100644
--- a/base/ca/src/com/netscape/ca/CertificateAuthority.java
+++ b/base/ca/src/com/netscape/ca/CertificateAuthority.java
@@ -43,7 +43,9 @@ import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.TreeMap;
+import java.util.TreeSet;
 import java.util.Vector;
+import java.util.concurrent.CountDownLatch;
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpSession;
@@ -51,11 +53,18 @@ import javax.servlet.http.HttpSession;
 import netscape.ldap.LDAPAttribute;
 import netscape.ldap.LDAPAttributeSet;
 import netscape.ldap.LDAPConnection;
+import netscape.ldap.LDAPConstraints;
+import netscape.ldap.LDAPControl;
 import netscape.ldap.LDAPEntry;
 import netscape.ldap.LDAPException;
 import netscape.ldap.LDAPModification;
 import netscape.ldap.LDAPModificationSet;
+import netscape.ldap.LDAPSearchConstraints;
 import netscape.ldap.LDAPSearchResults;
+import netscape.ldap.controls.LDAPEntryChangeControl;
+import netscape.ldap.controls.LDAPPersistSearchControl;
+import netscape.ldap.util.DN;
+
 import netscape.security.pkcs.PKCS10;
 import netscape.security.util.DerOutputStream;
 import netscape.security.util.DerValue;
@@ -101,6 +110,7 @@ import com.netscape.certsrv.base.PKIException;
 import com.netscape.certsrv.ca.AuthorityID;
 import com.netscape.certsrv.ca.CADisabledException;
 import com.netscape.certsrv.ca.CAEnabledException;
+import com.netscape.certsrv.ca.CAMissingKeyException;
 import com.netscape.certsrv.ca.CANotFoundException;
 import com.netscape.certsrv.ca.CANotLeafException;
 import com.netscape.certsrv.ca.CATypeException;
@@ -150,6 +160,8 @@ import com.netscape.cmscore.request.RequestSubsystem;
 import com.netscape.cmscore.security.KeyCertUtil;
 import com.netscape.cmscore.util.Debug;
 import com.netscape.cmsutil.crypto.CryptoUtil;
+import com.netscape.cmsutil.ldap.LDAPPostReadControl;
+import com.netscape.cmsutil.ldap.LDAPUtil;
 import com.netscape.cmsutil.ocsp.BasicOCSPResponse;
 import com.netscape.cmsutil.ocsp.CertID;
 import com.netscape.cmsutil.ocsp.CertStatus;
@@ -176,11 +188,13 @@ import com.netscape.cmsutil.ocsp.UnknownInfo;
  * @author lhsiao
  * @version $Revision$, $Date$
  */
-public class CertificateAuthority implements ICertificateAuthority, 
ICertAuthority, IOCSPService {
+public class CertificateAuthority
+implements ICertificateAuthority, ICertAuthority, IOCSPService, 
Runnable {
 public static final String OFFICIAL_NAME = "Certificate Manager";
 
 public final static OBJECT_IDENTIFIER OCSP_NONCE = new 
OBJECT_IDENTIFIER("1.3.6.1.5.5.7.48.1.2");
 
+private static ILdapConnFactory dbFactory = null;
 private static final Map caMap =
 Collections.synchronizedSortedMap(new TreeMap());
 protected CertificateAuthority hostCA = null;
@@ -188,6 +202,7 @@ public class CertificateAuthority implements 
ICertificateAuthority, ICertAuthori
 protected AuthorityID authorityParentID = null;
 protected String authorityDescription = null;
 protected boolean authorityEnabled = true;
+private boolean hasKeys = false;
 
 protected ISubsystem mOwner = null;
 protected IConfigStore mConfig = null;
@@ -283,6 +298,19 @@ public class CertificateAuthority implements