Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Graphene with Gramine everywhere #10

Merged
merged 1 commit into from
Sep 28, 2021

Conversation

dimakuv
Copy link

@dimakuv dimakuv commented Sep 10, 2021

This includes using github.com/gramineproject/gramine link and changing executables from e.g. graphene-sgx to gramine-sgx.

NOTE: This depends on gramineproject/gramine#17


This change is Reviewable

Copy link
Contributor

@veenasai2 veenasai2 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 8 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: Intel) (waiting on @dimakuv)


templates/Dockerfile.ubuntu18.04.build.template, line 33 at r1 (raw file):


# Copy Graphene runtime and signer tools to /graphene/meson_build_output
RUN mkdir -p /graphene/Tools \

We don't need this now.


templates/Dockerfile.ubuntu18.04.build.template, line 37 at r1 (raw file):


# TODO: remove this copy after argv_serializer becomes a part of Meson build
COPY --from=graphene /graphene/Tools/argv_serializer /graphene/Tools

This can also be removed


templates/Dockerfile.ubuntu18.04.build.template, line 47 at r1 (raw file):

# Generate trusted arguments if required
{% if not insecure_args %}
RUN /graphene/Tools/argv_serializer {{binary}} {{binary_arguments}} "{{"\" \"".join(cmd)}}" > /trusted_argv

This one should be /gramine/meson_build_output/bin/graphene-argv-serializer or /gramine/meson_build_output/bin/gramine-argv-serializer.


templates/Dockerfile.ubuntu18.04.compile.template, line 19 at r1 (raw file):

        python3-protobuf \
        wget \
    && python3 -B -m pip install toml>=0.10

We need to add "pip meson ninja " to avoid meson version dependency error (>=0.55) and "Could not detect Ninja v1.8.2 or newer" error. Also, pkg-config should be installed with "apt-get install pkg-config", otherwise meson build fails.

Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 8 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv and @veenasai2)

a discussion (no related file):
@veenasai2 Thanks for the review, I will apply your comments when doing the rebase (today or next week).



templates/Dockerfile.ubuntu18.04.build.template, line 33 at r1 (raw file):

Previously, veenasai2 wrote…

We don't need this now.

Thanks, I will rebase this PR later so that it uses proper steps

@dimakuv dimakuv force-pushed the dimakuv/replace-graphene-with-gramine branch from 61e8a8c to dc8a70c Compare September 24, 2021 15:09
Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 10 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: Intel) (waiting on @veenasai2)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@veenasai2 Thanks for the review, I will apply your comments when doing the rebase (today or next week).

Done.



templates/Dockerfile.ubuntu18.04.build.template, line 33 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Thanks, I will rebase this PR later so that it uses proper steps

Done.


templates/Dockerfile.ubuntu18.04.build.template, line 37 at r1 (raw file):

Previously, veenasai2 wrote…

This can also be removed

Done.


templates/Dockerfile.ubuntu18.04.build.template, line 47 at r1 (raw file):

Previously, veenasai2 wrote…

This one should be /gramine/meson_build_output/bin/graphene-argv-serializer or /gramine/meson_build_output/bin/gramine-argv-serializer.

Done.


templates/Dockerfile.ubuntu18.04.compile.template, line 19 at r1 (raw file):

Previously, veenasai2 wrote…

We need to add "pip meson ninja " to avoid meson version dependency error (>=0.55) and "Could not detect Ninja v1.8.2 or newer" error. Also, pkg-config should be installed with "apt-get install pkg-config", otherwise meson build fails.

Done. In a slightly different way (followed https://github.com/gramineproject/gramine/blob/master/.ci/ubuntu18.04.dockerfile)

Copy link
Contributor

@veenasai2 veenasai2 left a comment

Choose a reason for hiding this comment

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

Functionality-wise this PR works fine. Tested python-gsc image, ra-tls-secret-prov image, gsc with debug mode, all are working fine. Thanks.

Reviewable status: 0 of 10 files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 8 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)


templates/entrypoint.manifest.template, line 20 at r2 (raw file):

# !! INSECURE !! Allow passing command-line arguments from the host without validation.
# Most Docker images rely on runtime arguments and hence, a more general technique is required.
# The issue is documented at https://github.com/gramineproject/graphene/issues/1520.

Please update this to https://github.com/gramineproject/gsc/issues/13.

This includes using `github.com/gramineproject/gramine` link and
changing executables from e.g. `graphene-sgx` to `gramine-sgx`.

The name "Graphene" was deemed too common, could be impossible to
trademark, and collided with several other software projects. Thus,
a new name "Gramine" was chosen.

Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
@dimakuv dimakuv force-pushed the dimakuv/replace-graphene-with-gramine branch from dc8a70c to c2c453a Compare September 28, 2021 13:00
Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow)


templates/entrypoint.manifest.template, line 20 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please update this to https://github.com/gramineproject/gsc/issues/13.

Done (using rebase, so we can merge quicker)

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

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

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

Successfully merging this pull request may close these issues.

3 participants