Skip to content

makefiles/suit: make use of SUIT_SEC_PASSWORD optional #20862

Merged
benpicco merged 2 commits intoRIOT-OS:masterfrom
benpicco:drop-SUIT_SEC_PASSWORD
Sep 30, 2024
Merged

makefiles/suit: make use of SUIT_SEC_PASSWORD optional #20862
benpicco merged 2 commits intoRIOT-OS:masterfrom
benpicco:drop-SUIT_SEC_PASSWORD

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Sep 13, 2024 •
edited by Teufelchen1
Loading

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

@benpicco
makefiles/suit: drop use of SUIT_SEC_PASSWORD
1517949 
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
Copy Markdown

riot-ci commented Sep 13, 2024 •
edited
Loading

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
Copy Markdown
Contributor

kfessel commented Sep 25, 2024 •
edited
Loading

How will this work in an CI / automatic build?

@Teufelchen1
Copy link
Copy Markdown
Contributor

Teufelchen1 commented Sep 25, 2024

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
Copy Markdown
Contributor Author

benpicco commented Sep 26, 2024 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Contributor Author

benpicco commented Sep 30, 2024

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
maribu
maribu reviewed Sep 30, 2024
View reviewed changes
@benpicco benpicco force-pushed the drop-SUIT_SEC_PASSWORD branch from c4a12bf to 50e3d61 Compare September 30, 2024 14:38
maribu
maribu approved these changes Sep 30, 2024
View reviewed changes
Copy link
Copy Markdown
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
@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

Reviewers

@maribu maribu maribu approved these changes

@bergzand bergzand Awaiting requested review from bergzand

@Teufelchen1 Teufelchen1 Awaiting requested review from Teufelchen1

@fabian18 fabian18 Awaiting requested review from fabian18

@kfessel kfessel Awaiting requested review from kfessel

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@benpicco @riot-ci @kfessel @Teufelchen1 @maribu