@jmagne suggested possible replacement for ldapjdk classes:

• https://docs.oracle.com/javase/7/docs/api/javax/naming/ldap/LdapName.html
• https://docs.oracle.com/javase/7/docs/api/javax/naming/ldap/Rdn.html

Proposed patch to address:

Also: Here is simple function I used to test to see if the right track was taken:

private static void testLdapName() {

    String dn1 = "UID=123456,CN=Lee Coomber,OU=Development,O=LShift Ltd.,L=London,ST=England,C=UK";
    String dndup1 = "UID=123456,CN=Lee Coomber,OU=Development,O=LShift Ltd.,L=London,ST=England,C=UK";
    X500Principal pal1 = new X500Principal(dn1);
    try {
        LdapName certSubjdn  = new LdapName(pal1.toString());
        LdapName subjDn = new LdapName(dndup1);
        if(certSubjdn.equals(subjDn))
                System.out.println("equal!");
            else
                System.out.println("not equal!");
        if (LDAPDN.equals(pal1.toString(), dndup1))
            System.out.println("ldap sdk equal!");
        else
            System.out.println("ldap sdk not equal!");
        LdapName dn = new LdapName(dn1);
        List<Rdn> rdns =  dn.getRdns();
        ArrayList<String> values = new ArrayList<String>();
        // The getRdns method returns the list in reverse order
        // therefore, traverse in reverse order.
        for (int i = rdns.size() - 1; i >= 0; i--) {
            Rdn rdn = rdns.get(i);
            values.add(rdn.getValue().toString());
        }
        String joinedLdapName = StringUtils.join(values, " - ");
        System.out.println("joinedLdapName: " + joinedLdapName);
        DN dnLdapSdk = new DN(pal1.getName());
        String[] valuesLdapSdk = dnLdapSdk.explodeDN(true);
        String joinedLdapSdkName = StringUtils.join(valuesLdapSdk, " - ");
        System.out.println("joinedLdapSDK: " + joinedLdapSdkName);
    } catch (InvalidNameException e) {
        System.out.println("exception!");
    }

Just one thing, if InvalidNameException happens, instead of returning null (which indicates that there is no certificate with the specified subject DN), we should let the caller know that the specified subject DN is invalid. This can be done by rethrowing the exception:

} catch (InvalidNameException e) {
    throw new RuntimeException(e);
}

or by removing the try-catch block and declaring the exception:

public PKCS12CertInfo getCertBySubjectDN(PKCS12 pkcs12, String subjectDN)
        throws CertificateException, InvalidNameException {
}

Everything else is good.

Also, once the code is fixed, we should remove references to LDAPJDK in the classpath and RPM spec file.

I pushed the patch as is since I need to create a new build:

• https://github.com/dogtagpki/jss/commit/e4cae037372e3ec065c9ba0ca176aa7587b12c3c

Any remaining issues can be addressed later.

@edewata / @jmagne:

Reviewing this patch in JSS, I'm not quite sure the behavior is the same, though I lack a suitable test case to prove that their not.

I'm concerned in particular with this line: https://github.com/dogtagpki/jss/commit/e4cae037372e3ec065c9ba0ca176aa7587b12c3c#diff-847833f74cb9d5fa665928020df88f32R584

I'm not sure return null is the correct solution at this point. We're iterating over a set of PKCS12CertInfos and looking for a match. Under the old code, if a subjectDN failed to parse, we'd skip it (if they're unequal) and continue to the next one. Under the new code, we'd return null because parsing would (now) throw an exception.

See: https://github.com/dogtagpki/jss/commit/e4cae037372e3ec065c9ba0ca176aa7587b12c3c#diff-847833f74cb9d5fa665928020df88f32R584

The DN constructor here doesn't throw any exceptions and instead returns an empty object, so as long as one of the two can be parsed, if the other can't be, the result is unequal.

Under this code, if one of the two compared LdapNames can't be parsed, we raise an exception and return null. The key difference is that under the old system, a later PKCS12CertInfo could match (and thus could be parsed, etc.), making the result non-null.

OTOH, not sure if that's a likely use case to run into. Just my 2c. Thoughts?

I cranked out this thing quickly as response to the immediate issue. I think you are correct though. It is possible that one of the members of the list is bogus and would bomb out when the correct member has yet to be considered.

I think what might work would be to kind of separate the two constructors and evaluate them separately:

            LdapName certSubjdn  = new LdapName(certSubjectDN.toString());
            LdapName subjDn = new LdapName(subjectDN);

The first of those two is what is fished out out of the loop, with the second being a constant as far as I can remember. Correct if wrong.

There fore I would guess that failing subjDn might be call to abort, but a bad certSubdn, should just continue the loop. Perhaps the subjDn constructor can be made outside the loop, I can't see the whole code right now, so correct me if I"m wrong.

Also, if you like you could use that test code a put up and play with it to test the scenario.

Good catch though. Also, when we do decide to abort we can try out edewata's suggestion to throw an exception.

Strictly, the contents of this issue (circular dependency) are resolved. What remains is a discussion about subject DN parsing. I've filed https://github.com/dogtagpki/jss/issues/637 to be a placeholder.