Review LDAP domain name parsing

[cipherboy]

See also: https://pagure.io/jss/issue/16

Reproducing comments here:

@cipherboy said:

@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: e4cae03#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: e4cae03#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?

---

@jmagne said:

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.