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

Offline signing of TAs: Improve the documentation #223

Merged

Conversation

jclsn
Copy link
Contributor

@jclsn jclsn commented Nov 16, 2023

The documentation was partly faulty:

  • The private key was specified for the digest and stitch steps
  • The Nitro HSM section was not correct and did not execute in the shell

This commit extends the documentation and explicitly sets elfs, digest, signature and .ta locations for completeness. This is needed for automated build environments like Yocto, since it builds TAs in several different recipes.

Fixes #224

@jenswi-linaro
Copy link
Contributor

Please add a Signed-off-by: tag with your name and a mail address.

@jclsn jclsn force-pushed the improve-ta-offline-signing-section branch 7 times, most recently from d43ca08 to eeccbc8 Compare November 16, 2023 21:36
@jclsn
Copy link
Contributor Author

jclsn commented Nov 16, 2023

I think it should be good now

@jclsn jclsn force-pushed the improve-ta-offline-signing-section branch from eeccbc8 to 159ced1 Compare November 16, 2023 22:30
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

Commit header line:

building: improve offline TA signing documentation


.. note::
It may be necessary to make use of the ``--ta-version`` flag here in some cases,
e.g when building Widevine's oemcrypto with the Yocto toolchain. Check the make output of optee-os
Copy link
Contributor

Choose a reason for hiding this comment

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

s/e.g/e.g./
s/flag/option/ ?
Yocto toolchain or Yocto environment?

For consistency, prefer 100 char/line max.t

By curiosity: why does Yocto require --ta-version to be specified for the Widewive TAs? If not set, the default TA version number used is 0.

@jclsn
Copy link
Contributor Author

jclsn commented Nov 17, 2023

Yocto environment would probably be more precise. Actually Yocto is the project, Bitbake the build system and it builds its own toolchains.

In my use case I needed the in-tree TAs and oemcrypto for Widevine, which comes with its own recipe that pulls TA devkit as a dependency. I couldn't sign it manually at first and it took me quite a while to figure this out. I had to enable verbose build output, look at the make command to see that the version was different.

So it may not even be Yocto but the TA devkit version oemcrypto ships with or requires. Since Yocto is very prominent nowadays I wanted to give this hint here.

Do you think it is too much?

@etienne-lms
Copy link
Contributor

In my use case I needed the in-tree TAs

Which in-tree do you need? There doesn't seem to be an in-tree TA related to WV support.
Maybe my question is off topic and you can discard it.

and oemcrypto for Widevine, which comes with its own recipe that pulls TA devkit as a dependency. I couldn't sign it manually at first and it took me quite a while to figure this out. I had to enable verbose build output, look at the make command to see that the version was different.

I guess you could sign the TA but could not use it on your target due to your TAs (of some of) being already used on your target with a version number value higher than 0.

I think the doc should not say that Yocto requires --ta-version to be set. You should rephrase that sentence to recall that --ta-version argument should be used to set the TA version number when needed.

By the way, what is the right wording of --ta-version argument in the script: it's an option? an (optional) argument? a flag? For consistency with the script help output message, I would prefer "argument".

@jclsn jclsn force-pushed the improve-ta-offline-signing-section branch from 159ced1 to 517f0be Compare November 17, 2023 08:13
@jclsn
Copy link
Contributor Author

jclsn commented Nov 17, 2023

Which in-tree do you need? There doesn't seem to be an in-tree TA related to WV support.
I am not sure. I just kept the pkcs11, avb and trusted-keys TAs that shipped with OP-TEE.

I guess you could sign the TA but could not use it on your target due to your TAs (of some of) being already used on your target with a version number value higher than 0.

Well, in my case the in-tree TAs were compiled with the default version 0 and oemcrypto with 0x20100711 for some reason. I tried enforcing the version 0 in the build, but without luck. The only thing that worked was signing with this TA version set.

By the way, what is the right wording of --ta-version argument in the script: it's an option? an (optional) argument? a flag? For consistency with the script help output message, I would prefer "argument".

I changed it to argument and wrapped the lines before hitting the 100 character limit.

@jclsn jclsn force-pushed the improve-ta-offline-signing-section branch from 517f0be to 3045c90 Compare November 17, 2023 08:27
@etienne-lms
Copy link
Contributor

Well, in my case the in-tree TAs were compiled with the default version 0 and oemcrypto with 0x20100711 for some reason. I tried enforcing the version 0 in the build, but without luck. The only thing that worked was signing with this TA version set.

The version number is used for protection against version rollback TA binaries. The secure storage of your target likely stored that your WV TA has been used with that version number so OP-TEE will only accept WV TA with version number higher of equal to that value. It fully make sense to use TA versioning. TA version number is a TA property defined by the GP TEE specification. Few info can be found in OP-TEE doc on TAs.

I changed it to argument and wrapped the lines before hitting the 100 character limit.

May I ask you to append fixup commit(s) on top of your series rather than force pushing an updated commit? It would help reviewers to see what you have modified regarding the previously reviewed version of your changes.

@jclsn
Copy link
Contributor Author

jclsn commented Nov 17, 2023

May I ask you to append fixup commit(s) on top of your series rather than force pushing an updated commit? It would help reviewers to see what you have modified regarding the previously reviewed version of your changes.

So rather make many commits and squash afterwards? I can do that from now on.

It there anything else you would like me to change?

@etienne-lms
Copy link
Contributor

So rather make many commits and squash afterwards? I can do that from now on.

Yes please. Thanks.

It there anything else you would like me to change?

Yes, please: discard .giticonfig file.

@jclsn jclsn force-pushed the improve-ta-offline-signing-section branch from 3045c90 to f8ecb7b Compare November 17, 2023 12:26
@jclsn
Copy link
Contributor Author

jclsn commented Nov 17, 2023

Oh sorry. This one I force-pushed though, since I don't want it to end up in the history.

@jclsn
Copy link
Contributor Author

jclsn commented Nov 27, 2023

Is this good to merge or should I still change things?

@jenswi-linaro
Copy link
Contributor

Looks good to me.
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

@etienne-lms
Copy link
Contributor

Commit header line:
building: improve offline TA signing documentation

With that addressed
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>

The documentation was partly faulty:

- The private key was specified for the digest and stitch steps
- The Nitrokey HSM section was not correct and did not execute in the shell

This commit extends the documentation and explicitly sets elfs, digest,
signature and .ta locations for completeness. This is needed for
automated build environments like Yocto, since it builds TAs in several
different recipes.

Signed-off-by: Jan Claussen <jan.claussen10@web.de>
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
@jclsn jclsn force-pushed the improve-ta-offline-signing-section branch from f8ecb7b to 212f263 Compare November 27, 2023 09:58
@jclsn
Copy link
Contributor Author

jclsn commented Nov 27, 2023

Sorry, I force-pushed again. This is a habit from work, where we use Gitlab that can still display the diffs between the pushes.

@jbech-linaro
Copy link
Contributor

Thanks for the contribution @jclsn , I'm merging this now!

@jbech-linaro jbech-linaro merged commit c884b7f into OP-TEE:master Nov 27, 2023
1 check was pending
@jclsn
Copy link
Contributor Author

jclsn commented Nov 27, 2023

You are welcome! Happy to be able to help

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.

Offline signing of Trusted Applications section is partially incorrect
4 participants