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

tests: add spread test for passphrase support #14867

Open
wants to merge 9 commits into
base: fde-manager-features
Choose a base branch
from
Open

tests: add spread test for passphrase support #14867

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0b131a3
tests: add spread test for hybrid passphrase support
ZeyadYasser Dec 17, 2024
df1a7b4
tests/nested/manual/muinstaller-core: add passphrase smoke test variant
ZeyadYasser Dec 17, 2024
d897ea6
fixup! tests: exit if qemu does not support socket serial
ZeyadYasser Dec 20, 2024
a4cae1c
fixup! tests: replace jq with gojq
ZeyadYasser Dec 20, 2024
522f767
fixup! tests: fix unbound PASSPHRASE variable for default case
ZeyadYasser Dec 20, 2024
0f3c300
fixup! tests: address review comments
ZeyadYasser Jan 14, 2025
1d1a2df
fixup! tests: revert to using netcat for sending passphrase to nested vm
ZeyadYasser Jan 28, 2025
685389a
fixup! tests: revert remote.wait-for changes
ZeyadYasser Feb 4, 2025
6678a7f
fixup! tests: address review comments (thanks @alfonsosanchezbeato)
ZeyadYasser Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions tests/lib/assertions/developer1-2410-classic-dangerous.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
{
"type": "model",
"authority-id": "developer1",
"series": "16",
"brand-id": "developer1",
"model": "developer1-2410-classic-dangerous",
"architecture": "amd64",
"timestamp": "2024-04-09T22:00:00+00:00",
"grade": "dangerous",
"base": "core22",
Copy link
Member

Choose a reason for hiding this comment

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

We probably want core24 here. Or is it maybe necessary because the kernel in classic-24.10/edge uses core24 incorrectly?

Copy link
Contributor Author

@ZeyadYasser ZeyadYasser Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

AFAIK, FDE passphrase support is aimed for classic-25.04, which seems to use core22 as its base https://github.com/canonical/models/blob/master/ubuntu-classic-2504-amd64.json. I was mostly trying to mimic the target system. I don't mind switching to core24 though.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth asking in the next sync, the target should be core24 eventually.

"classic": "true",
"distribution": "ubuntu",
"serial-authority": [
"generic"
],
"snaps": [
{
"default-channel": "classic-24.10/edge",
"id": "UqFziVZDHLSyO3TqSWgNBoAdHbLI4dAH",
"name": "pc",
"type": "gadget"
},
{
"default-channel": "24.10/edge",
"id": "pYVQrBcKmBa0mZ4CCN7ExT6jH8rY1hza",
"name": "pc-kernel",
"type": "kernel"
},
{
"default-channel": "latest/edge",
"id": "amcUKQILKXHHTlmSa7NMdnXSx02dNeeT",
"name": "core22",
"type": "base"
},
{
"default-channel": "latest/stable",
"id": "PMrrV4ml8uWuEUDBT8dSGnKUYbevVhc4",
"name": "snapd",
"type": "snapd"
}
]
}
14 changes: 14 additions & 0 deletions tests/lib/nested.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1204,13 +1204,20 @@ nested_start_core_vm_unit() {
PARAM_RTC="${NESTED_PARAM_RTC:-}"
PARAM_EXTRA="${NESTED_PARAM_EXTRA:-}"

local EXPECT_PASSPHRASE
EXPECT_PASSPHRASE=${NESTED_EXPECT_PASSPHRASE:-}
Copy link
Member

Choose a reason for hiding this comment

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

It seems that the practice is insted to have a statement like : "${NESTED_EXPECT_PASSPHRASE:=}" at the bgining of nested.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, Thanks! updated


# Open port 7777 on the host so that failures in the nested VM (e.g. to
# create users) can be debugged interactively via
# "telnet localhost 7777". Also keeps the logs
#
# XXX: should serial just be logged to stdout so that we just need
# to "journalctl -u $NESTED_VM" to see what is going on ?
if "$QEMU" -version | grep '2\.5'; then
if [ -z "$EXPECT_PASSPHRASE" ]; then
echo "internal error: NESTED_EXPECT_PASSPHRASE is set and qemu doesn't support chardev over socket"
Copy link
Contributor

Choose a reason for hiding this comment

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

do you remember which systems this old version appears on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently it is xenial https://launchpad.net/ubuntu/xenial/+source/qemu.

bboozzoo marked this conversation as resolved.
Show resolved Hide resolved
exit 1
fi
# XXX: remove once we no longer support xenial hosts
PARAM_SERIAL="-serial file:${NESTED_LOGS_DIR}/serial.log"
else
Expand Down Expand Up @@ -1371,6 +1378,13 @@ nested_start_core_vm_unit() {
${PARAM_CD} \
${PARAM_EXTRA} " "${PARAM_REEXEC_ON_FAILURE}"

if [ -n "$EXPECT_PASSPHRASE" ]; then
# Wait for passphrase prompt from serial log file.
retry -n 120 --wait 1 sh -c "MATCH \"Please enter the passphrase\" < ${NESTED_LOGS_DIR}/serial.log"
# Enter passphrase to continue boot.
echo "$EXPECT_PASSPHRASE" | netcat -N localhost 7777
fi

local EXPECT_SHUTDOWN
EXPECT_SHUTDOWN=${NESTED_EXPECT_SHUTDOWN:-}

Expand Down
34 changes: 28 additions & 6 deletions tests/lib/tools/setup_nested_hybrid_system.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ run_muinstaller() {
local label="${7}"
local disk="${8}"
local kern_mods_comp="${9:-}"
Copy link
Contributor

Choose a reason for hiding this comment

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

otherwise the parameter indices and the shifting, combined with set -u make no sense. 9th parameter cannot be optional and have a default value expansion if 10th is expected to be present and if it's missing the code fails due to errors on unset variables

Suggested change
local kern_mods_comp="${9:-}"
local kern_mods_comp="${9}"

local passphrase="${10}"
shift 10
local extra_muinstaller_args=("${@}")

# ack the needed assertions
snap ack "${kernel_assertion}"
Expand Down Expand Up @@ -164,10 +167,15 @@ fi
EOF
remote.exec "chmod +x /home/user1/mk-classic-rootfs-wrapper.sh"

remote.exec "sudo muinstaller \
-label ${label} \
-device ${install_disk} \
-rootfs-creator /home/user1/mk-classic-rootfs-wrapper.sh"
muinstaller_args=()
muinstaller_args+=("-label $label")
muinstaller_args+=("-device $install_disk")
muinstaller_args+=("-rootfs-creator /home/user1/mk-classic-rootfs-wrapper.sh")
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't each argument be separate? Like args+=("-label" "$label"), etc. I think that in the end is not relevant as remote.exec somewhat is possibly splitting arguments, but still.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this, updated

if [ -n "$passphrase" ]; then
muinstaller_args+=("-passphrase \"$passphrase\"")
fi
muinstaller_args+=("${extra_muinstaller_args[@]}")
remote.exec sudo muinstaller "${muinstaller_args[@]}"

remote.exec "sudo sync"

Expand Down Expand Up @@ -196,7 +204,11 @@ EOF
fi

# Start installed image
tests.nested create-vm core --keep-firmware-state
if [ -n "$passphrase" ]; then
tests.nested create-vm core --keep-firmware-state --expect-passphrase "$passphrase"
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: maybe better just --passphrase? expect-passphrase sounds a bit like a boolean.

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 agree, updated

else
tests.nested create-vm core --keep-firmware-state
fi
}

main() {
Expand All @@ -209,6 +221,8 @@ main() {
local label="classic"
local disk=""
local kern_mods_comp=""
local passphrase=""
local extra_muinstaller_args=()
while [ $# -gt 0 ]; do
case "$1" in
--model)
Expand Down Expand Up @@ -248,6 +262,14 @@ main() {
kern_mods_comp="${2}"
shift 2
;;
--passphrase)
passphrase="${2}"
shift 2
;;
--extra-muinstaller-arg)
extra_muinstaller_args+=("${2}")
shift 2
;;
--*|-*)
echo "Unknown option ${1}"
exit 1
Expand Down Expand Up @@ -338,7 +360,7 @@ main() {

run_muinstaller "${model_assertion}" "${store_dir}" "${gadget_snap}" \
"${gadget_assertion}" "${kernel_snap}" "${kernel_assertion}" "${label}" \
"${disk}" "${kern_mods_comp}"
"${disk}" "${kern_mods_comp}" "${passphrase}" "${extra_muinstaller_args[@]}"
Copy link
Member

Choose a reason for hiding this comment

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

The way it is initialized, you get only one argument here, ${extra_muinstaller_args[0]}

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 am not sure I fully understand.

I created a sample script to try and understand better but it seems to behave as expected.

#!/bin/bash

run_muinstaller() {
    local arg1="$1"
    shift 1
    local extra_muinstaller_args=("${@}")

    echo "arg1: $arg1"

    for arg in "${extra_muinstaller_args[@]}"
    do
    echo "extra_muinstaller_arg: $arg"
    done
}

main() {
    local extra_muinstaller_args=()
    while [ $# -gt 0 ]; do
        case "$1" in
            --extra-muinstaller-arg)
                extra_muinstaller_args+=("${2}")
                shift 2
                ;;
            --*|-*)
                echo "Unknown option ${1}"
                exit 1
                ;;
            *)
                shift
                ;;
        esac
    done

    run_muinstaller "test-arg1" "${extra_muinstaller_args[@]}"
}

main "$@"

Running ./test.sh --extra-muinstaller-arg "-kdf-type=pbkdf2" --extra-muinstaller-arg "-passphrase=PASS"

arg1: test-arg1
extra_muinstaller_arg: -kdf-type=pbkdf2
extra_muinstaller_arg: -passphrase=PASS

Copy link
Member

Choose a reason for hiding this comment

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

I thought --extra-muinstaller-arg was called like --extra-muinstaller-arg "<arg1> <arg2>", not individually per argument - but this makes sense, thanks for the explanation.

)
}

