Re: [Pki-devel] [PATCH] pki-core-issuerDN-encoding.patch

OK, all this sounds reasonable. If the common cases have been tested to work. ACK ----- Original Message ----- > From: "Christina Fu" <cfu(a)redhat.com&gt; > To: "John Magne" <jmagne(a)redhat.com&gt; > Cc: pki-devel(a)redhat.com > Sent: Friday, October 24, 2014 11:11:33 AM > Subject: Re: [Pki-devel] [PATCH] pki-core-issuerDN-encoding.patch > > Jack, > thanks for the review. Please see response below. > > On 10/24/2014 11:02 AM, John Magne wrote: >> Christina: >> >> Looks good and glad to hear its tested to work, which solves a hairy >> problem. >> >> Just a couple of questions about a few places in the code: >> >> >> 1. In diff --git a/base/ca/src/com/netscape/ca/CertificateAuthority.java >> b/base/ca/src/com/netscape/ca/CertificateAuthority.java >> >> We have this block: >> >> + String caSigningCertStr = caSigningCfg.getString("cert", ""); >> + if (caSigningCertStr.equals("")) { >> + CMS.debug("CertificateAuthority:initSigUnit: >> ca.signing.cert not found"); >> + } else { //ca cert found >> + CMS.debug("CertificateAuthority:initSigUnit: ca cert >> found"); >> + mCaCert = new X509CertImpl(CMS.AtoB(caSigningCertStr)); >> + // this ensures the isserDN and subjectDN have the same >> encoding >> + // as that of the CA signing cert >> + CMS.debug("CertificateAuthority: initSigUnit 1- setting >> mIssuerObj and mSubjectObj"); >> + mSubjectObj = mCaCert.getSubjectObj(); >> + // this mIssuerObj is the "issuerDN" obj for the certs >> this CA >> + // issues, NOT necessarily the isserDN obj of the CA >> signing cert >> + mIssuerObj = new >> CertificateIssuerName((X500Name)mSubjectObj.get(CertificateIssuerName.DN_NAME)); >> + } >> + >> >> Looks like you create a member variable mSubjectObj and an associated >> getter method. >> It seems that perhaps this is only used locally in this method to help >> create mIssuerObj, which is accessed later. >> Do we need this or did I miss something? > I decided to make it available though you are right that it is not > needed at this point. I just figured since it's there, I will make it > available for future references. >> Also, what is supposed to happen when caSigningCertStr == "" ?? >> >> Later on we have this in the same method: >> >> + mSubjectObj = mCaCert.getSubjectObj(); >> + if (mSubjectObj != null) { >> + // this ensures the isserDN and subjectDN have the same >> encoding >> + // as that of the CA signing cert >> + CMS.debug("CertificateAuthority: initSigUnit - setting >> mIssuerObj and mSubjectObj"); >> + // this mIssuerObj is the "issuerDN" obj for the certs >> this CA >> + // issues, NOT necessarily the isserDN obj of the CA >> signing cert >> + // unless the CA is self-signed >> + mIssuerObj = >> + new >> CertificateIssuerName((X500Name)mSubjectObj.get(CertificateIssuerName.DN_NAME)); >> + } >> >> Question about this and other similar NULL checks for mSubjectObj. Can this >> really be null if everything was set up >> during the initialization phase of the CertificateAuthority? >> >> Also here if it is NULL, what happens, or is the intent to just keep going? > The code took different passes during installation and startup. At very > early stage of the installation, the value is not expected to be there, > so we do want it to pass, and at later times, it will become available. > That is why we don't want to do anything if it's null. > I was being extra careful not to change the code path behavior and only > focus on getting the subject DN setup for later use. >> >> I was curious about those two since this appears to be pretty invasive to >> the system. >> >> >> thanks, >> jack >> >> >> ----- Original Message ----- >>> From: "Christina Fu" <cfu(a)redhat.com&gt; >>> To: pki-devel(a)redhat.com >>> Sent: Friday, October 24, 2014 9:25:22 AM >>> Subject: [Pki-devel] [PATCH] pki-core-issuerDN-encoding.patch >>> >>> Attached please find the fix for the following ticket: >>> >>> https://fedorahosted.org/pki/ticket/1190 CA: issuer DN encoding not >>> preserved at issuance with signing cert signed by an external CA >>> >>> thanks in advance for a review. >>> Christina >>> >>> >>> >>> _______________________________________________ >>> Pki-devel mailing list >>> Pki-devel(a)redhat.com >>> https://www.redhat.com/mailman/listinfo/pki-devel >