RFC: Add sgx-sign plugins

[woju]

How to test this PR?

Rewrite some plugin (openpcsc or AKV?) and see if the API is sufficient.

---

This change is 

[donporter]

Reviewed 9 of 11 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @woju)

---

Documentation/python/writing-sgx-sign-plugins.rst line 40 at r3 (raw file):

Furthermore, your function needs to be packaged into Python distribution, which
will include and entry point from ``gramine.sgx_sign`` group. The entry point

This sentence is not super clear to me. Do you mean "an entry point"?

---

Documentation/python/writing-sgx-sign-plugins.rst line 42 at r3 (raw file):

will include and entry point from ``gramine.sgx_sign`` group. The entry point
needs to be named as your plugin and the callable it points to is the click
command.

Consider explaining specifically that the example below is in a file named 'sign_with_file' and perhaps narrate the control flow of how different hooks are called. This could also be below the example. I think what is there is probably sufficient, but I would feel more confident with a bit more explanation of the example code.

---

Documentation/python/writing-sgx-sign-plugins.rst line 49 at r3 (raw file):

For full example, please see ``sgx_sign.py`` file (note that ``graminelibos``
package is not package with ``setuptools``, so metadata is providedd manuall).

typos: "packaged", "provided manually"

[dimakuv]

Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @woju)

a discussion (no related file):
There is a general problem with this PR: this plugin arch is great for direct use of Gramine signing tools. But we will need some solution for GSC, because we must install the plugin inside the Docker container (so, change Docker images?).

See #1115 (the last item) for more context.

---

[woju]

Reviewable status: 12 of 13 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @donporter)

---

Documentation/python/writing-sgx-sign-plugins.rst line 40 at r3 (raw file):

Previously, donporter (Don Porter) wrote…

This sentence is not super clear to me. Do you mean "an entry point"?

Done. Yes, I did.

---

Documentation/python/writing-sgx-sign-plugins.rst line 42 at r3 (raw file):
No, it's in a file named sgx_sign.py (as in :caption:) and the colon in the entrypoint declaration delimits file and function name. I've added a .. seealso:: to an external documentation about entry points in general, for people who never did entry points in Python.

narrate the control flow of how different hooks are called

I think all relevant parts are in this part here, above example, but in reverse order, because this is written in the order of what the developer will need to write to glue all the parts together. Do you think it would be better to also repeat all this, but in the order of how imperative exectution happens inside gramine-sgx-sign?

---

Documentation/python/writing-sgx-sign-plugins.rst line 49 at r3 (raw file):

Previously, donporter (Don Porter) wrote…

typos: "packaged", "provided manually"

Done.

[donporter]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @woju)

---

Documentation/python/writing-sgx-sign-plugins.rst line 42 at r3 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

No, it's in a file named sgx_sign.py (as in :caption:) and the colon in the entrypoint declaration delimits file and function name. I've added a .. seealso:: to an external documentation about entry points in general, for people who never did entry points in Python.

narrate the control flow of how different hooks are called

I think all relevant parts are in this part here, above example, but in reverse order, because this is written in the order of what the developer will need to write to glue all the parts together. Do you think it would be better to also repeat all this, but in the order of how imperative exectution happens inside gramine-sgx-sign?

I see. I wasn't reading this as the end user would read it. Remind me how I see a preview of a doc PR?

[woju]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @donporter)

---

Documentation/python/writing-sgx-sign-plugins.rst line 42 at r3 (raw file):

Previously, donporter (Don Porter) wrote…

I see. I wasn't reading this as the end user would read it. Remind me how I see a preview of a doc PR?

1. sudo apt-get install doxygen python3-sphinx python3-sphinx-rtd-theme
2. fetch the repo, then checkout the branch of this PR (git fetch && git checkout woju/python-sgx-sign-plugins — this will create local branch with upstream set to my branch in the remote; alternatively you can git checkout origin/woju/python-sgx-sign-plugins or sth to not create branch and just work off detached HEAD)
3. cd Documentation && make html
4. firefox _build/html/index.html (this file directly would be firefox _build/html/python/writing-sgx-sign-plugins.html)

(or chromium [...] or whatever browser you prefer)

https://gramine.readthedocs.io/en/latest/devel/howto-doc.html#building-documentation

[woju]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dimakuv and @donporter)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

There is a general problem with this PR: this plugin arch is great for direct use of Gramine signing tools. But we will need some solution for GSC, because we must install the plugin inside the Docker container (so, change Docker images?).

See #1115 (the last item) for more context.

Yes, this by itself doesn't solve the GSC issue, but I plan that you'll be able to package the gsc solution together, by supplying another entrypoint for gsc that will plug into relevant part of signing image. Not in scope for this PR.

---

[dimakuv]

Reviewed 9 of 11 files at r1, 2 of 2 files at r2, 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @donporter and @woju)

a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

Yes, this by itself doesn't solve the GSC issue, but I plan that you'll be able to package the gsc solution together, by supplying another entrypoint for gsc that will plug into relevant part of signing image. Not in scope for this PR.

Could you expand a bit on this idea of supplying a "helper" entrypoint?

