Support otherName (Microsoft UPN) in subjectAltName

[tlhackque]

Description

Certificates such as the attached fail to parse the subjectAltName, and die.

In the triggering case, the Microsoft "User Principal Name", an "otherName" variant of "GeneralName" is not supported.

In fact, any otherName in the subjectAltName will fail to parse and die.

Expected behaviour

Certificate should parse subjectaltname and return the UPN string, as with the attached patch. otherNames should not cause subjectaltname to die.

The patch is written to easily allow for any future "otherName"s that turn up. They will parse, but depending on their content, may require ASN.1 definitions similar to the one included for the UPN OID. But at least they will no longer die, and any other names will be available..

Actual behaviour

subjectaltname() dies with "Unable to decode SubjectAltName"

Operating system and version

Not OS specfici.

Crypt::OpenSSL::X509 version

1.915

Perl version

5.8.8 and more recent (not Perl version-dependent)

OpenSSL version

Various, not OpenSSL version dependent.

Step by step guide to reproducing the issue

perl -MCrypt::OpenSSL::X509 -MData::Dumper \
        -e'print Dumper( Crypt::OpenSSL::X509->new_from_file("upn-cert.pem")->subjectaltname)'
Unable to decode SubjectAltName: decode error at /usr/lib/perl5/site_perl/5.8.8/Convert/ASN1/_decode.pm line 253.

With patch:

 perl -MCrypt::OpenSSL::X509 -MData::Dumper \
           -e'print Dumper( Crypt::OpenSSL::X509->new_from_file("upn-cert.pem")->subjectaltname)'
$VAR1 = [
          {
            'otherName' => {
                             'value' => {
                                          'microsoftUPN' => 'johnDoe@example.com'
                                        },
                             'type' => '1.3.6.1.4.1.311.20.2.3'
                           }
          }
        ];

Supporting files: (remove the .txt extensions when downloading) Crypt_OpenSSL_X509_subjectaltname.patch upn-cert.pem

[timlegge]

The referenced PR outputs:

$VAR1 = [
          {
            "otherName" => "microsoftUPN::johnDoe\@example.com"
          }
        ];

[tlhackque]

Why '::' vs. ':'? Also, what if some future otherName decides to have a more complex structure - e.g. a hierarchy? That's why I left the extra level of {}..

Note I also corrected ipAddress_txt to iPAddress_txt.

[timlegge]

Really I added the second : since openssl does:

openssl x509 -in certs/upn-cert.pem -text | grep othername
                othername: UPN::johnDoe@example.com

as for the parsing of the hash I did it only for the oid 1.3.6.1.4.1.311.20.2.3 which likely means I should have a else for something else

oid 1.3.6.1.4.1.311.20.2.3 is defined as a UPN containing the UPN value

[tlhackque]

Consistency is good. I didn't check openssl. So in that case, the "microsoft" should disappear so it outputs:

$VAR1 = [
          {
            "otherName" => "UPN::johnDoe\@example.com"
          }
        ];

[timlegge]

Yeah, I did think of that. It makes sense. Maybe review the changes I made to your patch in PR #118 it will help when @jonasbn reviews it

[tlhackque]

Will do. Thanks for the prompt action.

[tlhackque]

as for the parsing of the hash I did it only for the oid 1.3.6.1.4.1.311.20.2.3 which likely means I should have a else for something else

Oh, there's no 'else'. One would add another registeroid to decode other formats.

[timlegge]

as for the parsing of the hash I did it only for the oid 1.3.6.1.4.1.311.20.2.3 which likely means I should have a else for something else

Oh, there's no 'else'. One would add another registeroid to decode other formats.

Yeah, it looks like it works as is. I wonder if it makes sense to have a way to pass in an OID that could use the same parsing if the structure of the data is the same. Probably more than is required at this point

[tlhackque]

The problem is that we don't know what the structure of the data would be. One would likely have to pass in some asn.1 - and that's not something I'd want to expose.

Your special casing of this OID to make it look like openssl might be a unique case. But if there's more than one, I'd probably use a hash of 'special' oid => sub { formatter }. Or something like that. That way, the lookup for 'special' doesn't grow into a long if/else tree; it's one check.

[tlhackque]

I ran into one more in this family: RFC 4985 SRVName, which is another otherName.

openssl dump is similar, though the string encoding differs (It's an IA5String rather than UTF8):

        X509v3 extensions:
            X509v3 Subject Alternative Name:
                DNS:domain.org, othername: SRVName::_webapp.domain.org

Revised patch, test certificate attached. Many more of these and as I hinted yesterday: it would probably make sense to have an exceptions hash drive these decodes instead of the current code fragments. Something like like this should work:

# Define otherName exceptions & formatting
my %exceptions = ( 'o.i.d' => { asn1 => q( ...), format => sub {} }, ... );.
my @exceptions ;
push @exceptions, Convert::ASN1->new->prepare($exceptions{$_}->{asn1}) and $asn->registeroid( $_, $exceptions[-1])
    for( keys %exceptions); 
...
elsif ( $item eq 'otherName' ) {
  if( (my $xc = $exceptions{$otherName->{type}) ) {
    $name->{otherName} = $xc->{format($otherName->{value});
  }
}

But the attached updated patch will do for now.

srvname.pem Crypt_OpenSSL_X509_subjectaltnameSRVName.patch

[timlegge]

I added it to the PR - I will let @jonasbn make any decisions around whetehr to chec to the -txt names that you included in your patch. The feature is new enough and the only documented name was for rfc822Name that I would prefer a release with a Changelog that it could break your code if you have been parsing it your self. His call though...

Yes, if there is enough of these cases then something like you suggested might be preferable to avoid many special cases in the code.

[timlegge]

@tlhackque I have been looking at this again. I am thinking of adding a subjectaltname_as_text or subjectaltname_v2 and putting your _text back in. It could also just be _v2 and leave the original the same. Any thoughts...

[timlegge]

@jonasbn what is your thoughts on this? I would term the non-conversion of the binary fields to text as a bug in the initial implementation but @tlhackque may be one of many who have adjusted to it and the change would break backward compatibility