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

[LibOS] Make all pipe and UDS names Gramine-instance-specific #1761

Merged
merged 1 commit into from
May 4, 2024

Conversation

dimakuv
Copy link
Contributor

@dimakuv dimakuv commented Feb 6, 2024

Description of the changes

Commit "[LibOS,PAL] Move assigning name to a pipe/UDS from PAL to LibOS" had a small bug: the new LibOS logic lost the Gramine-instance-specific part of the name for some pipes and all UNIX Domain Sockets (UDSes).

This led to a benign but annoying bug of e.g. two gramine-direct Gramine instances being able to establish a common UDS. This could have been considered a feature, but two gramine-sgx Gramine instances would (correctly) fail to establish a common UDS, because each gramine-sgx instance has its own encryption key, and thus two UDSes would not be able to establish a secure TLS channel (even though they see each other because of this bug).

To make behavior of gramine-direct and gramine-sgx uniform, this commit forces all pipe names and UDS names to have the Gramine-instance-specific part in the name. I.e., previously UDSes had names on the host Linux like @/gramine/<hash-of-uds-name> and now they have names like @/gramine/<gramine-instance-id>/<hash-of-uds-name>.

References:

How to test this PR?

Run two Gramine instances, one creating and listening on the UDS, another connecting to the same UDS.

  • Without this PR, two gramine-direct instances will establish a connection. (gramine-sgx instances won't because of failed TLS handshake.)
  • With this PR, two gramine-direct instances will not be able to establish a connection.

This change is Reviewable

@dimakuv dimakuv force-pushed the dimakuv/force-pipe-uds-to-one-gramine-instance branch from 10349c8 to b9f1c99 Compare April 30, 2024 12:59
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 2 of 2 files at r1, 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)


libos/src/libos_init.c line 585 at r1 (raw file):

    *hdl = pipe;
    len = snprintf(uri, size, URI_PREFIX_PIPE "%lu/%s", g_pal_public_state->instance_id, pipename);
    assert(len < size); /* must hold because above we did the same but with longer prefix */

I don't like this, it silently assumes that one prefix is longer than the other and will silently turn into a bug if we rename the prefixes.
Please either add a static assert for the lengths or just turn this into a normal if.

Copy link
Contributor 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: 1 of 2 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), "fixup! " found in commit messages' one-liners (waiting on @mkow)


libos/src/libos_init.c line 585 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I don't like this, it silently assumes that one prefix is longer than the other and will silently turn into a bug if we rename the prefixes.
Please either add a static assert for the lengths or just turn this into a normal if.

Done.

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 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), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)


libos/src/libos_init.c line 586 at r2 (raw file):

    len = snprintf(uri, size, URI_PREFIX_PIPE "%lu/%s", g_pal_public_state->instance_id, pipename);
    static_assert(static_strlen(URI_PREFIX_PIPE) < static_strlen(URI_PREFIX_PIPE_SRV),
                  "pipe prefixes are wrong");

Suggestion:

"without this condition the assert below should be changed into an `if`"

Copy link
Contributor 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: 1 of 2 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), "fixup! " found in commit messages' one-liners (waiting on @mkow)


libos/src/libos_init.c line 586 at r2 (raw file):

    len = snprintf(uri, size, URI_PREFIX_PIPE "%lu/%s", g_pal_public_state->instance_id, pipename);
    static_assert(static_strlen(URI_PREFIX_PIPE) < static_strlen(URI_PREFIX_PIPE_SRV),
                  "pipe prefixes are wrong");

Done.

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: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

Copy link
Contributor

@kailun-qin kailun-qin 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 2 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

Commit "[LibOS,PAL] Move assigning name to a pipe/UDS from PAL to LibOS"
had a small bug: the new LibOS logic lost the Gramine-instance-specific
part of the name for some pipes and all UNIX Domain Sockets (UDSes).

This led to a benign but annoying bug of e.g. two `gramine-direct`
Gramine instances being able to establish a common UDS. This could have
been considered a feature, but two `gramine-sgx` Gramine instances would
(correctly) fail to establish a common UDS, because each `gramine-sgx`
instance has its own encryption key, and thus two UDSes would not be
able to establish a secure TLS channel (even though they see each other
because of this bug).

To make behavior of `gramine-direct` and `gramine-sgx` uniform, this
commit forces all pipe names and UDS names to have the
Gramine-instance-specific part in the name. I.e., previously UDSes had
names on the host Linux like `@/gramine/<hash-of-uds-name>` and now they
have names like `@/gramine/<gramine-instance-id>/<hash-of-uds-name>`.

Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
@mkow mkow force-pushed the dimakuv/force-pipe-uds-to-one-gramine-instance branch from 31cff28 to 609e9af Compare May 3, 2024 22:12
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 all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

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

@mkow mkow merged commit 609e9af into master May 4, 2024
18 checks passed
@mkow mkow deleted the dimakuv/force-pipe-uds-to-one-gramine-instance branch May 4, 2024 12:00
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.

None yet

3 participants