Skip to content

Draft: Fixes #116, Fixes #117 - revised patch and test certs from @tlhackque #118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
timlegge wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Draft: Fixes #116, Fixes #117 - revised patch and test certs from @tlhackque #118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
621a2dc
Fixes #116, Fixes #117 - revised patch and test certs from @tlhackque
timlegge May 19, 2024
c2ca103
Add SRVName support to the for the SAN Parsing
timlegge May 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion X509.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ sub Crypt::OpenSSL::X509::subjectaltname {
my $ok = $asn->prepare(q<
AnotherName ::= SEQUENCE {
type OBJECT IDENTIFIER,
value [0] EXPLICIT ANY } --DEFINED BY type-id }
value [0] EXPLICIT ANY DEFINED BY type }

EDIPartyName ::= SEQUENCE {
nameAssigner [0] DirectoryString OPTIONAL,
Expand Down Expand Up @@ -158,6 +158,7 @@ sub Crypt::OpenSSL::X509::subjectaltname {
GeneralNames ::= SEQUENCE OF GeneralName

GeneralName ::= CHOICE {
otherName [0] AnotherName,
rfc822Name [1] IA5String,
dNSName [2] IA5String,
x400Address [3] ANY, --ORAddress,
Expand All @@ -172,6 +173,16 @@ sub Crypt::OpenSSL::X509::subjectaltname {
die '*** Could not prepare definition: '.$asn->error()
if !$ok;

# Microsoft's User Principal Name (Smart Card Logon)
my $upn = Convert::ASN1->new or die( "New UPN" );
$upn->prepare(q(UPN UTF8String)) or die ( "Prepare UPN" );
$asn->registeroid( '1.3.6.1.4.1.311.20.2.3', $upn );

Copy link

@tlhackque tlhackque May 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch in the issue updated to add support for 1.3.6.1.5.5.7.8.7 ServiceName RFC 4985

# RFC 4985 Service name
my $srv = Convert::ASN1->new or die( "New SRV" );
$srv->prepare(q(SRVName IA5String)) or die ( "Prepare SRV" );
$asn->registeroid( '1.3.6.1.5.5.7.8.7', $srv );

# This is an important bit - if you don't do the find the decode
# will randomly fail/succeed. This is required to work
my $asn_node = $asn->find('SubjectAltName')
Expand All @@ -180,6 +191,31 @@ sub Crypt::OpenSSL::X509::subjectaltname {
my $san = $asn_node->decode($bin_data)
or die 'Unable to decode SubjectAltName: '.$asn_node->error;

foreach my $name ( @$san ) {
foreach my $item (keys %$name) {
if( $item eq 'iPAddress' ) {
my $ip = $name->{$item};
if( length $ip == 4 ) {
$name->{iPAddress} = sprintf( '%d.%d.%d.%d', unpack( 'C4', $ip ) );
} elsif( length $ip == 16 ) {
$name->{iPAddress} = sprintf( '%x:%x:%x:%x:%x:%x:%x:%x', unpack( 'n8', $ip ) );
} else {
Copy link

@tlhackque tlhackque May 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted separately: I don't think it's a good idea to replace the binary with text.

It would have been the right thing at day 1, but the binary has been there for some time. I, for one, have code that deals with it. So this is a breaking change. For me, no big deal, but we don't know what other code might be out there.

Adding the _txt key in my original patch ensured that it wouldn't break any existing code, but still provides a clean interface for humans (and for most other Perl libraries, that want text.) I used _txt so the key could be referenced without quoting it.

It seems like the cleanest solution given where we are today. It's consistent with the way everything else works - you pick the key that you want. And it doesn't break anything.

I don't see any advantage to adding an 'as_text' function - that would be one more step for the consumer.

Copy link
Contributor Author

@timlegge timlegge May 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I would be tempted to add a parameter that could be used with subjectaltname to specify whether to add the extra _txt fields or replace the fields with parsed values.

Copy link

@tlhackque tlhackque May 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? I don't see what problem a parameter solves.

Seems like it would just add more complication in the API. The new keys are harmless - no existing code should notice them, it should just access the old keys. Any new code would use the new keys, unless it did something odd (like feed them to the *sockaddr_in functions).

Requiring an option to get the old behavior still breaks old code. Requiring an option to get the new (txt) keys seems unnecessary since they shouldn't bother anyone. (I suppose there's the case of code that assumes that there's only one key in the hash - but I don't have any sympathy for that.)

You also have to test both cases of the selector - as written, your test only needs to verify that both values are correct.

And, should anyone want both, a selector would require two calls. Perhaps where you do pass a value to a sockaddr (binary) and log it (text).

What am I missing?

$name->{iPAddress} = unpack( 'H*', $ip );
}
} elsif ( $item eq 'otherName' ) {
my $otherName = $name->{otherName};
if ( $otherName->{type} eq '1.3.6.1.4.1.311.20.2.3' ||
$otherName->{type} eq '1.3.6.1.5.5.7.8.7' ) {
my $value;
Copy link

@tlhackque tlhackque May 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here to process the ServiceName OID the same way.

foreach my $val (keys %{$otherName->{value}}) {
$value .= $val . "::" . $otherName->{value}{$val};
}
$name->{otherName} = $value;
}
}
}
}

return $san;
}

Expand Down
20 changes: 20 additions & 0 deletions certs/ipacert.pem
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
-----BEGIN CERTIFICATE-----
MIIDAzCCAeugAwIBAgICA+gwDQYJKoZIhvcNAQELBQAwFjEUMBIGA1UEAwwLZXhh
bXBsZS5uZXQwHhcNMjQwNTE4MjI1NzM5WhcNMzQwNTE2MjI1NzM5WjAWMRQwEgYD
VQQDDAtleGFtcGxlLm5ldDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEB
ALMEHPyELA3raG9/ATjFGhl3iFX1no+Iko7TxW68O7PX083+g+jOPVLY7lDSExkz
y3ka2Y/lldRPdi0HoEDl1kTYXsC25zNrRIgePSNF4gsFqHKtJCxIf40M7o3Ic/i6
ginVAi8pVDEXkag4i1Lna6ERA9ExU68M31klB1Dm9ZimodN3wC5hMlMS4jjM7hhc
SEXsxmTgBs57yBNIkUVgbOBpDJZs7ZZPlVPjyafQtJebiDfi/HUbFELA/KTWgSrn
s5WYPf5/Pjt30j0Hx5C+Po8g5C6z9Jk+aOe+SXt+S1XUa6bvLu0DvG964sjWtYqN
qI5MFJSpZrAzJt+5vHu/XRECAwEAAaNbMFkwDwYDVR0TBAgwBgEB/wIBADBGBgNV
HREEPzA9ggtleGFtcGxlLm5ldIcEwAAAAYcEwAAAAocQIAENuAEjAAAAAAAAAAAA
BIcQIAECuAEjAAAAAAAAAAAABTANBgkqhkiG9w0BAQsFAAOCAQEAYlbJosKKRi1E
m0As7w+LsZeOsFBvUuH62FJLMet3S/nbtXbqvjaiCcH+KOknwJ7dVby2OToEtmb9
t5SuhuZuADNbjPM+QOO6E5Ti1pIz+3gluqhfumGbmLRSf1IaDaQg0u2JnRiEICP0
w6q43ln6+0A+i92n/CfTiSwtWQi4QJC/V31S33t+fvZ5yHZpA+U7PjNjLUvGPOQ9
jl4SM42jDQ/Ob46EeDPhS8CEG2SsgCtFuvn4EwOdoZ1VedJA4q2VfJUpew5KJton
8PkTA8407DhuwAXXzoGnczm8VNuxTEL11QrRKZe/t9v9sOjLIb4dmvmz665nzcPE
PFYLjo3fJQ==
-----END CERTIFICATE-----

20 changes: 20 additions & 0 deletions certs/srvname.pem
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
-----BEGIN CERTIFICATE-----
MIIDLTCCAhWgAwIBAgIUWCot9rNObq8LrwCy3zqVKM5LZNswDQYJKoZIhvcNAQEL
BQAwUzELMAkGA1UEBhMCVVMxDjAMBgNVBAgMBVRleGFzMQ8wDQYDVQQHDAZBdXN0
aW4xEzARBgNVBAoMCk15IENvbXBhbnkxDjAMBgNVBAMMBU15IENBMB4XDTIxMDIy
MzExMjk0N1oXDTIxMDMyNTExMjk0N1owETEPMA0GA1UEAwwGZm9vYmFyMIIBIjAN
BgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA0xNcCng0SCct0v53G0wUaH07augd
1CT/GDdvX53VNVsni4bFRu039vhD8JIx9ohhmbs2EhgoUsdH7PmX1i8wOuF9NbGr
HVCMd/gvwX03O8ecLHlwDFc4Do4rU84fi9c8PHiB9jaqmY7Mnn9xsoGi8trVyK6e
s9d7y8rD1Depgl3UkXuMUaNz3oLLcsj8ck2UPrtUa2kVn1Pqczs3PZKMO+R0+9BX
bv1MpFEK8SWVbMBmJZXc0qwrRrbzOlUFXtSTJW7C6UgrmdBSd8ll5d4V7RXiVsNm
tr2YcIGHDsjgOeyRaNLIm0C1K/VDh83AJGfoMUPx9JQPEFfA0rGhBxtKsQIDAQAB
ozswOTA3BgNVHREEMDAuggpkb21haW4ub3JnoCAGCCsGAQUFBwgHoBQWEl93ZWJh
cHAuZG9tYWluLm9yZzANBgkqhkiG9w0BAQsFAAOCAQEAHDWrfRtsYdO6r++MPMr9
ECEWazsIhFz6vRKaifgnzymC3/U1tRilpKH1aJWB8x5JHMvHkwhaP/GsQI/AYYq/
opznPvEbjnVTIF5NALXBi0HqT7xBVlColfFHZ/OQ9cs190kWWzHQZXzWevbgLMd6
QFZMHF0Z575OA6jOROhpt3n+p94dYpnCXSZtZ9Su62OyFddGrGEnisTvDHuBlhmr
00/pPhu6Jt6Kk1zVeWp3AHtPT20iihQIbH+GZHkEmpRH3YcNikePefwnYHILsnpP
2+q8VbAFXsY8VbKisoU3g+sKc1E3dXkg509VAxjxRuAU0xdSJONvMyUL6zn544w5
bQ==
-----END CERTIFICATE-----
11 changes: 11 additions & 0 deletions certs/upn-cert.pem
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
-----BEGIN CERTIFICATE-----
MIIBoTCCAUagAwIBAgIRAIPqMfAzi3GkNeRTRJLFkkswCgYIKoZIzj0EAwIwHjEc
MBoGA1UEAwwTam9obkRvZUBleGFtcGxlLmNvbTAeFw0yNDA1MTcxODU3MDBaFw0y
NTA1MTcxODU3MDBaMB4xHDAaBgNVBAMME2pvaG5Eb2VAZXhhbXBsZS5jb20wWTAT
BgcqhkjOPQIBBggqhkjOPQMBBwNCAAT1L7LWhF85IOFgdxvSVqVhwypneowwJq+c
XR80zUhLJbAsd+EA6GZwhFmjNts3L9Vhpw7WfN6p5DCyQHyQa61ho2UwYzAOBgNV
HQ8BAf8EBAMCBaAwEwYDVR0lBAwwCgYIKwYBBQUHAwIwDAYDVR0TAQH/BAIwADAu
BgNVHREEJzAloCMGCisGAQQBgjcUAgOgFQwTam9obkRvZUBleGFtcGxlLmNvbTAK
BggqhkjOPQQDAgNJADBGAiEAqUejaH3IJznnb/zLPoz0IzBcI380UEDP/EyGuKDg
o/0CIQDZgr6tAF/GAjCw7z/qzWpS0YfwlBAcoY4XLa0Yl2R0ag==
-----END CERTIFICATE-----
2 changes: 1 addition & 1 deletion t/san.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use Test::More tests => 17;
use Test::More tests => 23;

BEGIN { use_ok('Crypt::OpenSSL::X509') };

Expand Down