Skip to content
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

Enable --template option to use with sign-image (for use with HSMs, for example) #112

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

svenkata9
Copy link

@svenkata9 svenkata9 commented Dec 9, 2022

Signed-off-by: Sankaranarayanan Venkatasubramanian sankaranarayanan.venkatasubramanian@intel.com

Description

The gsc tool cannot do production signing on the gramized docker images today, and this PR enables that. This PR introduces passing a 'self-contained' Dockerfile as an argument to gsc sign-image with the option --template. For example, using a template like this, we can do production signing with Azure Key Vault. Once this PR is merged, contrib repo will be updated with templates for Azure Key Vault and OpenSSL engine.

How to test this PR?

For testing this PR, you will need to use the Dockerfile template mentioned above for use with AKV, or create one based on this for using local signing. and then you will be able to sign the unsigned gsc image using ./gsc sign-image --template Dockerfile.sign.akv.debian.template


This change is Reviewable

… for example)

Signed-off-by: Sankaranarayanan Venkatasubramanian <sankaranarayanan.venkatasubramanian@intel.com>
@svenkata9 svenkata9 changed the title Enable --template option to use with sign-image (for use with HSMs,for example) Enable --template option to use with sign-image (for use with HSMs,for example) Dec 9, 2022
@svenkata9 svenkata9 changed the title Enable --template option to use with sign-image (for use with HSMs,for example) Enable --template option to use with sign-image (for use with HSMs, for example) Dec 9, 2022
Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @svenkata9)

a discussion (no related file):
You need to update the docs:

  1. https://gramine.readthedocs.io/projects/gsc/en/latest/#gsc-sign-image-signs-a-graminized-image -- add --template option
  2. https://gramine.readthedocs.io/projects/gsc/en/latest/#stages-of-building-graminized-sgx-docker-images -- in stage 3, mention that users can also supply their own Dockerfile templates, to e.g. sign with HSM flows.

a discussion (no related file):
I'm not sure if --template is the best name for this option. Maybe --custom-dockerfile? I don't know, I can't come up with a better name. So not blocking.


a discussion (no related file):
The user needs to manually modify the templates, e.g. specify the AKV URL: https://github.com/svenkata9/contrib/blob/54ee9fc3aa1ba367bf203f769c51b555a8310576/Integrations/azure/akv-sign/Dockerfile.sign.akv.debian.template#L23

It would be better to provide such names (e.g. AKV URL) in the gsc sign-image command itself. But on the other hand, I understand that this won't be extensible and only confusing...



-- commits line 2 at r1:
This commit title is too long. Let's split into two: title and message body:

Add `--template` option to use with `sign-image` command

Currently `gsc sign-image` command only provides one way to
sign images -- using Gramine's default `gramine-sgx-sign` tool;
this logic is encapsulated in `Dockerfile.common.sign.template`.

This commit adds the ability to sign images using user-specified
tools, e.g. for signing with hardware HSMs provided by Azure KeyVault
and OpenSSL engine.

.gitignore line 24 at r1 (raw file):

*.sig
*.token
/templates/Dockerfile.sign.user.template

This doesn't look right. I suggest:

  1. Add the removal of this file in gsc.py (in the finally clause)
  2. Remove this line from here.

gsc.py line 353 at r1 (raw file):

    distro, _ = distro.split(':')
    env.loader = jinja2.FileSystemLoader('templates/')
    sign_template = []

This should be sign_template = None


gsc.py line 354 at r1 (raw file):

    env.loader = jinja2.FileSystemLoader('templates/')
    sign_template = []
    build_args = []

This should be a dict, not a list: build_args = {}


gsc.py line 370 at r1 (raw file):

    else:
        shutil.copy(args.template, 'templates/Dockerfile.sign.user.template')
        sign_template = env.get_template(f'{distro}/Dockerfile.sign.user.template')

Actually, don't you want to propagate the key into the user template:

        build_args = {"key": args.key}

this way users won't need to manually modify this line in the template: https://github.com/svenkata9/contrib/blob/54ee9fc3aa1ba367bf203f769c51b555a8310576/Integrations/azure/akv-sign/Dockerfile.sign.akv.debian.template#L24

See the example with passphrase usage here:


gsc.py line 382 at r1 (raw file):

    finally:
        if args.template is None:
            os.remove(tmp_build_key_path)

Please add the removal of the copied templates/Dockerfile.sign.user.template, smth like:

    else:
        os.remove('templates/Dockerfile.sign.user.template')

gsc.py line 516 at r1 (raw file):

sub_sign.add_argument('-k', '--key', help='Key to sign the Intel SGX enclaves inside the Docker image.')
sub_sign.add_argument('-t', '--template',
                      help='Custom Dockerfile/template to use for signing, say, with a HSM.')

say, with a HSM -> e.g. with an HSM


