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

rm: security: update stm32mp1 article #588

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

igoropaniuk
Copy link
Contributor

@igoropaniuk igoropaniuk commented Aug 22, 2023

Add details about implicit build-time signing and using provision script.

Readiness

  • Merge (pending reviews)
  • Merge after date or event
  • Draft

Overview

Why merge this PR? What does it solve?

Checklist

Optional. Add a 'x' to steps taken.
You can fill this out after opening the PR. "Did I..."

  • Run spelling and grammar check, preferably with linter.
  • Avoid changing any header associated with a link/reference.
  • Step through instructions (or ask someone to do so).
  • Review for wordiness
  • Match tone and style of page/section.
  • Run make linkcheck.
  • View HTML in a browser to check rendering.
  • Use semantic newlines.
  • follow best practices for commits.
    • Descriptive title written in the imperative.
    • Include brief overview of QA steps taken.
    • Mention any related issues numbers.
    • End message with sign off/DCO line (-s, --signoff).
    • Sign commit with your gpg key (-S, --gpg-sign).
    • Squash commits if needed.
  • Request PR review by a technical writer and at least one peer.

Comments

Any thing else that a maintainer/reviewer should know.
This could include potential issues, rational for approach, etc.

@igoropaniuk
Copy link
Contributor Author

depends on foundriesio/meta-lmp#1267

@igoropaniuk igoropaniuk force-pushed the stm32mp15-secure-boot branch from a378dbd to 4093551 Compare August 23, 2023 19:11
@doanac
Copy link
Member

doanac commented Aug 23, 2023

@kprosise
Copy link
Contributor

@igoropaniuk In process of reviewing, I will provide a full review on August 29th, during my usual PR review day.

Copy link
Contributor

@kprosise kprosise left a comment

Choose a reason for hiding this comment

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

Left some suggestions.

source/reference-manual/security/secure-boot-stm32mp1.rst Outdated Show resolved Hide resolved
source/reference-manual/security/secure-boot-stm32mp1.rst Outdated Show resolved Hide resolved
source/reference-manual/security/secure-boot-stm32mp1.rst Outdated Show resolved Hide resolved
source/reference-manual/security/secure-boot-stm32mp1.rst Outdated Show resolved Hide resolved
source/reference-manual/security/secure-boot-stm32mp1.rst Outdated Show resolved Hide resolved
source/reference-manual/security/secure-boot-stm32mp1.rst Outdated Show resolved Hide resolved
source/reference-manual/security/secure-boot-stm32mp1.rst Outdated Show resolved Hide resolved
source/reference-manual/security/secure-boot-stm32mp1.rst Outdated Show resolved Hide resolved
source/reference-manual/security/secure-boot-stm32mp1.rst Outdated Show resolved Hide resolved
@doanac
Copy link
Member

doanac commented Sep 1, 2023

@igoropaniuk igoropaniuk force-pushed the stm32mp15-secure-boot branch from 2a89713 to 1bc2640 Compare September 1, 2023 06:47
@igoropaniuk
Copy link
Contributor Author

@kprosise thanks! I've applied all suggestions, also adjusted an article and added a couple of sentences about manual creation of combo images

@doanac
Copy link
Member

doanac commented Sep 1, 2023

@igoropaniuk igoropaniuk force-pushed the stm32mp15-secure-boot branch 2 times, most recently from 362074b to a481465 Compare September 1, 2023 09:45
@doanac
Copy link
Member

doanac commented Sep 1, 2023

@igoropaniuk igoropaniuk force-pushed the stm32mp15-secure-boot branch from a481465 to e883ae6 Compare September 1, 2023 09:50
@doanac
Copy link
Member

doanac commented Sep 1, 2023

Copy link
Contributor

@kprosise kprosise left a comment

Choose a reason for hiding this comment

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

LGTM!

source/reference-manual/security/secure-boot-stm32mp1.rst Outdated Show resolved Hide resolved
source/reference-manual/security/secure-boot-stm32mp1.rst Outdated Show resolved Hide resolved
source/reference-manual/security/secure-boot-stm32mp1.rst Outdated Show resolved Hide resolved
Comment on lines +91 to +99
#
# STM32CubeProgrammer STM32MP Signing Tool configuration
#
STM32_ROT_SIGN_ENABLE ??= "1"
STM32_CUBE_PATH ??= "/usr/local/STMicroelectronics/STM32Cube/STM32CubeProgrammer"
STM32_ROT_KEY_PATH ??= "${TOPDIR}/../tools/lmp-tools/security/stm32mp1/"
STM32_ROT_KEY_PATH[vardepsexclude] += "TOPDIR"
STM32_ROT_KEY_PASSWORD ??= "foundries"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could it be created a factory variable to enable this from the factory-config.yml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really doubt that it's good idea to share RoT keys on CI server.
I would use this approach only on for building final boot image on private host.

source/reference-manual/security/secure-boot-stm32mp1.rst Outdated Show resolved Hide resolved
Add details about implicit build-time signing and using provision
script.

Signed-off-by: Igor Opaniuk <[email protected]>
@igoropaniuk
Copy link
Contributor Author

@angolini all comments addressed

@igoropaniuk
Copy link
Contributor Author

@kprosise Let me know if you're ok to merge it

@doanac
Copy link
Member

doanac commented Sep 14, 2023

@kprosise
Copy link
Contributor

@igoropaniuk LGTM, shall I go ahead and merge it now?

@igoropaniuk
Copy link
Contributor Author

@kprosise yes, please go ahead

@kprosise kprosise merged commit ed24314 into foundriesio:main Sep 14, 2023
1 check passed
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.

5 participants