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.

Sign up for GitHub

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

Jump to bottom

feat(internal): Pass 8 CSPRNG random integer words #1899

Merged
merged 4 commits into from
Dec 4, 2024
Merged

feat(internal): Pass 8 CSPRNG random integer words #1899

merged 4 commits into from
Dec 4, 2024

Conversation

craciunoiuc
Copy link
Member

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran make fmt on your commit series before opening this PR;
  • Updated relevant documentation.

Description of changes

@craciunoiuc craciunoiuc force-pushed the craciunoiuc/random-rng branch from e92dc44 to 28df6c4 Compare September 23, 2024 11:19
nderjung
nderjung approved these changes Sep 23, 2024
View reviewed changes
Copy link
Member

@nderjung nderjung left a comment

Choose a reason for hiding this comment

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

All good on my side implementation-wise. Thanks!

Regarding the interaction with Unikraft itself, I'd like to cc @razvand and @michpappas before merging to help verify everything in conjunction with unikraft/unikraft#1496

@craciunoiuc craciunoiuc force-pushed the craciunoiuc/random-rng branch from 28df6c4 to 1429fe9 Compare September 24, 2024 10:25
@craciunoiuc
Copy link
Member Author

Updated to check for RDRAND/RDSEED

If they are both present then ignore the config option and let Unikraft take its own bytes from the device/generator

@craciunoiuc craciunoiuc force-pushed the craciunoiuc/random-rng branch 2 times, most recently from e7925f2 to 5771822 Compare October 17, 2024 10:13
@craciunoiuc craciunoiuc changed the title feat(internal): Pass 10 CSPRNG random bytes feat(internal): Pass 8 CSPRNG random integer words Oct 17, 2024
michpappas
michpappas requested changes Nov 27, 2024
View reviewed changes
Copy link
Member

@michpappas michpappas left a comment

Choose a reason for hiding this comment

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

@craciunoiuc just a few comments. Also if you update this, it would be great if you posted a few of the generated cmdlines.

unikraft/export/v0/ukrandom/params.go Outdated Show resolved Hide resolved
internal/cli/kraft/run/runner_kraftfile_runtime.go Outdated Show resolved Hide resolved
@craciunoiuc
feat(unikraft): Add ukrandom library parameters
b7c27ee 
Signed-off-by: Cezar Craciunoiu <cezar.craciunoiu@unikraft.io>
@craciunoiuc craciunoiuc force-pushed the craciunoiuc/random-rng branch from 5771822 to b6dbfba Compare November 27, 2024 16:47
@craciunoiuc
Copy link
Member Author 

qemu-system-x86_64 -append random.seed=[ 0xef59fa23 0x1eba1280 0xb0972ca2 0xc734f02f 0x2079dc04 0x158e0490 0x656d5095 0xa407c115 ] -- /usr/bin/nginx  -cpu qemu64,+pdpe1gb,+rdrand,+rdseed,-vmx,-svm -daemonize -device virtio-net-pci,mac=02:b0:b0:84:3e:01,netdev=hostnet0 -device pvpanic -display none -kernel /home/cez/.local/share/kraftkit/runtime/99876f61-99a0-4072-a2f8-5b6483635e20/unikraft/bin/kernel -machine pc -m size=128M -monitor unix:/home/cez/.local/share/kraftkit/runtime/99876f61-99a0-4072-a2f8-5b6483635e20/qemu_mon.sock,server,nowait -name 99876f61-99a0-4072-a2f8-5b6483635e20 -netdev user,id=hostnet0,hostfwd=tcp::8081-:80 -nographic -no-reboot -S -parallel none -pidfile /home/cez/.local/share/kraftkit/runtime/99876f61-99a0-4072-a2f8-5b6483635e20/machine.pid -qmp unix:/home/cez/.local/share/kraftkit/runtime/99876f61-99a0-4072-a2f8-5b6483635e20/qemu_control.sock,server,nowait -qmp unix:/home/cez/.local/share/kraftkit/runtime/99876f61-99a0-4072-a2f8-5b6483635e20/qemu_events.sock,server,nowait -rtc base=utc -serial file:/home/cez/.local/share/kraftkit/runtime/99876f61-99a0-4072-a2f8-5b6483635e20/machine.log -smp cpus=1,threads=1,sockets=1 -vga none