UPDATE: Ah, entrypoints not in the sense of Docker ENTRYPOINT, but in the sense of Python entrypoints: https://setuptools.pypa.io/en/latest/userguide/entry_point.html#advertising-behavior

---

a discussion (no related file):
Every PR from Woju is a tour de force in Python advanced programming.

---

---

Documentation/python/writing-sgx-sign-plugins.rst line 42 at r3 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

1. sudo apt-get install doxygen python3-sphinx python3-sphinx-rtd-theme
2. fetch the repo, then checkout the branch of this PR (git fetch && git checkout woju/python-sgx-sign-plugins — this will create local branch with upstream set to my branch in the remote; alternatively you can git checkout origin/woju/python-sgx-sign-plugins or sth to not create branch and just work off detached HEAD)
3. cd Documentation && make html
4. firefox _build/html/index.html (this file directly would be firefox _build/html/python/writing-sgx-sign-plugins.html)

(or chromium [...] or whatever browser you prefer)

https://gramine.readthedocs.io/en/latest/devel/howto-doc.html#building-documentation

@donporter For your convenience (and mine), I created a temporary page: https://gramine.readthedocs.io/en/woju-python-sgx-sign-plugins/python/writing-sgx-sign-plugins.html

---

Documentation/python/writing-sgx-sign-plugins.rst line 7 at r4 (raw file):

========================================

SGX cryptosystem uses RSA-3072 with modulus 3 for signing SIGSTRUCT. However

a SIGSTRUCT?

---

Documentation/python/writing-sgx-sign-plugins.rst line 10 at r4 (raw file):

there are different arrangements where suitable keys are kept and used for
operations. Keyfile is not always available (e.g. HSMs explicitly prevent users
from extracting keys), so we need adaptable ways of signing the enclaves. This

signing the enclaves -> signing enclaves

---

Documentation/python/writing-sgx-sign-plugins.rst line 18 at r4 (raw file):

``standalone_mode=False``.

The command needs to return singing function that will be passed to

typo: signing

---

Documentation/python/writing-sgx-sign-plugins.rst line 18 at r4 (raw file):

``standalone_mode=False``.

The command needs to return singing function that will be passed to

return -> maybe define?

---

Documentation/python/writing-sgx-sign-plugins.rst line 56 at r4 (raw file):

For full example, please see ``sgx_sign.py`` file (note that ``graminelibos``
package is not packaged with ``setuptools``, so metadata is provided manually).

What's the point of this note? I guess you mean that we install those dist-info/ files manually, using Meson? Why would this knowledge be important to end users?

UPDATE: Ok, this note probably tries to explain the discrepancy between what we have in our core repo (without setuptools installation) and what you write here in setup.py script.

Is there any better way to word it?.. This is quite confusing.

---

Documentation/python/writing-sgx-sign-plugins.rst line 72 at r4 (raw file):

       return functools.partial(sign, key=key), [key]

   def sign(data, *, key):

What is this * arg? What does it symbolize?

---

Documentation/python/writing-sgx-sign-plugins.rst line 77 at r4 (raw file):

.. code-block:: python
   :caption: setup.py

Isn't setup.py deprecated? According to this document: https://setuptools.pypa.io/en/latest/userguide/quickstart.html#entry-points-and-automatic-script-creation, it's better to use pyproject.toml

Un the other hand, looks like other, newer syntaxes are too new and not stable enough.

---

debian/control line 39 at r4 (raw file):

 libprotobuf-c1,
 python3,
 python3 (>= 3.10) | python3-pkg-resources,

Out of curiosity, how to read this expression?

UPDATE: IIUC, it's a logical or: either depend on Python 3.10+, but if version of Python is lower, then depend on python3-pkg-resources. Right?

---

python/gramine-sgx-sign line 66 at r4 (raw file):

    # pylint: disable=too-many-arguments, too-many-locals

    sign_func = get_sgx_sign_plugin(with_)(args=plugin_args, standalone_mode=False)

Actually, how does this work with these (args= ...) brackets? The examples I looked at only have no-args explanations of entrypoints.

To me, the current syntax looks like we got the plugin function (via get_sgx_sign_plugin(with_)), and now we perform a function call with arguments (args= ...). But apparently that's not how it should be read? This somehow "configures" the loaded plugin function with these specific arguments and forces it to be non-standalone execution, but the actual call of this plugin function will happen later. Is my understanding correct?

---

python/gramine-sgx-sign line 101 at r4 (raw file):

        #
        # - `.manifest.sgx` depends on all files we just expanded
        # - `.sig` additionally depends on libpal and key

and key -- this is stale now

---

python/graminelibos/sgx_sign.py line 551 at r4 (raw file):

@click.command(add_help_option=False)
@click.help_option('--help-file')
@click.option('--key', '-k', metavar='FILE',

Here we don't follow your advice of Also consider prefixing your options with --PLUGIN- to avoid conflicting with generic options. Is it because this is a legacy argument, and we don't want to change it? Maybe we can add an alias --file-key?

---

python/graminelibos/sgx_sign.py line 556 at r4 (raw file):

    help='specify signing key (.pem) file')
def sign_with_file(key):
    return functools.partial(sign_with_local_key, key=key), [key]

What's the deal with returning a second object which is a list with a single item key?

