Skip to content

feat: add support for 'sbkey' being passed in through ENV #543

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

feat: add support for 'sbkey' being passed in through ENV #543

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
59baa4f
feat: add support for 'sbkey' profile 'env'
mikn May 16, 2025
4b6568c
Update tools/buildsys/src/builder.rs
mikn Jun 2, 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
17 changes: 17 additions & 0 deletions tools/buildsys/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -821,6 +821,23 @@ fn secrets_args() -> Result<Vec<String>> {
);
}

// Add environment variables for secure boot signing keys, mapping them to the
// corresponding file names that the local profile expects
let env_to_file_map = [
("BUILDSYS_SBKEY_SHIM_SIGN_KEY_CONTENT", "shim-sign.key"),
("BUILDSYS_SBKEY_CODE_SIGN_KEY_CONTENT", "code-sign.key"),
("BUILDSYS_SBKEY_CONFIG_SIGN_KEY_CONTENT", "config-sign.key"),
];

for (env_var, file_name) in env_to_file_map {
if let Ok(content) = env::var(env_var) {
if !content.is_empty() {
// Only add this secret if the environment variable is set and not empty
args.build_secret("env", file_name, env_var);
}
}
}

let ca_bundle_var = "BUILDSYS_CACERTS_BUNDLE_OVERRIDE";
let ca_bundle_value =
env::var(ca_bundle_var).context(error::EnvironmentSnafu { var: ca_bundle_var })?;
Expand Down
19 changes: 15 additions & 4 deletions twoliter/embedded/Makefile.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,18 +688,29 @@ profile="${BUILDSYS_SBKEYS_PROFILE_DIR}"

found=0
# A local profile has signing keys and certificates, while an AWS profile
# has a config for the aws-kms-pkcs11 helper. Either type is supported.
# has a config for the aws-kms-pkcs11 helper. The environment variables profile
# has keys provided through BUILDSYS_SBKEY_* environment variables. Any of
# these profile types is supported.
if [ -s "${profile}/shim-sign.key" ] && \
[ -s "${profile}/shim-sign.crt" ] && \
[ -s "${profile}/code-sign.key" ] && \
[ -s "${profile}/code-sign.crt" ] ; then
[ -s "${profile}/code-sign.crt" ] && \
[ -s "${profile}/config-sign.key" ] ; then
Comment on lines +697 to +698
Copy link
Contributor

@bcressey bcressey May 30, 2025

Choose a reason for hiding this comment

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

I'm not thrilled with this interface since it creates ambiguity as to what keys will be used if both the environment variable and the file are present for a profile.

Here, we'd pass this first set of checks but silently end up using the environment variables instead of the files, assuming that the last value for a particular secret on the docker build command line is what ends up being used.

This isn't really a new issue, but the more types of signing profiles we have, the more potential for confusion that exists.

It might be better to have a different "type" of profile, with a file like sign.env that gave the variables that are expected to be set, e.g.:

SHIM_SIGN_KEY_ENV_VAR=...
CODE_SIGN_KEY_ENV_VAR=...
CONFIG_SIGN_KEY_ENV_VAR=...

Then a check could assert that we have only one type for our set of signing keys:

  • kms-sign.json
  • shim-sign.key, code-sign.key, and config-sign.key
  • sign.env

(In the KMS path, config-sign.key is a public key, not a private key, so it doesn't fit cleanly.)

Copy link
Contributor Author

@mikn mikn Jun 2, 2025

Choose a reason for hiding this comment

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

This seems like perhaps a larger project, to cleanly separate the profiles also in the builder docker stage.

It was actually the initial approach I took, but realised that it increased the code required significantly for little benefit (without an understanding of the longer roadmap of signing key support), hence I decided to limit the PR to outside of the docker build stage.

I think the two modifications you mention, correcting the prefix to BUILDSYS_SBKEYS_ instead of BUILDSYS_SBKEY_ and injecting the profile name into the env variable name are very good improvements, but I am wary of injecting logic into the docker build stage also unless there are other benefits you see?

Putting the profile name into the environment variables should be a reasonable defence against confusion of which keys are being used.

let found+=1
elif [ -s "${profile}/kms-sign.json" ] ; then
elif [ -s "${profile}/kms-sign.json" ] && \
[ -s "${profile}/config-sign.key" ] ; then
let found+=1
# Environment variables case - check if all required env vars are set
elif [ -n "${BUILDSYS_SBKEY_SHIM_SIGN_KEY_CONTENT}" ] && \
[ -n "${BUILDSYS_SBKEY_CODE_SIGN_KEY_CONTENT}" ] && \
[ -n "${BUILDSYS_SBKEY_CONFIG_SIGN_KEY_CONTENT}" ] && \
Comment on lines +703 to +706
Copy link
Contributor

@bcressey bcressey May 30, 2025

Choose a reason for hiding this comment

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

It'd be better to associate these variables with the profile somehow, since otherwise the wrong keys could be used if the profile is changed.

Rough example:

SHIM_SIGN_KEY_CONTENT="BUILDSYS_SBKEYS_${BUILDSYS_SBKEYS_PROFILE^^}_SHIM_SIGN_KEY_CONTENT"
CODE_SIGN_KEY_CONTENT="BUILDSYS_SBKEYS_${BUILDSYS_SBKEYS_PROFILE^^}_CODE_SIGN_KEY_CONTENT"
CONFIG_SIGN_KEY_CONTENT="BUILDSYS_SBKEYS_${BUILDSYS_SBKEYS_PROFILE^^}_CONFIG_SIGN_KEY_CONTENT"

if [ -n "${!SHIM_SIGN_KEY_CONTENT}" ] && \
   [ -n "${!CODE_SIGN_KEY_CONTENT}" ] && \
   [ -n "${!CONFIG_SIGN_KEY_CONTENT}" ] ; then
  echo "FOUND KEYS"
fi

[ -s "${profile}/shim-sign.crt" ] && \
[ -s "${profile}/code-sign.crt" ] ; then
let found+=1
fi

expected=1
for f in {PK,KEK,db,vendor}.crt config-sign.key efi-vars.{json,aws} ; do
for f in {PK,KEK,db,vendor}.crt efi-vars.{json,aws} ; do
let expected+=1
[ -s "${profile}/${f}" ] && let found+=1
done
Expand Down