this is how it looks for one run, note that quotes don't show up in the logs

michpappas
michpappas approved these changes Nov 27, 2024
View reviewed changes
Copy link
Member

@michpappas michpappas 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, thanks a lot!

Reviewed-by: Michalis Pappas michalis@unikraft.io

@craciunoiuc
feat(internal): Pass 8 CSPRNG random integer words
4a8d1c0 
Depends-on: unikraft/unikraft#1496

Signed-off-by: Cezar Craciunoiu <cezar.craciunoiu@unikraft.io>
@craciunoiuc craciunoiuc force-pushed the craciunoiuc/random-rng branch 3 times, most recently from 183beaf to c24a60a Compare November 28, 2024 17:38
nderjung
nderjung approved these changes Nov 28, 2024
View reviewed changes
Copy link
Member

@nderjung nderjung left a comment

Choose a reason for hiding this comment

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

Thanks everyone! Looks good from my side. I'll merge this when unikraft/unikraft#1496 is merged into staging.

@nderjung
Copy link
Member

Linking for completeness: klauspost/cpuid#152

@craciunoiuc craciunoiuc force-pushed the craciunoiuc/random-rng branch from c24a60a to 3f4fa37 Compare November 28, 2024 18:46
@razvand razvand self-assigned this Nov 29, 2024
michpappas
michpappas requested changes Nov 29, 2024
View reviewed changes
Copy link
Member

@michpappas michpappas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@craciunoiuc besides the comments below, I think we should have the conditions on for generating the cmdline rewritten in a more clear / readable way, especially given the security-related nature of that code 🙏🏼

Also since your changes for FEAT_RNG were merged upstream, please consider if you should also update the CPUID check for Arm too.

internal/cli/kraft/run/runner_kraftfile_runtime.go Outdated Show resolved Hide resolved
internal/cli/kraft/run/runner_kraftfile_unikraft.go Outdated Show resolved Hide resolved
internal/cli/kraft/run/runner_package.go Outdated Show resolved Hide resolved
machine/firecracker/v1alpha1.go Outdated Show resolved Hide resolved
machine/qemu/v1alpha1.go Outdated Show resolved Hide resolved
@craciunoiuc
Copy link
Member Author

Checks are automatically done when checking for the RDRAND features. Imagine it as a an interface with different implementations depending on the OS running the library. So all is good.

@craciunoiuc craciunoiuc force-pushed the craciunoiuc/random-rng branch 2 times, most recently from 4cc2f51 to 37a3372 Compare December 2, 2024 15:37
@craciunoiuc craciunoiuc force-pushed the craciunoiuc/random-rng branch from 37a3372 to 918cd7b Compare December 2, 2024 16:04
@craciunoiuc craciunoiuc force-pushed the craciunoiuc/random-rng branch from 918cd7b to cb8609d Compare December 2, 2024 16:08
michpappas
michpappas approved these changes Dec 2, 2024
View reviewed changes
Copy link
Member

@michpappas michpappas 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, thanks!

Reviewed-by: Michalis Pappas michalis@unikraft.io

@craciunoiuc
gomod(deps): Bump 'klauspost/cpuid' to the latest version
3b378d6 
Also update API usage.

Signed-off-by: Cezar Craciunoiu <cezar.craciunoiu@unikraft.io>
@craciunoiuc
fix(machine): Add 'rdrand' feature even if qemu < 8.0.0
5d007fa 
This was added in qemu 4.0.0 and we refuse to run
when 4.2.0 or lower is used anyway. Unikraft can now
with rdrand only, so this is not a hard requriment.

Signed-off-by: Cezar Craciunoiu <cezar.craciunoiu@unikraft.io>
@craciunoiuc craciunoiuc force-pushed the craciunoiuc/random-rng branch from cb8609d to 5d007fa Compare December 3, 2024 17:14
razvand
razvand approved these changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

@razvand razvand merged commit 768c6e6 into unikraft:staging Dec 4, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michpappas michpappas michpappas approved these changes

@nderjung nderjung nderjung approved these changes

@razvand razvand razvand approved these changes

Assignees

@razvand razvand

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@craciunoiuc @nderjung @razvand @michpappas