-
Notifications
You must be signed in to change notification settings - Fork 3k
public_key: Add missing macros #9865
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.
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
public_key: Add missing macros #9865
Conversation
CT Test Results 2 files 17 suites 4m 46s ⏱️ Results for commit b50e148. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
af8629b
to
fd7eed4
Compare
d7e729b
to
64c3a55
Compare
5b84b31
to
17a932a
Compare
17a932a
to
8445094
Compare
get_asn1_module('Certificate') -> 'PKIX1Explicit-2009'; | ||
get_asn1_module('SubjectAltName') -> 'PKIX1Implicit-2009'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this would be nice: to be able to call public_key:der_encode('SubjectAltName', SANExtension)
. I use that to include a SubjectAltName extension in a CSR extensionRequest
.
Unfortunately PKIX1Implicit-2009
does not define the necessary enc_SubjectAltName/2
function, so this fails with {:error, {:asn1, {{:undefined_type, :SubjectAltName}, ...}
. There is enc_ext-SubjectAltName/2
, though.
There seem to be some inconsistencies in this respect for various certificate extensions. For example, AuthorityKeyIdentifier
and SubjectKeyIdentifier
do expose enc_Xxx
and dec_Xxx
functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Humm I will consult my co-worker about why asn1 functions have ended up like that now, and see what we can do next week sometime. Will be busy with Code BEAM Stockholm on Monday.
30e9e31
to
51f41b6
Compare
51f41b6
to
2218246
Compare
@voltone Please try again, I hope this should also be fixed now. |
Thanks @IngelaAndin! I had to replace But I now get errors (that I didn't think I got before) on encoding CRL distribution point into certificates. I'm trying to get to the bottom of that one, but today's a holiday here so I can't give it my full attention. I will update you once I know more. |
@voltone One thing that I changed back is that CRLDistributionPoints extension is not decoded until later if you use the pkix_decode_cert(Der, otp) option as that lets you workaround some certs that do not specify CRLDistributionPoints but wrongly encodes them as asn1 NULL. But that should only affect you if you try to encode it back without decoding it first. |
The issue happens when I call The exception and stack trace (in Elixir syntax):
The extension list in the certificate passed to [{crl_distribution_point,
{'Extension',
{2,5,29,31},
false,
[{'DistributionPoint',
{fullName,
[{uniformResourceIdentifier,
"http://localhost:55550/rsa/root_ca.crl"}]},
asn1_NOVALUE,asn1_NOVALUE}]}}] |
Ah, wait, it is DER encoded in the TBSCertificate record, so: {'OTPTBSCertificate',v3,16734941980052244352,
{'SignatureAlgorithm',{1,2,840,10045,4,3,2},asn1_NOVALUE},
{rdnSequence,
[[{'AttributeTypeAndValue',
{2,5,4,10},
{utf8String,<<"Elixir.X509.Test.Suite">>}}],
[{'AttributeTypeAndValue',{2,5,4,3},{utf8String,<<"Root CA">>}}]]},
{'Validity',{utcTime,"250609104751Z"},{utcTime,"350609105251Z"}},
{rdnSequence,
[[{'AttributeTypeAndValue',
{2,5,4,10},
{utf8String,<<"Elixir.X509.Test.Suite">>}}],
[{'AttributeTypeAndValue',
{2,5,4,3},
{utf8String,<<"Intermediate CA">>}}]]},
{'OTPSubjectPublicKeyInfo',
{'PublicKeyAlgorithm',
{1,2,840,10045,2,1},
{namedCurve,{1,2,840,10045,3,1,7}}},
{'ECPoint',
<<4,129,171,34,28,246,230,27,120,167,14,244,21,107,217,251,244,
32,90,182,201,233,16,163,42,181,193,114,139,106,232,158,252,
182,83,129,163,51,213,239,49,58,55,149,161,50,21,186,3,22,203,
42,172,26,0,58,179,119,10,224,233,219,183,19,240>>}},
asn1_NOVALUE,asn1_NOVALUE,
[{'Extension',{2,5,29,19},true,{'BasicConstraints',true,0}},
{'Extension',{2,5,29,15},true,[digitalSignature,keyCertSign,cRLSign]},
{'Extension',{2,5,29,37},false,[{1,3,6,1,5,5,7,3,1},{1,3,6,1,5,5,7,3,2}]},
{'Extension',
{2,5,29,14},
false,
<<204,251,70,82,43,99,219,198,5,27,79,187,9,121,196,247,70,57,116,162>>},
{'Extension',
{2,5,29,35},
false,
{'AuthorityKeyIdentifier',
<<69,194,3,82,159,15,122,2,1,233,40,32,160,45,63,120,184,251,193,
253>>,
asn1_NOVALUE,asn1_NOVALUE}},
{'Extension',
{2,5,29,31},
false,
<<48,48,48,46,160,44,160,42,134,40,104,116,116,112,58,47,47,108,
111,99,97,108,104,111,115,116,58,53,53,53,55,54,47,101,99,100,
115,97,47,114,111,111,116,95,99,97,46,99,114,108>>}]} |
It's the last extension, with oid Should I be passing it differently to |
Well I guess as we allow partial decode, that is CRLDistributionPoints will be decode later when used in CRL verification procedure if enabled, we should add a encode clause to handle it is already encoded. |
2218246
to
1039144
Compare
@voltone Try again |
Yes, working now, thanks @IngelaAndin! I will review a few more things before I approve the PR, ok? Or are you in a rush to release 8.0.1? |
@voltone that is a relative question. We want it to be part of the upcoming patch but that depends on other things that this PR. But I think this will be merged at latest tomorrow to not potentially slow things down. |
1039144
to
b50e148
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This addresses all the issues I ran into. Some things are a bit different than in prior OTP versions, and therefore require conditional code that takes into account the public_key application version, but in the end all functionality I depend on can be made to work with this PR. Thanks!
closes #9857