Change positional args manifest and key to optional args

[svenkata9]

Signed-off-by: Sankaranarayanan Venkatasubramanian [email protected]

Description of the changes

This PR changes the positional arguments manifest and key that gsc build and gsc sign-image commands take respectively to optional ones. This change is being done separately now to accommodate change for production signing #112 (to avoid introducing the breaking change for key as optional argument)

How to test this PR?

docker pull python
./gsc build -L --manifest test/generic.manifest python
./gsc sign-image -k enclave-key.pem python

and run the resulting gsc-python image.

---

This change is 

[dimakuv]

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

---

-- commits line 2 at r1:
Can we shorten this title by replacing arguments -> args (2x)

---

-- commits line 2 at r1:
I think Change sounds better than Update

---

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

        user_manifest_contents = user_manifest_file.read()

    user_manifest_dict = tomli.loads(user_manifest_contents)

If I understand correctly, if user skips --manifest ..., then ./gsc build will fail here.

Instead, we should allow to omit --manifest, in this case user_manifest_dict should be assigned to empty dict. So I think we need something like this:

    if args.manifest:
        user_manifest_contents = ''
        if not os.path.exists(args.manifest):
            raise FileNotFoundError(f'Manifest file {args.manifest} does not exist')
        with open(args.manifest, 'r') as user_manifest_file:
            user_manifest_contents = user_manifest_file.read()
        user_manifest_dict = tomli.loads(user_manifest_contents)
    else:
        user_manifest_dict = {} # no user-provided manifest

Please test GSC when --manifest is used and when it is omitted.

---

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

    tmp_build_key_path = tmp_build_path / 'gsc-signer-key.pem'
    tmp_build_sign_path = tmp_build_path / 'sign.sh'
    shutil.copyfile(os.path.abspath(args.key), tmp_build_key_path)

What happens if user didn't specify --key some.key? Will GSC fail with some complicated error at this line?

Can we add a check here that if not args.key or not os.path.exists(args.key): print("some error"); sys.exit(1)?

---

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

sub_sign.add_argument('-c', '--config_file', type=argparse.FileType('r', encoding='UTF-8'),
    default='config.yaml', help='Specify configuration file.')
sub_sign.add_argument('image', help='Name of the application (base) Docker image.')

Can we also move this argument after all args (i.e., after -p)? So that it's easy to see that this is the positional mandatory arg.

---

Documentation/index.rst line 150 at r1 (raw file):

.. option:: -k

   Used to sign the Intel SGX enclave

I would expand to Key file used to sign the Intel SGX enclave

[svenkata9]

Reviewable status: 2 of 4 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

---

-- commits line 2 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can we shorten this title by replacing arguments -> args (2x)

Done

---

-- commits line 2 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I think Change sounds better than Update

Done

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

If I understand correctly, if user skips --manifest ..., then ./gsc build will fail here.

Instead, we should allow to omit --manifest, in this case user_manifest_dict should be assigned to empty dict. So I think we need something like this:

    if args.manifest:
        user_manifest_contents = ''
        if not os.path.exists(args.manifest):
            raise FileNotFoundError(f'Manifest file {args.manifest} does not exist')
        with open(args.manifest, 'r') as user_manifest_file:
            user_manifest_contents = user_manifest_file.read()
        user_manifest_dict = tomli.loads(user_manifest_contents)
    else:
        user_manifest_dict = {} # no user-provided manifest

Please test GSC when --manifest is used and when it is omitted.

Done.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What happens if user didn't specify --key some.key? Will GSC fail with some complicated error at this line?

Can we add a check here that if not args.key or not os.path.exists(args.key): print("some error"); sys.exit(1)?

Done.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can we also move this argument after all args (i.e., after -p)? So that it's easy to see that this is the positional mandatory arg.

Done.

---

Documentation/index.rst line 150 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I would expand to Key file used to sign the Intel SGX enclave

Done.

[dimakuv]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all 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: Intel), "fixup! " found in commit messages' one-liners

[svenkata9]

@woju @mkow Could you review? This is a breaking change, but needed one - without which we aren't able to do production signing. So, we are making all arguments except IMAGE as optional.