Expand Down
10 changes: 7 additions & 3 deletions tests/lib/tools/tests.nested
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ create_vm() {
;;
esac

local NESTED_PARAM_CD NESTED_CPUS NESTED_MEM NESTED_PARAM_EXTRA NESTED_KEEP_FIRMWARE_STATE
local NESTED_PARAM_CD NESTED_CPUS NESTED_MEM NESTED_PARAM_EXTRA NESTED_KEEP_FIRMWARE_STATE NESTED_EXPECT_PASSPHRASE
while [ $# -gt 0 ]; do
case "$1" in
--param-cdrom)
Expand All @@ -156,16 +156,20 @@ create_vm() {
NESTED_KEEP_FIRMWARE_STATE=1
shift
;;
--expect-passphrase)
NESTED_EXPECT_PASSPHRASE="$2"
shift 2
;;
*)
echo "tests.nested: unsupported parameter $1" >&2
exit 1
;;
esac
done

export NESTED_PARAM_CD NESTED_CPUS NESTED_MEM NESTED_PARAM_EXTRA NESTED_KEEP_FIRMWARE_STATE
export NESTED_PARAM_CD NESTED_CPUS NESTED_MEM NESTED_PARAM_EXTRA NESTED_KEEP_FIRMWARE_STATE NESTED_EXPECT_PASSPHRASE
"$action"
unset NESTED_PARAM_CD NESTED_CPUS NESTED_MEM NESTED_PARAM_EXTRA NESTED_KEEP_FIRMWARE_STATE
unset NESTED_PARAM_CD NESTED_CPUS NESTED_MEM NESTED_PARAM_EXTRA NESTED_KEEP_FIRMWARE_STATE NESTED_EXPECT_PASSPHRASE
}

