Skip to content
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

Added parsing of v3 extension subject key identifier #2018

Closed

Conversation

PeterFredrikssonAF
Copy link

@PeterFredrikssonAF PeterFredrikssonAF commented Sep 16, 2018

No description provided.

johanenglund
johanenglund previously approved these changes Nov 12, 2018
MBEDTLS_ASN1_OCTET_STRING ) ) == 0 )
{
crt->subject_key_id.len = len;
crt->subject_key_id.tag = MBEDTLS_ASN1_OCTET_STRING;

Choose a reason for hiding this comment

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

Minor: Double white space

@@ -800,6 +800,21 @@ static int x509_get_crt_ext( unsigned char **p,
return( ret );
break;

case MBEDTLS_X509_EXT_SUBJECT_KEY_IDENTIFIER:
if( ( ret = mbedtls_asn1_get_tag( p, end_ext_data, &len,
MBEDTLS_ASN1_OCTET_STRING ) ) == 0 )

Choose a reason for hiding this comment

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

Minor: Please swap the branches here: return immediately if ret != 0, and otherwise proceed with the setup of crt->subject_key_id.

crt->subject_key_id.len = len;
crt->subject_key_id.tag = MBEDTLS_ASN1_OCTET_STRING;
crt->subject_key_id.p = *p;
*p +=len;

Choose a reason for hiding this comment

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

Minor: Space missing

library/oid.c Outdated
@@ -278,6 +278,10 @@ static const oid_x509_ext_t oid_x509_ext[] =
{ ADD_LEN( MBEDTLS_OID_NS_CERT_TYPE ), "id-netscape-certtype", "Netscape Certificate Type" },
MBEDTLS_X509_EXT_NS_CERT_TYPE,
},
{
{ ADD_LEN( MBEDTLS_OID_SUBJECT_KEY_IDENTIFIER ), "id-ce-subjectKeyIdentifier", "Subject Key Identifier" }, //Peter added

Choose a reason for hiding this comment

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

Minor: Please remove the comment.

Copy link

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I haven't studied the details of the extension yet, but noted a few style issues that could be fixed immediately. Also, could you please add an entry to the ChangeLog?

@RonEld
Copy link
Contributor

RonEld commented Feb 18, 2019

@PeterFredrikssonAF thank you for your contribution!

I apologize for delay, however I just noticed you haven't accepted out CLA.
Unfortunately our policy is to not accept contributions, without a Contributor’s Licence Agreement (CLA) signed or authorised by yourself or your employer.

If this is a personal contribution, the easiest way to do this is if you create an mbed account and accept this click through agreement. Alternatively, you can find a slightly different agreement to sign here, which can be signed and returned to us, and is applicable if you don't want to create an mbed account or alternatively if this is a corporate contribution.

Thanks for your understanding and again, thanks for the contribution!

@RonEld RonEld added enhancement CLA requested needs-review Every commit must be reviewed by at least two team members, labels Feb 18, 2019
@awakecoding
Copy link

@hanno-arm we ran into the same issue today (looking for a way to get the subject key identifier). Unlike the original PR, we would also need the authority key identifier, which also appears to be missing. For the time being I'll patch my branch to add such functionalities. If I have the time after I'll create a clean PR.

@danh-arm
Copy link
Contributor

danh-arm commented Apr 2, 2020

Hi @PeterFredrikssonAF and @awakecoding

I'm sorry this PR stalled a while ago. It looks like this was partly due to the lack of a CLA. The good news is our contributing guidelines have a changed and a CLA is no longer required. We just need a "Signed-off-by:" to indicate acceptance of the DCO.

The PR still seems relevant. Let us know if you have time to submit a reworked/rebased PR.

Regards

Dan.

@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Apr 2, 2020
@gilles-peskine-arm
Copy link
Contributor

This will probably be subsumed by #1425 (depending on how that design ends up).

@Himanshu-nxp
Copy link

Himanshu-nxp commented Apr 5, 2021

Hi All,

i am facing issue with the above given implementation.
issue:
while parsing the V3 certificate the issuer id and subjcet Id field of "mbedtls_x509_crt" structure is not populated.
On debugging i found that in "x509_get_uid" function the tag value which is fetched from certificate is "A3" but expected tag value is "A1" due to which i am getting error "MBEDTLS_ERR_ASN1_UNEXPECTED_TAG".
certificated used:
"
MIIFADCCA+igAwIBAgIBBzANBgkqhkiG9w0BAQsFADCBjzELMAkGA1UEBhMCVVMxEDAOBgNVBAgTB0FyaXpvbmExEzARBgNVBAcTClNjb3R0c2RhbGUxJTAjBgNVBAoTHFN0YXJmaWVsZCBUZWNobm9sb2dpZXMsIEluYy4xMjAwBgNVBAMTKVN0YXJmaWVsZCBSb290IENlcnRpZmljYXRlIEF1dGhvcml0eSAtIEcyMB4XDTExMDUwMzA3MDAwMFoXDTMxMDUwMzA3MDAwMFowgcYxCzAJBgNVBAYTAlVTMRAwDgYDVQQIEwdBcml6b25hMRMwEQYDVQQHEwpTY290dHNkYWxlMSUwIwYDVQQKExxTdGFyZmllbGQgVGVjaG5vbG9naWVzLCBJbmMuMTMwMQYDVQQLEypodHRwOi8vY2VydHMuc3RhcmZpZWxkdGVjaC5jb20vcmVwb3NpdG9yeS8xNDAyBgNVBAMTK1N0YXJmaWVsZCBTZWN1cmUgQ2VydGlmaWNhdGUgQXV0aG9yaXR5IC0gRzIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDlkGZL7PlGcakgg77pbL9KyUhpgXVObST2yxcT+LBxWYR6ayuFpDS1FuXLzOlBcCykLtb6Mn3hqN6UEKwxwcDYav9ZJ6t21vwLdGu4p64/xFT0tDFE3ZNWjKRMXpuJyySDm+JXfbfYEh/JhW300YDxUJuHrtQLEAX7J7oobRfpDtZNuTlVBv8KJAV+L8YdcmzUiymMV33a2etmGtNPp99/UsQwxaXJDgLFU793OGgGJMNmyDd+MB5FcSM1/5DYKp2N57CSTTx/KgqT3M0WRmX3YISLdkuRJ3MUkuDq7o8W6o0OPnYXv32JgIBEQ+ct4EMJddo26K3biTr1XRKOIwSDAgMBAAGjggEsMIIBKDAPBgNVHRMBAf8EBTADAQH/MA4GA1UdDwEB/wQEAwIBBjAdBgNVHQ4EFgQUJUWBaFAmOD07LSy+zWrZtj2zZmMwHwYDVR0jBBgwFoAUfAwyH6fZMH/EfWijYqihzqsHWycwOgYIKwYBBQUHAQEELjAsMCoGCCsGAQUFBzABhh5odHRwOi8vb2NzcC5zdGFyZmllbGR0ZWNoLmNvbS8wOwYDVR0fBDQwMjAwoC6gLIYqaHR0cDovL2NybC5zdGFyZmllbGR0ZWNoLmNvbS9zZnJvb3QtZzIuY3JsMEwGA1UdIARFMEMwQQYEVR0gADA5MDcGCCsGAQUFBwIBFitodHRwczovL2NlcnRzLnN0YXJmaWVsZHRlY2guY29tL3JlcG9zaXRvcnkvMA0GCSqGSIb3DQEBCwUAA4IBAQBWZcr+8z8KqJOLGMfeQ2kTNCC+Tl94qGuc22pNQdvBE+zcMQAiXvcAngzgNGU0+bE6TkjIEoGIXFs+CFN69xpk37hQYcxTUUApS8L0rjpf5MqtJsxOYUPl/VemN3DOQyuwlMOS6eFfqhBJt2nk4NAfZKQrzR9voPiEJBjOeT2pkb9UGBOJmVQRDVXFJgt5T1ocbvlj2xSApAer+rKluYjdkf5lO6Sjeb6JTeHQsPTIFwwKlhR8Cbds4cLYVdQYoKpBaXAko7nv6VrcPuuUSvC33l8Odvr7+2kDRUBQ7nIMpBKGgc0T0U7EPMpODdIm8QC3tKai4W56gf0wrHofx1l7"

files:
i am attaching the files for the reference.
Screenshot (17)
Screenshot (2)

@daverodgman daverodgman added the historical-reviewing Currently reviewing (for legacy PR/issues) label Apr 8, 2022
@daverodgman
Copy link
Contributor

This seems to be covered by #3243. It also would need tests, and has had a possible bug reported, so on that basis I'm going to close this.

If #3243 isn't sufficient and this is still useful, we'd be happy to look at an updated version of this, either re-opened or a new PR.

@daverodgman daverodgman closed this Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants