| Summary: | kernel-fit-image signing key file sanity check requires a .key and a .crt file instead just one of them | ||
|---|---|---|---|
| Product: | [Build System, Metadata & Runtime] OE-Core | Reporter: | Tobias Pistora <pistora.tobias> |
| Component: | oe-core other | Assignee: | Tobias Pistora <pistora.tobias> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | Medium | CC: | ccasciato, mathieu.dubois-briand, paul, randy.macleod |
| Version: | 6.0.3 | ||
| Target Milestone: | 6.2 | ||
| Hardware: | x86 | ||
| OS: | Multiple | ||
| Whiteboard: | |||
| OS type for building Yocto: | --- | Type of Regression: | --- |
| Verified: | Documentation change: | No (bug/feature does not impact docs) | |
Hi Tobias, Is signing expected to work with only the key file? i.e. is the certificate actually needed? The code has moved around a little on master but it looks like it's doing the same check in _check_sign_key_files(), so this would want changing on master as well as wrynose if we do change things. (putting into needinfo state) (In reply to Paul Barker from comment #1) > Hi Tobias, > > Is signing expected to work with only the key file? i.e. is the certificate > actually needed? > > The code has moved around a little on master but it looks like it's doing > the same check in _check_sign_key_files(), so this would want changing on > master as well as wrynose if we do change things. Hi Paul, thanks for pointing that out, I did not look deeper into arguments for image generation. I just updated to Yocto 6 and ran into that new error. However, reading the related mkimage man pages lets me assume that the .crt file (which should contain the public key) is optional. The .key file (which contains the private key) is mandatory (https://man.archlinux.org/man/mkimage.1.en): " -k key-directory --key-dir key-directory Specifies the directory containing keys to use for signing. This directory should contain a private key file name.key for use with signing, and a certificate name.crt (containing the public key) for use with verification. The public key is only necessary when embedding it into another device tree using -K. name is the value of the signature node's ‘key-name-hint’ property. " The code in "openembedded-core/meta/lib/oe/fitimage.py" runs mkimage (right below the sanity check) as follows: " self._mkimage_sign, '-F', '-k', self._sign_keydir, '-r', fitfile " Where "self._sign_keydir" holds the path to the key files. As far as I understand the man page here, the public key is only used by mkimage when the -K option is used. But the -K option is never used in "fitimage.py". " -K key-destination --key-dest key-destination Specifies a compiled device tree binary file (typically .dtb) to write public key information into. When a private key is used to sign an image, the corresponding public key is written into this file for run-time verification. Typically the file here is the device tree binary used by CONFIG_OF_CONTROL in U-Boot. " However, my first bug assumption and proposed solution have not been right. This seems to require a bit more investigation. In my understanding the .crt file is not used by the current implementation. And in case the -K option will be implemented later, it would be optional, together with the .crt file. What I do not know is: does any other component require the .crt file (maybe Uboot does)? But in this case I would expect a sanity check in the component that requires it... For fixing the issue with minimal side effects I would suggest to keep the current sanity check as it is but just for the .key file (which is mandatory anyway). A warning could be prompted when the .crt file is missing. (In reply to Tobias Pistora from comment #3) > (In reply to Paul Barker from comment #1) > > Hi Tobias, > > > > Is signing expected to work with only the key file? i.e. is the certificate > > actually needed? > > > > The code has moved around a little on master but it looks like it's doing > > the same check in _check_sign_key_files(), so this would want changing on > > master as well as wrynose if we do change things. > > Hi Paul, > > thanks for pointing that out, I did not look deeper into arguments for image > generation. I just updated to Yocto 6 and ran into that new error. However, > reading the related mkimage man pages lets me assume that the .crt file > (which should contain the public key) is optional. The .key file (which > contains the private key) is mandatory > (https://man.archlinux.org/man/mkimage.1.en): > " > -k key-directory > --key-dir key-directory > Specifies the directory containing keys to use for signing. This > directory should contain a private key file name.key for use with signing, > and a certificate name.crt (containing the public key) for use with > verification. The public key is only necessary when embedding it into > another device tree using -K. name is the value of the signature node's > ‘key-name-hint’ property. > " > > The code in "openembedded-core/meta/lib/oe/fitimage.py" runs mkimage (right > below the sanity check) as follows: > " > self._mkimage_sign, > '-F', > '-k', self._sign_keydir, > '-r', fitfile > " > Where "self._sign_keydir" holds the path to the key files. > > As far as I understand the man page here, the public key is only used by > mkimage when the -K option is used. But the -K option is never used in > "fitimage.py". > " > -K key-destination > --key-dest key-destination > Specifies a compiled device tree binary file (typically .dtb) to write > public key information into. When a private key is used to sign an image, > the corresponding public key is written into this file for run-time > verification. Typically the file here is the device tree binary used by > CONFIG_OF_CONTROL in U-Boot. > " > > However, my first bug assumption and proposed solution have not been right. > This seems to require a bit more investigation. In my understanding the .crt > file is not used by the current implementation. And in case the -K option > will be implemented later, it would be optional, together with the .crt > file. What I do not know is: does any other component require the .crt file > (maybe Uboot does)? But in this case I would expect a sanity check in the > component that requires it... > > For fixing the issue with minimal side effects I would suggest to keep the > current sanity check as it is but just for the .key file (which is mandatory > anyway). A warning could be prompted when the .crt file is missing. small correction: the -K option could be add to the command via "mkimage_sign_args" function argument... Hi Tobias, Based on the mkimage man page you linked, we probably can just check for the .key file. Please submit a patch to the OE-core master branch to implement this. Once this is merged in master we can consider a backport to wrynose. The said patch was merged: https://git.openembedded.org/openembedded-core/commit/?id=dcc00da32303fa52ed1bf6853f2062b182682d98 |
Using the kernel-fit-image class in wrynose Yocto together with UBOOT_SIGN_ENABLE = "1", UBOOT_SIGN_KEYDIR = "/any/path", UBOOT_SIGN_KEYNAME = "key_file" and the file at /any/path/key_file.key causes the signature file sanity check to fail. The error message is "/any/path/key_file.key or .crt does not exist". Looking at the sanity check in "openembedded-core/meta/lib/oe/fitimage.py", line 589: the check condition seems to be the issue: # Some sanity checks because mkimage exits with 0 also without needed keys sign_key_path = os.path.join(self._sign_keydir, self._sign_keyname_conf) if not os.path.exists(sign_key_path + '.key') or not os.path.exists(sign_key_path + '.crt'): bb.fatal("%s.key or .crt does not exist" % sign_key_path) if self._sign_individual: sign_key_img_path = os.path.join(self._sign_keydir, self._sign_keyname_img) if not os.path.exists(sign_key_img_path + '.key') or not os.path.exists(sign_key_img_path + '.crt'): bb.fatal("%s.key or .crt does not exist" % sign_key_img_path) The current logic is "throw an error when one of the file types in sign_key_path is missing". The expected logic would be "throw an error when both file types in sign_key_path is missing". Therefore, changing the "or" to an "and" would resolve this issue. Notice that the same issue probably applies to the condition 4 lines below that does the same check with "sign_key_img_path" variable (I have not tested that). Workaround: My current workaround for this issue is adding a duplicate of the key_file.key file in the same directory and naming it key_file.crt. Using a symbolic link instead of the duplicate may also work.