is_nested() {
Expand Down
33 changes: 31 additions & 2 deletions tests/nested/manual/muinstaller-core/task.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@ environment:
NESTED_ENABLE_TPM: true
NESTED_ENABLE_SECURE_BOOT: true

# by default, we don't do passphrase authentication
PASSPHRASE: ""

# encrypted + passphrase auth smoke test
# TODO: extract a more comprehensive core24 test similar
# to tests/nested/manual/passphrase-support-on-hybrid.
PASSPHRASE/passphrase_auth: "ubuntu"

# unencrypted case
NESTED_ENABLE_TPM/plain: false
NESTED_ENABLE_SECURE_BOOT/plain: false
Expand Down Expand Up @@ -47,7 +55,6 @@ prepare: |
echo "This test needs test keys to be trusted"
exit
fi
apt install dosfstools

restore: |
rm -rf ./classic-root
Expand All @@ -58,6 +65,16 @@ execute: |
#shellcheck source=tests/lib/nested.sh
. "$TESTSLIB"/nested.sh

# TODO: remove this condition when passphrase support lands.
if [ -n "$PASSPHRASE" ]; then
echo "SKIPPING: passphrase support has not landed yet"
exit 0
fi
# if ! os.query is-noble && [ -n "$PASSPHRASE" ]; then
# echo "SKIPPING: only run passphrase support variant for core24"
# exit 0
# fi

version="$(nested_get_version)"

# Retrieve the gadget
Expand Down Expand Up @@ -107,6 +124,9 @@ execute: |
snap pack ./snap-with-comps --filename=snap-with-comps.snap
snap pack ./comp1 --filename=snap-with-comps+comp1.comp

# TODO: refactor preparation/hacks in a reusable script
# similar to setup_nested_hybrid_system.sh

# prepare a core seed
SEED_DIR="core-seed"
wget -q https://raw.githubusercontent.com/snapcore/models/master/ubuntu-core-"$version"-amd64-dangerous.model -O my.model
Expand Down Expand Up @@ -230,6 +250,11 @@ execute: |
remote.push optional-install.json
muinstaller_args="$muinstaller_args -optional ./optional-install.json"
fi
if [ -n "$PASSPHRASE" ]; then
# pbkdf2 is used because it is less memory intensive than in
# process argon2i* variants.
muinstaller_args="$muinstaller_args -passphrase $PASSPHRASE --kdf-type pbkdf2"
fi

remote.exec "sudo muinstaller $muinstaller_args"

Expand All @@ -245,7 +270,11 @@ execute: |
mv fake-disk.img "$NESTED_IMAGES_DIR/$IMAGE_NAME"

# Start installed image
tests.nested create-vm core --keep-firmware-state
if [ -n "$PASSPHRASE" ]; then
tests.nested create-vm core --keep-firmware-state --expect-passphrase "$PASSPHRASE"
else
tests.nested create-vm core --keep-firmware-state
fi

# things look fine
remote.exec "cat /etc/os-release" | MATCH 'NAME="Ubuntu Core"'
Expand Down
1 change: 0 additions & 1 deletion tests/nested/manual/muinstaller-real/task.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ prepare: |
echo "This test needs test keys to be trusted"
exit
fi
apt install dosfstools kpartx
"$TESTSTOOLS"/store-state setup-fake-store "$STORE_DIR"

restore: |
Expand Down
Loading
Loading