Add support for password-protected signing key #173
Conversation
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @jkr0103)
a discussion (no related file):
I'm very happy to remove that expect script and dependency! Thanks for this PR.
-- commits line 2 at r1:
password-protected
-- commits line 3 at r1:
I would add a commit body message like this:
Now that core Gramine's `gramine-sgx-sign` tool supports
the `--passphrase` argument, we don't need to `expect` tool.
templates/Dockerfile.common.sign.template line 10 at r1 (raw file):
--manifest /gramine/app_files/entrypoint.manifest \ --output /gramine/app_files/entrypoint.manifest.sgx \ --passphrase $passphrase
What if there is no passphrase (i.e. a classic ./gsc sign-image <image_name> enclave-key.pem)? Won't this gramine-sgx-sign invocation break? Can you test this scenario?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @jkr0103)
templates/Dockerfile.common.sign.template line 10 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What if there is no passphrase (i.e. a classic
./gsc sign-image <image_name> enclave-key.pem)? Won't thisgramine-sgx-signinvocation break? Can you test this scenario?
Oops. Your question made me realise that gramineproject/gramine#1581 is wrong. Because if passphrase is empty str, then it is returned directly (passphrase and passphrase.encode() is '' and ''.encode(), so '', because left side of and operator is false-ish; see https://docs.python.org/3/reference/expressions.html#boolean-operations). It will traceback for the same reason as in PR description.
I'm sorry for my lack of imagination while speed-drafting #1581.
jkr0103
force-pushed
the
jkr0103/support_protected_signing_key
branch
from
aa24df6
to
3d2cd72
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
password-protected
Done.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would add a commit body message like this:
Now that core Gramine's `gramine-sgx-sign` tool supports the `--passphrase` argument, we don't need to `expect` tool.
Done.
templates/Dockerfile.common.sign.template line 10 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Oops. Your question made me realise that gramineproject/gramine#1581 is wrong. Because if
passphraseis emptystr, then it is returned directly (passphrase and passphrase.encode()is'' and ''.encode(), so'', because left side ofandoperator is false-ish; see https://docs.python.org/3/reference/expressions.html#boolean-operations). It will traceback for the same reason as in PR description.I'm sorry for my lack of imagination while speed-drafting #1581.
Indeed --passphrase arg needs non-empty password otherwise it complains Error: Option '--passphrase' requires an argument.
Are we going to fix it in Gramine? otherwise I have to add if else contition and add --passphrase based on password length.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @jkr0103 and @woju)
templates/Dockerfile.common.sign.template line 10 at r1 (raw file):
Previously, jkr0103 (Jitender Kumar) wrote…
Indeed
--passphrasearg needs non-empty password otherwise it complainsError: Option '--passphrase' requires an argument.Are we going to fix it in Gramine? otherwise I have to add
if elsecontition and add--passphrasebased on password length.
@woju I think it shouldn't be a big problem? I mean, an empty-string argument in command-line args is something very unexpected, and I don't think it's too common to merit a separate handling for this case.
@jkr0103 I think it's better to fix it here, in GSC. As you said, an if else condition should be good.
jkr0103
changed the title
There was a problem hiding this comment.
Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
templates/Dockerfile.common.sign.template line 10 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@woju I think it shouldn't be a big problem? I mean, an empty-string argument in command-line args is something very unexpected, and I don't think it's too common to merit a separate handling for this case.
@jkr0103 I think it's better to fix it here, in GSC. As you said, an
if elsecondition should be good.
I have fixed it in GSC
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @jkr0103 and @woju)
templates/Dockerfile.common.sign.template line 10 at r1 (raw file):
Previously, jkr0103 (Jitender Kumar) wrote…
I have fixed it in GSC
Hm, looks complex. Can't you do Bash parameter substitution:
RUN {% block path %}{% endblock %} gramine-sgx-sign \
--key /gramine/app_files/gsc-signer-key.pem \
--manifest /gramine/app_files/entrypoint.manifest \
--output /gramine/app_files/entrypoint.manifest.sgx \
${passphrase:+--passphrase $passphrase}
You won't need passphrase_name at all then.
@woju Is the above a right POSIX shell kung-fu?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
templates/Dockerfile.common.sign.template line 10 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Hm, looks complex. Can't you do Bash parameter substitution:
RUN {% block path %}{% endblock %} gramine-sgx-sign \ --key /gramine/app_files/gsc-signer-key.pem \ --manifest /gramine/app_files/entrypoint.manifest \ --output /gramine/app_files/entrypoint.manifest.sgx \ ${passphrase:+--passphrase $passphrase}You won't need
passphrase_nameat all then.@woju Is the above a right POSIX shell kung-fu?
${passphrase:+--passphrase $passphrase} works fine. Shell I make the change in PR?
And sgx-sign tool also support --password along with -p and --passphrase here: https://github.com/gramineproject/gramine/blob/94899398023e2dd2b9486aac03cc2996c2f1b13d/python/graminelibos/sgx_sign.py#L591 but GSC doesn't, code:
Line 547 in 5099b64
Can I make same change in GSC?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @jkr0103 and @woju)
templates/Dockerfile.common.sign.template line 10 at r1 (raw file):
Shell I make the change in PR?
Yep, I'm 95% sure that it's a correct and portable way. Still would like to know @woju's opinion, but to me this approach looks correct.
Can I make same change in GSC?
Yes, I think it makes sense to have Gramine and GSC use/allow the same options.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, 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)
templates/Dockerfile.common.sign.template line 10 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Shell I make the change in PR?
Yep, I'm 95% sure that it's a correct and portable way. Still would like to know @woju's opinion, but to me this approach looks correct.
Can I make same change in GSC?
Yes, I think it makes sense to have Gramine and GSC use/allow the same options.
@woju Could you provide your inputs?
There was a problem hiding this comment.
Reviewed 6 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, 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)
a discussion (no related file):
I've created gramineproject/gramine#1611 which fixes the case for empty passphrase. It can be merged before or after this PR, in case someone elects to use empty (but set) passwords.
templates/Dockerfile.common.sign.template line 10 at r1 (raw file):
${passphrase+--passphrase "$passphrase"} if you'd like shell-fu — no colon before plus and add quotes inside {} and don't add quotes outside of {}, I believe, in case passphrase is either set but empty or contains whitespace.
empty-string argument in command-line args is something very unexpected
It's common enough that we can't discount it, but I argue this needs to be fixed in Gramine proper (#1611). GSC should just pass through.
Test for gsc's --passphrase:
- set and non-empty (should result in
gramine-sgx-sign ... --passphrase "abc") - set but empty (should result in
gramine-sgx-sign ... --passphrase "") - unset (should result in
gramine-sgx-sign ...— no `--passphrase added at all)
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02, and we're interested in "set but null" case in the table therein.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, 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)
templates/Dockerfile.common.sign.template line 10 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
${passphrase+--passphrase "$passphrase"}if you'd like shell-fu — no colon before plus and add quotes inside{}and don't add quotes outside of{}, I believe, in case passphrase is either set but empty or contains whitespace.empty-string argument in command-line args is something very unexpected
It's common enough that we can't discount it, but I argue this needs to be fixed in Gramine proper (#1611). GSC should just pass through.
Test for gsc's
--passphrase:
- set and non-empty (should result in
gramine-sgx-sign ... --passphrase "abc")- set but empty (should result in
gramine-sgx-sign ... --passphrase "")- unset (should result in
gramine-sgx-sign ...— no `--passphrase added at all)https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02, and we're interested in "set but null" case in the table therein.
I tried ${passphrase+--passphrase "$passphrase"} and it doesn't work without colon. Error below:
Code snippet:
File "/gramine/meson_build_output/lib/python3.9/site-packages/graminelibos/sgx_sign.py", line 595, in sign_with_file
private_key = load_private_key_from_pem_file(key, passphrase and passphrase.encode())
File "/gramine/meson_build_output/lib/python3.9/site-packages/graminelibos/sgx_sign.py", line 608, in load_private_key_from_pem_file
private_key = serialization.load_pem_private_key(
File "/usr/lib/python3/dist-packages/cryptography/hazmat/primitives/serialization/base.py", line 18, in load_pem_private_key
return backend.load_pem_private_key(data, password)
File "/usr/lib/python3/dist-packages/cryptography/hazmat/backends/openssl/backend.py", line 1244, in load_pem_private_key
return self._load_key(
File "/usr/lib/python3/dist-packages/cryptography/hazmat/backends/openssl/backend.py", line 1447, in _load_key
utils._check_byteslike("password", password)
File "/usr/lib/python3/dist-packages/cryptography/utils.py", line 36, in _check_byteslike
raise TypeError("{} must be bytes-like".format(name))
TypeError: password must be bytes-like
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, 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 @jkr0103 and @woju)
templates/Dockerfile.common.sign.template line 10 at r1 (raw file):
Previously, jkr0103 (Jitender Kumar) wrote…
I tried
${passphrase+--passphrase "$passphrase"}and it doesn't work without colon. Error below:
@woju I think we can't use he + version (that substitutes word if parameter is Set But Null). That's because IIUC in Docker, the ARG passphrase line literally creates a passphrase variable, i.e. it is never NULL (but empty if there was no passphrase given to the Docker build).
So I think we should stick to the :+ version.
There was a problem hiding this comment.
Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion, 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)
templates/Dockerfile.common.sign.template line 10 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@woju I think we can't use he
+version (that substitutes word if parameter is Set But Null). That's because IIUC in Docker, theARG passphraseline literally creates apassphrasevariable, i.e. it is never NULL (but empty if there was no passphrase given to the Docker build).So I think we should stick to the
:+version.
I have updated the PR as validation need to start ASAP. @woju please provide your inputs if latest changes still need some change.
@anjalirai-intel you may start validating the PR.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
templates/Dockerfile.common.sign.template line 10 at r1 (raw file):
@woju I think we can't use he
+version (that substitutes word if parameter is Set But Null). That's because IIUC in Docker, theARG passphraseline literally creates apassphrasevariable, i.e. it is never NULL (but empty if there was no passphrase given to the Docker build).
If so, then yes, :+ is the correct thing to do. I'm resolving.
Error below:
This should have been fixed by gramineproject/gramine#1611.
There was a problem hiding this comment.
Reviewed 5 of 7 files at r1, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @jkr0103)
templates/Dockerfile.common.sign.template line 11 at r4 (raw file):
--manifest /gramine/app_files/entrypoint.manifest \ --output /gramine/app_files/entrypoint.manifest.sgx \ ${passphrase:+--passphrase "$passphrase"}
Does this work if the password contains " or \?
Also, saving user key's password to a file on the disk doesn't seem like a good practice.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @jkr0103)
templates/Dockerfile.common.sign.template line 11 at r4 (raw file):
Also, saving user key's password to a file on the disk doesn't seem like a good practice.
Hmm, or maybe not? How does this line actually work?
// Regardless of this, the question about special chars still stands.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @jkr0103)
templates/Dockerfile.common.sign.template line 11 at r4 (raw file):
Also, saving user key's password to a file on the disk doesn't seem like a good practice.
Hmm, or maybe not? How does this line actually work?
Why do you think there's anything about files in this line? It's a bash/shell substitution syntax, see the table here: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02. The password string is passed as an input argument everywhere here.
Does this work if the password contains
"or\?
Good question. I guess @jkr0103 will have to experiment with such passphrases. And report the results. @mkow If these symbols need to be escaped during gsc build --passphrase <...>, would it be enough to add this note in the GSC documentation?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @jkr0103)
templates/Dockerfile.common.sign.template line 11 at r4 (raw file):
Why do you think there's anything about files in this line? It's a bash/shell substitution syntax, see the table here: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02. The password string is passed as an input argument everywhere here.
Ok, I've read more about this and it's not Bash and not Jinja, but Docker doing the substitution here before running the command.
btw. quoting Dockerfile ARG documentation:
Warning:
It is not recommended to use build-time variables for passing secrets like GitHub keys, user credentials etc. Build-time variable values are visible to any user of the image with thedocker historycommand.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @jkr0103)
templates/Dockerfile.common.sign.template line 11 at r4 (raw file):
Ok, I've read more about this and it's not Bash and not Jinja, but Docker doing the substitution here before running the command.
Where did you read this?
btw. quoting Dockerfile
ARGdocumentation:
But that's why we have a trick of FROM ... (a multi-stage build), which removes all files and all ENVs and ARGs from the previous image, see my other comment where I highlight this place.
templates/Dockerfile.common.sign.template line 15 at r4 (raw file):
# This trick removes all temporary files from the previous commands (including gsc-signer-key.pem # and passphrase) FROM {{image}}
To @mkow: this trick removes the previous ARG from the final signed image.
To make sure myself, I ran this PR on my machine (as described in the PR description, with a passphase-protected key) and did:
$ docker history --no-trunc gsc-python:bullseye | grep test@123
< returns nothing >
Looking at docker history, it's also clear that the steps in-between two FROM keywords are "vanished":
$ docker history gsc-python:bullseye
IMAGE CREATED CREATED BY SIZE COMMENT
21dfb8f165fe 10 minutes ago /bin/sh -c #(nop) COPY file:6c832ef8e306f999… 9.35MB
d3d03dbd709c 10 minutes ago /bin/sh -c #(nop) COPY file:ba82810b1d6c2ef4… 1.81kB
340188eab1f0 19 minutes ago /bin/sh -c #(nop) ENTRYPOINT ["/bin/bash" "… 0B
e75637d26da0 19 minutes ago |0 /bin/sh -c chmod u+x /gramine/app_files/a… 4.19MB
...
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
templates/Dockerfile.common.sign.template line 11 at r4 (raw file):
Does this work if the password contains
"or\?
I tried with " and \ as below and that worked:
openssl genrsa -3 -aes128 -passout pass:" test@123" -out enclave-key.pem 3072
./gsc sign-image --password " test@123" redis:7.0.10 enclave-key.pem
openssl genrsa -3 -aes128 -passout pass:"test@123 -out enclave-key.pem 3072
./gsc sign-image --password "test@123 redis:7.0.10 enclave-key.pem
openssl genrsa -3 -aes128 -passout pass:\test@123 -out enclave-key.pem 3072
./gsc sign-image --password \test@123 redis:7.0.10 enclave-key.pem
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
templates/Dockerfile.common.sign.template line 11 at r4 (raw file):
Previously, jkr0103 (Jitender Kumar) wrote…
Does this work if the password contains
"or\?I tried with
"and\as below and that worked:openssl genrsa -3 -aes128 -passout pass:" test@123" -out enclave-key.pem 3072
./gsc sign-image --password " test@123" redis:7.0.10 enclave-key.pemopenssl genrsa -3 -aes128 -passout pass:"test@123 -out enclave-key.pem 3072
./gsc sign-image --password "test@123 redis:7.0.10 enclave-key.pemopenssl genrsa -3 -aes128 -passout pass:\test@123 -out enclave-key.pem 3072
./gsc sign-image --password \test@123 redis:7.0.10 enclave-key.pem
\ and " will work in "$passphrase" if docker does correct thing in the setting of this variable in the environment. Same as in plain (non-docker) shell, if " is already in the content of the variable, then substituting the variable wrapped in " changes nothing, because the content of variable is considered already "quoted" and the " quotes surrounding the substitution are not, so they're removed (after word splitting). Same for \, it's already quoted.
POSIX shell is almost dumb string replacement, but not quite. To have complete mental model, you need to know which characters are considered quoted and how word splitting works. Here it's correct.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
templates/Dockerfile.common.sign.template line 15 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
To @mkow: this trick removes the previous
ARGfrom the final signed image.To make sure myself, I ran this PR on my machine (as described in the PR description, with a passphase-protected key) and did:
$ docker history --no-trunc gsc-python:bullseye | grep test@123 < returns nothing >Looking at
docker history, it's also clear that the steps in-between twoFROMkeywords are "vanished":$ docker history gsc-python:bullseye IMAGE CREATED CREATED BY SIZE COMMENT 21dfb8f165fe 10 minutes ago /bin/sh -c #(nop) COPY file:6c832ef8e306f999… 9.35MB d3d03dbd709c 10 minutes ago /bin/sh -c #(nop) COPY file:ba82810b1d6c2ef4… 1.81kB 340188eab1f0 19 minutes ago /bin/sh -c #(nop) ENTRYPOINT ["/bin/bash" "… 0B e75637d26da0 19 minutes ago |0 /bin/sh -c chmod u+x /gramine/app_files/a… 4.19MB ...
You are right that this will clear the password from the final image, because of "FROM" command.
IMHO it's still not a good solution because you are saving a password to a PEM file in the unsigned_image which means that you are leaking passwords all over the signing machine. Also while signing another user might see your password in ps(1) command (so in general I'm not a fun of --passphrase option). If you are leaking this password, why is PEM protected with a password at all?
Maybe you can consider using RUN --mount=type=secret from buildx? But this doesn't allow to do stdin IMHO. https://render.com/docs/docker-secrets
Or use docker secrets from swarm? https://docs.docker.com/engine/swarm/secrets/ I guess you could add a temporary secret in gsc, build it, and then remove it. Or require a user to create secret and then just accept the name of the secret.
The more hardcore solution, which I have suggested in a different project, would be to do a fake "multistage" build.
When you finish building unsigned_image, then run it to do gramine-sgx-sign and use the output of these as input to build another Dockerfile.
Then the passphrase could be passed as stdin.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
templates/Dockerfile.common.sign.template line 15 at r4 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
You are right that this will clear the password from the final image, because of "FROM" command.
IMHO it's still not a good solution because you are saving a password to a PEM file in the
unsigned_imagewhich means that you are leaking passwords all over the signing machine. Also while signing another user might see your password inps(1)command (so in general I'm not a fun of--passphraseoption). If you are leaking this password, why is PEM protected with a password at all?Maybe you can consider using
RUN --mount=type=secretfrom buildx? But this doesn't allow to do stdin IMHO. https://render.com/docs/docker-secrets
Or use docker secrets from swarm? https://docs.docker.com/engine/swarm/secrets/ I guess you could add a temporary secret in gsc, build it, and then remove it. Or require a user to create secret and then just accept the name of the secret.The more hardcore solution, which I have suggested in a different project, would be to do a fake "multistage" build.
When you finish buildingunsigned_image, then run it to dogramine-sgx-signand use the output of these as input to build another Dockerfile.
Then the passphrase could be passed as stdin.
Actually, my hardcore idea could be simplified to:
docker build -t Dockerfile .
docker run --mountpoints=... --entrypoint=gramine-sign --key ... --manifest .. --output ...
docker commit
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
templates/Dockerfile.common.sign.template line 11 at r4 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
\and"will work in"$passphrase"if docker does correct thing in the setting of this variable in the environment. Same as in plain (non-docker) shell, if"is already in the content of the variable, then substituting the variable wrapped in"changes nothing, because the content of variable is considered already "quoted" and the"quotes surrounding the substitution are not, so they're removed (after word splitting). Same for\, it's already quoted.POSIX shell is almost dumb string replacement, but not quite. To have complete mental model, you need to know which characters are considered quoted and how word splitting works. Here it's correct.
agree
templates/Dockerfile.common.sign.template line 15 at r4 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Actually, my hardcore idea could be simplified to:
docker build -t Dockerfile . docker run --mountpoints=... --entrypoint=gramine-sign --key ... --manifest .. --output ... docker commit
The final signed GSC image doesn't leak the password but the dangling image generated as part of multistage build does leak the password and password can be retrieve using docker history <dangling_image_id>. If we can delete this dangling image than we should be good. I am about to create PR for deleting this dangling image.
There was a problem hiding this comment.
Reviewed 1 of 7 files at r1, 1 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
templates/Dockerfile.common.sign.template line 15 at r4 (raw file):
Previously, jkr0103 (Jitender Kumar) wrote…
The final signed GSC image doesn't leak the password but the dangling image generated as part of multistage build does leak the password and password can be retrieve using
docker history <dangling_image_id>. If we can delete this dangling image than we should be good. I am about to create PR for deleting this dangling image.
I have create PR #175 for deleting the dangling image generated during GSC signing pocess.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
templates/Dockerfile.common.sign.template line 15 at r4 (raw file):
Previously, jkr0103 (Jitender Kumar) wrote…
I have create PR #175 for deleting the dangling image generated during GSC signing pocess.
@oshogbo @woju Agreed- FROM trick wouldn't really help if the signing machine is compromised with wrong users authenticated to use it. It only helps in the finally deployed image to not carry secrets (this is how we take out the signing key itself out of the final image since the founding days of GSC). Can we continue with the same security scope for now and while we work separately on hardening solutions for the build phase?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
There was a problem hiding this comment.
Reviewed 5 of 7 files at r1, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
Now that core Gramine's `gramine-sgx-sign` tool supports the `--passphrase` argument, we don't need the `expect` tool. Signed-off-by: jkr0103 <[email protected]>
There was a problem hiding this comment.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
templates/Dockerfile.common.sign.template line 15 at r4 (raw file):
Previously, aneessahib (Anees Sahib) wrote…
@oshogbo @woju Agreed- FROM trick wouldn't really help if the signing machine is compromised with wrong users authenticated to use it. It only helps in the finally deployed image to not carry secrets (this is how we take out the signing key itself out of the final image since the founding days of GSC). Can we continue with the same security scope for now and while we work separately on hardening solutions for the build phase?
I think what @aneessahib said is a consensus for now. And we hope for @oshogbo to submit a PR to fix all those issues with correctly using secret files/passwords :)
This PR seems to be good to be merged, so I'll do it now.
dimakuv
force-pushed
the
jkr0103/support_protected_signing_key
branch
from
8606fce
to
b7d5968
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
Description of the changes
Now that core Gramine's
gramine-sgx-signtool supports the--passphraseargument, we don't need theexpecttool.Fixes #148
Prerequisite: gramineproject/gramine#1581
How to test this PR?
Use protected signing key for signing the GSC image for any workload. make sure to supply
passphrasewith signing command.Examples:
Create protected signing key:
openssl genrsa -3 -aes128 -passout pass:test@123 -out enclave-key.pem 3072Sign GSC image:
./gsc sign-image <image_name> enclave-key.pem --passphrase test@123This change is