Refactor tkey-verification: new ways of accessing compiled-in assets #19
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
dehanj
reviewed
May 21, 2024
dehanj
force-pushed
the
signer
branch
5 times, most recently
from
9ec8987 to
5b577a9
Compare
mchack-work
force-pushed
the
signer
branch
3 times, most recently
from
f1a1757 to
15b27d8
Compare
mchack-work
force-pushed
the
signer
branch
6 times, most recently
from
d71b79a to
5118321
Compare
mchack-work
changed the title
mchack-work
force-pushed
the
signer
branch
2 times, most recently
from
c1d20ad to
979ac2c
Compare
Please have a look @dehanj. Do you think we should squash the entire thing into something like the new PR description or do we leave the individual commits? Some of them are mostly small fixes. Not sure what to do about the first commit... It's from the tkey-libs branch which has been abondened. Do we even try to remove it? |
mchack-work
force-pushed
the
signer
branch
2 times, most recently
from
5b130a4 to
e9288e3
Compare
- We include compiled device app binaries in the repo. - We move internal packages used to get at assets like app bins, firmwares, and vendor pubkeys under the tkey-verification command.
We turn this into a command in tkey-verification itself instead.
- tkeyclient instead of old tk1 - tkeysign instead of tk1sign - Update go packages - Use digests as strings - Rename some variables as a consequence of above
- Include long-lived CA and certificates for testing purposes. - Remove gotools and, with it, the certstrap tool. Instead let the user install it themselves, if wanted. - Change README and Makefile accordingly.
Terminate in a nice way if the user types Control-C or something.
- Silence lint for 0644 mode - Pre-alloc slices - Remove unused variables - Revise linting config - Enable er113 - Disable unparam
Especially when doing verification we just want to output the UDI and if the TKey is genuine or not.
We split the errors into different categories to make it more easy for a user to reason about what went wrong. - commFailed: I/O errors, communication with the TKey, with an HTTP server, or a local file. - parseFailure: Parsing something from external sources failed. - missing: Something is missing in the binary to complete a verification. - notFound: Using external data we can't find something. - verificationFailed: a problem with verification of firmware, the vendor signature or the signature of the challenge.
- Build actual man page depending on scdoc source - Add convenience target for man page
Update because of the refactoring to include the binaries in the repo, allow several vendor keys, and change the building. Co-authored-by: dehanj <jobson@tillitis.se>
- Use constant errors. - Use more specific details as log messages. - Add check for correct app digest length. - Move MessageLen to only place where it's used, in the API's error handling, and make it a constant.
- Use constant errors where possible - Use error types when not possible.
- Verify against all known vendor pubkeys - Remove the check for just 1 vendor key. - Increase error reporting.
Exit cleanly on errors, closing the TKey connection.
Explain the assets and their functions: - appbins - firmwares - vendor pubkeys
Used only by removed program show-pubkey
We index the device apps by hash. Also look after indexing that the app name and tag is the expected one.
- Latest() -> Current() - latestAppHash -> currentAppHash
Since we verify against all known vendor keys we might as well make the digest of the app used for vendor signing configurable instead. It's only used when we do the `serve-siger` command. Let the vendor decide which of the built-in apps they want to use. When we list the vendor keys in --built, list all of them, not just the formerly built-in current vendor key.
- Instead of hard coded digest of the app to be used for device signing during provisioning, we add it to the configuration file as `signingapphash`. - As a consequence of above, redo configuration loading and make it explictly for either server-signer or remote-sign commands. - As a consequence of this, move TLS config and server setup to the remote sign command directly. - Remove the undocumented sign-challenge command instead of updating it to work with the new version.
dehanj
approved these changes
Jul 2, 2024
There was a problem hiding this comment.
Managed to squash another 6 commits.
At this stage it looks good to me.
I have verified:
- multiple vendor signing key support, and hence do signing and verifying
- that it does not break verification files out in the field
- Setting up the a test environment with QEMU
- the new speed command
- Added the correct year for the copyright notices
- Revised part of readme (squashed)
Ready to merge.
Allow setting of speed for communication with TKey in all commands.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactoring to:
Include the device binaries in the repo directly. Remove the build scripts for verisigner. Use the ordinary tkey-device-signer for new stuff. If a user wants to build verisigner, they have to use old tags and the instructions there.
Use ordinary tkeyclient package instead of internal version.
Use the ordinary tkeysign package to communicate with both signer, these days updated to be able to get a firmware hash, and the old verisigner.
Use new way of accessing internal assets like: vendor public keys, known firmwares, and the app binaries. We're now more idiomatic in the access and use something like
foos := NewFoo()with methods onfooto do something with them.Support multiple vendor keys at the same time for verification.
Make the external command
show-pubkeyinto just another command intkey-verification.Error handling much changed. Still a little different in verify.go compared to the rest of the program. Not sure what we do about that. Perhaps just leave it.
A good way to start to understand the refactoring is to look in the new Implementation notes
Closes #18