Use challenge response for authentication #1586

rbehjati · 2020-10-14T15:57:18Z

Summary of the changes:

Adds a user module, which defines a virtual node (Implement downgrading privilege for user principals authenticated over gRPC / HTTP #1452) named UserNode, to oak_runtime/node/http. An instance of UserNode is created for every incoming HTTP request. This node gets the identity of the user as its declassification and endorsement privileges. The functionality of UserNode is mostly copied from HttpServerNode, in particular, the function inject_http_request. In HttpServerNode the function inject_http_request is updated to instead create the UserNode instance and forward the HTTP request and the required invocation channels to it.
- Something similar is needed for gRPC.
- Does this node need to be destroyed when the handling of the request is completed, or is the Runtime smart enough to remove the node itself?
Adds a simple implementation of the challenge response (Implement support for generic challenge-response style authentication #1357), using a fixed challenge phrase (oak-challenge)
- The signed challenge is added as an HTTP header in the examples and unit tests, but for now the code assumes that the signed challenge is optional. Not sure if this a valid assumption though.
- New protobuf messages SignedChallenge, UserIdentityTag, UserNodeConfiguration, and OuterHttpInvocation are added.
The UserNode with its privilege uses the user's identity to correctly set the labels on the invocation channels used for the interaction with the Oak node (Set the user's identity as the confidentiality tag in invocation channels for HTTP and gRPC server nodes #1428).
Tests are updated accordingly.
Documentation will be updated in a separate PR.

Here is an attempt to visualize this change:

Checklist

Pull request affects core Oak functionality (e.g. runtime, SDK, ABI)
- I have written/updated tests that cover the code changes.
- I have checked that these tests are run by
  Cloudbuild
- I have updated documentation accordingly.
- I have raised an issue to
  cover any TODOs and/or unfinished work.
Pull request includes prototype/experimental work that is under
construction.

conradgrobler

I was not involved in the original design discussions, so I am unsure on the background. Could you please add some information on why these label calculations are done in a separate node rather than in the HTTP server pseudo-node (similar to the current gRPC pseudo-node implementation)?

Is this just to make a logical separation more clear in the code, or is there a technical reason that would make this approach more secure (or some other technical advantage)?

conradgrobler · 2020-10-23T08:14:02Z

oak_runtime/src/node/http/user.rs

+        startup_handle: oak_abi::Handle,
+        _notify_receiver: oneshot::Receiver<()>,
+    ) {
+        let _unit_result = self.try_run(runtime, startup_handle);


This seems weird that try_run returns a result, but is only used in one place where the result is ignored.

I did this to be able to use ? in try_run instead of using a match or if let for every statement in the function. This is changed now, but I think in general using anyhow or a specific error type can fix this issue.

conradgrobler · 2020-10-23T08:18:51Z

oak_runtime/src/node/http/user.rs

+            handle: response_reader,
+        });
+        let response = response_receiver.receive(&runtime).map_err(|err| {
+            error!(


Optional: I personally don't like this pattern of using map_err to do logging. I think it is cleaner if mapper functions like map_err do not produce side-effects. One option might be to define an error type that contains the message and it's severity. This can then be logged as eitehr a warning or error at the higher level where the result is processed.

Will anyhow::Context fit here?

I like the idea of using anyhow, but anyhow::Context is only implemented for Result types that use StdError as the error type. To be able to use anyhow::Context here, we have to wrap the OakStatus in an anyhow::Error. I also noticed anyhow is not used in the oak_runtime. Not sure if it is something in our backlog, or if the plan is to avoid anyhow in the oak_runtime.

In my new PR, I have added a specific error type (HttpError), and have replaced all such uses of map_err.

conradgrobler · 2020-10-23T08:23:37Z

oak_runtime/src/node/http/user.rs

+        let tag = match config.privilege.is_empty() {
+            true => None,
+            false => Some(oak_abi::label::tag::Tag::UserIdentityTag(UserIdentityTag {
+                public_key: config.privilege,


The naming seems confusing: using privilege in the proro to represent a public key in the config, but later it is used to represent the entire node privilege.

UserNode is removed from my new PR. So this issue should be resolved.

conradgrobler · 2020-10-23T08:27:15Z

oak_runtime/src/node/http/user.rs

+                .collect(),
+        };
+        let response_writer_label = Label {
+            confidentiality_tags: privilege


Should this check whether user identity tag was included in the original label? If the user did not include the identity in the original request, this could unnecessarily increase the confidentiality.

I had a chat with @tiziano88 and I think it is correct to set the confidentiality tag of the response channel to the user's identity regardless of the request label. Checking the label should then be done in the Router node, implemented by the application developers.

But I agree with you. If the request label does not have the user's identity, the application won't be able to provide the user with a useful response (for instance, even a bad request response, in case the request is missing a certain header, is not possible). But IIUC this is intentional, and the point about using IFC and enforcing unidirectional communication. I think the solution is to instruct the application developers to make sure that the clients they develop include the user's identity as a confidentiality tag in the request label. I believe, disjunctions are needed to allow/simplify this.

conradgrobler · 2020-10-23T08:39:13Z

oak_runtime/src/node/http/user.rs

+        &self,
+        runtime: &RuntimeProxy,
+        invocation_channel: oak_abi::Handle,
+    ) -> Result<(), ()> {


Result<(),()> does not seem like a particularly useful type as it does not really convey much information and the results do not seem to be used anywhere.

It allows using the ? operator. I am now using HttpError as the error type.

rbehjati · 2020-10-23T10:05:16Z

I was not involved in the original design discussions, so I am unsure on the background. Could you please add some information on why these label calculations are done in a separate node rather than in the HTTP server pseudo-node (similar to the current gRPC pseudo-node implementation)?

According to the docs, the integrity component of the request receiver channel must be set to the identity of the user. However, the HTTP server pseudo-Node cannot create a channel with a non-empty integrity component. The user node has the privilege that allows it to create such channels. Does the gRPC implementation actually set the integrity of the request receiver channel to the user's identity? Do we have examples that show that it works?

I had a discussion with @tiziano88 and realized that the OuterHttpInvocation that the HTTP server pseudo-node sends to the UserNode needs to be changed, as currently it publicly exposes the request.

The other issue is the one that I am experimenting with in #1614. If the request label is non-empty, the Oak node won't be able to read the request. This means that neither the before approach nor the new approach work!

conradgrobler · 2020-10-23T11:12:38Z

According to the docs, the integrity component of the request receiver channel must be set to the identity of the user. However, the HTTP server pseudo-Node cannot create a channel with a non-empty integrity component. The user node has the privilege that allows it to create such channels. Does the gRPC implementation actually set the integrity of the request receiver channel to the user's identity? Do we have examples that show that it works?

Integrity label support is not currently implemented for the gRPC server node, but it should be possible to do so. Seeing that the gRPC and HTTP server pseudo-nodes are part of the runtime and therefore the TCB they should in effect have the "top" integrity label. Therefore if we had a way of representing "top" as a label, it would resolve the issue. That seems to me to be a cleaner approach than creating a new node that gets assigned per-user integrity by the runtime. In both cases the runtime must be trusted to create the appropriate label, but in the case of the extra node there is significant overhead without clear security or safety improvements.

I had a discussion with @tiziano88 and realized that the OuterHttpInvocation that the HTTP server pseudo-node sends to the UserNode needs to be changed, as currently it publicly exposes the request.

I agree.

The other issue is the one that I am experimenting with in #1614. If the request label is non-empty, the Oak node won't be able to read the request. This means that neither the before approach nor the new approach work!

Any user-specific label on the request requires a "router" pattern where the router node (public) would create a new node with the right label and forward the invocation to the new node which should handle the request and produce the response.

rbehjati · 2020-10-23T12:03:16Z

Integrity label support is not currently implemented for the gRPC server node, but it should be possible to do so. Seeing that the gRPC and HTTP server pseudo-nodes are part of the runtime and therefore the TCB they should in effect have the "top" integrity label. Therefore if we had a way of representing "top" as a label, it would resolve the issue. That seems to me to be a cleaner approach than creating a new node that gets assigned per-user integrity by the runtime. In both cases the runtime must be trusted to create the appropriate label, but in the case of the extra node there is significant overhead without clear security or safety improvements.

What do you mean by "top" integrity label? Currently, the default choice for labelling the new nodes and channels is public_untrusted. Are you suggesting that is should be changed to public_fully_trusted (for those nodes and channels that belong to the TCB)?

I agree that that would be a more elegant solution, but I don't know what are its implications for our security guarantees, and NI proofs.

Any user-specific label on the request requires a "router" pattern where the router node (public) would create a new node with the right label and forward the invocation to the new node which should handle the request and produce the response.

Currently (with public_untrusted being the default label, and the label of the router node), this would not work in general. We may need intermediate nodes with specific privileges to be able to create user/request-specific Oak nodes with the right labels.

conradgrobler · 2020-10-23T12:26:28Z

What do you mean by "top" integrity label? Currently, the default choice for labelling the new nodes and channels is public_untrusted. Are you suggesting that is should be changed to public_fully_trusted (for those nodes and channels that belong to the TCB)?

Yes.

I agree that that would be a more elegant solution, but I don't know what are its implications for our security guarantees, and NI proofs.

I don't think it is a problem. The TCB is fully trusted, so to explicitly assign a "fully_trusted" integrity label to parts of it should be fine.

Any user-specific label on the request requires a "router" pattern where the router node (public) would create a new node with the right label and forward the invocation to the new node which should handle the request and produce the response.

Currently (with public_untrusted being the default label, and the label of the router node), this would not work in general. We may need intermediate nodes with specific privileges to be able to create user/request-specific Oak nodes with the right labels.

I don't think intermediate nodes will help. The untrusted intial node will not be able to create channels to communicate with the more trusted intermediate nodes (and probably not be able to create these nodes either). If there is some exception that allows it, it breaks IFC (or implicitly grants the router "fully_trusted" integrity). The router node also needs to be "public_fully_trusted" to function correctly. As an aside, this is part of the reason for the configuration-based router node RFC: a well-reviewed, reusable node that can act in a fully trusted way.

Integrity tags in general are currently still problematic. E.g. we don't have a way to create the initial Wasm node with any level of trusted integrity (apart from perhaps its own Wasm hash), so it severely limits the propagation of integrity labels through the system.

rbehjati · 2020-10-23T12:41:42Z

I don't think intermediate nodes will help. The untrusted intial node will not be able to create channels to communicate with the more trusted intermediate nodes (and probably not be able to create these nodes either).

The intermediate nodes won't be more trusted, but they'd have extra privileges. I don't think we have any IFC rules that restrict the privileges of a node. At least currently, while we still have separate privileges instead of robust declassification and transparent endorsement.

conradgrobler · 2020-10-23T13:02:44Z

The intermediate nodes won't be more trusted, but they'd have extra privileges. I don't think we have any IFC rules that restrict the privileges of a node. At least currently, while we still have separate privileges instead of robust declassification and transparent endorsement.

Good point. This is the case currently. But it seems that is still does not solve the problem. We can't currently assign arbitrary privilege to any nodes outside of the TCB, so these nodes must be generic nodes in the TCB. How will they decide whether or not they should create the more trusted nodes? If they always just create the nodes that they are told to create, it is equivalent to just granting the privilege to the original node that creates the intermediate node. Then just granting "fully_trusted" privilege to the original node is a simpler, but security-wise equivalent solution.

rbehjati · 2020-10-23T13:32:24Z

Then just granting "fully_trusted" privilege to the original node is a simpler, but security-wise equivalent solution.

Good point :)

rbehjati · 2020-10-23T15:06:19Z

@conradgrobler @daviddrysdale @ipetr0v @tiziano88 Following the discussions on Slack I think it is best to abandon this PR for now. If I leave out the integrity labels, then it would be possible to implement the first increment of the challenge-response without the virtual UserNode (or a node with fully_trusted integrity label). So, the PR could split into two.

tiziano88 · 2020-10-23T15:45:27Z

I agree, I think this may be split even further:

implement proper privilege for the bearer token auth (though this may be throwaway work if we decide to remove it)
implement challenge / response mechanism, and assign privilege correctly (only for confidentiality labels)
separately look into integrity labels (I think this is not a priority for now)
separately look into splitting nodes per user (or we can discuss on an issue / slack thread if necessary)
fix tests related to assigning labels based on HTTP headers (already started in Adds a new HTTP test with a non-empty request label #1614)

tiziano88 · 2020-10-26T21:32:05Z

oak_abi/proto/label.proto

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


As I am rewriting the chat app to use labels, it turns out that we use public keys to represent things other than users (e.g. chat rooms), so I think we should use a more generic terminology, so perhaps just use PublicKeyIdentityTag or similar?

Thanks. I'll change the name.

I have changed the name in my new PR, but I now wonder if PublicKeyIdentityTag still corresponds to the user sub-lattice. I guess there should be a one-to-one correspondence between the tag types and the principals. Or do we consider chat room to have the same principal nature as users?

rbehjati

Thanks for the reviews. I have created a new PR (#1652) that leaves out setting the user's identity in the integrity label of the request channel. I have applied the comments in the new PR.

rbehjati · 2020-10-30T16:51:37Z

oak_runtime/src/node/http/user.rs

+        let tag = match config.privilege.is_empty() {
+            true => None,
+            false => Some(oak_abi::label::tag::Tag::UserIdentityTag(UserIdentityTag {
+                public_key: config.privilege,


UserNode is removed from my new PR. So this issue should be resolved.

rbehjati · 2020-10-30T17:22:57Z

oak_runtime/src/node/http/user.rs

+            handle: response_reader,
+        });
+        let response = response_receiver.receive(&runtime).map_err(|err| {
+            error!(


I like the idea of using anyhow, but anyhow::Context is only implemented for Result types that use StdError as the error type. To be able to use anyhow::Context here, we have to wrap the OakStatus in an anyhow::Error. I also noticed anyhow is not used in the oak_runtime. Not sure if it is something in our backlog, or if the plan is to avoid anyhow in the oak_runtime.

In my new PR, I have added a specific error type (HttpError), and have replaced all such uses of map_err.

rbehjati · 2020-10-30T17:24:50Z

oak_runtime/src/node/http/user.rs

+        startup_handle: oak_abi::Handle,
+        _notify_receiver: oneshot::Receiver<()>,
+    ) {
+        let _unit_result = self.try_run(runtime, startup_handle);


I did this to be able to use ? in try_run instead of using a match or if let for every statement in the function. This is changed now, but I think in general using anyhow or a specific error type can fix this issue.

rbehjati · 2020-10-31T12:41:36Z

oak_runtime/src/node/http/user.rs

+                .collect(),
+        };
+        let response_writer_label = Label {
+            confidentiality_tags: privilege


I had a chat with @tiziano88 and I think it is correct to set the confidentiality tag of the response channel to the user's identity regardless of the request label. Checking the label should then be done in the Router node, implemented by the application developers.

But I agree with you. If the request label does not have the user's identity, the application won't be able to provide the user with a useful response (for instance, even a bad request response, in case the request is missing a certain header, is not possible). But IIUC this is intentional, and the point about using IFC and enforcing unidirectional communication. I think the solution is to instruct the application developers to make sure that the clients they develop include the user's identity as a confidentiality tag in the request label. I believe, disjunctions are needed to allow/simplify this.

rbehjati · 2020-10-31T12:46:08Z

oak_runtime/src/node/http/user.rs

+        &self,
+        runtime: &RuntimeProxy,
+        invocation_channel: oak_abi::Handle,
+    ) -> Result<(), ()> {


It allows using the ? operator. I am now using HttpError as the error type.

rbehjati · 2020-11-02T14:30:31Z

oak_abi/proto/label.proto

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


I have changed the name in my new PR, but I now wonder if PublicKeyIdentityTag still corresponds to the user sub-lattice. I guess there should be a one-to-one correspondence between the tag types and the principals. Or do we consider chat room to have the same principal nature as users?

rbehjati · 2020-11-03T11:57:53Z

Replaced with #1652

google-cla bot added the cla: yes label Oct 14, 2020

rbehjati added the WIP Work in progress label Oct 14, 2020

rbehjati force-pushed the oak-1357-challenge-resp branch 6 times, most recently from 13d9fab to 333ada4 Compare October 20, 2020 12:05

rbehjati force-pushed the oak-1357-challenge-resp branch from a0bb0bc to 2352508 Compare October 21, 2020 19:28

rbehjati added 21 commits October 22, 2020 12:39

protos

78ac4c3

parse signature

b176f7a

labels

0b4c891

Add UserNode

fc4376f

Use UserNode in HttpServerNode

dbe4eae

fixed channels - test fails because of permission denied

d204d0d

Fixed privilege in the UserNode

7f90fa9

docs

1f3901f

re-enable tests

9e0af4d

refactoring

3ec1774

verify signature

b472f5a

use identity in abitest

2454df5

base64 signed challenge

1411d43

cleanup

7622dd6

working

963e413

cleanup

07575bf

formatting

3ef3409

remove unused include

f39b35a

test with non-empty label

25c2d0a

test with user-label

14d1e41

docs

7109974

rbehjati force-pushed the oak-1357-challenge-resp branch from 2352508 to 7109974 Compare October 22, 2020 11:39

rbehjati marked this pull request as ready for review October 22, 2020 12:46

rbehjati removed the WIP Work in progress label Oct 22, 2020

rbehjati requested review from tiziano88, daviddrysdale, ipetr0v and conradgrobler October 22, 2020 12:47

conradgrobler reviewed Oct 23, 2020

View reviewed changes

rbehjati mentioned this pull request Oct 23, 2020

Adds a new HTTP test with a non-empty request label #1614

Closed

tiziano88 reviewed Oct 26, 2020

View reviewed changes

rbehjati mentioned this pull request Nov 2, 2020

Use challenge response for authentication #1652

Merged

rbehjati commented Nov 2, 2020

View reviewed changes

rbehjati closed this Nov 3, 2020

ipetr0v removed their request for review November 3, 2020 14:01

rbehjati deleted the oak-1357-challenge-resp branch March 25, 2021 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use challenge response for authentication #1586

Use challenge response for authentication #1586

rbehjati commented Oct 14, 2020 •

edited

Loading

conradgrobler left a comment

conradgrobler Oct 23, 2020

rbehjati Oct 30, 2020

conradgrobler Oct 23, 2020

ipetr0v Oct 29, 2020

rbehjati Oct 30, 2020

conradgrobler Oct 23, 2020

rbehjati Oct 30, 2020

conradgrobler Oct 23, 2020

rbehjati Oct 31, 2020

conradgrobler Oct 23, 2020

rbehjati Oct 31, 2020

rbehjati commented Oct 23, 2020

conradgrobler commented Oct 23, 2020

rbehjati commented Oct 23, 2020

conradgrobler commented Oct 23, 2020

rbehjati commented Oct 23, 2020

conradgrobler commented Oct 23, 2020

rbehjati commented Oct 23, 2020

rbehjati commented Oct 23, 2020

tiziano88 commented Oct 23, 2020

tiziano88 Oct 26, 2020

rbehjati Oct 27, 2020

rbehjati Nov 2, 2020

rbehjati left a comment

rbehjati Oct 30, 2020

rbehjati Oct 30, 2020

rbehjati Oct 30, 2020

rbehjati Oct 31, 2020

rbehjati Oct 31, 2020

rbehjati Nov 2, 2020

rbehjati commented Nov 3, 2020

Use challenge response for authentication #1586

Use challenge response for authentication #1586

Conversation

rbehjati commented Oct 14, 2020 • edited Loading

Checklist

conradgrobler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbehjati commented Oct 23, 2020

conradgrobler commented Oct 23, 2020

rbehjati commented Oct 23, 2020

conradgrobler commented Oct 23, 2020

rbehjati commented Oct 23, 2020

conradgrobler commented Oct 23, 2020

rbehjati commented Oct 23, 2020

rbehjati commented Oct 23, 2020

tiziano88 commented Oct 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbehjati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbehjati commented Nov 3, 2020

rbehjati commented Oct 14, 2020 •

edited

Loading