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 support for custom-tlv-string #425

Merged
merged 4 commits into from
Apr 5, 2024

Conversation

notronrj
Copy link

@notronrj notronrj commented Apr 1, 2024

Added the ability to create a custom-tlv using an ASCII string to the keytools/sign tool.

Example usage:

sign --custom-tlv-string 0x0030 "0.99.910(6)" --no-sign --sha256 ${projectBaseDir}/release/zephyr.bin 6

This will create a custom TLV tag as if you'd used --custom-tlv-buffer 0x0030 302E39392E393130283629
Tag: 0030 Len: 11 Val: 302E39392E393130283629

The above invocation of the sign tool generates the following header in the binary:

image

Showing that the two commands are equivalent see the following:

Invocation:
sign --custom-tlv-string 0x0030 "0.99.910(6)" --custom-tlv-buffer 0x0031 302E39392E393130283629 --no-sign --sha256 ${projectBaseDir}/release/zephyr.bin 6

Custom TLVS: 2
TLV 0
----
Tag: 0030 Len: 11 Val: 302E39392E393130283629
-----
TLV 1
----
Tag: 0031 Len: 11 Val: 302E39392E393130283629
-----

And the resulting header in the binary:

image

Note: This PR came out of a support ticket at https://wolfssl.zendesk.com/hc/en-us/requests/17637

@dgarske dgarske self-assigned this Apr 1, 2024
Copy link
Contributor

@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.

Thanks Jim for the PR. We'll review and test. We may need to sort out the contributor agreement details. We will followup in the ticket.

tools/keytools/sign.c Outdated Show resolved Hide resolved
@notronrj notronrj requested a review from dgarske April 2, 2024 18:36
@dgarske
Copy link
Contributor

dgarske commented Apr 2, 2024

Contributor agreement approved. @danielinux over to you.

dgarske
dgarske previously approved these changes Apr 3, 2024
Copy link
Contributor

@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.

Looks good to me. I was able to test using ./tools/keytools/sign --custom-tlv-string 0x0030 "0.99.910(6)" --no-sign --sha256 test-app/image.bin 6.
It would be nice to have documentation added to docs/Signing.md. It would also be nice to have a test case added to .github/workflows/test-custom-tlv-simulator.yml.
Over to @danielinux to finalize.

@dgarske dgarske removed their assignment Apr 3, 2024
@notronrj
Copy link
Author

notronrj commented Apr 3, 2024

Looks good to me. I was able to test using ./tools/keytools/sign --custom-tlv-string 0x0030 "0.99.910(6)" --no-sign --sha256 test-app/image.bin 6. It would be nice to have documentation added to docs/Signing.md. It would also be nice to have a test case added to .github/workflows/test-custom-tlv-simulator.yml. Over to @danielinux to finalize.

I updated the documentation. I hope you don't mind.

@dgarske
Copy link
Contributor

dgarske commented Apr 3, 2024

Looks good to me. I was able to test using ./tools/keytools/sign --custom-tlv-string 0x0030 "0.99.910(6)" --no-sign --sha256 test-app/image.bin 6. It would be nice to have documentation added to docs/Signing.md. It would also be nice to have a test case added to .github/workflows/test-custom-tlv-simulator.yml. Over to @danielinux to finalize.

I updated the documentation. I hope you don't mind.

Thank you so much!

@danielinux
Copy link
Member

danielinux commented Apr 3, 2024

@notronrj this looks good! Thanks for taking the time to update the doc as well!

I think we should also add a non-regression test to the github workflow:

--- a/.github/workflows/test-keytools.yml
+++ b/.github/workflows/test-keytools.yml
@@ -264,3 +264,8 @@ jobs:
           ./tools/keytools/sign --ecc256 --sha256 --custom-tlv-buffer 0x46 48656C6C6F20776F726C64 test-app/image.elf wolfboot_signing_private_key.der 3
           grep "Hello world" test-app/image_v3_signed.bin
 
+      - name: Sign app with custom string TLV included
+        run: |
+          ./tools/keytools/sign --ecc256 --sha256 --custom-tlv-string 0x46 "Hello world" test-app/image.elf wolfboot_signing_private_key.der 3
+          grep "Hello world" test-app/image_v3_signed.bin
+

[edit: updated patch to include Tag]

@notronrj notronrj requested a review from dgarske April 4, 2024 02:53
@danielinux
Copy link
Member

@notronrj could you please add a commit with the added test in the github workflow ?

@danielinux danielinux removed the request for review from dgarske April 4, 2024 06:52
@notronrj
Copy link
Author

notronrj commented Apr 4, 2024

@danielinux taking care of that now.

I'm having an issue pushing the github workflow commit. See below.

image

The error is:
! [remote rejected] custom-tlv-string -> custom-tlv-string (refusing to allow a Personal Access Token to create or update workflow .github/workflows/test-keytools.yml without workflow scope)
error: failed to push some refs to 'https://github.com/notronrj/wolfBoot.git'

tact@zephyr:~/development/wolfBoot$ git log
commit ec8e537 (HEAD -> custom-tlv-string)
Author: Jim Norton [email protected]
Date: Thu Apr 4 10:34:31 2024 -0400

Added custom-tlv-string non-regression test to github workflows

What am I missing?

Thank you.

@notronrj
Copy link
Author

notronrj commented Apr 4, 2024

@danielinux
I fixed my git workflow issue. My personal access token didn't have workflow set. See, you learn something new everyday!

@dgarske dgarske requested a review from danielinux April 4, 2024 14:59
@notronrj
Copy link
Author

notronrj commented Apr 4, 2024

@dgarske @danielinux
Non-regression tests seem to be hung?

@danielinux danielinux merged commit 8fa5562 into wolfSSL:master Apr 5, 2024
80 checks passed
@notronrj
Copy link
Author

notronrj commented Apr 5, 2024

Thank you all for accepting the PR.

@notronrj notronrj deleted the custom-tlv-string branch April 5, 2024 11:06
@danielinux
Copy link
Member

Thank you!

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.

3 participants