[python] Rework graphenelibos python library #72
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 21 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL)
-- commits, line 4 at r1:
blocking until done.
boryspoplawski
force-pushed
the
borys/python
branch
from
b26dbdf
to
578263e
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 21 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)
a discussion (no related file):
TOOD: write documentation (but first we need to settle whether the new APIs are ok)
There was a problem hiding this comment.
Submitting partial review for now, the rest will come tomorrow.
Reviewed 16 of 21 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 17 of 21 files reviewed, 15 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 @boryspoplawski)
a discussion (no related file):
Does this fix RFC #11 fully or only partially ?
Only partially, missing parts: (quoting the RFC)
- Manifests will be renamed: app.toml.jinja (the template) and app.toml (after rendering and trusted files measurement).
graphene-sgx-get-mrenclavenew tool, will calculate MRENCLAVE based on manifest andlibpal.sographene-sgx-keygennew tool, will generate serviceable RSA-3072 key in proper place- The default key location will be in
~/.config/graphene/enclave-key.pem(really in$XDG_CONFIG_HOME).
But the current version should be fine for the upcoming release.
python/graphene-gen-depend, line 2 at r2 (raw file):
*/
Please drop this
python/graphene-gen-depend, line 11 at r2 (raw file):
@click.option('--manifest', '-manifest', 'manifest_path'
How does this work? Do we have 2 different names for this option? ditto for the whole PR
python/graphene-gen-depend, line 16 at r2 (raw file):
encoding='UTF-8'
This is the default in the whole Python3 world, do we want to specify it explicitly? I guess this is to highlight that our manifests are in UTF?
btw. Python docs use lowercase for encoding names ('utf-8'). ditto for the whole PR
python/graphene-manifest, line 2 at r2 (raw file):
#!/usr/bin/env python3 # SPDX-License-Identifier: LGPL-3.0-or-later */
ditto
python/graphene-sgx-get-token, line 2 at r2 (raw file):
#!/usr/bin/env python3 # SPDX-License-Identifier: LGPL-3.0-or-later */
ditto
python/graphene-sgx-sign, line 2 at r2 (raw file):
#!/usr/bin/env python3 # SPDX-License-Identifier: LGPL-3.0-or-later */
ditto
python/graphene-sgx-sign, line 14 at r2 (raw file):
help='Output .manifest.sgx file (manifest augmented with autogenerated fields)') @click.option('--libpal', '-libpal', help='Input libpal file') @click.option('--key', '-key', required=True, help='specify signing key(.pem) file')
missing space before (.pem)
python/meson.build, line 5 at r2 (raw file):
install_data([ 'graphene-manifest', 'graphene-gen-depend',
please sort
python/graphenelibos/init.py, line 16 at r2 (raw file):
# pylint: disable=wrong-import-position from .manifest import Manifest if '@SGX_ENABLED@' == 'True': # please kill me
We should rather drop this comment before merging ;)
python/graphenelibos/sgx_get_token.py, line 2 at r2 (raw file):
#!/usr/bin/env python3 # SPDX-License-Identifier: LGPL-3.0-or-later */
ditto
python/graphenelibos/sgx_sign.py, line 2 at r2 (raw file):
#!/usr/bin/env python3 # SPDX-License-Identifier: LGPL-3.0-or-later */
ditto
python/graphenelibos/sigstruct.py, line 2 at r2 (raw file):
#!/usr/bin/env python3 # SPDX-License-Identifier: LGPL-3.0-or-later */
ditto
There was a problem hiding this comment.
Reviewable status: 10 of 22 files reviewed, 15 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @mkow)
a discussion (no related file):
graphene-sgx-get-mrenclave new tool, will calculate MRENCLAVE based on manifest and libpal.so
graphene-sgx-keygen new tool, will generate serviceable RSA-3072 key in proper place
These 2 I could easily add, but not sure if it's worth it, in this PR. Also I don't see much point in graphene-sgx-keygen
python/graphene-gen-depend, line 2 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
*/Please drop this
Done.
I've grepped whole repo and dropped all occurrences of this.
python/graphene-gen-depend, line 11 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
@click.option('--manifest', '-manifest', 'manifest_path'How does this work? Do we have 2 different names for this option? ditto for the whole PR
A long one and short one. Usually the short one is one letter (e.g. -m), but I wanted to preserve old cli, so left old versions everywhere.
python/graphene-gen-depend, line 16 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
encoding='UTF-8'This is the default in the whole Python3 world, do we want to specify it explicitly? I guess this is to highlight that our manifests are in UTF?
btw. Python docs use lowercase for encoding names ('utf-8'). ditto for the whole PR
I've just used what was already here, introduced in f8c12bac stating Also, scripts now explicitly use UTF-8 when reading/writing the manifest files because they are written in TOML which forces UTF-8.
python/graphene-manifest, line 2 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto
Done.
python/graphene-sgx-get-token, line 2 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto
Done.
python/graphene-sgx-sign, line 2 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto
Done.
python/graphene-sgx-sign, line 14 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
missing space before
(.pem)
Done.
python/meson.build, line 5 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
please sort
Done.
python/graphenelibos/init.py, line 16 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
We should rather drop this comment before merging ;)
Done.
python/graphenelibos/sgx_get_token.py, line 2 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto
Done.
python/graphenelibos/sgx_sign.py, line 2 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto
Done.
python/graphenelibos/sigstruct.py, line 2 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto
Done.
There was a problem hiding this comment.
Reviewed 1 of 21 files at r1, 11 of 11 files at r3, 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 (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @mkow)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
graphene-sgx-get-mrenclave new tool, will calculate MRENCLAVE based on manifest and libpal.so
graphene-sgx-keygen new tool, will generate serviceable RSA-3072 key in proper placeThese 2 I could easily add, but not sure if it's worth it, in this PR. Also I don't see much point in
graphene-sgx-keygen
We can do this in another PR, but we should then create a TODOlist in #11. No strong opinion about graphene-sgx-keygen from my side, I guess we need to discuss it.
Ah, and also use cryptography (https://cryptography.io/) instead of subprocess.run('openssl') and hand-crafted RSA signature is not done.
python/graphene-gen-depend, line 11 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
A long one and short one. Usually the short one is one letter (e.g.
-m), but I wanted to preserve old cli, so left old versions everywhere.
Ok, but we should deprecate this version at some point...
python/graphene-gen-depend, line 16 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I've just used what was already here, introduced in f8c12bac stating
Also, scripts now explicitly use UTF-8 when reading/writing the manifest files because they are written in TOML which forces UTF-8.
I guess it was just missed there. Could you at least make it lowercase? The uppercased version looks odd to me.
python/graphenelibos/manifest.py, line 111 at r3 (raw file):
def hash_path(path): # FIXME: is this name ok? Doesn't it sound like the path itself is hashed?
It's bad IMO, why not just hash_file?
python/graphenelibos/manifest.py, line 141 at r3 (raw file):
if path.is_dir(): if not uri.endswith('/'): # XXX: do we want to check this?
I think so. I'd just drop this "XXX" and leave the code as is.
python/graphenelibos/manifest.py, line 143 at r3 (raw file):
# XXX: do we want to check this? raise ManifestError(f'Directory URI ({uri}) does not end with "/"') for sub_path in filter(pathlib.Path.is_file, path.rglob('*')):
You dropped sorting from here, why? It made the manifest generation reproducible.
python/graphenelibos/manifest.py, line 174 at r3 (raw file):
loader.setdefault('preload', '') # Current toml versions (< 1.0) do not support non homogeneous arrays
non homogeneous -> non-homogeneous (or nonhomogeneous)
python/graphenelibos/manifest.py, line 225 at r3 (raw file):
if tf['uri'] in preloads: preloads_seen = True return preloads_seen or not preloads
What if there are no preloads, but TFs were not expanded yet? Won't this return True?
python/graphenelibos/manifest.py, line 244 at r3 (raw file):
if self.has_tfs_expanded() and self['sgx']['trusted_files']: raise ManifestError('Trusted files are already expanded in this manifest cannot decide ' 'which files are local')
This sentence is either missing punctuation or some "so"/"because"/"then".
python/graphenelibos/sigstruct.py, line 64 at r3 (raw file):
raise KeyError(f'unknown field name {key}') except struct.error: raise ValueError(f'{val} does not match requred format {self.fields[key][1]}')
requred -> required
There was a problem hiding this comment.
Reviewed 2 of 11 files at r3.
Reviewable status: all files reviewed, 24 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @mkow)
a discussion (no related file):
These 2 I could easily add, but not sure if it's worth it, in this PR.
It's not.
Also I don't see much point in graphene-sgx-keygen
This is nice to have, for easy UX. Nothing you couldn't do with openssl genrsa or whatever you please. The main point is to create the key in right path and with correct parameters.
python/graphene-gen-depend, line 1 at r3 (raw file):
#!/usr/bin/env python3
Why is this another command, instead of an optional, side output of graphene-manifest? Because it's not needed before first make run, the use case is that on subsequent runs the output is regenerated only when needed. Is it "because that's how it was before"?
python/graphene-gen-depend, line 11 at r3 (raw file):
@click.command() @click.option('--manifest', '-manifest', 'manifest_path', required=True, help='Input .manifest file')
@click.option(..., type=click.File('r', encoding='utf-8')), and in main the object will be an already open'd file which you don't have to close yourself. Filename is .name attribute.
https://click.palletsprojects.com/en/8.0.x/api/#click.File
python/graphene-gen-depend, line 12 at r3 (raw file):
@click.command() @click.option('--manifest', '-manifest', 'manifest_path', required=True, help='Input .manifest file') @click.option('--libpal', '-libpal', default=os.path.join(_CONFIG_PKGLIBDIR, 'sgx/libpal.so'),
@click.option(..., type=click.Path(exists=True, dir_okay=False))
https://click.palletsprojects.com/en/8.0.x/api/#click.Path
python/graphene-gen-depend, line 14 at r3 (raw file):
@click.option('--libpal', '-libpal', default=os.path.join(_CONFIG_PKGLIBDIR, 'sgx/libpal.so'), help='Input libpal file', show_default=True) @click.option('--output', '-output', required=True, help='Output .manifest.d file')
@click.option(..., type=click.File('w')), and I'm not sure about encoding, probably should be left like the rest of the system, because the file contains path names.
python/graphene-sgx-sign, line 11 at r3 (raw file):
@click.command() @click.option('--output', '-output', required=True,
click.File
python/graphene-sgx-sign, line 13 at r3 (raw file):
@click.option('--output', '-output', required=True, help='Output .manifest.sgx file (manifest augmented with autogenerated fields)') @click.option('--libpal', '-libpal', help='Input libpal file')
click.Path, and this should have a default.
python/graphene-sgx-sign, line 14 at r3 (raw file):
help='Output .manifest.sgx file (manifest augmented with autogenerated fields)') @click.option('--libpal', '-libpal', help='Input libpal file') @click.option('--key', '-key', required=True, help='specify signing key (.pem) file')
click.Path I guess
python/graphene-sgx-sign, line 15 at r3 (raw file):
@click.option('--libpal', '-libpal', help='Input libpal file') @click.option('--key', '-key', required=True, help='specify signing key (.pem) file') @click.option('--manifest', '-manifest', 'manifest_path', required=True,
click.File
python/graphene-sgx-sign, line 17 at r3 (raw file):
@click.option('--manifest', '-manifest', 'manifest_path', required=True, help='Input .manifest file') @click.option('--sigfile', '-sigfile', help='Output .sig file')
click.File('wb') probably
python/graphenelibos/init.py, line 16 at r3 (raw file):
# pylint: disable=wrong-import-position from .manifest import Manifest if '@SGX_ENABLED@' == '1':
Why not just
try:
from .sgx_whatever import something
#...
except ImportError:
pass?
python/graphenelibos/manifest.py, line 111 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
It's bad IMO, why not just
hash_file?
+1
python/graphenelibos/manifest.py, line 141 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I think so. I'd just drop this "XXX" and leave the code as is.
+1
python/graphenelibos/manifest.py, line 150 at r3 (raw file):
class Manifest: _env = make_env()
Jinja env should not belong to this class, it should either be global or created in main().
python/graphenelibos/manifest.py, line 152 at r3 (raw file):
_env = make_env() def __init__(self, manifest_str):
Something is wrong about instantiating manifest from string/bytes vs get_mrenclave. In the current version there's no way to get mrenclave from a manifest given by file without load+dump again, but it'll be needed when you are on the other end of RA-TLS and would like to verify mrenclave against known manifest. In that path you should just calculate from bytes, as read from file, because loading and dumping might change order, drop comments and/or whitespace etc.
I don't know where this should be fixed, the easiest way probably would be to accept bytes as arguments to get_mrenclave. The Pythonic Way™ would be to make Manifest instances immutable and have it hold two parallel data structures, bytes and dict, so if you need the other, you silently generate it.
python/graphenelibos/manifest.py, line 228 at r3 (raw file):
def expand_all_trusted_files(self): assert not self.has_tfs_expanded()
I don't like this logic. Shouldn't this just expand the files that remain to be expanded, and leave alone those which are already expanded? This way it would be idempotent and calling this twice shouldn't be a problem.
Same for dependencies I guess, aren't the dependencies "whatever is to be expanded yet" (possibly nothing)?
There was a problem hiding this comment.
Reviewable status: 17 of 22 files reviewed, 24 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @woju)
a discussion (no related file):
Previously, woju (Wojtek Porczyk) wrote…
These 2 I could easily add, but not sure if it's worth it, in this PR.
It's not.
Also I don't see much point in graphene-sgx-keygen
This is nice to have, for easy UX. Nothing you couldn't do with
openssl genrsaor whatever you please. The main point is to create the key in right path and with correct parameters.
All right, let's add the TODO list after the review.
python/graphene-gen-depend, line 16 at r2 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I guess it was just missed there. Could you at least make it lowercase? The uppercased version looks odd to me.
I've left the explicit encoding - after all we now expose and API, so if somebody uses our library and changed the encoding in their script, things would break.
python/graphene-gen-depend, line 1 at r3 (raw file):
Is it "because that's how it was before"?
Yes. Also, normal manifests don't need it, only the sgx ones (because hashes change), yet it can only be generated based on a normal manifest (i.e. before TFs are expanded). It got a bit messy.
python/graphene-gen-depend, line 11 at r3 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
@click.option(..., type=click.File('r', encoding='utf-8')), and in main the object will be an already open'd file which you don't have to close yourself. Filename is.nameattribute.
I didn't use File because I needed it's path and didn't know .name exists. Where can I find such info? click documentation is silent about it...
python/graphene-gen-depend, line 12 at r3 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
@click.option(..., type=click.Path(exists=True, dir_okay=False))
Done.
python/graphene-gen-depend, line 14 at r3 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
@click.option(..., type=click.File('w')), and I'm not sure about encoding, probably should be left like the rest of the system, because the file contains path names.
Done. I've added explicit encoding - these path names must be 'utf-8' encodable, because they end up in our manifests (and TOML requires UTF-8).
python/graphene-sgx-sign, line 11 at r3 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
click.File
Done.
python/graphene-sgx-sign, line 13 at r3 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
click.Path, and this should have a default.
Done. default is useless here, as the API call where it's used get_mrenclave must handle missing libpal anyway.
python/graphene-sgx-sign, line 14 at r3 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
click.PathI guess
Yeah. We should be able to change to File when we migrate to cryptography.
python/graphene-sgx-sign, line 15 at r3 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
click.File
Done.
python/graphene-sgx-sign, line 17 at r3 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
click.File('wb')probably
We have to open the file manually anyway (sometimes), so I left this as is. Maybe this could be done with some mixture of callbacks, proper laziness and extracting other arguments (manifest_file) from context, but I'm not familiar enough with click and it would probably be ugly.
python/graphenelibos/init.py, line 16 at r3 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Why not just
try: from .sgx_whatever import something #... except ImportError: pass?
This proved very useful while debugging. I can change it as the last thing, but I don't like it: it would silence all ImportErrors from these submodules.
python/graphenelibos/manifest.py, line 111 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
It's bad IMO, why not just
hash_file?
Done.
python/graphenelibos/manifest.py, line 141 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I think so. I'd just drop this "XXX" and leave the code as is.
Done.
python/graphenelibos/manifest.py, line 143 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
You dropped sorting from here, why? It made the manifest generation reproducible.
Hm, so that was the reason behind that sort? Is rglob(*) not reproducible if run on the same fs layout?
python/graphenelibos/manifest.py, line 150 at r3 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Jinja env should not belong to this class, it should either be global or created in
main().
Why? It's tightly couple with this class (used and required) and we have only one instance of it shared by all instanced of Manifest class. Also if you ever want to create some specialized Manifest subclass, you can adjust it only for that subclass. This lead me to putting it here.
python/graphenelibos/manifest.py, line 152 at r3 (raw file):
Keep in mind that there are some functions/APIs that I did not change (e.g. actual mrenclave calculations).
Something is wrong about instantiating manifest from string/bytes vs get_mrenclave. In the current version there's no way to get mrenclave from a manifest given by file without load+dump again, but it'll be needed when you are on the other end of RA-TLS and would like to verify mrenclave against known manifest.
This is not true, other end in case of RATLS can, and actually have to, parse the manifest, so load() is inevitable. dump() might not be needed - discussed below.
In that path you should just calculate from bytes, as read from file, because loading and dumping might change order, drop comments and/or whitespace etc.
That's true, see below.
I don't know where this should be fixed, the easiest way probably would be to accept bytes as arguments to get_mrenclave. The Pythonic Way™ would be to make Manifest instances immutable and have it hold two parallel data structures, bytes and dict, so if you need the other, you silently generate it.
We cannot make all Manifest instances immutable, because this class is used everywhere.
What I can do is create a subclass SignedManifest (or ConstManifest or some other name, this manifest actually doesn't contain signature), which would be immutable and contain both parsed and raw data. Unfortunately, we store a dict in Manifest, which itself could be immutabel in ConstManifest but any sub-dictionary would still be mutable (in theory), but I guess that's the best we can do in python.
I don't have a better idea.
python/graphenelibos/manifest.py, line 174 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
non homogeneous->non-homogeneous(ornonhomogeneous)
Done.
python/graphenelibos/manifest.py, line 225 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
What if there are no preloads, but TFs were not expanded yet? Won't this return
True?
No. It would return True if there are no preloads and no TFs. The loop above checks whether a TF is expanded already and directly returns False otherwise.
python/graphenelibos/manifest.py, line 228 at r3 (raw file):
Shouldn't this just expand the files that remain to be expanded, and leave alone those which are already expanded?
This is exactly what this function does.
This way it would be idempotent and calling this twice shouldn't be a problem.
Not true. It expands loader.preload entries, so calling it twice would double them, which is an error.
I guess we could parse loader.preload and then expand only these parts of it that are not already expanded.
In any case expanding TFs twice is an error or at least useless thing to do, so this check sounds useful.
python/graphenelibos/manifest.py, line 244 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This sentence is either missing punctuation or some "so"/"because"/"then".
Done.
python/graphenelibos/sigstruct.py, line 64 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
requred->required
Done.
There was a problem hiding this comment.
Reviewed 4 of 5 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @woju)
python/graphenelibos/init.py, line 16 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
This proved very useful while debugging. I can change it as the last thing, but I don't like it: it would silence all
ImportErrors from these submodules.
+1, except ImportError: sounds like a footgun, as it catches also import errors from deeper levels
python/graphenelibos/manifest.py, line 143 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Hm, so that was the reason behind that sort? Is
rglob(*)not reproducible if run on the same fs layout?
I think rglob() doesn't have any guarantees about the order, I guess it just calls readdir() recursively and filters the results.
python/graphenelibos/manifest.py, line 225 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
No. It would return
Trueif there are no preloads and no TFs. The loop above checks whether a TF is expanded already and directly returnsFalseotherwise.
Ah, I see.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @mkow)
Pal/regression/Bootstrap3.manifest, line 6 at r3 (raw file):
loader.argv0_override = "Bootstrap3" sgx.trusted_files = [ "file:Bootstrap3" ]
No spaces inside parenthesis.
python/graphene-gen-depend, line 1 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Is it "because that's how it was before"?
Yes. Also, normal manifests don't need it, only the sgx ones (because hashes change), yet it can only be generated based on a normal manifest (i.e. before TFs are expanded). It got a bit messy.
Sure, I guess we can sort it out later.
python/graphene-gen-depend, line 11 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I didn't use
Filebecause I needed it's path and didn't know.nameexists. Where can I find such info? click documentation is silent about it...
Because this is a general python feature, documented in https://docs.python.org/3/library/io.html#io.FileIO.name. Every open()ed file has such attribute and I guess click devs assumed you should have known it already.
python/graphene-gen-depend, line 12 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Done.
Wait, why there's sgx/ hardcoded there?
python/graphene-gen-depend, line 14 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Done. I've added explicit encoding - these path names must be 'utf-8' encodable, because they end up in our manifests (and TOML requires UTF-8).
I guess it's OK. Technically you can have invalid UTF-8 strings in TOML, they're encoded with surrogates, but I'm not sure what would then break.
python/graphene-sgx-sign, line 17 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
We have to open the file manually anyway (sometimes), so I left this as is. Maybe this could be done with some mixture of callbacks, proper laziness and extracting other arguments (
manifest_file) from context, but I'm not familiar enough with click and it would probably be ugly.
OK, let's think about this another time.
python/graphenelibos/init.py, line 16 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
+1,
except ImportError:sounds like a footgun, as it catches also import errors from deeper levels
The problem is, everyone using this modules has to do their own try/except, because just importing graphenelibos means you can have those attributes or not, so we're not solving anything with this contraption.
So maybe we shouldn't expose those APIs as part of this module, but have something like graphenelibos.sgx instead?
python/graphenelibos/manifest.py, line 111 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Done.
I like this.
python/graphenelibos/manifest.py, line 150 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Why? It's tightly couple with this class (used and required) and we have only one instance of it shared by all instanced of
Manifestclass. Also if you ever want to create some specializedManifestsubclass, you can adjust it only for that subclass. This lead me to putting it here.
It starts to matter when you have loaders for templates and/or some configurable globals (like "debug" maybe?). Those are typically set somewhere around main.
python/graphenelibos/manifest.py, line 152 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Keep in mind that there are some functions/APIs that I did not change (e.g. actual mrenclave calculations).
Something is wrong about instantiating manifest from string/bytes vs get_mrenclave. In the current version there's no way to get mrenclave from a manifest given by file without load+dump again, but it'll be needed when you are on the other end of RA-TLS and would like to verify mrenclave against known manifest.
This is not true, other end in case of RATLS can, and actually have to, parse the manifest, so
load()is inevitable.dump()might not be needed - discussed below.In that path you should just calculate from bytes, as read from file, because loading and dumping might change order, drop comments and/or whitespace etc.
That's true, see below.
I don't know where this should be fixed, the easiest way probably would be to accept bytes as arguments to get_mrenclave. The Pythonic Way™ would be to make Manifest instances immutable and have it hold two parallel data structures, bytes and dict, so if you need the other, you silently generate it.
We cannot make all
Manifestinstances immutable, because this class is used everywhere.What I can do is create a subclass
SignedManifest(orConstManifestor some other name, this manifest actually doesn't contain signature), which would be immutable and contain both parsed and raw data. Unfortunately, we store a dict inManifest, which itself could be immutabel inConstManifestbut any sub-dictionary would still be mutable (in theory), but I guess that's the best we can do in python.I don't have a better idea.
"Immutable" doesn't need to be truly immutable, it'd be enough to have the actual mutable thingy _-private and have all exposed methods which "change" something return new instances. Also there are primitives for read-only dicts like https://docs.python.org/3/library/types.html#types.MappingProxyType (this is actually a view, you need to have underlying dict) and maybe TOML parser could be persuaded to return such objects. Or Manifest.from_dict could change dicts recursively to mapping proxies.
But if this is too much work, then maybe just get_mrenclave should accept bytes (or rb-opened file), and parse them to a Manifest itself?
python/graphenelibos/manifest.py, line 228 at r3 (raw file):
I guess we could parse loader.preload and then expand only these parts of it that are not already expanded.
What's the difference between a file that wasn't expanded and preloads? Isn't it just that it's on a different list before being measured?
Also, IIUC this means that it's an error to write a manifest with preload already present in trusted files. That's probably not a useful thing to do, but not that harmful either (if you didn't make a mistake while copy-pasting the hash).
There was a problem hiding this comment.
Reviewable status: 20 of 24 files reviewed, 10 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @woju)
-- commits, line 1 at r6:
TODO (at rebase): reorder commits, so FIX is the first.
Pal/regression/Bootstrap3.manifest, line 6 at r3 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
No spaces inside parenthesis.
Why? This looks more readable.
python/graphene-gen-depend, line 11 at r3 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Because this is a general python feature, documented in https://docs.python.org/3/library/io.html#io.FileIO.name. Every
open()ed file has such attribute and I guess click devs assumed you should have known it already.
Well, how could I knew they are not using any wrappers?
python/graphene-gen-depend, line 12 at r3 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Wait, why there's
sgx/hardcoded there?
What is hardcoded where?
python/graphenelibos/init.py, line 16 at r3 (raw file):
The problem is, everyone using this modules has to do their own try/except, because just importing graphenelibos means you can have those attributes or not, so we're not solving anything with this contraption.
What? This is substituted by meson, just like some variables above. So if you've installed SGX version, they you have these files and if you didn't, they they are not imported. No need for any try+except.
python/graphenelibos/manifest.py, line 143 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I think
rglob()doesn't have any guarantees about the order, I guess it just callsreaddir()recursively and filters the results.
Done.
We also base on the fact that dict is ordered, which is only true since python 3.7 (in practice 3.6).
Also we base on the fact that the python TOML implemenation we use is ordered, which I'm not sure is true.
Given the above I'm not sure we want this change and I'm thinking about reverting it (i.e. there seem to be no point in it).
python/graphenelibos/manifest.py, line 150 at r3 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
It starts to matter when you have loaders for templates and/or some configurable globals (like "debug" maybe?). Those are typically set somewhere around main.
If I put _env in global scope it wont change anything w.r.t. arguments you gave.
python/graphenelibos/manifest.py, line 152 at r3 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
"Immutable" doesn't need to be truly immutable, it'd be enough to have the actual mutable thingy
_-private and have all exposed methods which "change" something return new instances. Also there are primitives for read-only dicts like https://docs.python.org/3/library/types.html#types.MappingProxyType (this is actually a view, you need to have underlying dict) and maybe TOML parser could be persuaded to return such objects. OrManifest.from_dictcould change dicts recursively to mapping proxies.But if this is too much work, then maybe just
get_mrenclaveshould accept bytes (orrb-opened file), and parse them to a Manifest itself?
Postponing this for now, need to think about a good solution (e.g. issue: Manifest exposes a __setitem__, but we can also change it indirectly, e.g. manifest['sgx']['something'] = 'asdf').
python/graphenelibos/manifest.py, line 228 at r3 (raw file):
What's the difference between a file that wasn't expanded and preloads? Isn't it just that it's on a different list before being measured?
Preloads are expanded implicitly and are not on TFs list by default.
Also, IIUC this means that it's an error to write a manifest with preload already present in trusted files.
Yes it is an error to do so, just like specifying same file twice.
That's probably not a useful thing to do, but not that harmful either (if you didn't make a mistake while copy-pasting the hash).
That's true.
I've changed it so that you can manually expand preloads
Update: nvm, I've decided to completely remove has_tfs_expanded as currently it was only used in deps generation, which we probably will remove in the future.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @woju)
python/graphenelibos/manifest.py, line 143 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Done.
We also base on the fact that dict is ordered, which is only true since python 3.7 (in practice 3.6).
Also we base on the fact that the python TOML implemenation we use is ordered, which I'm not sure is true.
Given the above I'm not sure we want this change and I'm thinking about reverting it (i.e. there seem to be no point in it).
I'd leave it. That we aren't reproducible yet doesn't mean that we shouldn't make the code reproducible. It will make doing it later easier.
There was a problem hiding this comment.
Reviewed 10 of 21 files at r1, 7 of 11 files at r3, 3 of 5 files at r4, 1 of 1 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @woju)
-- commits, line 32 at r7:
build -> built (2x)
linked each version is linked... -- broken English
-- commits, line 47 at r7:
Why do you write for Trusted Files? These should be Protected Files.
Also, I think we should also allow the opposite case: translate O_RDONLY to O_RDWR because it may also require changes in the PF metadata (see e.g. the PF rename PR: #31)
LibOS/shim/test/fs/Makefile, line 52 at r7 (raw file):
common_copy_mmap.o: CFLAGS += -DCOPY_MMAP common_copy_mmap.o: common_copy.c $(call cmd,cc_o_c)
How did this bug manifest itself? Why didn't we notice it previously?
Pal/regression/Makefile, line 80 at r7 (raw file):
graphene = direct endif pal_lib ?= $(shell graphene-manifest -c 'EXTRACT = "{{ graphene.pkglibdir }}"' | grep -F EXTRACT | sed -e "s/EXTRACT = \"\(.*\)\"/\1/")/$(graphene)/libpal.so
What does it do? Why is it needed?
Pal/src/host/Linux-SGX/db_files.c, line 137 at r7 (raw file):
* (e.g. some metadata got invalidated). */ flags = (flags & ~O_ACCMODE) | O_RDWR; }
I would change flags unconditionally and add a comment about this, that Protected Files sometimes need to be read/written even if they are opened with another access mode.
#31 needs this as well. That PR goes to great lengths to conditionally change flags as well (only for the rename flow). With the unconditional assignment, a lot of complexity goes away.
python/graphene-sgx-get-token, line 16 at r7 (raw file):
token = get_token(sig) if output: output.write(token)
Why is output not required? What is the usage of graphene-sgx-get-token --sig myfile?
python/graphenelibos/manifest.py, line 140 at r7 (raw file):
if path.is_dir(): if not uri.endswith('/'): raise ManifestError(f'Directory URI ({uri}) does not end with "/"')
Why do you have this artificial requirement? Will it not break existing manifest templates?
python/graphenelibos/sgx_sign.py, line 61 at r7 (raw file):
for opt, bit in options_dict.items(): if manifest_sgx[opt] == 1: val |= bit
Some items in options_dict are not single-bit. A good example is offs.SGX_XFRM_AVX512 which has three bits set.
So at least change bit -> bits. I don't see that there can be any problems with the current code, but take this multiple-bit case into consideration.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 17 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @woju)
LibOS/shim/test/fs/Makefile, line 52 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
How did this bug manifest itself? Why didn't we notice it previously?
Make chose the version randomly and everything ended up compiled with RDWR, so it just worked.
Pal/regression/Makefile, line 80 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What does it do? Why is it needed?
Because the old -c was weird, it only rendered the Jinja template, but didn't treat this as TOML. And the old argument was an invalid TOML, so it exploded on Borys' rework.
python/graphenelibos/manifest.py, line 140 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you have this artificial requirement? Will it not break existing manifest templates?
I like it, I think this is a very reasonable requirement for manifest readability.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @woju)
python/graphenelibos/manifest.py, line 140 at r7 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I like it, I think this is a very reasonable requirement for manifest readability.
Hm, ok.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @woju)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
TOOD: write documentation (but first we need to settle whether the new APIs are ok)
This feature is expected to interact with signing facility for 2 step signing that has a clear boundary defined in Intel dev. reference document. the difference should be clarified and elaborated first for us to evaluate the changes/impact on existing signing facility, thanks.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @woju)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you write
for Trusted Files? These should beProtected Files.Also, I think we should also allow the opposite case: translate
O_RDONLYtoO_RDWRbecause it may also require changes in the PF metadata (see e.g. thePF renamePR: #31)
This workaround may need to be further explained in documentation w.r.t security and also produce warning messages when it got triggered, thanks.
Pal/regression/Bootstrap3.manifest, line 6 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Why? This looks more readable.
Looks more readability for me.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @woju)
Previously, bigdata-memory (Gordon King) wrote…
This workaround may need to be further explained in documentation w.r.t security and also produce warning messages when it got triggered, thanks.
This has nothing to do with security.
There was a problem hiding this comment.
Reviewable status: 20 of 25 files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @woju)
a discussion (no related file):
This feature is expected to interact with signing facility for 2 step signing that has a clear boundary defined in Intel dev. reference document.
I do not understand this.
Generally, now we expose a python API, which can use any signing callback whatsoever.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
build->built(2x)
linked each version is linked...-- broken English
TODO: at rebase
Previously, mkow (Michał Kowalczyk) wrote…
This has nothing to do with security.
@dimakuv typo, need to fix at rebase. As for other change: sure, but let's not do this here (or submit another PR which we can merge quickly, then I would drop this commit)
@bigdata-memory no, it does not need to
LibOS/shim/test/fs/Makefile, line 52 at r7 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Makechose the version randomly and everything ended up compiled with RDWR, so it just worked.
Yeah, this PR added another dependency on the final binary, which changed the order of building. Previously everything was RW.
Pal/regression/Makefile, line 80 at r7 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Because the old
-cwas weird, it only rendered the Jinja template, but didn't treat this as TOML. And the old argument was an invalid TOML, so it exploded on Borys' rework.
Exactly
python/graphene-sgx-get-token, line 16 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why is
outputnot required? What is the usage ofgraphene-sgx-get-token --sig myfile?
I've just preserver old CLI syntax 1 to 1. The answer is: no, even slightest, idea.
python/graphenelibos/manifest.py, line 152 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Postponing this for now, need to think about a good solution (e.g. issue:
Manifestexposes a__setitem__, but we can also change it indirectly, e.g.manifest['sgx']['something'] = 'asdf').
get_mrenclave now takes a path, I've figured this is the best way to stop ppl shooting their feet, and integrates ok-ish with other functions. It now also returns a manifest, in case you need that too (since it's already parsed).
python/graphenelibos/manifest.py, line 140 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Hm, ok.
I thought we had that requirement already. Anyway, all current manifests are written like that.
There was a problem hiding this comment.
Reviewable status: 18 of 25 files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @woju)
Pal/src/host/Linux-SGX/db_files.c, line 137 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would change
flagsunconditionally and add a comment about this, that Protected Files sometimes need to be read/written even if they are opened with another access mode.#31 needs this as well. That PR goes to great lengths to conditionally change
flagsas well (only for the rename flow). With the unconditional assignment, a lot of complexity goes away.
Let's stick the discussion to one place (in commit messages).
python/graphenelibos/sgx_sign.py, line 61 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Some items in
options_dictare not single-bit. A good example isoffs.SGX_XFRM_AVX512which has three bits set.So at least change
bit->bits. I don't see that there can be any problems with the current code, but take this multiple-bit case into consideration.
Done.
There was a problem hiding this comment.
Reviewed 2 of 3 files at r8, 2 of 3 files at r9, 5 of 5 files at r10, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @bigdata-memory, @boryspoplawski, @dimakuv, @mkow, and @woju)
python/graphene-sgx-get-token, line 16 at r7 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I've just preserver old CLI syntax 1 to 1. The answer is: no, even slightest, idea.
I'd stop supporting it, I can't imagine someone calling this tool without this argument.
There was a problem hiding this comment.
Reviewed 1 of 3 files at r8, 5 of 5 files at r10, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @bigdata-memory, @boryspoplawski, @dimakuv, @mkow, and @woju)
Previously, boryspoplawski (Borys Popławski) wrote…
@dimakuv typo, need to fix at rebase. As for other change: sure, but let's not do this here (or submit another PR which we can merge quickly, then I would drop this commit)
@bigdata-memory no, it does not need to
@boryspoplawski I would create a separate PR, looks like we need it. It will also simplify significantly #31
python/graphene-gen-depend, line 8 at r10 (raw file):
import os import click
Python linter complained? Why this reordering?
python/graphene-sgx-get-token, line 16 at r7 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I'd stop supporting it, I can't imagine someone calling this tool without this argument.
Let's make it required here and see if Jenkins fails
python/graphenelibos/manifest.py, line 140 at r7 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I thought we had that requirement already. Anyway, all current manifests are written like that.
OK. Though it's surprising -- are we really that disciplined in our manifests? Cool.
python/graphenelibos/sgx_sign.py, line 447 at r10 (raw file):
def get_mrenclave(manifest_path, date, libpal=None):
The name doesn't make much sense now. get_mrenclave returns not only the MRENCLAVE, but also the manifest dict? Please rename.
Also, why date here? You only need the date for the SIGSTRUCT generation (you put it there as one of the fields). But for manifest/MRENCLAVE generation it is not needed at all. I also think that we can simply remove the date print from the below, it is useless.
python/graphenelibos/sgx_sign.py, line 475 at r10 (raw file):
print(f' attr.xfrm: {attr["xfrms"]:#x}') print(f' misc_select: {attr["misc_select"]:#x}') print(f' date: {attr["year"]:04d}-{attr["month"]:02d}-{attr["day"]:02d}')
We can delete this print, it's useless.
python/graphenelibos/sgx_sign.py, line 514 at r10 (raw file):
def get_tbssigstruct(manifest_path, date, args=None):
This looks weird -- we either specify manifest_path OR we specify a tuple args = (mrenclave, manifest). Can we somehow change the signature of this func?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 16 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @bigdata-memory, @boryspoplawski, @dimakuv, @mkow, and @woju)
python/graphene-gen-depend, line 8 at r10 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Python linter complained? Why this reordering?
Yup. This looks a bit weird with only 3 imports, but this is just import grouping - first group is for standard imports, second are third-party and third is our own.
python/graphenelibos/sgx_sign.py, line 447 at r10 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The name doesn't make much sense now.
get_mrenclavereturns not only the MRENCLAVE, but also the manifest dict? Please rename.Also, why
datehere? You only need the date for the SIGSTRUCT generation (you put it there as one of the fields). But for manifest/MRENCLAVE generation it is not needed at all. I also think that we can simply remove the date print from the below, it is useless.
You need to pass the original signing date if you want to reproduce the MRENCLAVE (note, that this is from the enclave user perspective, not enclave author or host).
Or maybe we're missing something and this date doesn't go into MRENCLAVE at all?
python/graphenelibos/sgx_sign.py, line 475 at r10 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We can delete this print, it's useless.
The date is required to be able to reproduce MRENCLAVE, see above.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 15 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @bigdata-memory, @boryspoplawski, @dimakuv, @mkow, and @woju)
python/graphenelibos/sgx_sign.py, line 447 at r10 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
You need to pass the original signing date if you want to reproduce the MRENCLAVE (note, that this is from the enclave user perspective, not enclave author or host).
Or maybe we're missing something and this date doesn't go into MRENCLAVE at all?
This cannot be true. MRENCLAVE shouldn't depend on the date at all (why would it?), and I looked through our sgx_sign.py script, nowhere it uses the date (other than for the final SIGSTRUCT generation).
There was a problem hiding this comment.
Reviewable status: all files reviewed, 15 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @woju)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
This feature is expected to interact with signing facility for 2 step signing that has a clear boundary defined in Intel dev. reference document.
I do not understand this.
Generally, now we expose a python API, which can use any signing callback whatsoever.
You can check out the description in SGX Developer Reference(https://download.01.org/intel-sgx/sgx-linux/2.11/docs/Intel_SGX_Developer_Reference_Linux_2.11_Open_Source.pdf) P18, you can also find the associated screenshots in #2617
There was a problem hiding this comment.
Reviewable status: 22 of 25 files reviewed, 15 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @woju)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@boryspoplawski I would create a separate PR, looks like we need it. It will also simplify significantly #31
Done in #78
This needs to be dropped at rebase.
python/graphene-gen-depend, line 8 at r10 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Yup. This looks a bit weird with only 3 imports, but this is just import grouping - first group is for standard imports, second are third-party and third is our own.
pylint complained on the order of imports (standard before third-party), empty lines added just for clarity.
python/graphene-sgx-get-token, line 16 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Let's make it required here and see if Jenkins fails
All usages have it (grepped manually).
Done.
python/graphenelibos/sgx_sign.py, line 447 at r10 (raw file):
Yeah, it seems to just be a remnant of an older version, which had this and sigstruct generation in one place.
Fixed.
The name doesn't make much sense now. get_mrenclave returns not only the MRENCLAVE, but also the manifest dict? Please rename.
Yes it does, but only for convenience (because we have to parse it inside anyway). I left the old name, because I expect most users will ignore the returned manifest and the main point of this function is to get MRENCLAVE.
If you insist on this change, please give me some hint on the new name.
python/graphenelibos/sgx_sign.py, line 475 at r10 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
The date is required to be able to reproduce MRENCLAVE, see above.
Nope it is not.
python/graphenelibos/sgx_sign.py, line 514 at r10 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This looks weird -- we either specify
manifest_pathOR we specify a tupleargs = (mrenclave, manifest). Can we somehow change the signature of this func?
How exactly would you do that? Having one argument and check if with isinstance would be ugly.
There was a problem hiding this comment.
Reviewable status: 22 of 25 files reviewed, 15 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @woju)
a discussion (no related file):
Previously, bigdata-memory (Gordon King) wrote…
You can check out the description in SGX Developer Reference(https://download.01.org/intel-sgx/sgx-linux/2.11/docs/Intel_SGX_Developer_Reference_Linux_2.11_Open_Source.pdf) P18, you can also find the associated screenshots in #2617
I still have no idea what you mean by "clear boundary ".
We expose an API, which you can use to sign the sigstruct in any way you desire (including sending the data to a signing facility).
There was a problem hiding this comment.
Reviewed 3 of 3 files at r11, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @bigdata-memory, @boryspoplawski, @dimakuv, @mkow, and @woju)
python/graphenelibos/sgx_sign.py, line 447 at r10 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Yeah, it seems to just be a remnant of an older version, which had this and sigstruct generation in one place.
Fixed.The name doesn't make much sense now. get_mrenclave returns not only the MRENCLAVE, but also the manifest dict? Please rename.
Yes it does, but only for convenience (because we have to parse it inside anyway). I left the old name, because I expect most users will ignore the returned manifest and the main point of this function is to get MRENCLAVE.
If you insist on this change, please give me some hint on the new name.
Ah, I actually thought that it depends on the date. If not, it can be dropped from here (as Borys just did).
python/graphenelibos/sgx_sign.py, line 475 at r10 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Nope it is not.
Yeah, now I already know ;)
There was a problem hiding this comment.
Reviewed 3 of 3 files at r11, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @bigdata-memory, @boryspoplawski, @dimakuv, @mkow, and @woju)
python/graphenelibos/sgx_sign.py, line 447 at r10 (raw file):
If you insist on this change, please give me some hint on the new name.
Doesn't matter that much. Resolving.
python/graphenelibos/sgx_sign.py, line 514 at r10 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
How exactly would you do that? Having one argument and check if with
isinstancewould be ugly.
Can't come up with something simple. Resolving.
There was a problem hiding this comment.
Reviewed 2 of 21 files at r1, 1 of 5 files at r4, 1 of 3 files at r9.
Reviewable status: all files reviewed, 9 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @bigdata-memory, @boryspoplawski, @dimakuv, and @mkow)
Pal/regression/Bootstrap3.manifest, line 6 at r3 (raw file):
Previously, bigdata-memory (Gordon King) wrote…
Looks more readability for me.
In all other languages we don't have whitespace inside parens and brackets, why would we do things differently in TOML?
python/graphene-gen-depend, line 11 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Well, how could I knew they are not using any wrappers?
Oh they do (esp. for lazy, atomic=True, etc.), but even then it would be a bug to filter this attribute, so they pass all of them: https://github.com/pallets/click/blob/c1a42a1c545c138266be61c7b0fda02b9a423fe0/src/click/utils.py#L135-L136. Also, there aren't any other ways to open in file in python apart from os.open() which returns an int and possibly ctypes, so one way or another you'll get this name.
python/graphene-gen-depend, line 12 at r3 (raw file):
os.path.join(_CONFIG_PKGLIBDIR, 'sgx/libpal.so')
python/graphenelibos/init.py, line 16 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
The problem is, everyone using this modules has to do their own try/except, because just importing graphenelibos means you can have those attributes or not, so we're not solving anything with this contraption.
What? This is substituted by meson, just like some variables above. So if you've installed SGX version, they you have these files and if you didn't, they they are not imported. No need for any try+except.
I mean when you actually use this by importing the module (not by launching cli tools from shell, but from python, as advertised). You then have to do your own try/except, because you need to check if the installed graphene wasn't configured with -Dsgx=disabled. Otherwise, your enterprise tool will barf a traceback.
python/graphenelibos/manifest.py, line 150 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
If I put
_envin global scope it wont change anything w.r.t. arguments you gave.
Environment is logically more tied to the file than to any class or function inside, because typically you have FileSystemLoader there, which has path from which you load templates. The path is typically calculated relative to __file__. That you create it using make_env() which does correct thing does not change this.
python/graphenelibos/manifest.py, line 152 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
get_mrenclavenow takes a path, I've figured this is the best way to stop ppl shooting their feet, and integrates ok-ish with other functions. It now also returns a manifest, in case you need that too (since it's already parsed).
OK. It's good enough for now.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @bigdata-memory, @boryspoplawski, @mkow, and @woju)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
All right, let's add the TODO list after the review.
List of things to be done (needs to be moved to #11 after final rebase):
- "Manifests will be renamed:
app.toml.jinja(the template) andapp.toml(after rendering and trusted files measurement)." - "
graphene-sgx-get-mrenclavenew tool, will calculate MRENCLAVE based on manifest andlibpal.so" - this needs rethinking, asmrenclaveis not enough. Probably we will just add a tool to generate and printTBSSIGSTRUCT. Then we could also removeget_mrenclavefrom API - "
graphene-sgx-keygennew tool, will generate serviceable RSA-3072 key in proper place" - "The default key location will be in
~/.config/graphene/enclave-key.pem(really in$XDG_CONFIG_HOME)." - idk about this one, also my system does not haveXDG_CONFIG_HOMEset. - "use cryptography (https://cryptography.io/) instead of
subprocess.run('openssl')and hand-crafted RSA signature" - remove all prints from python library; add some to our script wrappers:
gramine-sgx-signetc (probably behind-v/--verboseflag).
python/graminelibos/sgx_sign.py, line 447 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Could you add this to the TODOlist we discuss above then? (and then to the corresponding issue)
Done.
python/graminelibos/sgx_sign.py, line 471 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Ideally, the library should have 0 prints and only the final tools would print some info, where needed.
I think that's what we should do. But we can do this later, but please add this to the TODOlist above.
Done.
There was a problem hiding this comment.
Reviewable status: 31 of 34 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), "fixup! " found in commit messages' one-liners (waiting on @bigdata-memory, @boryspoplawski, @dimakuv, @mkow, and @woju)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
List of things to be done (needs to be moved to #11 after final rebase):
- "Manifests will be renamed:
app.toml.jinja(the template) andapp.toml(after rendering and trusted files measurement)."- "
graphene-sgx-get-mrenclavenew tool, will calculate MRENCLAVE based on manifest andlibpal.so" - this needs rethinking, asmrenclaveis not enough. Probably we will just add a tool to generate and printTBSSIGSTRUCT. Then we could also removeget_mrenclavefrom API- "
graphene-sgx-keygennew tool, will generate serviceable RSA-3072 key in proper place"- "The default key location will be in
~/.config/graphene/enclave-key.pem(really in$XDG_CONFIG_HOME)." - idk about this one, also my system does not haveXDG_CONFIG_HOMEset.- "use cryptography (https://cryptography.io/) instead of
subprocess.run('openssl')and hand-crafted RSA signature"- remove all prints from python library; add some to our script wrappers:
gramine-sgx-signetc (probably behind-v/--verboseflag).
Update: removed get_mrenclave from API, as it's not enough for secure verification. Probably need to add some EnclaveIdentity class, which could be derived from Sigstruct and contain only user verifiable data (e.g. could skip date), that we could pretty-print.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r14, 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), "fixup! " found in commit messages' one-liners (waiting on @bigdata-memory, @boryspoplawski, @mkow, and @woju)
There was a problem hiding this comment.
Reviewed 3 of 3 files at r14, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @bigdata-memory, @boryspoplawski, @mkow, and @woju)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
Update: removed
get_mrenclavefrom API, as it's not enough for secure verification. Probably need to add someEnclaveIdentityclass, which could be derived fromSigstructand contain only user verifiable data (e.g. could skip date), that we could pretty-print.
Btw, I introduced something like this in GSC: gramineproject/gsc#21. I believe I covered all relevant enclave info there. You can have something similar in your API.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @bigdata-memory, @boryspoplawski, and @mkow)
a discussion (no related file):
"The default key location will be in ~/.config/graphene/enclave-key.pem (really in $XDG_CONFIG_HOME)." - idk about this one, also my system does not have XDG_CONFIG_HOME set.
That's OK. https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html:
If $XDG_CONFIG_HOME is either not set or empty, a default equal to $HOME/.config should be used.
No one ever sets this, but in case anyone does, we should be XDG® Compliant™.
python/graphenelibos/manifest.py, line 150 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Only advantage of keeping this in class is that you can modify it for each subclass. I don't see any advantages of putting this in global.
Oh, and I forgot about another problem: this env might be not only for manifests, but for other files that we hypothetically would render. For example, if we ever merged GSC into this repo (not that we should), then we would probably use the same env for gsc's templates too. Env really belongs to the app, not to any particular class.
Please, can I get Python laid out as I like it? It will be me who would be reviewing it in the future.
There was a problem hiding this comment.
Reviewable status: 29 of 36 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), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @woju)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
I still have no idea what you mean by "clear boundary ".
We expose an API, which you can use to sign the sigstruct in any way you desire (including sending the data to a signing facility).
Unblocking as we seem to have no existing documentation, except graphene-manifest manpage, which does not need any updates.
Update: added small manpages for gramine-sgx-sign and gramine-sgx-get-token. gramine-gen-depend was not documented, since it will probably be deleted soon.
Python API documentation is not yet documented, added it to the TODO list below.
a discussion (no related file):
Previously, woju (Wojtek Porczyk) wrote…
"The default key location will be in ~/.config/graphene/enclave-key.pem (really in $XDG_CONFIG_HOME)." - idk about this one, also my system does not have XDG_CONFIG_HOME set.
That's OK. https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html:
If $XDG_CONFIG_HOME is either not set or empty, a default equal to $HOME/.config should be used.
No one ever sets this, but in case anyone does, we should be XDG® Compliant™.
All right.
Also add to the list:
- document python API - actually where to put this? In what format?
a discussion (no related file):
Updated all short options in all python scripts as it turns out we did not use them (e.g. -o instead of -output).
python/graphenelibos/manifest.py, line 150 at r3 (raw file):
Moved to __init__, because we don't have time for this, the release is happening soon.
Please, can I get Python laid out as I like it? It will be me who would be reviewing it in the future.
No you cannot. Others will review these as well.
There was a problem hiding this comment.
Reviewed 7 of 7 files at r15, 1 of 1 files at r16, all commit messages.
Reviewable status: all 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), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @woju)
Documentation/manpages/gramine-sgx-get-token.rst, line 11 at r16 (raw file):
======== :command:`gramine-sgx-get-token` [*OPTION*]... --sig sigstruct_file
This is incorrectly formatted after rendering: no const-width and things like e.g. -- changed to –.
Documentation/manpages/gramine-sgx-get-token.rst, line 17 at r16 (raw file):
=========== :program:`gramine-sgx-get-token` is used to generate the SGX Token file for
Token -> token
Documentation/manpages/gramine-sgx-sign.rst, line 11 at r16 (raw file):
======== :command:`gramine-sgx-sign` [*OPTION*]... --output output_manifest
ditto
Documentation/manpages/gramine-sgx-sign.rst, line 17 at r16 (raw file):
=========== :program:`gramine-sgx-sign` is used to expand Truested Files and generate
Truested -> Trusted
There was a problem hiding this comment.
Reviewable status: 35 of 37 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), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @woju)
Documentation/manpages/gramine-sgx-get-token.rst, line 11 at r16 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This is incorrectly formatted after rendering: no const-width and things like e.g.
--changed to–.
This is in manpages directory and renders correctly if you use man to open in (after make man).
As to why https://gramine.readthedocs.io/en/latest/manpages/is_sgx_available.html is rendering incorrectly, I have no idea, but it's unrelated to this PR (the tool I've linked here is not modified in this PR and renders incorrectly).
Documentation/manpages/gramine-sgx-get-token.rst, line 17 at r16 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Token->token
Done.
Documentation/manpages/gramine-sgx-sign.rst, line 11 at r16 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto
ditto
Documentation/manpages/gramine-sgx-sign.rst, line 17 at r16 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Truested->Trusted
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r17, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @woju)
Documentation/manpages/gramine-sgx-get-token.rst, line 11 at r16 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
This is in manpages directory and renders correctly if you use man to open in (after
make man).
As to why https://gramine.readthedocs.io/en/latest/manpages/is_sgx_available.html is rendering incorrectly, I have no idea, but it's unrelated to this PR (the tool I've linked here is not modified in this PR and renders incorrectly).
Ok, other already existing pages are also broken :/ We'll need to fix this separately.
CC: @woju.
There was a problem hiding this comment.
Reviewed 5 of 7 files at r15, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @woju)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)
a discussion (no related file):
where to put this? In what format?
Preferably as Google-style docstrings parsed with https://www.sphinx-doc.org/en/master/usage/extensions/napoleon.html. Those should be included using https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html in descriptive documents somewhere in Documentation/.
Documentation/manpages/gramine-sgx-get-token.rst, line 11 at r16 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Ok, other already existing pages are also broken :/ We'll need to fix this separately.
CC: @woju.
You don't usually list any options in synopsis, so in theory this should be included in *options*. The problem stems from the fact that --sig is mandatory. Optimally we should fix the invocation to just gramine-sgx-get-token INPUT [OUTPUT].
Not now, obviously. Maybe add TODO item to revise all CLIs.
python/graminelibos/init.py, line 87 at r15 (raw file):
add_globals_from_python(env) add_globals_misc(env) return env
If you add this here instead of a separate file, at least add __all__ without all this stuff. https://docs.python.org/3/tutorial/modules.html?highlight=__all__#importing-from-a-package
There was a problem hiding this comment.
Reviewable status: 35 of 38 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 @boryspoplawski, @dimakuv, @mkow, and @woju)
a discussion (no related file):
Preferably as Google-style docstrings
Sounds good, will do.
in descriptive documents somewhere in Documentation/
Since it will be you who would be reviewing it in the future, please tell me what to put and where exactly, I don't want to mess with your flows.
Documentation/manpages/gramine-sgx-get-token.rst, line 11 at r16 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
You don't usually list any options in synopsis, so in theory this should be included in
*options*. The problem stems from the fact that--sigis mandatory. Optimally we should fix the invocation to justgramine-sgx-get-token INPUT [OUTPUT].Not now, obviously. Maybe add TODO item to revise all CLIs.
The problem is with gramine-sgx-sign which has 3 mandatory arguments - I think that's too much for unnamed args. So I think we need (at least) a workaround anyway.
python/graminelibos/init.py, line 87 at r15 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
If you add this here instead of a separate file, at least add
__all__without all this stuff. https://docs.python.org/3/tutorial/modules.html?highlight=__all__#importing-from-a-package
Moved to another file.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r18, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @woju)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
Preferably as Google-style docstrings
Sounds good, will do.
in descriptive documents somewhere in Documentation/
Since it will be you who would be reviewing it in the future, please tell me what to put and where exactly, I don't want to mess with your flows.
Here you are: https://github.com/gramineproject/gramine/tree/woju/wip-python-docs-for-borys
Obviously subject to approval from other maintainers, who will also review it. I don't insist on menu placement nor python/ subdirectory, but it might come handy for splitting off examples or whatnot.
Documentation/manpages/gramine-sgx-get-token.rst, line 11 at r16 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
The problem is with
gramine-sgx-signwhich has 3 mandatory arguments - I think that's too much for unnamed args. So I think we need (at least) a workaround anyway.
Yes, I agree. Can we rethink this after putting the key in .config/gramine, so the key becomes optional? That will be 2 arguments, which I'd argue is OK.
python/graminelibos/init.py, line 87 at r15 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Moved to another file.
Thank you.
Previously, `copy_common.o` was built only once even though it depended on a define. Now it is correctly built twice and each version is linked only to appropriate tests. Signed-off-by: Borys Popławski <[email protected]>
boryspoplawski
force-pushed
the
borys/python
branch
from
9afff83
to
9a4fd15
Compare
This commit introduces new python APIs for managing manifests and SGX sigstructs, including MRENCLAVE generation, signing and token generation. This required a couple of rewrites, which hopefuly resulted in cleaner code. Signed-off-by: Borys Popławski <[email protected]>
There was a problem hiding this comment.
Reviewed 7 of 7 files at r19, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkow)
There was a problem hiding this comment.
Reviewed 7 of 7 files at r19, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
Description of the changes
Too tired to write full description at the moment, will fix later.
Does this fix RFC #11 fully or only partially ?
This change is