templates/ubuntu/Dockerfile.sign.user.template line 2 at r1 (raw file):

{% extends "debian/Dockerfile.sign.user.template" %}

Please remove this empty line.

Copy link
Contributor

@dimakuv dimakuv left a 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, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @svenkata9)


templates/centos/Dockerfile.sign.user.template line 1 at r1 (raw file):

{% extends "Dockerfile.sign.user.template" %}

As I mentioned in gramineproject/contrib#20, I don't think you need these three new template files at all.

Copy link
Author

@svenkata9 svenkata9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 6 files reviewed, 12 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)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I'm not sure if --template is the best name for this option. Maybe --custom-dockerfile? I don't know, I can't come up with a better name. So not blocking.

I did consider --dockerfile, --custom-script, --script and few others, but since we use all Dockerfiles as templates, I settled on this.


a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

The user needs to manually modify the templates, e.g. specify the AKV URL: https://github.com/svenkata9/contrib/blob/54ee9fc3aa1ba367bf203f769c51b555a8310576/Integrations/azure/akv-sign/Dockerfile.sign.akv.debian.template#L23

It would be better to provide such names (e.g. AKV URL) in the gsc sign-image command itself. But on the other hand, I understand that this won't be extensible and only confusing...

Yes, the idea is to have the sample templates (in contrib) and have users modify according to their needs by filling in the URL and key name. If we provide options like --url, then we will need to add stuff like --engine for OpenSSL engine-based signing and we will end up with lot many confusing options. So, to keep it simple, it is --template <Dockerfile> that contains all the information.


a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You need to update the docs:

  1. https://gramine.readthedocs.io/projects/gsc/en/latest/#gsc-sign-image-signs-a-graminized-image -- add --template option
  2. https://gramine.readthedocs.io/projects/gsc/en/latest/#stages-of-building-graminized-sgx-docker-images -- in stage 3, mention that users can also supply their own Dockerfile templates, to e.g. sign with HSM flows.

Done.



-- commits line 2 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This commit title is too long. Let's split into two: title and message body:

Add `--template` option to use with `sign-image` command

Currently `gsc sign-image` command only provides one way to
sign images -- using Gramine's default `gramine-sgx-sign` tool;
this logic is encapsulated in `Dockerfile.common.sign.template`.

This commit adds the ability to sign images using user-specified
tools, e.g. for signing with hardware HSMs provided by Azure KeyVault
and OpenSSL engine.

ACK.


gsc.py line 353 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This should be sign_template = None

Done.


gsc.py line 354 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This should be a dict, not a list: build_args = {}

Done.


gsc.py line 370 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Actually, don't you want to propagate the key into the user template:

        build_args = {"key": args.key}

this way users won't need to manually modify this line in the template: https://github.com/svenkata9/contrib/blob/54ee9fc3aa1ba367bf203f769c51b555a8310576/Integrations/azure/akv-sign/Dockerfile.sign.akv.debian.template#L24

See the example with passphrase usage here:

Like I mentioned above, the idea is for the user to provide a Dockerfile as template that contains all the information including key URL and name.


gsc.py line 382 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please add the removal of the copied templates/Dockerfile.sign.user.template, smth like:

    else:
        os.remove('templates/Dockerfile.sign.user.template')

Not valid comment anymore since we removed the user template concept and consuming the Dockerfile as is.


gsc.py line 516 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

say, with a HSM -> e.g. with an HSM

Done.


templates/centos/Dockerfile.sign.user.template line 1 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

As I mentioned in gramineproject/contrib#20, I don't think you need these three new template files at all.

Done.


templates/ubuntu/Dockerfile.sign.user.template line 2 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please remove this empty line.

Done.


.gitignore line 24 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This doesn't look right. I suggest:

  1. Add the removal of this file in gsc.py (in the finally clause)
  2. Remove this line from here.

Eliminated the need for Dockerfile.sign.user.template (and the extensions for OS flavors). So, we can mark this comment as resolved.

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 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)

