X509ExtensionFactory generates incorrect extension for subjectAltName

[roadrunner2]

The extension generated for subjectAltName by X509ExtensionFactory is missing a sequence. Take this code snippet:

extensions = OpenSSL::X509::ExtensionFactory.new
ext = extensions.create_extension("subjectAltName", "email:[email protected],DNS:a.b.com")
File.open("/tmp/san.ext", "w") { |f| f.print(ext.to_der) }

The DER of this extension should look like (and does so under MRI)

   0 30   31: SEQUENCE {
   2 06    3:   OBJECT IDENTIFIER subjectAltName (2 5 29 17)
   7 04   24:   OCTET STRING, encapsulates {
   9 30   22:       SEQUENCE {
  11 81   11:         [1] '[email protected]'
  24 82    7:         [2] 'a.b.com'
            :         }
            :       }
            :   }

But the actual DER of the created extension under JRuby is

   0 30   32: SEQUENCE {
   2 06    3:   OBJECT IDENTIFIER subjectAltName (2 5 29 17)
   7 04   25:   OCTET STRING, encapsulates {
   9 81   23:       [1] '[email protected],DNS:a.b.com'
            :       }
            :   }

Note the missing sequence, and the fact that both values are in one string.

The core issues are that X509ExtensionFactory.parseSubjectAltName() returns a GeneralName instead of a GeneralNames (sequence of GeneralName), and that it fails to parse multiple names properly.

Due to the missing sequence, it's currently completely impossible to generate a (valid) certificate with a subject-alt-name extension.

Lastly, pull request #123 appears to be related to this.

[kares]

please give 0.9.21 a try a let us know if there's smt wrong. assuming its fixed here.

[duritong]

Unfortunately, the problem with #123 still persists, right? So the representation of SubjectAltNames is still flawed, though this problem might be hidden now...

[kares]

@duritong its a different issue (and that is why its still open) that would take more time which I did not have.
am grateful for any help as progress is otherwise low with only 1 person managing the project forward occasionally (also have other priorities atm when eventually I get work done on jossl)

[roadrunner2]

@kares Thanks for looking into this. While your change appears to fix one very special case, the parsing is still fairly broken if the string doesn't happen to list email first, or it contains just a URI, etc. E.g. all of these still fail:

• DNS:a.b.com,email:[email protected]
• URI:https://a.b.com/,DNS:a.b.com
• IP:1.2.3.4,email:[email protected],dirName:/O=Acme/CN=John Doe
• RID:1.3.6.1.3.100.200

IMO the code needs to look something like the following (untested and incomplete):

private static GeneralNames parseSubjectAltNames(final String valuex) throws IOException {
    final String[] vals = valuex.split("(?<!\\\\),");   // allow one level of escaping of ','
    final GeneralName[] names = new GeneralName[vals.length];
    for ( int i = 0; i < vals.length; i++ ) {
        parseSubjectAltName(vals[i]);
    }
    return new GeneralNames(names);
}

private static GeneralName parseSubjectAltName(final String valuex) throws IOException {
    if ( valuex.startsWith(DNS_) ) {
        final String dns = valuex.substring(DNS_.length());
        return new GeneralName(GeneralName.dNSName, dns);
    }
    if ( valuex.startsWith(DNS_Name_) ) {
        final String dns = valuex.substring(DNS_Name_.length());
        return new GeneralName(GeneralName.dNSName, dns);
    }
    if ( valuex.startsWith(URI_) ) {
        final String uri = valuex.substring(URI_.length());
        return new GeneralName(GeneralName.uniformResourceIdentifier, uri);
    }
    ...
}

[kares]

OK, we need a piece of test with that code in a PR. thanks.

[kares]

so if I take #140 it still doesn't fix the test presented at #123 ... any chance for getting it done properly once and for all? seems that the hide-and-seek game will go on for a while, or does #123 just no longer matter?

[duritong]

\o/ thank you!

[roadrunner2]

@kares Thank you for merging.

Regarding #123, I took a quick look at it, and the current failure boils down to the function X509Extension.newExtension(). Specifically the block spanning lines 144 through 158 looks completely bogus and should IMO just be removed - with that this test passes, but it breaks test test_resolve_extensions - however, that test is broken, because it assumes multiple extensions with the same OID (subjectAltName), something which is explicitly forbidden. I'll submit a pull request for that.

In short the #123 failure is now due to a different issue than what this pull request fixed.

But frankly, there are still a bunch of things in the X509Extensions.java code that look highly suspect, if not downright wrong.

[kares]

@roadrunner2 thanks you so much for looking, sounds like a plan - please fire up a PR if you get a chance.
I (also) only work on jossl on my free time and context switching (figuring out what to do from comments) is expensive for my single threaded mindset :)

[roadrunner2]

@kares See #144. And yes, I understand, we all have limited time.