Skip to content

Add wc_GetPubKeyDerFromCert()#4642

Merged
dgarske merged 1 commit intowolfSSL:masterfrom
cconlon:pubKeyDerFromX509
Dec 15, 2021
Merged

Add wc_GetPubKeyDerFromCert()#4642
dgarske merged 1 commit intowolfSSL:masterfrom
cconlon:pubKeyDerFromX509

Conversation

@cconlon
Copy link
Copy Markdown
Member

@cconlon cconlon commented Dec 8, 2021

This PR adds the API wc_GetPubKeyDerFromCert() in asn.c/asn_public.h. Used to get the DER encoded public key from a pre-parsed DecodedCert struct.

This PR also wraps two sections of ParseCert() that use XMALLOC with guards for ifndef WOLFSSL_NO_MALLOC. Customer user for this function will not be allowed to use dynamic memory.

Note: This PR has been re-worked since original comments were left.

@cconlon cconlon self-assigned this Dec 8, 2021
Comment thread wolfcrypt/src/asn.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe say: ASN.1 encoded X.509 certificate?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, switched

Comment thread wolfcrypt/src/asn.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would be handy to support PEM format here also, but a user can always call wc_CertPemToDer. Also it would be nice to support the CERTREQ_TYPE arg in ParseCertRelative.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe call it wc_GetPubKeyDerFromX509?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Great feedback. Added support for type, which supports CERTREQ_TYPE and CERT_TYPE. Added support for format which supports CTC_FILETYPE_PEM or CTC_FILETYPE_ASN1. PEM support is protected with #ifndef WOLFSSL_NO_MALLOC, as PemToDer() uses XMALLOC. DO-178 will not allow dynamic memory, so this will provide an easy way to cut this functionality out for those users.

@cconlon cconlon changed the title Add wc_GetPubKeyDerFromX509Der() Add wc_GetPubKeyDerFromX509() Dec 9, 2021
Copy link
Copy Markdown
Member

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Great work! Just one minor issue with variable declarations not all at top in api.c for the unit test.

Comment thread tests/api.c Outdated
Copy link
Copy Markdown
Member

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

If the PR #4645 is merged is there anything you might change here?

I could envision an API that takes the DecodedCert struct and supports PEM/DER and the CERT_TYPE and CERTREQ_TYPE.

Then getting the public key is just referencing publicKey and pubKeySize.

Comment thread wolfcrypt/src/asn.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason to not also get on the WOLFSSL_PEM_TO_DER macro?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agreed, should be guarded by this as well. May remove this code, pending discussion below.

@cconlon
Copy link
Copy Markdown
Member Author

cconlon commented Dec 10, 2021

In general, I'm a fan of merging #4645 regardless. Re your comment, we could offload the requirement to the user to call wc_KeyPemToDer(), wc_InitDecodedCert(), and wc_ParseCert(). Then, calling this API would simply return a copy of decoded->pubKey of size decoded-pubKeySize. type and format would no longer be needed at this API's scope.

For users who want to only get the pub key, making them not call 3 other APIs seems easier. But, for users who want to get more than the pub key out of DecodedCert, it's more efficient to have them call the other APIs first, then have simple accessor functions. Although it will make it slightly more difficult usability wise for my user, I'm leaning towards converting this to a simple wrapper as suggested for performance/optimization. Thoughts?

@cconlon cconlon changed the title Add wc_GetPubKeyDerFromX509() Add wc_GetPubKeyDerFromCert() Dec 15, 2021
@cconlon cconlon assigned dgarske and unassigned cconlon Dec 15, 2021
@dgarske dgarske merged commit abe8696 into wolfSSL:master Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants