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

adjust pkcs12 example to print out list of certificates found #366

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

Conversation

JacobBarthelmeh
Copy link
Contributor

No description provided.

if (i != 0 && !(i%16)) printf("\n");
printf("%02X", certDer[i]);
}
printf("\n");
XFREE(certDer, NULL, DYNAMIC_TYPE_PKCS);
}

/* itterate through list if was not passed as null and free each node */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest updating comment to mention display of certs in list as code is no longer just freeing each node

if (current->buffer != NULL) {
printf("[CERT %d] :", certIdx++);
for (i = 0; i < current->bufferSz; i++)
printf("%02X", current->buffer[i]);
Copy link
Contributor

Choose a reason for hiding this comment

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

The certificate HEX data is printed differently than the data for previous certificates. Specifically, the previous HEX data was displayed in 16-byte rows. The way the certificate list data is currently displayed makes the output look inconsistent, and there might also be some utility to having 16-byte rows, as it's easier to visually parse to a specific location (?).

I'd suggest modifying the certificate list HEX data print logic to match that used for other HEX data. I realize that the [CERT %d] : text printed prior to each certificate will push the first 16-byte row out of alignment with with subsequent rows... I'd suggest adding a newline to the end of the "CERT" text so the first row of HEX data for each certificate starts at column zero.

WC_DerCertList* next = current->next;
WC_DerCertList* next;

next = current->next;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: there's an extra space between next and =.

if (keyDer != NULL) {
printf("HEX of Private Key Read (DER format) :\n");
for (i = 0; i < keySz; i++) {
if (i != 0 && !(i%16)) printf("\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

A question: I don't see anything explicitly forbidding use of if without brackets in the coding standards (and I know you didn't create this code, just moved it)... There is an implicit mention of using "K&R" style and if as an example... Do we generally allow the "no-bracket" formatting for if statements? .. maybe only when there isn't an else statement?

@JacobBarthelmeh
Copy link
Contributor Author

Thanks for the great review! I added a commit to address the feedback.

if (current->buffer != NULL) {
printf("\n[CERT %d] :", certIdx++);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add newline after cert number: [CERT %d] :\n... Current output (without new-line) looks like this:

HEX of Certificate LIST (DER format) :

[CERT 0] :308204AA30820392A003020102020900
B7B69033661B6B23300D06092A864886

Adding new-line would align HEX output (looks a little cleaner!)

Copy link
Contributor

@tim-weller-wolfssl tim-weller-wolfssl left a comment

Choose a reason for hiding this comment

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

On second thought, don't worry about last change-request, it's trivial and better to have functionality in the product.

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.

None yet

2 participants