-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add AKV Dockerfile templates for signing with gsc #20
base: master
Are you sure you want to change the base?
Add AKV Dockerfile templates for signing with gsc #20
Conversation
Signed-off-by: Sankaranarayanan Venkatasubramanian <[email protected]>
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.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @svenkata9)
-- commits
line 3 at r1:
gsc
-> GSC
Integrations/azure/akv-sign/Dockerfile.sign.akv.centos.template
line 1 at r1 (raw file):
FROM {{image}} as unsigned_image
Could you add a top-level comment that this template is derived from a generic https://github.com/gramineproject/gsc/blob/master/templates/Dockerfile.common.sign.template
Integrations/azure/akv-sign/Dockerfile.sign.akv.centos.template
line 16 at r1 (raw file):
RUN dnf install -y https://packages.microsoft.com/config/rhel/8/packages-microsoft-prod.rpm RUN dnf install -y azure-cli {% endblock %}
Actually, why do you need these {% block ... %}
in these Dockerfiles? These Dockerfiles are already "custom" and they don't need to reflect the GSC's template structure.
I think you can:
- safely remove
block install
- copy
block path
contents from https://github.com/gramineproject/gsc/blob/dbc17b6a118d0fb13411d3acd1f1dfc032b77b83/templates/centos/Dockerfile.sign.template#L3
And in the companion PR gramineproject/gsc#112, you won't need that those templates/centos/Dockerfile.sign.user.template
at all!
Integrations/azure/akv-sign/Dockerfile.sign.akv.centos.template
line 17 at r1 (raw file):
RUN dnf install -y azure-cli {% endblock %}
You should switch back to original user:
# Switch back to original app_image user
USER {{app_user}}
Otherwise you'll end up with entrypoing.manifest.sgx
being accessible only by root, and the whole thing will fail for non-root Docker images.
Integrations/azure/akv-sign/Dockerfile.sign.akv.centos.template
line 26 at r1 (raw file):
RUN {% block path %}{% endblock %} /gramine/app_files/gramine-sgx-akv-sign \ --url <akv_mhsm_url> \ --key <akv_sign_key> \
As I mentioned in the PR gramineproject/gsc#112, you could reuse ARG key
and then --key $key
Integrations/azure/akv-sign/Dockerfile.sign.akv.centos.template
line 29 at r1 (raw file):
--manifest /gramine/app_files/entrypoint.manifest \ --output /gramine/app_files/entrypoint.manifest.sgx
Aren't you supposed to do something like RUN az logout
for sanity?
Integrations/azure/akv-sign/Dockerfile.sign.akv.centos.template
line 35 at r1 (raw file):
COPY --from=unsigned_image /gramine/app_files/*.sig /gramine/app_files/ COPY --from=unsigned_image /gramine/app_files/*.sgx /gramine/app_files/
Remove empty line
Integrations/azure/akv-sign/Dockerfile.sign.akv.debian.template
line 1 at r1 (raw file):
FROM {{image}} as unsigned_image
Please apply all the same comments as in the file above.
Integrations/azure/akv-sign/README.md
line 7 at r1 (raw file):
deployments, you should use a key secured in a Hardware Security Module (HSM). This directory contains the plugin to Gramine tools and templates that enable
Maybe and templates
-> , as well as Dockerfile templates
?
Otherwise it reads a bit weird, as if there is one plugin to "Gramine tools and templates".
Integrations/azure/akv-sign/README.md
line 37 at r1 (raw file):
## Templates for use with Gramine Shielded Containers (GSC)
Please start with a sentence like This directory contains two Dockerfile templates, intended for use with GSC's sign-image command
.
Integrations/azure/akv-sign/README.md
line 39 at r1 (raw file):
GSC `sign-image` command can take in a user supplied Dockerfile as an argument to `--template` and sign the graminized docker image. These
and sign
-> to sign
Integrations/azure/akv-sign/README.md
line 40 at r1 (raw file):
GSC `sign-image` command can take in a user supplied Dockerfile as an argument to `--template` and sign the graminized docker image. These templates can be used when a HSM is needed for signing. This directory has
These templates can be used ...
-- this sentence looks redundant, just remove it.
Integrations/azure/akv-sign/README.md
line 41 at r1 (raw file):
as an argument to `--template` and sign the graminized docker image. These templates can be used when a HSM is needed for signing. This directory has templates for using AKV to sign the graminized docker image. Please
This directory has ...
-- with my proposed first sentence, this sentence becomes redundant. I'd remove it.
Integrations/azure/akv-sign/README.md
line 42 at r1 (raw file):
templates can be used when a HSM is needed for signing. This directory has templates for using AKV to sign the graminized docker image. Please note that these are templates and the users will need to update the template
the users will need
-> users need
Integrations/azure/akv-sign/README.md
line 43 at r1 (raw file):
templates for using AKV to sign the graminized docker image. Please note that these are templates and the users will need to update the template with the required details to make it a 'self-contained' Dockerfile before
No need for '
around self-contained
Integrations/azure/akv-sign/README.md
line 44 at r1 (raw file):
note that these are templates and the users will need to update the template with the required details to make it a 'self-contained' Dockerfile before passing it to `sign-image` command.
sign-image
-> gsc sign-image
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.
Reviewable status: 0 of 3 files reviewed, 16 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
gsc
->GSC
ACK.
Integrations/azure/akv-sign/Dockerfile.sign.akv.centos.template
line 1 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you add a top-level comment that this template is derived from a generic https://github.com/gramineproject/gsc/blob/master/templates/Dockerfile.common.sign.template
Done.
Integrations/azure/akv-sign/Dockerfile.sign.akv.centos.template
line 16 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually, why do you need these
{% block ... %}
in these Dockerfiles? These Dockerfiles are already "custom" and they don't need to reflect the GSC's template structure.I think you can:
- safely remove
block install
- copy
block path
contents from https://github.com/gramineproject/gsc/blob/dbc17b6a118d0fb13411d3acd1f1dfc032b77b83/templates/centos/Dockerfile.sign.template#L3And in the companion PR gramineproject/gsc#112, you won't need that those
templates/centos/Dockerfile.sign.user.template
at all!
Done.
Integrations/azure/akv-sign/Dockerfile.sign.akv.centos.template
line 17 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You should switch back to original user:
# Switch back to original app_image user USER {{app_user}}
Otherwise you'll end up with
entrypoing.manifest.sgx
being accessible only by root, and the whole thing will fail for non-root Docker images.
Done.
Integrations/azure/akv-sign/Dockerfile.sign.akv.centos.template
line 26 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
As I mentioned in the PR gramineproject/gsc#112, you could reuse
ARG key
and then--key $key
As mentioned in the GSC PR on adding --template
, taking in --key will cause more confusion because we will need to take URL also as arguments to gsc sign-image
. Hence, let us just consider these as templates that the users will need to update before using.
Integrations/azure/akv-sign/Dockerfile.sign.akv.centos.template
line 29 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Aren't you supposed to do something like
RUN az logout
for sanity?
Done.
Integrations/azure/akv-sign/Dockerfile.sign.akv.centos.template
line 35 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Remove empty line
Done.
Integrations/azure/akv-sign/Dockerfile.sign.akv.debian.template
line 1 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please apply all the same comments as in the file above.
Done.
Integrations/azure/akv-sign/README.md
line 7 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Maybe
and templates
->, as well as Dockerfile templates
?Otherwise it reads a bit weird, as if there is one plugin to "Gramine tools and templates".
Done.
Integrations/azure/akv-sign/README.md
line 37 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please start with a sentence like
This directory contains two Dockerfile templates, intended for use with GSC's sign-image command
.
Done.
Integrations/azure/akv-sign/README.md
line 39 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
and sign
->to sign
Done.
Integrations/azure/akv-sign/README.md
line 40 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
These templates can be used ...
-- this sentence looks redundant, just remove it.
Done.
Integrations/azure/akv-sign/README.md
line 41 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This directory has ...
-- with my proposed first sentence, this sentence becomes redundant. I'd remove it.
Done.
Integrations/azure/akv-sign/README.md
line 42 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
the users will need
->users need
Done.
Integrations/azure/akv-sign/README.md
line 43 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
No need for
'
around self-contained
Done.
Integrations/azure/akv-sign/README.md
line 44 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
sign-image
->gsc sign-image
Done.
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.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @svenkata9)
Integrations/azure/akv-sign/Dockerfile.sign.akv.centos.template
line 26 at r1 (raw file):
Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…
As mentioned in the GSC PR on adding
--template
, taking in --key will cause more confusion because we will need to take URL also as arguments togsc sign-image
. Hence, let us just consider these as templates that the users will need to update before using.
Yep.
Integrations/azure/akv-sign/Dockerfile.sign.akv.centos.template
line 28 at r2 (raw file):
RUN az login RUN {% block path %}export PYTHONPATH="${PYTHONPATH}:$(find /gramine/meson_build_output/lib64 -type d -path '*/site-packages')" &&{% endblock %} \
You can remove {% block path %}
and {% endblock %}
thingies completely. You're not doing template expansion with these blocks anymore.
Integrations/azure/akv-sign/Dockerfile.sign.akv.debian.template
line 26 at r2 (raw file):
RUN az login RUN {% block path %}export PYTHONPATH="${PYTHONPATH}:$(find /gramine/meson_build_output/lib -type d -path '*/site-packages')" &&{% endblock %} \
ditto
Integrations/azure/akv-sign/README.md
line 40 at r2 (raw file):
This directory contains two Dockerfile templates, intended for use with GSC's `sign-image` command. GSC `sign-image` command can take in a user supplied Dockerfile as an argument to `--template` to sign the graminized docker image.
docker
-> Docker
Integrations/azure/akv-sign/README.md
line 43 at r2 (raw file):
Please note that these are templates and the users need to update the template with the required details to make it a self-contained Dockerfile before passing it to `gsc sign-image` command.
Maybe also add an example bash session? Like this:
An example of usage, for signing a Debian/Ubuntu based Docker image:
```
vim Dockerfile.sign.akv.debian.template
# update url and key placeholders to your AKV url and key in this file
./gsc sign-image --template Dockerfile.sign.akv.debian.template <your-docker-image>
```
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.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @svenkata9)
Integrations/azure/akv-sign/Dockerfile.sign.akv.debian.template
line 36 at r2 (raw file):
# This trick removes all temporary files from the previous commands FROM {{image}}
@svenkata9: This thing is the exact thing that for me is a firm NAK. Because GSC integrity should not rely on external templates.
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.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @woju)
Integrations/azure/akv-sign/Dockerfile.sign.akv.debian.template
line 36 at r2 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
@svenkata9: This thing is the exact thing that for me is a firm NAK. Because GSC integrity should not rely on external templates.
Wondering what might have made you miss seeing this in the current implementation. 🤔
https://github.com/gramineproject/gsc/blob/a60a4991d7351c1b7489444c694638ca9cc68e39/templates/Dockerfile.common.sign.template#L16
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.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @svenkata9)
Integrations/azure/akv-sign/Dockerfile.sign.akv.debian.template
line 36 at r2 (raw file):
Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…
Wondering what might have made you miss seeing this in the current implementation. 🤔
https://github.com/gramineproject/gsc/blob/a60a4991d7351c1b7489444c694638ca9cc68e39/templates/Dockerfile.common.sign.template#L16
I mean, what happens if someone provides a template without this line? It still "works", but ships the private key, or in case of HSM, possibly some creds for it, or whatever leftovers from signing process which might or might not be harmful, we don't know. And because we're security paranoids, we won't leave it to random developers on the Internet, who might not know why this line is there and what "temporary files" in the comment above really mean.
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.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @woju)
Integrations/azure/akv-sign/Dockerfile.sign.akv.debian.template
line 36 at r2 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
I mean, what happens if someone provides a template without this line? It still "works", but ships the private key, or in case of HSM, possibly some creds for it, or whatever leftovers from signing process which might or might not be harmful, we don't know. And because we're security paranoids, we won't leave it to random developers on the Internet, who might not know why this line is there and what "temporary files" in the comment above really mean.
Ideally, this is not needed, as signing with a HSM doesn't bring the key in the clear. This is to just make sure if there are any temp stuff, that will get cleared. Compare it with the way that we are doing now - key in the clear, and what if someone takes it and deletes this line? Did we think about it when we put this in?
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.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @svenkata9)
Integrations/azure/akv-sign/Dockerfile.sign.akv.debian.template
line 36 at r2 (raw file):
Ideally, this is not needed, as signing with a HSM doesn't bring the key in the clear.
Yes, but unfortunately we don't live in an ideal world. HSMs also (might) require some credentials. Do you know for sure if az logout
removes all relevant remains? I don't. And if it does now, we can't predict future. So we need to be careful.
This is to just make sure if there are any temp stuff, that will get cleared.
Yes, I agree. So to make sure, this needs to be in an invariant part of the template, i.e. something that user can't override.
Compare it with the way that we are doing now - key in the clear, and what if someone takes it and deletes this line? Did we think about it when we put this in?
Yes we did think about it. AFAIR some early version of GSC didn't do it and shipped the private key inside the resulting container, and this was caught only in review. (I can't find reference now, maybe @mkow remembers where that was exactly.) Because of that story, I insist that we don't leave any possibility for people to do it. The less moving parts are available to external developers, the better.
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.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @svenkata9, and @woju)
Integrations/azure/akv-sign/Dockerfile.sign.akv.debian.template
line 36 at r2 (raw file):
AFAIR some early version of GSC didn't do it and shipped the private key inside the resulting container, and this was caught only in review
Yep, that was the case in the initial GSC version I got for review - see https://reviewable.io/reviews/gramineproject/graphene/1430#-MAC4fJy7ftMPr-vjee9
az logout
removes all relevant remains
Overall, yes, we should do the signing in a separate container, extract the signature and then burn it down. Then we don't have to care about what az
command does internally. And it seems that it works like this now, but this key step of overwriting the image with secrets is now pushed to the layer written by external users? Is that right? If so, then it sounds like a risky design, as woju said (and I believe that's his point exactly).
Signed-off-by: Sankaranarayanan Venkatasubramanian [email protected]
These templates can be used after this PR Enable
--template
option to use withsign-image
(for use with HSMs, for example) by svenkata9 · Pull Request #112 · gramineproject/gsc (github.com) in GSC repo gets merged.This change is