a discussion (no related file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

I did consider --dockerfile, --custom-script, --script and few others, but since we use all Dockerfiles as templates, I settled on this.

Yes, I can't come up with anything better, so I'm fine.


a discussion (no related file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Yes, the idea is to have the sample templates (in contrib) and have users modify according to their needs by filling in the URL and key name. If we provide options like --url, then we will need to add stuff like --engine for OpenSSL engine-based signing and we will end up with lot many confusing options. So, to keep it simple, it is --template <Dockerfile> that contains all the information.

Ok, I agree.



gsc.py line 370 at r1 (raw file):

Previously, svenkata9 (Sankaranarayanan Venkatasubramanian) wrote…

Like I mentioned above, the idea is for the user to provide a Dockerfile as template that contains all the information including key URL and name.

Yes, ok, I agree


gsc.py line 518 at r2 (raw file):

    help='Key to sign the Intel SGX enclaves inside the Docker image.')
sub_sign.add_argument('-t', '--template',
                      help='Custom Dockerfile/template to use for signing, say, with an HSM.')

Please unify the indentation similar to how it is in other cases (4 spaces from beginning of line)


Documentation/index.rst line 148 at r2 (raw file):

   Specify configuration file. Default: :file:`config.yaml`

.. option:: -k KEY-FILE

No need to specify this KEY-FILE here. Looking at other option descriptions, we don't use such format (-option name).


Documentation/index.rst line 158 at r2 (raw file):

.. option:: -t

   Provide Dockerfile to use for signing the enclave (e.g., with an HSM)

Please also add a second sentence:

If this option is used, then `-k` and `-p` are ignored (that's because
the enclave signing key must be specified in the provided Dockerfile).

Documentation/index.rst line 283 at r2 (raw file):

   provided by the user via the :command:`gsc sign-image` command and copied
   into this Docker build stage. The users can also supply their own Dockerfile
   templates using `--template` option of this command to sign (e.g. with HSM).

Slight rewording:

   into this Docker build stage. The users can also supply their own Dockerfile
   templates using `-t`/`--template` command-line option for user-specific
   signining flows (e.g., signing with an HSM).

@svenkata9
Copy link
Author

@woju Please add your comments here and your proposal. I feel that is not solving the problem that we are trying to solve. There are multiple options that I investigated including passing arguments to gsc sign-image, but that will make it more complicated. Theis --template is based on the existing template that we use for local signing and introduces only the HSM specific stuff.

Copy link
Member

@woju woju left a 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)

a discussion (no related file):
I object to --template option as it stands, because:

  • GSC integrity should not rely on external template. It takes one clueless developer in the future to remove this line, and GSC would still "work" apart from the small detail that it ships private key used for signing. See comment in Add AKV Dockerfile templates for signing with gsc contrib#20
  • the user shouldn't be required to know GSC implenentation details (i.e. not only this FROM device, but also COPY paths, PYTHONPATH etc.) and in any case should not rely on those details being stable (i.e. we should be able to change anything there without breaking user's overrides.

Argument that "it's us who will do this" and/or "this will live in our repositories" is invalid, because people will take this file as an example, will copy it elsewhere and adapt to their infrastructure.

Secondary consideration is, I don't want to favour any particular HSM or cloud unless they provide resources for CI in the project. Otherwise we can't test this, and by carrying this in our repo, there's an implicit expectation that we don't break it.

So I see two ways forward:

  1. we fit azure signing into signing plugins (RFC: Add sgx-sign plugins gramine#1118) (iiuc this means that we add dependencies for those other azure-specific modules and include subprocess calls to az login and logout) and provide --requirements option that would install this plugin (from either git+http:// or local repo, as supported by pip), then passthrough the arguments to gramine-sgx-sign similarly to $password or so
  2. if akv signing doesn't fit into signing plugins, then this needs to be rewritten that only parts of the template are changed, but the overall structure is not to be rewritten by the user

One possible way to do the second option would be to have --template-dir that would add an override FileSystemLoader like this:

loader_gsc_templates = jinja2.FileSystemLoader('templates')
env = jinja2.Environment(loader=jinja2.ChoiceLoader([
    jinja2.PrefixLoader({'': loader_gsc_templates, }, '!'),
    jinja2.FileSystemLoader(user_provided_templates_path),
    loader_gsc_templates,
]))

Then you can instrument this signing templates with {% block %}s and inherit from the original templates:

{% extends '!debian/sign.dockerfile' %}
{% block post-install %}
{{ super() }}
RUN python3 -m pip install whatever
RUN apt-get install also-whatever
{% endblock %}
{% extends '!rhel/sign.dockerfile' %}
{% block post-install %}
{{ super() }}
RUN python3 -m pip install whatever
RUN yum install also-whatever-but-rpm
{% endblock %}
{% extends '!sign.dockerfile' %}
{% block pre-sign %}
RUN az login
{% endblock %}

{% block post-sign %}
RUN az logout
{% endblock %}

But this would require reorganising the templates to allow not repeating ourselves, rethink names of templates and blocks, document those names, care about backwards compatibility etc.

PS 1b if support for encrypted keys is required, gramineproject/gramine#1197 is an RFC


Copy link
Author

@svenkata9 svenkata9 left a 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)

a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

I object to --template option as it stands, because:

  • GSC integrity should not rely on external template. It takes one clueless developer in the future to remove this line, and GSC would still "work" apart from the small detail that it ships private key used for signing. See comment in Add AKV Dockerfile templates for signing with gsc contrib#20
  • the user shouldn't be required to know GSC implenentation details (i.e. not only this FROM device, but also COPY paths, PYTHONPATH etc.) and in any case should not rely on those details being stable (i.e. we should be able to change anything there without breaking user's overrides.

