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?

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?

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

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 >