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

Add top privilege tag #1633

Merged
merged 1 commit into from
Oct 29, 2020
Merged

Add top privilege tag #1633

merged 1 commit into from
Oct 29, 2020

Conversation

rbehjati
Copy link
Contributor

Ref #1631

Copy link
Contributor Author

@rbehjati rbehjati left a comment

Choose a reason for hiding this comment

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

@tiziano88 @conradgrobler I am not entirely happy with the naming. Let me know if you have better suggestions :)

@@ -66,8 +66,18 @@ impl crate::proto::oak::label::Label {
// The target label must have (compared to the self label):
// - same or more confidentiality tags
// - same or fewer integrity tags
self_confidentiality_tags.is_subset(&other_confidentiality_tags)
&& other_integrity_tags.is_subset(&self_integrity_tags)
(other_confidentiality_tags.contains(&Tag::universal_top_tag())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this check for completeness, but I am not sure if we ever want to use the top tag as part of a label (perhaps when we switch to robust declassification and transparent endorsement?). Currently, I am only using it to set the privilege of HTTP and gRPC server nodes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM, @aferr should take a look too, please update the comment as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@tiziano88 tiziano88 requested a review from aferr October 28, 2020 13:11
Copy link
Collaborator

@tiziano88 tiziano88 left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -82,3 +83,8 @@ message TlsEndpointTag {
// using the set of Certificate Authorities (CA) that are available to it.
string authority = 1;
}

// Message representing top-secret. Declaring this as an enum results in conflicting `derive`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not yet a confidentiality label, it is just a principal that may be used as part of a label, so top-secret is not accurate, I would just mention that this is the top element of the principal lattice, and perhaps that it may be used in a confidentiality label to represent the top-secret level, and also as privilege to represent infinite downgrading privilege.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Updated the comment.

@@ -82,3 +83,8 @@ message TlsEndpointTag {
// using the set of Certificate Authorities (CA) that are available to it.
string authority = 1;
}

// Message representing top-secret. Declaring this as an enum results in conflicting `derive`
// rules between the ones that Prost automatically generates for enums, and the ones that are
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure I understand the sentence about enum, could you clarify? Are you talking about a Rust enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to have it as an enum. Prost converts this to a Rust enum, but then there is conflict in the derived annotation:

error[E0119]: conflicting implementations of trait `std::cmp::Eq` for type `proto::oak::label::TopTag`:
   --> /opt/my-project/oak_runtime/target/debug/build/oak_abi-941eb48ba258a197/out/oak.label.rs:111:10
    |
111 | #[derive(Eq, Hash)]
    |          ^^ conflicting implementation for `proto::oak::label::TopTag`
...
114 | #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, ::prost::Enumeration)]
    |                                         -- first implementation here
    |
    = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

It is related to this issue: https://github.com/danburkert/prost/issues/332.

Removed it from the comment.

@@ -66,8 +66,18 @@ impl crate::proto::oak::label::Label {
// The target label must have (compared to the self label):
// - same or more confidentiality tags
// - same or fewer integrity tags
self_confidentiality_tags.is_subset(&other_confidentiality_tags)
&& other_integrity_tags.is_subset(&self_integrity_tags)
(other_confidentiality_tags.contains(&Tag::universal_top_tag())
Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM, @aferr should take a look too, please update the comment as well.

}

impl Tag {
/// Convenience function for creating top privilege tag that can declassify all types of tags.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this comment mixes privilege and tags a bit, AFAICT it is just creating an instance of the top tag. Also I would just call it top, universal_top seems just another name for the same thing, and it is already called top elsewhere (and in literature)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I included universal in the name to distinguish it from the case where we'll have separate tops for each sub-lattice. I have changed it to top for now.


impl Tag {
/// Convenience function for creating top privilege tag that can declassify all types of tags.
pub fn universal_top_tag() -> Tag {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need a method for this particular tag but not others? Or do we already have methods for the others elsewhere? I seem to remember we have constructors for various types of labels (though perhaps not tags).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have methods for creating other types of tags, but they are not members of Tag. They are ordinary functions. top is a special value, and felt more like Label::public_untrusted, so I had it as a method. Now changed it to be a function too.

oak_runtime/src/lib.rs Outdated Show resolved Hide resolved
oak_runtime/src/lib.rs Outdated Show resolved Hide resolved
// Remove all the confidentiality tags that the Node may declassify.
confidentiality_tags: initial_label

// Remove all the confidentiality tags that the Node may declassify.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aferr is there a more elegant way of computing a downgraded label in general, perhaps using meet and join operator from the underlying principal lattice, if we had them?

Copy link

Choose a reason for hiding this comment

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

If we go with the NMIFC rules for downgrading, meet/join would be closer to how its done in that paper and the related ones. With privilege as a separate thing from integrity, it might work out similarly if you just take the type rule from that paper and replace the integrity projection with the downgrade privilege. I haven't fully thought this out though.

I think the way you have it here is perfectly fine, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have a link to the paper?

@@ -126,6 +126,10 @@ impl Node for GrpcServerNode {
"grpc-server"
}

fn get_privilege(&self) -> NodePrivilege {
NodePrivilege::top_privilege()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a comment about why this needs this privilege?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would have also mentioned that this is because we don't have a more restrictive privilege that corresponds to just the user tags for now, anyways I think we have discussed it enough in issues and slack, so perhaps fine to omit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a to-do comment.

}

{
// Reading from a more confidential Channel is allowed because of the privilege.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am planning on implementing a change (see #1301) that will stop this from working. A more accurate test that will continue working in future would be that a non-public node is able to write to a less confidential (e.g. public) channel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a copy of another test (create_channel_with_more_confidential_label_from_public_node_with_privilege_ok) except that it uses the top privilege. I think it is better to update both when #1301 is done.

I think the test you described requires creating an additional node with a more confidential label, which is a more complicated scenario than the tests here. Also, I just noticed that several assertions fail in these tests, but they all still return ok (so the CI builds don't fail). I will fix the tests separately.

@@ -126,6 +126,12 @@ impl Node for GrpcServerNode {
"grpc-server"
}

fn get_privilege(&self) -> NodePrivilege {
// This node needs to have `top` privilege to be able to declassify data tagged with a
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I think it would be even clearer to say something like "... to declassify data tagged with any arbitrary user identities"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

// Remove all the confidentiality tags that the Node may declassify.
confidentiality_tags: initial_label

// Remove all the confidentiality tags that the Node may declassify.
Copy link

Choose a reason for hiding this comment

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

If we go with the NMIFC rules for downgrading, meet/join would be closer to how its done in that paper and the related ones. With privilege as a separate thing from integrity, it might work out similarly if you just take the type rule from that paper and replace the integrity projection with the downgrade privilege. I haven't fully thought this out though.

I think the way you have it here is perfectly fine, though.

.contains(&top())
{
true => vec![],
false => initial_label
Copy link

Choose a reason for hiding this comment

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

I'm not a Rust programmer, so I might just not know Rust conventions. For me this would be slightly more intuitive if it was an if/else expression to check for top and then a match within the else (not top) branch.

The way you have it already is fine, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Changed it to an if-else expression.

.can_declassify_confidentiality_tags
.contains(&top())
{
vec![]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is also worth a comment, it seems a pretty crucial step and we don't want our future selves to accidentally get this wrong when changing things around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -126,6 +126,10 @@ impl Node for GrpcServerNode {
"grpc-server"
}

fn get_privilege(&self) -> NodePrivilege {
NodePrivilege::top_privilege()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would have also mentioned that this is because we don't have a more restrictive privilege that corresponds to just the user tags for now, anyways I think we have discussed it enough in issues and slack, so perhaps fine to omit.

* Adds a new Tag type to represent top-secret and top privilege
* Sets the privilege of HTTP and gRPC server nodes to this top privilege
@rbehjati rbehjati merged commit 8d42c31 into project-oak:main Oct 29, 2020
@rbehjati rbehjati deleted the top-tag branch October 29, 2020 16:19
@github-actions
Copy link

Reproducibility Index:

16b55d0a88500636cbf972badd9f038d6be2df545cb744416188c071e3504a25  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
5a0c908513bec25e3f862324e1a778ef6a413c005a56f2d86a8e5beb39268a80  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
a89cb29ccd9fd2eaa07dc7d291652e267098cd85982fc9791361780b0344aba7  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
92cc8796545fe48319d665f2ec109a1b137e6f6196925d582e7b788906db2499  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
f2076b2ea4cabccef32adae9258b49d0c85f33d1331f143aee2da61a11a2210e  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
445e81b75b541ad001a239f0948c4fcfd5c8010349d085f18296683537e8f91c  ./examples/target/wasm32-unknown-unknown/release/http_server.wasm
c480b298334fff420ac5611b71c56e221deb6f0bebd3b795824d009b902e63de  ./examples/target/wasm32-unknown-unknown/release/injection.wasm
d69e74f29e97d52ab9734a3b25399b942263c62010d92aae9898c1095b040c84  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
4811067509a6627893d67050b467a172863cec6fb907fb086da175086ee9aee9  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
4febd7a312551ab3522bbadf4a2733e3e62574c636b8c4bbf2d35d4a55c6be35  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
b0c18cd0e76f1267aa41b49843f4ce3692ff71adce35dc66579df3b26f2b261a  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
6abd67828d93bb826c649f7c5dd140c744150e0400abb50d72a0adc3c3dedf65  ./examples/target/wasm32-unknown-unknown/release/trusted_database.wasm
b548ff84c39e5cd07e7e6061bd24a43fdfe02825cd9b538188bdd7a8465cf509  ./oak_loader/target/x86_64-unknown-linux-musl/release/oak_loader

Reproducibility Index diff:

diff --git a/reproducibility_index b/reproducibility_index
index d73a1b3..dee2e58 100644
--- a/reproducibility_index
+++ b/reproducibility_index
@@ -1,13 +1,13 @@
-eaf09ff79fb99b14c0767375330ba41d0720fb994a57dce5877389b4e6a647ad  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
-01b372e23b5ccaf05d4121eb2974b9f81f7b996166d2d59d2c7d467a3d4ccbb7  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
-119a4f61d98b0f81ce8d31aa96ee02a6b560f50c1990b64a9b58060e04a427ce  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
-0c0e52b81a09bd858c7a74a0c5878d88dd633e194cda331db04e6869e451e5f9  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
-7a932a2be8c7b246bee1c3ad8298506c4d755e260a0f607e2a5d63eacb6611ef  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
-c41c9e1c92bdf620276a6e5141712686602eb3bc871569799d86094feaee2b2b  ./examples/target/wasm32-unknown-unknown/release/http_server.wasm
-d0449e18e3c76274cf73aebb21d0e81ed3e5df7abf9072d988ef4c2c6196d1fe  ./examples/target/wasm32-unknown-unknown/release/injection.wasm
-20daf4cd4e879d79da66a11fa82aeabd4af4843a57148e65d48e63559985bdb4  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
-0969c1602838391f0d51460725a27345269cac549027ccfde2ecfcfd1c4a42ff  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
-135c5fab34d1b87c9a28aa5f7b92fa34ee3a27da8c3ba17f534a969f97bca9a5  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
-afd26f8b08620db4e947c92c518d95534a3df4485f49a49e33d534457aa29590  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
-db7c0f6b7b9ae1070c207f24efc881e982d60f37d65368adc8016275b13d5483  ./examples/target/wasm32-unknown-unknown/release/trusted_database.wasm
-fcdbbc20ef57abcd9186df22779ac58ec797af2b72c0d705721baca72482560a  ./oak_loader/target/x86_64-unknown-linux-musl/release/oak_loader
+16b55d0a88500636cbf972badd9f038d6be2df545cb744416188c071e3504a25  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
+5a0c908513bec25e3f862324e1a778ef6a413c005a56f2d86a8e5beb39268a80  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
+a89cb29ccd9fd2eaa07dc7d291652e267098cd85982fc9791361780b0344aba7  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
+92cc8796545fe48319d665f2ec109a1b137e6f6196925d582e7b788906db2499  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
+f2076b2ea4cabccef32adae9258b49d0c85f33d1331f143aee2da61a11a2210e  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
+445e81b75b541ad001a239f0948c4fcfd5c8010349d085f18296683537e8f91c  ./examples/target/wasm32-unknown-unknown/release/http_server.wasm
+c480b298334fff420ac5611b71c56e221deb6f0bebd3b795824d009b902e63de  ./examples/target/wasm32-unknown-unknown/release/injection.wasm
+d69e74f29e97d52ab9734a3b25399b942263c62010d92aae9898c1095b040c84  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
+4811067509a6627893d67050b467a172863cec6fb907fb086da175086ee9aee9  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
+4febd7a312551ab3522bbadf4a2733e3e62574c636b8c4bbf2d35d4a55c6be35  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
+b0c18cd0e76f1267aa41b49843f4ce3692ff71adce35dc66579df3b26f2b261a  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
+6abd67828d93bb826c649f7c5dd140c744150e0400abb50d72a0adc3c3dedf65  ./examples/target/wasm32-unknown-unknown/release/trusted_database.wasm
+b548ff84c39e5cd07e7e6061bd24a43fdfe02825cd9b538188bdd7a8465cf509  ./oak_loader/target/x86_64-unknown-linux-musl/release/oak_loader

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

Successfully merging this pull request may close these issues.

4 participants