-
Notifications
You must be signed in to change notification settings - Fork 39
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.
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
base: develop
Are you sure you want to change the base?
Conversation
This allows you to pass in the signing keys needed to sign the artefacts both inside the full disk image, but also the update images. The two relevant for the update artefacts are: code_sign and config_sign, which signs the kernel image and the grub config respectively.
| # 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}" ] && \ |
There was a problem hiding this comment.
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}/code-sign.crt" ] && \ | ||
| [ -s "${profile}/config-sign.key" ] ; then |
There was a problem hiding this comment.
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.jsonshim-sign.key,code-sign.key, andconfig-sign.keysign.env
(In the KMS path, config-sign.key is a public key, not a private key, so it doesn't fit cleanly.)
There was a problem hiding this comment.
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.
Co-authored-by: Ben Cressey <[email protected]>
I promise, I have run both clippy and fmt before submitting this!
An interesting observation, which you may or may not be aware of, is that because the disk image is generated and the vendor.crt is embedded into the shim, as well as that the gpg key for the grub config signature and both of these are written into the
EFI-Apartition, even if you disable Secure Boot on the server - the deployment of an update downloaded from a TUF repository, using a different PKI chain for thecode-sign.keyand a different gpg key forconfig-sign.key, will fail on both the shim validation and grub validation (these are done always despite Secure Boot status).Of course, completely theoretical - I would never have been reckless enough to encounter the above situation.
Issue number:
Closes #542
Description of changes:
This introduces three new environment variables to the build process
BUILDSYS_SBKEY_SHIM_SIGN_KEY_CONTENT,BUILDSYS_SBKEY_CODE_SIGN_KEY_CONTENTandBUILDSYS_SBKEY_CONFIG_SIGN_KEY_CONTENT.This allows you to pass in the signing keys needed to sign the artefacts both inside the full disk image, but also the update images.
The primary use-case for this would be to more conveniently enable CI builds without having to manage secrets as files (which few CI systems support in any larger extent).
Testing done:
I tested building both with a local profile and with the environment variables (we use these in building our own images in CI currently).
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.