-
Notifications
You must be signed in to change notification settings - Fork 107
Distribute kbs-client with all the attesters built in #764
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: main
Are you sure you want to change the base?
Conversation
We've been shipping the KBS client with the sample attestor only, but there's no benefit to this and it confuses users. Instead, fix up our Makefile and dockerfile a little bit so that we always include all the attester. As such, drop sample-only from the tag. This will require some tweaks in the Kata and peer pods CIs when we next bump the Trustee version. Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>
Since we changed the tag of our staged kbs-client, update the release helper script. Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>
@@ -18,6 +19,6 @@ RUN if [ $(uname -m) != ${ARCH} ]; then \ | |||
RUN cd kbs && make ARCH=${ARCH} cli-static-linux && \ |
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.
Here we have static
. Do we need to change the name here as the new version should be a dynamic linked one?
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 think it is statically linked via target-feature=+crt-static
I was looking into building with musl, but it requires some more messing around and isn't supported on s390x. That can be our next step. I was also thinking about combining this target with the cli
target, but maybe not everyone will want the static stuff since it makes a larger binary. wdyt?
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.
Looks like the target is still sample only. Plz see here https://github.com/confidential-containers/trustee/blob/main/kbs/Makefile#L85-L93
I prefer to have static built tool. Thoughit is relatively larger, it would help users not to install too much dependencies. This can be a next step too because static compilation is a bit troublesome, especially involving underlying tpm and openssl library
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.
Thoughit is relatively larger, it would help users not to install too much dependencies.
This opens the question that do we want to ask users to install dependencies their TEE attester does not need? it's not very good user experience imp.
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.
@Xynnn007 I change the Makefile in this PR as well.
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.
Yeah, I look into musl. Even with musl tho I think the tss stuff will probably be dynamically linked, which isn't great given that one of the reasons I wanted to do this was to avoid having people need to install host packages.
So I'm not sure exactly what binary we should be shipping here.
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.
What if we ship a tools container image (including all the tools we have - kbs-client, trustee-attester, secrets etc) which can be run via docker/podman ?
We can provide examples of using the tools image in the official doc and that will at least give a ready-to-use artefact for users and avoid this complication of static builds.
I can take a stab if this is agreeable..
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.
@bpradipt Nice shot and coco key provider image we are using now https://confidentialcontainers.org/docs/features/encrypted-images/
btw we need to take care of the devs/configfs/.. that needed to be mounted.
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.
yeah using a container image could be a good idea.
Another approach is to differentiate the tools that we use for configuration and for retrieving the key. In the past we have talked about creating a trustee-cli that is only for configuration and doesn't need any of the attestation dependencies.
Another option would be to not build the tss-based attesters in the kbs-client by default.
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 like the idea to package all helper tools in an image.
See #762
We've been providing a kbs-client binary via oras that only has the sample attester built in. This has been fine for our test cases in the CI, but it's confusing to users, and ultimately I see no reason for us to provide a sample-only binary instead of one with all the attesters.
This PR gets rid of the sample-only binary and replaces it with one that has all the attesters.
I think this will work on all the different arches but I'm not 1000000 percent sure.