-
Notifications
You must be signed in to change notification settings - Fork 114
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
Introduce label lattice definition in Oak ABI #657
Conversation
15ffd79
to
03219d4
Compare
FTR @aferr is locked out of his own GitHub account but he sent me the following review via email, pasting here for reference: High level comments:Maybe it is useful to also add functions for meet and join in the label trait?
One possibly annoying thing about using sets for the definition of tags is that while you have an easy way of writing "public to everyone" and "trusted by everyone" (both are the empty set), I can't think of an easy way to write "secret to everyone" or "untrusted by everyone" which should both be the sets of all tags. Is there actually an easy way to get those in Rust? Both secret and untrusted seem useful to me. Untrusted is useful for values that come from outside the system. Another annoying thing about using sets of tags as labels is that you can't easily have a tag that represents the secrecy of "A or B" if A is a tag and B is a tag. and L1 join L2 is ( {A,B}, {} ) which is a label you could apply to a node that
you might like the opposite which should have:
You can kind of workaround this issue by instead giving Alice and Bob labels like ({Alice, AliceNBob}, {}), ({Bob, AliceNBob}. {}) because now when you compute the meet of these labels you are left with conf label {AliceNBob} that can't be read by C. Another solution would be to implement something more FLAM-like, but this would be a bunch of work and would also require a SAT solver. If it turns out that not having "totally secret" or "A or B" is annoying, I could possibly help with FLAM-like labels. Maybe this isn't too useful anyway, though. In any case, at a high level this would require:
More specific comments:label.rs Line 143: maybe call this public_trusted ? really this is a nitpick and not a big deal. policy.rs Line Line 54, Line 56. Are these set and clear tag functions exposed to applications? I am guessing they are not, but thought it was worth checking just in case. |
I am planning to add them in a future PR, as it's not super clear how / when they would be used yet. I guess their use will be motivated by some of the upcoming examples too (e.g. the Chat app).
What would be the use case of a Node or Channel having a label "secret to everyone" in practice? It seems useful from a theoretical point of view, but I'm not sure whether it can ever show up at runtime.
This is true, and I think it's a reasonable compromise for now, which we should revisit in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few general comments:
- Are the labels only to be used by the runtime? Perhaps they should be in the ABI if exposed to other things?
- I'm trying to remove protobuf dependencies right now from the runtime. Perhaps we could feature gate the labels? This would allow
no_std
for a certain feature set.
OK, that makes sense to me.
Right, you might not actually need it. A clear use-case for totally untrusted is values that come from the outside world. You have said that you do not care about integrity at the moment, but if you would like to eventually prove something like robustness, integrity will become important. Maybe for a client-server application, if you have a part of the server code that needs to store some global secret it is useful to label it as "totally secret". A totally secret node can't directly send messages anywhere, but it could declassify some safe value that is derived from a secret. For example, the server code could encrypt a message meant for a client node with a private key and declassify the ciphertext. It seems fine to not have a solution until you are sure you need it though.
SGTM |
Good point, they are kind of part of the ABI in the sense that both the runtime and the SDK will eventually depend on these definitions. I will try to move these definitions there and see if it works out, and then re-request your review.
Uhmm this may be problematic. We could gate it for the time being, but what is the ultimate solution to this problem? Should I start looking at alternatives? I mean, those labels don't need to be protobuf, they could be serialized in any other way (e.g. bincode, JSON, FIDL), but it would be nice to have a single canonical serialization format across runtime and SDK. @blaxill do you think the issues with protobuf and |
For e.g. the protobuf application configuration, I have Fixing Feature gating behind |
Thinking about it a little more, I'm guessing you intend for the labels to be (de)serialized by the runtime as they can contain tags with runtime values (e.g. your bearer tokens). Let me think a little bit if there is a third option. |
Prost My vote would be to feature gate it for std at the moment, I'll finish the runtime for no_std support, delaying the protobuf issue. Hopefully it will then be resolved upstream (for prost, I don't think the work has been done for rust-protobuf) or we could add support ourselves. |
Blocked by #666, so that I can just start using the new proto generation. |
e563bfa
to
975822e
Compare
Done
Yes they are exposed, these are automatically generated by protobuf. It should not matter anyways, once labels are sent over to the runtime, then it does not matter whether the application can keep mutating its local copy, right? |
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't immediately look like it belonged here, because it doesn't give constant values or host function signatures for things that flow across the Wasm ABI boundary.
However, I'm guessing that serialized Label
protos will flow over the ABI boundary at some point in future, and then any Rust code on either side of the ABI boundary will have a use for these Rust helpers.
So maybe add a module doc comment explaining that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
975822e
to
94e57d0
Compare
@tiziano88 happy to do it either way round, #666 and #656 need approval (or change requests :) ) |
Not used yet, but as a starting point for future functionality. It is in the ABI because it will be used by both the runtime and the SDK. Ref project-oak#630
94e57d0
to
b97a38c
Compare
Rebased on those PRs, the result looks good so I'll go ahead now! |
Not used yet, but as a starting point for future functionality.
It is in the ABI because it will be used by both the runtime and the
SDK.
Ref #630
Checklist
cover any TODOs and/or unfinished work.
construction.