UPDATE: Ok, the documentation says it: In addition to the signing function, you can return an iterable of local files that were accessed during signature generation (for the purpose of tracking dependencies).. Cool.

---

python/graminelibos.dist-info/METADATA.in line 2 at r4 (raw file):

Metadata-Version: 2.1
Name: @NAME@libos

Why do you have libos suffix here?

[woju]

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @dimakuv and @donporter)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you expand a bit on this idea of supplying a "helper" entrypoint?

UPDATE: Ah, entrypoints not in the sense of Docker ENTRYPOINT, but in the sense of Python entrypoints: https://setuptools.pypa.io/en/latest/userguide/entry_point.html#advertising-behavior

Yes exactly.

---

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Every PR from Woju is a tour de force in Python advanced programming.

I'm sorry for that.

---

---

Documentation/python/writing-sgx-sign-plugins.rst line 7 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

a SIGSTRUCT?

Done.

---

Documentation/python/writing-sgx-sign-plugins.rst line 10 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

signing the enclaves -> signing enclaves

Done.

---

Documentation/python/writing-sgx-sign-plugins.rst line 18 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

typo: signing

Done.

---

Documentation/python/writing-sgx-sign-plugins.rst line 18 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

return -> maybe define?

Uh, IDK. The programmer needs to define the signing function, the intermediate function (the @click.command) needs to return it.

---

Documentation/python/writing-sgx-sign-plugins.rst line 56 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What's the point of this note? I guess you mean that we install those dist-info/ files manually, using Meson? Why would this knowledge be important to end users?

UPDATE: Ok, this note probably tries to explain the discrepancy between what we have in our core repo (without setuptools installation) and what you write here in setup.py script.

Is there any better way to word it?.. This is quite confusing.

Yes, the reason for this note is that you won't be able to find setup.py. IDK how to craft this so it will be less confusing.

You certainly also know that there are several ways to define entrypoints ("setuptools is deprecated" comment below). E.g. because you might be using setup.cfg or pyproject.toml from PEP 517. We don't package for python ecosystem (we don't do wheels and don't publish on PyPI, so we have almost no use for any of the python packaging tools), but people who will be doing plugins will almost certainly want to do at least some of the python packaging (for them it's the exact opposite of our situation: they don't need meson). So what I've picked here is just an example how to do that. Our function is a good example of the function (not least because it's the only example), but there's no good way to show the entrypoints.

---

Documentation/python/writing-sgx-sign-plugins.rst line 72 at r4 (raw file):
This means that whatever is after * is keyword-only argument. You're forbidden from calling this with two positional arguments, but you need to explicitly pass the key argument with keyword:

>>> def foo(a, *, b):
...   pass
... 
>>> foo(1, 2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: foo() takes 1 positional argument but 2 were given
>>> foo(1, b=2)
>>> 

It's natural when you think about def foo(a, *args, b=None): (b is keyword-only for obvious reasons you can't pass b as positional arg, because positonal args will fall into *args), so it follows that specifying * without identifier causes 1) no varargs, 2) b is still keyword-only. Here's a spec with more rationale: https://peps.python.org/pep-3102/

I've used it here so people don't mess those functions and pass some other argument as mistake (other functions might have url= for some API or pin= for smartcard).

---

Documentation/python/writing-sgx-sign-plugins.rst line 77 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Isn't setup.py deprecated? According to this document: https://setuptools.pypa.io/en/latest/userguide/quickstart.html#entry-points-and-automatic-script-creation, it's better to use pyproject.toml

Un the other hand, looks like other, newer syntaxes are too new and not stable enough.

Not yet in Ubuntu 22.04: jammy has setuptools 0.59, and pyproject.toml is well supported from setuptools 0.61. And people who are reading this will probably understand what's happening in setup.py anyway.

---

debian/control line 39 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Out of curiosity, how to read this expression?

UPDATE: IIUC, it's a logical or: either depend on Python 3.10+, but if version of Python is lower, then depend on python3-pkg-resources. Right?

Right, and there's usually preference to first option: resolver will attempt to satisfy this dep by updating python3 (if available) before trying to craft a solution that would installing the other package. If I swapped the sides, it might cause it to install python3-pkg-resources even if python3 is available with sufficient version.

This obviously reflects if sys.version_info > (3, 10) in sgx_sign.py.

---

python/gramine-sgx-sign line 66 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Actually, how does this work with these (args= ...) brackets? The examples I looked at only have no-args explanations of entrypoints.

To me, the current syntax looks like we got the plugin function (via get_sgx_sign_plugin(with_)), and now we perform a function call with arguments (args= ...). But apparently that's not how it should be read? This somehow "configures" the loaded plugin function with these specific arguments and forces it to be non-standalone execution, but the actual call of this plugin function will happen later. Is my understanding correct?

No, first reading is correct. First you get a function from get_sgx_sign_plugin() as a value, and this value you use as a function, namely you call it with two arguments: (args=, standalone_mode=). What's not here is, this is not really the function that you've defined, but click.Command (note capital C) instance: https://click.palletsprojects.com/en/8.1.x/api/#click.Command. Well, you've defined it via click.command — note lower c — decorator, and this decorator wraps the function in the Command instance, but the Command.__call__ signature is not that function and has unrelated signature. click.Command.__call__ passes args= to the function what was def'd as *args, not directly.

---

python/gramine-sgx-sign line 101 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

and key -- this is stale now

Done.

---

python/graminelibos/sgx_sign.py line 551 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Here we don't follow your advice of Also consider prefixing your options with --PLUGIN- to avoid conflicting with generic options. Is it because this is a legacy argument, and we don't want to change it? Maybe we can add an alias --file-key?

I'm not sure. I obviosuly don't want to change it, and BTW this plugin gets called when you don't specify --with at all, so why would you have to have prefix if you don't know about this plugin.

Yes, this is rules for thee, but not for me, but this is the default plugin.

---

python/graminelibos/sgx_sign.py line 556 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What's the deal with returning a second object which is a list with a single item key?

UPDATE: Ok, the documentation says it: In addition to the signing function, you can return an iterable of local files that were accessed during signature generation (for the purpose of tracking dependencies).. Cool.

Yep.

---

python/graminelibos.dist-info/METADATA.in line 2 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why do you have libos suffix here?

Because @NAME@ is "gramine", but our python packages are called "graminelibos" (as in import graminelibos), which is remainder from the times when it was "graphenelibos" (because it collided with other python packages).

We could rename our python package to just "gramine", but that would be a bigger operation (IDK if we wouldn't have to do some compatibility module). So I want to do it if/when we'll do bigger refactoring (some things will need it), not now.

[dimakuv]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, 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 @donporter and @woju)

a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

I'm sorry for that.

:)

---

---

Documentation/python/writing-sgx-sign-plugins.rst line 18 at r4 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Uh, IDK. The programmer needs to define the signing function, the intermediate function (the @click.command) needs to return it.

Ok.

---

Documentation/python/writing-sgx-sign-plugins.rst line 56 at r4 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Yes, the reason for this note is that you won't be able to find setup.py. IDK how to craft this so it will be less confusing.

You certainly also know that there are several ways to define entrypoints ("setuptools is deprecated" comment below). E.g. because you might be using setup.cfg or pyproject.toml from PEP 517. We don't package for python ecosystem (we don't do wheels and don't publish on PyPI, so we have almost no use for any of the python packaging tools), but people who will be doing plugins will almost certainly want to do at least some of the python packaging (for them it's the exact opposite of our situation: they don't need meson). So what I've picked here is just an example how to do that. Our function is a good example of the function (not least because it's the only example), but there's no good way to show the entrypoints.

Ok, I don't know how to change the wording. This is quite technically challenging stuff here anyway, so readers will be expected to know about all this Python tooling.

---

Documentation/python/writing-sgx-sign-plugins.rst line 72 at r4 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

This means that whatever is after * is keyword-only argument. You're forbidden from calling this with two positional arguments, but you need to explicitly pass the key argument with keyword:

>>> def foo(a, *, b):
...   pass
... 
>>> foo(1, 2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: foo() takes 1 positional argument but 2 were given
>>> foo(1, b=2)
>>> 

It's natural when you think about def foo(a, *args, b=None): (b is keyword-only for obvious reasons you can't pass b as positional arg, because positonal args will fall into *args), so it follows that specifying * without identifier causes 1) no varargs, 2) b is still keyword-only. Here's a spec with more rationale: https://peps.python.org/pep-3102/

I've used it here so people don't mess those functions and pass some other argument as mistake (other functions might have url= for some API or pin= for smartcard).

Cool syntax, didn't know.

---

python/gramine-sgx-sign line 66 at r4 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

No, first reading is correct. First you get a function from get_sgx_sign_plugin() as a value, and this value you use as a function, namely you call it with two arguments: (args=, standalone_mode=). What's not here is, this is not really the function that you've defined, but click.Command (note capital C) instance: https://click.palletsprojects.com/en/8.1.x/api/#click.Command. Well, you've defined it via click.command — note lower c — decorator, and this decorator wraps the function in the Command instance, but the Command.__call__ signature is not that function and has unrelated signature. click.Command.__call__ passes args= to the function what was def'd as *args, not directly.

Ok, so we call the click.Command.__call__(). Looking at the code, it literally invokes the "wrapped" function: https://github.com/pallets/click/blob/49fe8fb9459a13aeec0d24a5997167f84c88d26f/click/core.py#L821

So at this point, we really called the sign_func(). But why did we call it now? Below we pass this func to the signing logic: sigstruct.sign(sign_func). So this func will be called twice?

---

python/graminelibos/sgx_sign.py line 551 at r4 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

I'm not sure. I obviosuly don't want to change it, and BTW this plugin gets called when you don't specify --with at all, so why would you have to have prefix if you don't know about this plugin.

Yes, this is rules for thee, but not for me, but this is the default plugin.

Ok, fair enough :)

---

python/graminelibos.dist-info/METADATA.in line 2 at r4 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Because @NAME@ is "gramine", but our python packages are called "graminelibos" (as in import graminelibos), which is remainder from the times when it was "graphenelibos" (because it collided with other python packages).

We could rename our python package to just "gramine", but that would be a bigger operation (IDK if we wouldn't have to do some compatibility module). So I want to do it if/when we'll do bigger refactoring (some things will need it), not now.

Ah, I see.

[woju]

Reviewable status: all files reviewed, 2 unresolved discussions, 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 @donporter)

---

python/gramine-sgx-sign line 66 at r4 (raw file):

Looking at the code, it literally invokes the "wrapped" function

Nope. Second () operator calls Command.__call__. sign_with_file is Command().callback, but please look closely that __call__ invokes self.main(args=args), and in main(), those args are put into Context() and then self.invoke(ctx) is called. Command.invoke(ctx) calls Context.invoke() which only then calls back Command().callback(*args) (which is the real sign_with_something() that was originally defd under the decorator.

(You didn't need to know about Context, which is a big pile of magic that needs to happen for other click things to fall nicely into their places.)

So the return value of Command().__call__(args=, standalone_mode=True) is the return value of sign_with_something(). Now that we've called sign_with_something(), it might have returned one of those two things:

return sign_func

or

return sign_func, extra_deps

So we speculatively assign the return value (either callable or tuple) to sign_func, but then probe it in try/except block below to extract extra_deps if those are present. After try/except sign_func is callable and extra_deps is (possibly empty) iterable.

sign_func is passed as argument to Sigstruct.sign(). Sigstruct.sign() calls sign_func().

Now the twist: sign_func is really functools.partial instance. So sign_func() calls functools.partial.__call__(), which picks set of arguments from partial attribute .args (which were put there by sign_with_file) and then calls sign_with_key or sth: https://github.com/python/cpython/blob/f7df17394906f2af51afef3c8ccaaab3847b059c/Modules/_functoolsmodule.c#L325-L331

sign_with_key does actual signing and returns 3 ints with actual crypto material. (We don't call those ints further, because they're not callable).

---

If you wonder why is all of that ^ stack: we need this because how Sigstruct.sign plugs. We need to have configurable arguments (keypath, url, pin) to a function passed as callback. Both those arguments, and also the function that accepts those (sign with key, with smartcard, ...) needs to be provided from CLI. We could remove one level of indirection by inverting the process of signing the sigstruct (we could pass Sigstruct as argument to an external function and expect that the function would return sigstruct with signature put into the struct. But this would require users to handle the sigstruct themselves (possibly including computing q1 and q2) and we also would have to check if user didn't mess with it too much.

[dimakuv]

Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @donporter)

---

python/gramine-sgx-sign line 66 at r4 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Looking at the code, it literally invokes the "wrapped" function

Nope. Second () operator calls Command.__call__. sign_with_file is Command().callback, but please look closely that __call__ invokes self.main(args=args), and in main(), those args are put into Context() and then self.invoke(ctx) is called. Command.invoke(ctx) calls Context.invoke() which only then calls back Command().callback(*args) (which is the real sign_with_something() that was originally defd under the decorator.

(You didn't need to know about Context, which is a big pile of magic that needs to happen for other click things to fall nicely into their places.)

So the return value of Command().__call__(args=, standalone_mode=True) is the return value of sign_with_something(). Now that we've called sign_with_something(), it might have returned one of those two things:

return sign_func

or

return sign_func, extra_deps

So we speculatively assign the return value (either callable or tuple) to sign_func, but then probe it in try/except block below to extract extra_deps if those are present. After try/except sign_func is callable and extra_deps is (possibly empty) iterable.

sign_func is passed as argument to Sigstruct.sign(). Sigstruct.sign() calls sign_func().

Now the twist: sign_func is really functools.partial instance. So sign_func() calls functools.partial.__call__(), which picks set of arguments from partial attribute .args (which were put there by sign_with_file) and then calls sign_with_key or sth: https://github.com/python/cpython/blob/f7df17394906f2af51afef3c8ccaaab3847b059c/Modules/_functoolsmodule.c#L325-L331

sign_with_key does actual signing and returns 3 ints with actual crypto material. (We don't call those ints further, because they're not callable).

---

If you wonder why is all of that ^ stack: we need this because how Sigstruct.sign plugs. We need to have configurable arguments (keypath, url, pin) to a function passed as callback. Both those arguments, and also the function that accepts those (sign with key, with smartcard, ...) needs to be provided from CLI. We could remove one level of indirection by inverting the process of signing the sigstruct (we could pass Sigstruct as argument to an external function and expect that the function would return sigstruct with signature put into the struct. But this would require users to handle the sigstruct themselves (possibly including computing q1 and q2) and we also would have to check if user didn't mess with it too much.

Ok, I actually missed that we have this code:

def sign_with_file(key):
    return functools.partial(sign_with_local_key, key=key), [key]

So the sign_with_file() function doesn't invoke the real-deal sign_with_local_key(), but instead returns the function instance (plus the dependency list).

Thanks for this tour, I understand everything now.

[woju]

Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @donporter)

---

python/gramine-sgx-sign line 66 at r4 (raw file):

Ok, I actually missed that we have this code:

This ^ statement is worrying, because this is exactly how people will have to implement this feature. For me this means this API is too complicated already. I'll look into it, if we can simplify this in any way.

Can we consider this API "unstable" or "provisional", and subject to change without notification until I'll do another round of research?

[dimakuv]

Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @donporter)

---

python/gramine-sgx-sign line 66 at r4 (raw file):

This ^ statement is worrying, because this is exactly how people will have to implement this feature.

Well, maybe just add a comment in that function? That it returns a function instance (and does NOT invoke the wrapped function).

Can we consider this API "unstable" or "provisional", and subject to change without notification until I'll do another round of research?

Yeah, I think we can, as no-one will be using this API in the near future (other than us).

[mkow]

Reviewed 8 of 11 files at r1, 2 of 2 files at r2, 1 of 2 files at r3, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @donporter and @woju)

a discussion (no related file):
I think this will slightly conflict with #1362 (docs reorg; the whole "PYTHON" section was removed and the contents moved to some other group) and IMO better to merge the reorg first, and then adjust this one than the other way around (in terms of conflicts, if you don't like the new grouping then please comment there).

---

---

Documentation/python/writing-sgx-sign-plugins.rst line 7 at r5 (raw file):

========================================

SGX cryptosystem uses RSA-3072 with modulus 3 for signing a SIGSTRUCT. However

Suggestion:

However,

---

Documentation/python/writing-sgx-sign-plugins.rst line 19 at r5 (raw file):

The command needs to return signing function that will be passed to
``Sigstruct.sign``. It returns 3-tuple:

Suggestion:

The signing function should return a 3-tuple:

---

Documentation/python/writing-sgx-sign-plugins.rst line 29 at r5 (raw file):

:func:`functools.partial`.

In addition to the signing function, you can return an iterable of local files

How exactly? The previous paragraph said that this function returns a function, but this says that you can somehow return an additional value.

Code quote:

In addition to the signing function, you can return

---

Documentation/python/writing-sgx-sign-plugins.rst line 42 at r5 (raw file):
Weird construction:

ASDF needs to [...] and ZXCV is the click command.

(first part says "needs" and the second "is", but they both state the requirements for the same thing)

I'd rather change that "is" to "needs".

---

Documentation/python/writing-sgx-sign-plugins.rst line 54 at r5 (raw file):

Example
-------

I tried using this + #1197 and it seems that one important thing is missing: I couldn't find a way to list available plugins. The help only says which one is the default (file), but doesn't seem to say what are the other options.

[donporter]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @woju)

---

Documentation/python/writing-sgx-sign-plugins.rst line 9 at r5 (raw file):

SGX cryptosystem uses RSA-3072 with modulus 3 for signing a SIGSTRUCT. However
there are different arrangements where suitable keys are kept and used for
operations. Keyfile is not always available (e.g. HSMs explicitly prevent users

"A keyfile"

"e.g.," (always a comma after e.g. and i.e.)

---

Documentation/python/writing-sgx-sign-plugins.rst line 34 at r5 (raw file):

Your command can accept any command-line arguments you need to complete the
signing (like path to keyfile, URL to some external API, PIN to smartcard etc.).

"smartcard, "

Also, don't need "etc" for a non-complete list that starts with "like"

---

Documentation/python/writing-sgx-sign-plugins.rst line 35 at r5 (raw file):

Your command can accept any command-line arguments you need to complete the
signing (like path to keyfile, URL to some external API, PIN to smartcard etc.).
It is strongly recommended that you provide ``--help-PLUGIN`` option (with

"provide the..." - more idiomatic

---

Documentation/python/writing-sgx-sign-plugins.rst line 36 at r5 (raw file):

signing (like path to keyfile, URL to some external API, PIN to smartcard etc.).
It is strongly recommended that you provide ``--help-PLUGIN`` option (with
your plugin name substituted for ``PLUGIN``). Also consider prefixing your

Also,

---

Documentation/python/writing-sgx-sign-plugins.rst line 39 at r5 (raw file):

options with ``--PLUGIN-`` to avoid conflicting with generic options.

Furthermore, your function needs to be packaged into Python distribution, which

Is this something our average reader knows how to do, or should we link some external documentation?

[woju]

Reviewable status: 3 of 14 files reviewed, 10 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv, @donporter, and @mkow)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

I think this will slightly conflict with #1362 (docs reorg; the whole "PYTHON" section was removed and the contents moved to some other group) and IMO better to merge the reorg first, and then adjust this one than the other way around (in terms of conflicts, if you don't like the new grouping then please comment there).

Rebased on top of master, resolved conflict with #1362.

---

---

Documentation/python/writing-sgx-sign-plugins.rst line 7 at r5 (raw file):

========================================

SGX cryptosystem uses RSA-3072 with modulus 3 for signing a SIGSTRUCT. However

Done.

---

Documentation/python/writing-sgx-sign-plugins.rst line 9 at r5 (raw file):

Previously, donporter (Don Porter) wrote…

"A keyfile"

"e.g.," (always a comma after e.g. and i.e.)

Done.

---

Documentation/python/writing-sgx-sign-plugins.rst line 19 at r5 (raw file):

The command needs to return signing function that will be passed to
``Sigstruct.sign``. It returns 3-tuple:

Done.

---

Documentation/python/writing-sgx-sign-plugins.rst line 29 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

How exactly? The previous paragraph said that this function returns a function, but this says that you can somehow return an additional value.

Python developers (and also Lua, for that matter) understand this. After "return sign_func" you write a comma and another thing that you return. Yes, this changes return type from scalar to tuple, which might be odd for people used to strongly typed languages, but this should be understandable.

---

Documentation/python/writing-sgx-sign-plugins.rst line 34 at r5 (raw file):

Previously, donporter (Don Porter) wrote…

"smartcard, "

Also, don't need "etc" for a non-complete list that starts with "like"

Done.

---

Documentation/python/writing-sgx-sign-plugins.rst line 36 at r5 (raw file):

Previously, donporter (Don Porter) wrote…

Also,

Done.

---

Documentation/python/writing-sgx-sign-plugins.rst line 39 at r5 (raw file):

Previously, donporter (Don Porter) wrote…

Is this something our average reader knows how to do, or should we link some external documentation?

"Python developers" generally know this. They certainly know packaging and probably know entrypoints, but if by chance they don't know entrypoints, there are two .. seealso:: items below this paragraph explaining what are "entrypoints" in Python universe (they are a feature of python packaging, but not ubiquitous, so if someone doesn't write CLI scripts or plugins, but for example webapps or ML notebooks, there's a chance s/he might have never encountered them).
.
Or I can write some explainer if you think that readers won't be "Python developers" at all.

---

Documentation/python/writing-sgx-sign-plugins.rst line 42 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Weird construction:

ASDF needs to [...] and ZXCV is the click command.

(first part says "needs" and the second "is", but they both state the requirements for the same thing)

I'd rather change that "is" to "needs".

Done.

---

Documentation/python/writing-sgx-sign-plugins.rst line 54 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I tried using this + #1197 and it seems that one important thing is missing: I couldn't find a way to list available plugins. The help only says which one is the default (file), but doesn't seem to say what are the other options.

It would show available options you if you gave invalid choice (gramine-sgx-sign --with=asdf), but yeah, it's not very discoverable. Try now gramine-sgx-sign --help. I can also make --list-plugins option that would list the plugins and exit.

[donporter]

Reviewed 11 of 11 files at r6, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @mkow and @woju)

---

Documentation/index.rst line 78 at r6 (raw file):

  and prepare a signing key.
- :doc:`Provide an application-specific configuration file<manifest-syntax>` -
  Gramine requires a so-called manifest file for each application.

I would remove "so-called" - in US English, it has a connotation of doubt/suspicion.

---

Documentation/python/writing-sgx-sign-plugins.rst line 39 at r5 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

"Python developers" generally know this. They certainly know packaging and probably know entrypoints, but if by chance they don't know entrypoints, there are two .. seealso:: items below this paragraph explaining what are "entrypoints" in Python universe (they are a feature of python packaging, but not ubiquitous, so if someone doesn't write CLI scripts or plugins, but for example webapps or ML notebooks, there's a chance s/he might have never encountered them).
.
Or I can write some explainer if you think that readers won't be "Python developers" at all.

See also links seem sufficient to me. thanks!

[mkow]

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

---

Documentation/python/writing-sgx-sign-plugins.rst line 29 at r5 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

I've rewritten this, also reordered things to group the topics. Maybe it will be more clear this ways.

Yup, now it's clear, thanks!

---

Documentation/python/writing-sgx-sign-plugins.rst line 25 at r7 (raw file):

which will include an entry point from ``gramine.sgx_sign`` group. The entry
point needs to be named as your plugin and the callable it points to needs to be
the click command.

Suggestion:

a click command

---

Documentation/python/writing-sgx-sign-plugins.rst line 39 at r7 (raw file):

:func:`functools.partial`.

Alternatively, the click command can return 2-tuple of:

Suggestion:

a 2-tuple of

[woju]

Reviewable status: 13 of 14 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow)

---

Documentation/python/writing-sgx-sign-plugins.rst line 25 at r7 (raw file):

which will include an entry point from ``gramine.sgx_sign`` group. The entry
point needs to be named as your plugin and the callable it points to needs to be
the click command.

No, this is actually good. We're talking about the same click command as in previous paragraph.

I can also say "the click command in question" if you prefer.

---

Documentation/python/writing-sgx-sign-plugins.rst line 39 at r7 (raw file):

:func:`functools.partial`.

Alternatively, the click command can return 2-tuple of:

Done.

[mkow]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: ITL)

---

Documentation/python/writing-sgx-sign-plugins.rst line 25 at r7 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

No, this is actually good. We're talking about the same click command as in previous paragraph.

I can also say "the click command in question" if you prefer.

Ah, it's from the previous paragraph, ok.

[dimakuv]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[kailun-qin]

Reviewed 2 of 11 files at r1, 1 of 2 files at r2, 10 of 11 files at r6, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @woju)

---

Documentation/python/writing-sgx-sign-plugins.rst line 1 at r8 (raw file):

.. default-domain:: py

Should we also update Documentation/index.rst accordingly?

---

python/gramine-sgx-sign line 38 at r8 (raw file):

    context_settings={'ignore_unknown_options': True},
    epilog=textwrap.dedent(f'''
        Use --with=PLUGIN --help-PLUGIN to get help about particular plugin.

hmm... How does this get help about particular plugin actually work?

1. The name of the plugin help option is defined by the plugin, i.e., not even with --help- prefix?
2. --help-PLUGIN is actually one of the 'plugin_args'? Then I imagine w/ only gramine-sgx-sign --with=PLUGIN --help-PLUGIN won't work as expected since the output (-o) and manifest (-m) are currently also “required” as inputs?
3. W/ --with=PLUGIN --help-PLUGIN, the program should exit after printing the help page, can we handle this case?

Code quote:

--help-PLUGIN

[woju]

Reviewable status: 13 of 15 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv, @donporter, @kailun-qin, and @mkow)

---

Documentation/python/writing-sgx-sign-plugins.rst line 1 at r8 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Should we also update Documentation/index.rst accordingly?

No, why? This is per-document, so when I write :class:`something`, then it means :py:class:, not :c:class:. The global default is in conf.py:

gramine/Documentation/conf.py

Line 84 in 7485c90

primary_domain = 'c'

---

python/gramine-sgx-sign line 38 at r8 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

hmm... How does this get help about particular plugin actually work?

1. The name of the plugin help option is defined by the plugin, i.e., not even with --help- prefix?
2. --help-PLUGIN is actually one of the 'plugin_args'? Then I imagine w/ only gramine-sgx-sign --with=PLUGIN --help-PLUGIN won't work as expected since the output (-o) and manifest (-m) are currently also “required” as inputs?
3. W/ --with=PLUGIN --help-PLUGIN, the program should exit after printing the help page, can we handle this case?

Done. All correct, thanks for catching this.

I wrote unconditionally --help-PLUGIN, because I also wrote in Documentation that people are supposed to add this option.

[kailun-qin]

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @woju)

---

Documentation/python/writing-sgx-sign-plugins.rst line 1 at r8 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

No, why? This is per-document, so when I write :class:`something`, then it means :py:class:, not :c:class:. The global default is in conf.py:

gramine/Documentation/conf.py

Line 84 in 7485c90

primary_domain = 'c'

Sorry for the confusions, I did not mean this specific line of code (perhaps I should comment in the top-level discussions).

I meant since we add this new doc, should we update the corresponding section (e.g., Develop Gramine) in Documentation/index.rst and also add it to the TOCs?

---

python/gramine-sgx-sign line 38 at r8 (raw file):

I wrote unconditionally --help-PLUGIN, because I also wrote in Documentation that people are supposed to add this option.

Sure, I'm fine w/ it.

[woju]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kailun-qin)

---

Documentation/python/writing-sgx-sign-plugins.rst line 1 at r8 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Sorry for the confusions, I did not mean this specific line of code (perhaps I should comment in the top-level discussions).

I meant since we add this new doc, should we update the corresponding section (e.g., Develop Gramine) in Documentation/index.rst and also add it to the TOCs?

There already is the change to that effect since r1, and it was adjusted when I rebased this after "docs reorganisation" at r6. Should I do something else there?

[kailun-qin]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @woju)

---

Documentation/python/writing-sgx-sign-plugins.rst line 1 at r8 (raw file):

There already is the change to that effect since r1, and it was adjusted when I rebased this after "docs reorganisation" at r6.

Ah yes, sorry I missed it -- it's indeed already added into the TOCs.

Should I do something else there?

Yes, I think we should also add some doc descriptions (for "writing plugins for signing SGX enclaves") in section Develop Gramine like:

gramine/Documentation/index.rst

Line 96 in 7485c90

- :doc:`Use Python API<python/api>` - Use Python API provided by Gramine.

.

[woju]

Reviewable status: 14 of 16 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: ITL) (waiting on @dimakuv, @donporter, @kailun-qin, and @mkow)

---

Documentation/python/writing-sgx-sign-plugins.rst line 1 at r8 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

There already is the change to that effect since r1, and it was adjusted when I rebased this after "docs reorganisation" at r6.

Ah yes, sorry I missed it -- it's indeed already added into the TOCs.

Should I do something else there?

Yes, I think we should also add some doc descriptions (for "writing plugins for signing SGX enclaves") in section Develop Gramine like:

gramine/Documentation/index.rst

Line 96 in 7485c90

- :doc:`Use Python API<python/api>` - Use Python API provided by Gramine.

.

Done. Also added description in gramine-sgx-sign manpage, which I linked from there.

[dimakuv]

Reviewed 1 of 2 files at r9, 2 of 2 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[kailun-qin]

Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[mkow]

Reviewed 2 of 2 files at r9, 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @woju)

---

python/gramine-sgx-sign line 78 at r10 (raw file):

        # - subcommand's help_option called ctx.exit() eagerly
        # - Context.exit() raised click.Exit exception
        # - this exception was catched in Context.main() and handled depending on standalone_mode

Suggestion:

this exception was caught

[woju]

Reviewable status: 15 of 16 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) (waiting on @dimakuv, @kailun-qin, and @mkow)

---

python/gramine-sgx-sign line 78 at r10 (raw file):

        # - subcommand's help_option called ctx.exit() eagerly
        # - Context.exit() raised click.Exit exception
        # - this exception was catched in Context.main() and handled depending on standalone_mode

Done.

[dimakuv]

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkow)

[mkow]

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved