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.
> > > 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
_______________________________________________
Pki-devel mailing list
Pki-devel@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