Argument that "it's us who will do this" and/or "this will live in our repositories" is invalid, because people will take this file as an example, will copy it elsewhere and adapt to their infrastructure.

Secondary consideration is, I don't want to favour any particular HSM or cloud unless they provide resources for CI in the project. Otherwise we can't test this, and by carrying this in our repo, there's an implicit expectation that we don't break it.

So I see two ways forward:

  1. we fit azure signing into signing plugins (RFC: Add sgx-sign plugins gramine#1118) (iiuc this means that we add dependencies for those other azure-specific modules and include subprocess calls to az login and logout) and provide --requirements option that would install this plugin (from either git+http:// or local repo, as supported by pip), then passthrough the arguments to gramine-sgx-sign similarly to $password or so
  2. if akv signing doesn't fit into signing plugins, then this needs to be rewritten that only parts of the template are changed, but the overall structure is not to be rewritten by the user

One possible way to do the second option would be to have --template-dir that would add an override FileSystemLoader like this:

loader_gsc_templates = jinja2.FileSystemLoader('templates')
env = jinja2.Environment(loader=jinja2.ChoiceLoader([
    jinja2.PrefixLoader({'': loader_gsc_templates, }, '!'),
    jinja2.FileSystemLoader(user_provided_templates_path),
    loader_gsc_templates,
]))

Then you can instrument this signing templates with {% block %}s and inherit from the original templates:

{% extends '!debian/sign.dockerfile' %}
{% block post-install %}
{{ super() }}
RUN python3 -m pip install whatever
RUN apt-get install also-whatever
{% endblock %}
{% extends '!rhel/sign.dockerfile' %}
{% block post-install %}
{{ super() }}
RUN python3 -m pip install whatever
RUN yum install also-whatever-but-rpm
{% endblock %}
{% extends '!sign.dockerfile' %}
{% block pre-sign %}
RUN az login
{% endblock %}

{% block post-sign %}
RUN az logout
{% endblock %}

But this would require reorganising the templates to allow not repeating ourselves, rethink names of templates and blocks, document those names, care about backwards compatibility etc.

PS 1b if support for encrypted keys is required, gramineproject/gramine#1197 is an RFC

If that is your concern, let us go with my first proposal where we make the templates for already available HSMs, rather than in a separate contrib repo. At any point in time, there is going to be specific requirements be it AKV, or something else, and we will need to support it. Today we claim shipping a productization ready Gramine without having a production signing available. Let us solve that problem first for what we know, with what we already have as constraints.

And, how does your proposal of --requirements better than introducing other options which you initially opposed?


Copy link
Member

@woju woju left a 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)

a discussion (no related file):

we make the templates for already available HSMs

You can't possibly make templates for all available HSMs, because someone somewhere will do their own proprietary template for their own proprietary HSM. You've just created massive footgun for those people, if they don't really understand how docker works and how gsc works. This would be acceptable were it for some internal write-once software, but this is a public, FOSS project. We don't do footguns.

Ultimately, I don't see much technical difference between keeping auxiliary templates here or in contrib, they'll serve the same purpose for other people: they'll copy them and adjust to other workflows. Either way it's no good.

And, how does your proposal of --requirements better than introducing other options which you initially opposed?

Because you don't allow random people anywhere near our dockerfiles and templates. This is something I desperately want to avoid. The option with inheritable templates tries to achieve the same thing, but it's a significantly bigger risk in term of APIs exposed that people will depend upon.

Yes, people will depend on it, and Hyrum's law is a real thing, that is, even if we don't promise anything about this interface, people will use it and will complain that we've broken it.


Copy link
Member

@woju woju left a 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)

a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

we make the templates for already available HSMs

You can't possibly make templates for all available HSMs, because someone somewhere will do their own proprietary template for their own proprietary HSM. You've just created massive footgun for those people, if they don't really understand how docker works and how gsc works. This would be acceptable were it for some internal write-once software, but this is a public, FOSS project. We don't do footguns.

Ultimately, I don't see much technical difference between keeping auxiliary templates here or in contrib, they'll serve the same purpose for other people: they'll copy them and adjust to other workflows. Either way it's no good.

And, how does your proposal of --requirements better than introducing other options which you initially opposed?

Because you don't allow random people anywhere near our dockerfiles and templates. This is something I desperately want to avoid. The option with inheritable templates tries to achieve the same thing, but it's a significantly bigger risk in term of APIs exposed that people will depend upon.

Yes, people will depend on it, and Hyrum's law is a real thing, that is, even if we don't promise anything about this interface, people will use it and will complain that we've broken it.

I've made a comment in gramineproject/contrib#20 at the exact line that's for me a NAK from security POV, but from maintainability all other exposed things like variable names are also problematic.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants