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

makefiles/suit: make use of SUIT_SEC_PASSWORD optional #20862

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Sep 13, 2024

Contribution description

Specifying the password of the SUIT private key on the command line and thereby committing it to shell history is a security issue.

Instead ask for the password interactively when an encrypted private key is used.

Testing procedure

if you don't have an encrypted SUIT key, create one first

$ make -C examples/suit_update SUIT_KEY=supersecret 
make: Entering directory '/home/benpicco/dev/RIOT/examples/suit_update'
Building application "suit_update" for "samr21-xpro" with CPU "samd21".

suit: generating key in /home/benpicco/.local/share/RIOT/keys
0) none
1) aes-256-cbc
Choose encryption for key file /home/benpicco/.local/share/RIOT/keys/supersecret.pem: 1
Enter PEM pass phrase:
Verifying - Enter PEM pass phrase:

sign a manifest with the new key

$ make -C examples/suit_update SUIT_KEY=supersecret suit/publish
make: Entering directory '/home/benpicco/dev/RIOT/examples/suit_update'
compiling /home/benpicco/dev/RIOT/dist/tools/riotboot_gen_hdr/bin/genhdr...
…
creating /home/benpicco/dev/RIOT/examples/suit_update/bin/samr21-xpro/riotboot_files/slot0.1726218363.bin...
creating /home/benpicco/dev/RIOT/examples/suit_update/bin/samr21-xpro/riotboot_files/slot1.1726218363.bin...

Enter encryption for key file /home/benpicco/.local/share/RIOT/keys/supersecret.pem: testtest

published "/home/benpicco/dev/RIOT/examples/suit_update/bin/samr21-xpro/suit_files/riot.suit.1726218363.bin"
       as "coap://localhost/fw/suit_update/samr21-xpro/riot.suit.1726218363.bin"
published "/home/benpicco/dev/RIOT/examples/suit_update/bin/samr21-xpro/suit_files/riot.suit.latest.bin"
       as "coap://localhost/fw/suit_update/samr21-xpro/riot.suit.latest.bin"
published "/home/benpicco/dev/RIOT/examples/suit_update/bin/samr21-xpro/riotboot_files/slot0.1726218363.bin"
       as "coap://localhost/fw/suit_update/samr21-xpro/slot0.1726218363.bin"
published "/home/benpicco/dev/RIOT/examples/suit_update/bin/samr21-xpro/riotboot_files/slot1.1726218363.bin"
       as "coap://localhost/fw/suit_update/samr21-xpro/slot1.1726218363

Issues/PRs references

Specifying the password of the SUIT private key on the command line
and thereby committing it to shell history is a security issue.

Instead ask for the password interactively when an encrypted private
key is used.
@github-actions github-actions bot added the Area: build system Area: Build system label Sep 13, 2024
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 13, 2024
@riot-ci
Copy link

riot-ci commented Sep 13, 2024

Murdock results

✔️ PASSED

50e3d61 makefiles/suit: allow to decrypt signing key with SUIT_SEC_PASSWORD

Success Failures Total Runtime
10197 0 10197 17m:27s

Artifacts

@kfessel
Copy link
Contributor

kfessel commented Sep 25, 2024

How will this work in an CI / automatic build?

@Teufelchen1
Copy link
Contributor

I am with @kfessel / passing through ENV should be possible. It also bothers me that beside the change of behavior / usage, there is no adjustment to the documentation or a tutorial. We have that documented somewhere, do we? 😰😱

@benpicco
Copy link
Contributor Author

benpicco commented Sep 26, 2024

We have that documented somewhere, do we? 😰😱

SUIT_SEC_PASSWORD is neither documented nor used in CI
(If you have an automated build that automatically decrypts the key by storing the password somewhere in cleartext, then what's the point of encrypting the key?)

@kfessel
Copy link
Contributor

kfessel commented Sep 26, 2024

Aren't there ci tools that provide a separate storage for password that are used in the build process and ingested through environment - but they probably are all providing a file-storage as well which is as easy to handle

If there is a buildsystem that just supports the first option or you want someone to not just be able to copy the keyfile (keyfiles might be floating around somwhere) with a seperate password ( in a "secure" ci env storage) you at least add a little extra barrier to just copy the file.

of cause no one should put their buildkeypassword int the make command line that would be crazy to do

@benpicco benpicco requested a review from kfessel September 26, 2024 12:28
@benpicco
Copy link
Contributor Author

I re-added the possibility to use SUIT_SEC_PASSWORD so CI that injects the password into the build environment keeps working.

@benpicco benpicco changed the title makefiles/suit: drop use of SUIT_SEC_PASSWORD makefiles/suit: make use of SUIT_SEC_PASSWORD optional Sep 30, 2024
@benpicco benpicco requested a review from maribu September 30, 2024 11:44
makefiles/suit.inc.mk Outdated Show resolved Hide resolved
@benpicco benpicco force-pushed the drop-SUIT_SEC_PASSWORD branch from c4a12bf to 50e3d61 Compare September 30, 2024 14:38
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Code looks good and I trust your testing

@benpicco benpicco enabled auto-merge September 30, 2024 15:18
@benpicco benpicco added this pull request to the merge queue Sep 30, 2024
Merged via the queue into RIOT-OS:master with commit 61df141 Sep 30, 2024
26 checks passed
@benpicco benpicco deleted the drop-SUIT_SEC_PASSWORD branch October 1, 2024 07:50
@benpicco benpicco added this to the Release 2024.10 milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants