Rewrite SGX signing in cryptography.io

[woju]

How to test this PR?

For unencrypted keys, CI is OK. If you want to test encrypted keys, generate one manually (gramine-sgx-gen-private-key can't do that) and test also manually.

---

This change is 

[woju]

python/graminelibos/sgx_sign.py line 585 at r1 (raw file):

        sys.exit(1)

    signature = private_key.sign(data, padding.PKCS1v15(), hashes.SHA256())

@mkow @dimakuv: padding.PKCS1v15 is documented as "not constant time": https://cryptography.io/en/latest/hazmat/primitives/asymmetric/rsa/#cryptography.hazmat.primitives.asymmetric.padding.PKCS1v15. Is this a problem? I think not, because 1) signing machine is assumed secure in SGX threat model, 2) the length is always the same (SHA256), but please someone better at crypto than me to check this.

[kailun-qin]

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

---

python/graminelibos/sgx_sign.py line 585 at r1 (raw file):

1. signing machine is assumed secure in SGX threat model

Yes.

2. the length is always the same (SHA256)

The length of what? And how it's immune from the (original) problem?

Anyway, IMO it's not a problem.

---

python/graminelibos/sgx_sign.py line 607 at r3 (raw file):

def sign_with_rsa_key(data, private_key):
    """Signs *data* using *key*.

private_key

Code quote:

key

[dimakuv]

Reviewed 1 of 1 files at r3, 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 (1 more required, approved so far: ITL) (waiting on @mkow and @woju)

a discussion (no related file):
Shouldn't there be a corresponding PR in GSC repo, that would:

• Remove https://github.com/gramineproject/gsc/blob/master/sign.sh
• Remove passphrase arg from GSC itself: https://github.com/gramineproject/gsc/blob/a60a4991d7351c1b7489444c694638ca9cc68e39/gsc.py#L504
• Fix around here: https://github.com/gramineproject/gsc/blob/a60a4991d7351c1b7489444c694638ca9cc68e39/gsc.py#L363
• Fix around here: https://github.com/gramineproject/gsc/blob/b7f0576219321c1b7d94b29661274419b009fc49/templates/Dockerfile.common.sign.template#L8

---

---

python/graminelibos/sgx_sign.py line 585 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

1. signing machine is assumed secure in SGX threat model

Yes.

2. the length is always the same (SHA256)

The length of what? And how it's immune from the (original) problem?

Anyway, IMO it's not a problem.

1 -- yes.
2 -- I din't understand the problem. SHA256 has the same length, yes, but how is this relevant?

Anyway, looks good to me.

---

python/graminelibos/sgx_sign.py line 555 at r3 (raw file):

    default=os.fspath(SGX_RSA_KEY_PATH),
    help='specify signing key (.pem) file')
@click.option('--passphrase', '--password', '-p', metavar='PASSPHRASE',

These two options are for compatibility with GSC (which uses --passphrase)?

---

python/graminelibos/sgx_sign.py line 584 at r3 (raw file):

def sign_with_local_key(data, key, passphrase=None):

I'm confused. Why do we need this one? How is it different from sgx_with_rsa_key()?

[woju]

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

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Shouldn't there be a corresponding PR in GSC repo, that would:

• Remove https://github.com/gramineproject/gsc/blob/master/sign.sh
• Remove passphrase arg from GSC itself: https://github.com/gramineproject/gsc/blob/a60a4991d7351c1b7489444c694638ca9cc68e39/gsc.py#L504
• Fix around here: https://github.com/gramineproject/gsc/blob/a60a4991d7351c1b7489444c694638ca9cc68e39/gsc.py#L363
• Fix around here: https://github.com/gramineproject/gsc/blob/b7f0576219321c1b7d94b29661274419b009fc49/templates/Dockerfile.common.sign.template#L8

Yes, there should be, and it's part of a longer discussion about signing templates in gsc. Specifically, this is an attempt to make gsc working with signing plugins without regressing on features (here, --passphrase). I've commented in gramineproject/gsc#112 about this.

---

---

python/graminelibos/sgx_sign.py line 585 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

1 -- yes.
2 -- I din't understand the problem. SHA256 has the same length, yes, but how is this relevant?

Anyway, looks good to me.

2. The length of SHA256 hash. Because padding depends on length of the data, so if the length is the same, then padding is the same and hopefully its manipulation takes the same time.

If it's good, then OK.

---

python/graminelibos/sgx_sign.py line 555 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

These two options are for compatibility with GSC (which uses --passphrase)?

Yes.

---

python/graminelibos/sgx_sign.py line 584 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I'm confused. Why do we need this one? How is it different from sgx_with_rsa_key()?

This function accepts filename, sign_with_rsa_key accepts an object from cryptography module (same as generate_private_key return cryptography object vs generate_private_key_pem return PEM data ready to be written to a file). sign_with_local_key was exported in __init__.py so it should remain as is, though we (in gramine repo) don't need it and the name also should be unchanged. Otherwise I'd call it sign_with_rsa_key_pem or so.

Technically it wasn't documented, because autodoc doesn't work with sgx_sign.py (and some others) because of the import of the offsets, so we didn't publish the docs for it. So if you'll argue we can change it, I can do it, but I don't see a need for it. I can also add a "not to be confused with ..." note to docstrings.

---

python/graminelibos/sgx_sign.py line 607 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

private_key

Done.

[woju]

Reviewable status: 0 of 1 files reviewed, 6 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, @kailun-qin, and @mkow)

a discussion (no related file):
WRT CI failure about --passphrase vs --password on 18.04: click 6.7 as shipped in bionic hasn't fixed this bug: pallets/click#793 pallets/click@f15f425.

So I see two options: either I add explicit option name, or we drop bionic now and don't care anymore.

---

[dimakuv]

Reviewed 1 of 1 files at r4, all commit messages.
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 @kailun-qin and @woju)

a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

Yes, there should be, and it's part of a longer discussion about signing templates in gsc. Specifically, this is an attempt to make gsc working with signing plugins without regressing on features (here, --passphrase). I've commented in gramineproject/gsc#112 about this.

I created an issue for this: gramineproject/gsc#148

---

a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

WRT CI failure about --passphrase vs --password on 18.04: click 6.7 as shipped in bionic hasn't fixed this bug: pallets/click#793 pallets/click@f15f425.

So I see two options: either I add explicit option name, or we drop bionic now and don't care anymore.

We dropped bionic, so we don't care anymore.

---

---

python/graminelibos/sgx_sign.py line 584 at r3 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

This function accepts filename, sign_with_rsa_key accepts an object from cryptography module (same as generate_private_key return cryptography object vs generate_private_key_pem return PEM data ready to be written to a file). sign_with_local_key was exported in __init__.py so it should remain as is, though we (in gramine repo) don't need it and the name also should be unchanged. Otherwise I'd call it sign_with_rsa_key_pem or so.

Technically it wasn't documented, because autodoc doesn't work with sgx_sign.py (and some others) because of the import of the offsets, so we didn't publish the docs for it. So if you'll argue we can change it, I can do it, but I don't see a need for it. I can also add a "not to be confused with ..." note to docstrings.

Could you a comment about this? That this name is historic and kept for compatibility, and that it differs from sgx_with_rsa_key() in the key argument type.

---

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

    if exponent != SGX_RSA_PUBLIC_EXPONENT:
        raise InvalidRSAKeyError(
            f'invalid RSA key: expected exponent {SGX_RSA_PUBLIC_EXPONENT}, got {exponent}')

We typically start with a capital letter

---

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

    Function used to generate an RSA signature over provided data using a 3072-bit private key with
    the public exponent of 3 (hard Intel SGX requirement on the key size and the exponent).
    Suitable to be used as a callback to :py:func:`graminelibos.Sigstruct.sign()`.

Isn't it a bit weird that we have two funcs with different arguments that are suitable as a callback for graminelibos.Sigstruct.sign():

• old sign_with_local_key() accepts key which is the path to a private key file
• new sign_with_rsa_key() accepts private_key which is the actual private key object

Or in Python this is considered normal?

[woju]

Reviewable status: 0 of 2 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 @dimakuv and @kailun-qin)

---

python/graminelibos/sgx_sign.py line 584 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you a comment about this? That this name is historic and kept for compatibility, and that it differs from sgx_with_rsa_key() in the key argument type.

Done.

I've also added "see also" for those functions. Here's how that would render, if it would render:

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

We typically start with a capital letter

Done.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Isn't it a bit weird that we have two funcs with different arguments that are suitable as a callback for graminelibos.Sigstruct.sign():

• old sign_with_local_key() accepts key which is the path to a private key file
• new sign_with_rsa_key() accepts private_key which is the actual private key object

Or in Python this is considered normal?

private_key is how cryptography.io calls the object in examples (they name their variable this way). key is from --key CLI argument, so you'd have to rename the variable somewhere in the middle of the call chain. IDK how to do this better. If I could, I would call the latter variable key_path or keypath. Or even mixed pem somewhere in there.

I've added "See also" (cf. other thread), maybe it will make it more clear which function to choose, were you a programmer that would need to choose one or another.

[dimakuv]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kailun-qin)

---

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

Previously, woju (Wojtek Porczyk) wrote…

private_key is how cryptography.io calls the object in examples (they name their variable this way). key is from --key CLI argument, so you'd have to rename the variable somewhere in the middle of the call chain. IDK how to do this better. If I could, I would call the latter variable key_path or keypath. Or even mixed pem somewhere in there.

I've added "See also" (cf. other thread), maybe it will make it more clear which function to choose, were you a programmer that would need to choose one or another.

Thanks, looks good to me now!

[mkow]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @kailun-qin and @woju)

a discussion (no related file):
Depends on #1118.

---

a discussion (no related file):
TODO for myself: re-test this and play with it after #1118 gets merged in its final form.

---

---

python/graminelibos/sgx_sign.py line 585 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

2. The length of SHA256 hash. Because padding depends on length of the data, so if the length is the same, then padding is the same and hopefully its manipulation takes the same time.

If it's good, then OK.

Yup, IMO it's ok exactly because of the two arguments which woju pointed out.

---

python/graminelibos/sgx_sign.py line 573 at r5 (raw file):

    with file:
        private_key = serialization.load_pem_private_key(
            file.read(), password=passphrase, backend=_cryptography_backend)

Isn't this the default? Actually, where is this documented? The examples in the docs doesn't use this argument when calling load_pem_private_key().

Code quote:

, backend=_cryptography_backend

---

python/graminelibos/sgx_sign.py line 575 at r5 (raw file):

            file.read(), password=passphrase, backend=_cryptography_backend)

    exponent = private_key.public_key().public_numbers().e

I think this will throw an exception if the loaded key is not RSA (e.g. DSA).

---

python/graminelibos/sgx_sign.py line 577 at r5 (raw file):

    exponent = private_key.public_key().public_numbers().e

    if exponent != SGX_RSA_PUBLIC_EXPONENT:

What about checking SGX_RSA_KEY_SIZE? SGX doesn't work with other sizes.

---

python/graminelibos/sgx_sign.py line 584 at r5 (raw file):

# NOTE: the name and argument name of this function is kept for compatibility, *key* is path to

I think we can break it, we have almost no users of this API currently. I.e. easier to break it now than in the future when it actually starts to be used widely.
Also, we have sign_with_rsa_key() (as you commented below) that has the same signature, but there key means a key object, not a path. Looks very confusing to me.

Code quote:

NOTE: the name and argument name of this function is kept for compatibility,

---

python/graminelibos/sgx_sign.py line 629 at r5 (raw file):

    See Also:
        :func:`sign_with_local_key`
            This function also signs *data*, but the key argument is path to PEM-encoded file.

All these explanations could go away if we just removed this confusing compatibility.

---

python/graminelibos/sgx_sign.py line 633 at r5 (raw file):

    public_numbers = private_key.public_key().public_numbers()
    assert public_numbers.e == SGX_RSA_PUBLIC_EXPONENT

The documentation for this function mentions the 3072-bit requirement, but there's no assert for it anywhere.

---

python/graminelibos/sgx_sign.py line 634 at r5 (raw file):

    public_numbers = private_key.public_key().public_numbers()
    assert public_numbers.e == SGX_RSA_PUBLIC_EXPONENT
    signature = private_key.sign(data, padding.PKCS1v15(), hashes.SHA256())

PKCS1 is not great, but I assume we are forced to use it here because of the SGX ABI?

[woju]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @kailun-qin and @mkow)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

Depends on #1118.

Yes, this is reflected in base branch.

---

---

python/graminelibos/sgx_sign.py line 573 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Isn't this the default? Actually, where is this documented? The examples in the docs doesn't use this argument when calling load_pem_private_key().

• https://cryptography.io/en/latest/faq/#what-happened-to-the-backend-argument
• https://cryptography.io/en/2.8/hazmat/backends/
• https://cryptography.io/en/2.8/hazmat/primitives/asymmetric/serialization/#cryptography.hazmat.primitives.serialization.load_pem_private_key

This is required to run (to not throw TypeError on function signature) on 20.04, which has python3-cryptography 2.8. Would you like a # TODO after deprecating 20.04: remove backend?

---

python/graminelibos/sgx_sign.py line 575 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I think this will throw an exception if the loaded key is not RSA (e.g. DSA).

Done. Yes, it will. DSA has no e number.

---

python/graminelibos/sgx_sign.py line 577 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

What about checking SGX_RSA_KEY_SIZE? SGX doesn't work with other sizes.

Done.

---

python/graminelibos/sgx_sign.py line 584 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I think we can break it, we have almost no users of this API currently. I.e. easier to break it now than in the future when it actually starts to be used widely.
Also, we have sign_with_rsa_key() (as you commented below) that has the same signature, but there key means a key object, not a path. Looks very confusing to me.

OK, so how we name the functions and arguments?

• private_key for cryptography.io object
• path for path to file
• file for file-like object in python

Now the functions (which ones do we want/need)?

• sign_with_private_key(private_key)
• sign_with_key_from_pem_file(file)
• sign_with_key_from_pem_path(path)

? (or some other names?)

Also, we have two functions for generating keys, and they will remain, because sometimes you just need key object, but sometimes you need it serialised to PEM and it's quite an invocation there so it's very convenient to just put it into another function.

---

python/graminelibos/sgx_sign.py line 633 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

The documentation for this function mentions the 3072-bit requirement, but there's no assert for it anywhere.

Done.

---

python/graminelibos/sgx_sign.py line 634 at r5 (raw file):
Yes, this is hardwired into silicon. SDM, vol. 3D, pt. 4, 38.13 describes SIGNATURE field:

The (3072-bit integer) SIGNATURE should be an RSA signature, where: a) the RSA modulus (MODULUS) is a 3072-
bit integer; b) the public exponent is set to 3; c) the signing procedure uses the EMSA-PKCS1-v1.5 format with DER
encoding of the “DigestInfo” value as specified in of PKCS#1 v2.1/RFC 3447.

Also IACR 2016/086 6.5, and figure 76 for a nice block diagram.

I can put some references in the comments if you like.

[mkow]

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

---

python/graminelibos/sgx_sign.py line 573 at r5 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

• https://cryptography.io/en/latest/faq/#what-happened-to-the-backend-argument
• https://cryptography.io/en/2.8/hazmat/backends/
• https://cryptography.io/en/2.8/hazmat/primitives/asymmetric/serialization/#cryptography.hazmat.primitives.serialization.load_pem_private_key

This is required to run (to not throw TypeError on function signature) on 20.04, which has python3-cryptography 2.8. Would you like a # TODO after deprecating 20.04: remove backend?

Yes, please add.

---

python/graminelibos/sgx_sign.py line 584 at r5 (raw file):
I like the proposed names.

Also, we have two functions for generating keys, and they will remain, because sometimes you just need key object, but sometimes you need it serialised to PEM and it's quite an invocation there so it's very convenient to just put it into another function.

I have no problem with this, looks reasonable.

---

python/graminelibos/sgx_sign.py line 634 at r5 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Yes, this is hardwired into silicon. SDM, vol. 3D, pt. 4, 38.13 describes SIGNATURE field:

The (3072-bit integer) SIGNATURE should be an RSA signature, where: a) the RSA modulus (MODULUS) is a 3072-
bit integer; b) the public exponent is set to 3; c) the signing procedure uses the EMSA-PKCS1-v1.5 format with DER
encoding of the “DigestInfo” value as specified in of PKCS#1 v2.1/RFC 3447.

Also IACR 2016/086 6.5, and figure 76 for a nice block diagram.

I can put some references in the comments if you like.

Yeah, putting some ref into a comment sounds useful.

[woju]

Reviewable status: 0 of 2 files reviewed, 7 unresolved discussions (waiting on @dimakuv, @kailun-qin, and @mkow)

---

python/graminelibos/sgx_sign.py line 573 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Yes, please add.

Done.

---

python/graminelibos/sgx_sign.py line 584 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I like the proposed names.

Also, we have two functions for generating keys, and they will remain, because sometimes you just need key object, but sometimes you need it serialised to PEM and it's quite an invocation there so it's very convenient to just put it into another function.

I have no problem with this, looks reasonable.

OK, I think I got it right this time. It boils down to {load,sign_with}_private_key{,_from_pem_file,_from_pem_path} (not all of them) and generate_private_key{,_pem}. _from_pem_* take optional passphrase argument.

---

python/graminelibos/sgx_sign.py line 629 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

All these explanations could go away if we just removed this confusing compatibility.

Those explanations are needed anyway, if we have three functions that do almost the same, you should be able to navigate the docs from one to another by clicking the link here.

---

python/graminelibos/sgx_sign.py line 634 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Yeah, putting some ref into a comment sounds useful.

Done.

[dimakuv]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: all 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 @kailun-qin, @mkow, and @woju)

---

python/graminelibos/sgx_sign.py line 619 at r7 (raw file):

    # SDM vol. 3D pt. 4 38.13, description of SIGNATURE field:
    #   The (3072-bit integer) SIGNATURE should be an RSA signature, where:
    #   a) the RSA modulus (MODULUS) is a 3072- bit integer;

3072-bit

---

python/graminelibos/sgx_sign.py line 622 at r7 (raw file):

    #   b) the public exponent is set to 3;
    #   c) the signing procedure uses the EMSA-PKCS1-v1.5 format with DER encoding of the
    #      “DigestInfo” value as specified in of PKCS#1 v2.1/RFC 3447.

in of -> in

---

python/graminelibos/sgx_sign.py line 630 at r7 (raw file):

def sign_with_private_key_from_pem_file(data, file, passphrase=None):
    """Signs *data* using key loaded from *path*.

Loaded from path? This is wrong.

[woju]

Reviewable status: 1 of 2 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, @kailun-qin, @mkow, and @woju)

---

python/graminelibos/sgx_sign.py line 619 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

3072-bit

Done. Thanks, this was copy-paste failure.

---

python/graminelibos/sgx_sign.py line 622 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

in of -> in

This is quoted as-is from SDM, only rewrapped. Please file a bug against SDM. :)

---

python/graminelibos/sgx_sign.py line 630 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Loaded from path? This is wrong.

Done. Also fixed Args: below.

[dimakuv]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @kailun-qin, @mkow, and @woju)

[kailun-qin]

Reviewed 1 of 2 files at r7, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @mkow and @woju)

The base branch was changed.

[mkow]

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

---

python/graminelibos/sgx_sign.py line 607 at r9 (raw file):

        :func:`sign_with_private_key_from_pem_file`
            This function also signs *data*, but the key argument is an already
            opened file

Suggestion:

opened file.

---

python/graminelibos/sgx_sign.py line 610 at r9 (raw file):

        :func:`sign_with_private_key_from_pem_path`
            This function also signs *data*, but the key argument is path to a file, not a file-like
            object

Suggestion:

object.

---

python/graminelibos/sgx_sign.py line 673 at r9 (raw file):

        :func:`sign_with_private_key`
            This function also signs *data*, but the key argument is
            :class:`cryptography.hazmat.primitives.asymmetric.rsa.RSAPrivateKey` instance

Suggestion:

:class:`cryptography.hazmat.primitives.asymmetric.rsa.RSAPrivateKey` instance.

---

python/graminelibos/sgx_sign.py line 676 at r9 (raw file):

        :func:`sign_with_private_key_from_pem_file`
            This function also signs *data*, but the key argument is an already
            opened file

Suggestion:

opened file.

[woju]

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

---

python/graminelibos/sgx_sign.py line 607 at r9 (raw file):

        :func:`sign_with_private_key_from_pem_file`
            This function also signs *data*, but the key argument is an already
            opened file

Done.

---

python/graminelibos/sgx_sign.py line 610 at r9 (raw file):

        :func:`sign_with_private_key_from_pem_path`
            This function also signs *data*, but the key argument is path to a file, not a file-like
            object

Done.

---

python/graminelibos/sgx_sign.py line 673 at r9 (raw file):

        :func:`sign_with_private_key`
            This function also signs *data*, but the key argument is
            :class:`cryptography.hazmat.primitives.asymmetric.rsa.RSAPrivateKey` instance

Done.

---

python/graminelibos/sgx_sign.py line 676 at r9 (raw file):

        :func:`sign_with_private_key_from_pem_file`
            This function also signs *data*, but the key argument is an already
            opened file

Done.

[kailun-qin]

Reviewed 12 of 12 files at r10, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mkow)

[dimakuv]

Reviewed 1 of 12 files at r10, all commit messages.
Reviewable status: 2 of 13 files reviewed, 4 unresolved discussions (waiting on @kailun-qin and @mkow)

[mkow]

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

[kailun-qin]

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