Draft: Fixes #116, Fixes #117 - revised patch and test certs from @tlhackque #118
Conversation
timlegge
changed the title
|
You have replaced the iPAddress binary value with the text. I had code that dealt with the binary, don't know if anyone else did. That's why I added the text as an alternate key rather than replace the binary. I can change my code easily enough, but that's a breaking change for anyone else... |
That is a good point - however I believe its the best long term approach but @jonasbn would need to approve/merge. I suppose an as_text function might be an option too |
|
@jonasbn I had some free time and thought I would take a look. I would want to review the sprintfs and unpacks again before thinking about merging. Also @tlhackque makes a couple of good points about the choices I made which could break people who were dealing with the binary in their own code. |
|
The sprintfs and unpacks are the same as what I used in Crypt::PKCS10 (quite a while ago). |
X509.pm
Outdated
|
|
||
| # Microsoft's User Principal Name (Smart Card Logon) | ||
| my $upn = Convert::ASN1->new or die( "New UPN" ); | ||
| $upn->prepare(q(microsoftUPN UTF8String)) or die ( "Prepare UPN" ); |
There was a problem hiding this comment.
Let's make it q(UPN) (drop the 'microsoft') ; agree that consistency with openssl's decode is a good thing.
I should have checked that; good catch.
| $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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| 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 ); | ||
|
|
There was a problem hiding this comment.
Patch in the issue updated to add support for 1.3.6.1.5.5.7.8.7 ServiceName RFC 4985
| } elsif ( $item eq 'otherName' ) { | ||
| my $otherName = $name->{otherName}; | ||
| if ( $otherName->{type} eq '1.3.6.1.4.1.311.20.2.3' ) { | ||
| my $value; |
There was a problem hiding this comment.
And here to process the ServiceName OID the same way.
2 participants
Description
Fixes #116, Fixes #117 by revising patch from @tlhackque. Parses some extra fields from the SAN of certificates/
Fixes/addresses (If applicable) # (issue)
Fixes #116, Fixes #117
Type of change
Please delete options that are not relevant.
Checklist
Test / Development Platform Information
Please see the issue template for a more information on provided the requested information.