oak_runtime: add simple roughtime client #428

conradgrobler · 2020-01-07T11:45:27Z

This is the initial implementation of a roughtime client to provide
trusted time to server components inside an enclave.

This is the initial implementation of a roughtime client to provide trusted time to server components inside an enclave.

tiziano88 · 2020-01-07T16:41:11Z

WORKSPACE

+http_archive(
+    name = "roughtime",
+    build_file = "@//third_party/roughtime:roughtime.BUILD",
+    #sha256 = "47c99712b4b34a6713ea7ecfec6860e6b88ba046df206386346de4c3409fe6dd",


un-comment this?

This is a bit of a problem. Each time this is downloaded it generates a new archive, so the hash changes. I will look into other options, such as using new_git_repository.

Changed to using new_git_repository with shallow_since to make it deterministic.

tiziano88 · 2020-01-07T16:41:31Z

WORKSPACE

+# Roughtime
+http_archive(
+    name = "roughtime",
+    build_file = "@//third_party/roughtime:roughtime.BUILD",


Suggested change

build_file = "@//third_party/roughtime:roughtime.BUILD",

build_file = "//third_party/roughtime:roughtime.BUILD",

tiziano88 · 2020-01-07T16:44:33Z

oak/server/time/roughtime_client.cc

+// Number of seconds that we will wait for a reply from the server.
+constexpr int kTimeoutSeconds = 3;
+
+// Number of times we will retry connecting to the server.


Suggested change

// Number of times we will retry connecting to the server.

// Number of times we will retry connecting to each server.

tiziano88 · 2020-01-07T16:44:39Z

oak/server/time/roughtime_client.cc

+// The minimum number of overlapping intervals we need to trust the time.
+constexpr int kMinOverlappingTimeIntervals = 2;
+
+// Number of seconds that we will wait for a reply from the server.


Suggested change

// Number of seconds that we will wait for a reply from the server.

// Number of seconds that we will wait for a reply from each server.

tiziano88 · 2020-01-07T16:45:31Z

oak/server/time/roughtime_client.cc

+constexpr size_t kReceiveBufferSize = roughtime::kMinRequestSize;
+
+// Maximum radius accepted for a roughtime response (1 minute in microseconds).
+constexpr uint32_t kMaxRadius = 60000000;


Rename to something that makes it clear what the unit is (e.g. *Microseconds)

tiziano88 · 2020-01-07T16:49:50Z

oak/server/time/roughtime_client.cc

+          &timestamp, &radius, &error, reinterpret_cast<const uint8_t*>(public_key.data()),
+          reinterpret_cast<const uint8_t*>(response.data()), response.size(), nonce.data())) {
+    return Status(asylo::error::GoogleError::INTERNAL,
+                  absl::StrFormat("Response from %s failed verification: %s", server.name.c_str(),


maybe s/failed verification/could not be parsed/?

tiziano88 · 2020-01-07T16:51:48Z

oak/server/time/roughtime_client.cc

+  Nonce<roughtime::kNonceLength> nonce = generator.NextNonce();
+  ASYLO_ASSIGN_OR_RETURN(response, SendRequest(server, nonce));
+  roughtime::rough_time_t timestamp;
+  uint32_t radius;


is this in seconds? could you clarify in the symbol name or in a comment?

tiziano88

LGTM, but I don't trust myself that much reviewing C++ code :) Adding @daviddrysdale as another pair of eyes, please wait for his approval too.

tiziano88 · 2020-01-09T14:51:30Z

WORKSPACE

@@ -297,3 +297,12 @@ http_archive(
    strip_prefix = "clang+llvm-8.0.0-x86_64-linux-gnu-ubuntu-18.04",
    url = "http://releases.llvm.org/8.0.0/clang+llvm-8.0.0-x86_64-linux-gnu-ubuntu-18.04.tar.xz",
 )
+
+# Roughtime
+new_git_repository(


What do we need from new_git_repository that git_repository does not support? I thought new was added during a transition period, but now it's fine to use the non-new version?

The build_file attribute is not supported on git_repository and I need a custom build file to change the visibility of the roughtime targets.

Ooh, that might be useful for some other things too…

daviddrysdale

Only a partial review, I'm afraid, but I thought it was worth sending out what I've got so far – I'll finish off later today.

daviddrysdale · 2020-01-10T07:46:55Z

WORKSPACE

@@ -297,3 +297,12 @@ http_archive(
    strip_prefix = "clang+llvm-8.0.0-x86_64-linux-gnu-ubuntu-18.04",
    url = "http://releases.llvm.org/8.0.0/clang+llvm-8.0.0-x86_64-linux-gnu-ubuntu-18.04.tar.xz",
 )
+
+# Roughtime
+new_git_repository(


Ooh, that might be useful for some other things too…

daviddrysdale · 2020-01-10T07:47:59Z

oak/server/time/BUILD

+)
+
+test_suite(
+    name = "host_tests",


W00t, tests. Could you update the collection of targets in scripts/run_tests to hit this too?

This test requires an internet connection and the Google and Clouflare roughtime servers to be up, so could lead to flakiness. For now I added it only for my own local testing.

Ah yes, good point.

daviddrysdale · 2020-01-10T07:53:23Z

oak/server/time/roughtime_client.h

+namespace oak {
+
+// Client for getting roughtime (in microseconds since Unix epoch) from multiple servers.
+// Based on https://roughtime.googlesource.com/roughtime/+/refs/heads/master/simple_client.cc


Should this be under third_party/ with a pristine upstream commit for provenance?

I am unsure. This differs very significantly from the original file. The socket code is mostly identical, but apart from that it is more a case of 'inspired by' than 'copied from'. It is also additionally inspired by the go code in a subdirectory.

Should I add a pristine copy of the entire roughtime repository into third_party/ and then after the commit add a header and make simple_client a library rather than a cli?

Probably OK as-is then (but if you were to do the third_party thing I don't think there's any need to copy the whole repo, just the bits that are used as a basis of the code – cf. 93902e6 & b3e20cb)

daviddrysdale · 2020-01-10T07:55:52Z

oak/server/time/roughtime_client.h

+#ifndef OAK_SERVER_TIME_ROUGHTIME_CLIENT_H_
+#define OAK_SERVER_TIME_ROUGHTIME_CLIENT_H_
+
+#include <stdint.h>


nit: Is this needed?

daviddrysdale · 2020-01-10T07:56:52Z

oak/server/time/BUILD

+    deps = [
+        "//oak/common:nonce_generator",
+        "@com_google_asylo//asylo/util:status",
+        "@roughtime//:client",


I think this is missing some direct deps (absl dependencies, Asylo logging).

daviddrysdale · 2020-01-10T07:58:33Z

oak/server/time/roughtime_client_test.cc

+
+namespace oak {
+
+TEST(RoughtimeClient, TestGetRoughtTime) {


nit: extra 't' in RoughtTime

daviddrysdale · 2020-01-10T07:59:08Z

oak/server/time/roughtime_client.h

+
+namespace oak {
+
+// Client for getting roughtime (in microseconds since Unix epoch) from multiple servers.


Which multiple servers? Are there some hardcoded in the underlying roughtime library?

These are currently hardcoded in roughtime_client.cc

Maybe add a note about that to the comment?

daviddrysdale · 2020-01-10T11:01:47Z

oak/server/time/roughtime_client.cc

+// Creates a UDP socket connected to the host and port.
+StatusOr<int> CreateSocket(const std::string& host, const std::string& port) {
+  addrinfo hints = {};
+  hints.ai_socktype = SOCK_DGRAM;


nit: hints.ai_family = AF_UNSPEC; explicitly?

daviddrysdale · 2020-01-10T11:06:38Z

oak/server/time/roughtime_client.cc

+  }
+
+  auto handle = create_socket_result.ValueOrDie();
+  const std::string request = roughtime::CreateRequest(nonce.data());


nit: move this line down to after the setsockopt call, nearer to first use.

daviddrysdale · 2020-01-10T11:07:24Z

oak/server/time/roughtime_client.cc

+  do {
+    send_size = send(handle, request.data(), request.size(), 0 /* flags */);
+  } while (send_size == -1 && errno == EINTR);
+  if (send_size < 0 || static_cast<size_t>(send_size) != request.size()) {


nit: could this be simpler as something like if (send_size != static_cast<ssize_t>(request.size())) { (moving the cast so the second arm encompasses the first)

daviddrysdale · 2020-01-10T11:11:09Z

oak/server/time/roughtime_client.cc

+                  "Network error on sending request to " + server.name);
+  }
+
+  uint8_t receive_buffer[kReceiveBufferSize];


nit: would making this const char rather than uint8 allow the reinterpret_cast below to be removed?

daviddrysdale · 2020-01-10T11:16:25Z

oak/server/time/roughtime_client.cc

+    return Status(asylo::error::GoogleError::INVALID_ARGUMENT,
+                  absl::StrFormat("Public key for server %s is not a valid base64 encoding.",
+                                  server.name.c_str()));
+  }


nit: maybe move l.163-l.168 up to the top of the function, so an invalid argument is detected before a request/response happens?

daviddrysdale · 2020-01-10T11:16:52Z

oak/server/time/roughtime_client.cc

+  ASYLO_ASSIGN_OR_RETURN(response, SendRequest(server, nonce));
+  roughtime::rough_time_t timestamp_microseconds;
+  uint32_t radius_microseconds;
+  std::string error;


nit: move these 3 declarations down closer to first use?

daviddrysdale · 2020-01-10T11:19:07Z

oak/server/time/roughtime_client.cc

+
+StatusOr<RoughtimeInterval> FindOverlap(const std::vector<RoughtimeInterval>& intervals,
+                                        const int min_overlap) {
+  for (auto interval : intervals) {


nit: const auto& might be better (http://go/totw/44), here and below.

daviddrysdale · 2020-01-10T11:45:52Z

oak/server/time/roughtime_client_test.cc

+  auto current = std::chrono::duration_cast<std::chrono::microseconds>(
+                     std::chrono::system_clock::now().time_since_epoch())
+                     .count();
+  auto time_or_status = RoughtimeClient::GetRoughTime();


BTW, is this test "live"? (i.e. would it work on a machine not connected to the internet?

This test requires an internet connection and both the Google and Cloudflare roughtime servers to be up and working.

Maybe add a comment and/or change the test name slightly ("..Live") so if someone runs it in isolation and it fails, they get a clue as to why.

daviddrysdale · 2020-01-10T11:46:19Z

oak/server/time/roughtime_client.cc

+                           (timestamp_microseconds + radius_microseconds)};
+}
+
+StatusOr<RoughtimeInterval> FindOverlap(const std::vector<RoughtimeInterval>& intervals,


Could we get unit tests for this at some point (not necessarily now)?

I am unsure of the best approach without unnecessarily increasing the visibility. Should I make this a private static method and use FRIEND_TEST, or should I move this to a separate library (eg roughtime_util.h) with only package level visibility? Or is there a better option?

daviddrysdale · 2020-01-10T15:44:59Z

oak/server/time/BUILD

+)
+
+test_suite(
+    name = "host_tests",


Ah yes, good point.

daviddrysdale · 2020-01-10T15:46:16Z

oak/server/time/roughtime_client_test.cc

+  auto current = std::chrono::duration_cast<std::chrono::microseconds>(
+                     std::chrono::system_clock::now().time_since_epoch())
+                     .count();
+  auto time_or_status = RoughtimeClient::GetRoughTime();


Maybe add a comment and/or change the test name slightly ("..Live") so if someone runs it in isolation and it fails, they get a clue as to why.

daviddrysdale · 2020-01-10T15:46:48Z

oak/server/time/roughtime_client.h

+
+namespace oak {
+
+// Client for getting roughtime (in microseconds since Unix epoch) from multiple servers.


Maybe add a note about that to the comment?

daviddrysdale · 2020-01-10T15:51:18Z

oak/server/time/roughtime_client.h

+namespace oak {
+
+// Client for getting roughtime (in microseconds since Unix epoch) from multiple servers.
+// Based on https://roughtime.googlesource.com/roughtime/+/refs/heads/master/simple_client.cc


Probably OK as-is then (but if you were to do the third_party thing I don't think there's any need to copy the whole repo, just the bits that are used as a basis of the code – cf. 93902e6 & b3e20cb)

oak_runtime: add simple roughtime client

d3d8582

This is the initial implementation of a roughtime client to provide trusted time to server components inside an enclave.

googlebot added the cla: yes label Jan 7, 2020

Clean up commented-out code.

3aea31e

conradgrobler added the WIP Work in progress label Jan 7, 2020

conradgrobler added 3 commits January 7, 2020 13:09

Implement retries and overlapping intervals

08db857

Add informational logging

5c4fac8

Fix formatting

09510d5

conradgrobler marked this pull request as ready for review January 7, 2020 16:34

conradgrobler requested a review from tiziano88 January 7, 2020 16:38

tiziano88 reviewed Jan 7, 2020

View reviewed changes

Address code review comments

833d2b9

conradgrobler removed the WIP Work in progress label Jan 8, 2020

conradgrobler requested a review from tiziano88 January 9, 2020 12:54

tiziano88 requested a review from daviddrysdale January 9, 2020 14:52

tiziano88 approved these changes Jan 9, 2020

View reviewed changes

daviddrysdale reviewed Jan 10, 2020

View reviewed changes

daviddrysdale approved these changes Jan 10, 2020

View reviewed changes

Address review comments

c499255

daviddrysdale approved these changes Jan 10, 2020

View reviewed changes

Respond to additional review comments

a782ad3

conradgrobler merged commit 1869878 into project-oak:master Jan 10, 2020

conradgrobler deleted the roughtime branch January 10, 2020 16:59

daviddrysdale mentioned this pull request Mar 20, 2020

Support Roughtime from Rust #730

Closed

	build_file = "@//third_party/roughtime:roughtime.BUILD",
	build_file = "//third_party/roughtime:roughtime.BUILD",

	// Number of times we will retry connecting to the server.
	// Number of times we will retry connecting to each server.

	// Number of seconds that we will wait for a reply from the server.
	// Number of seconds that we will wait for a reply from each server.


		namespace oak {

		// Client for getting roughtime (in microseconds since Unix epoch) from multiple servers.

oak_runtime: add simple roughtime client #428

oak_runtime: add simple roughtime client #428

Conversation

conradgrobler commented Jan 7, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiziano88 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

daviddrysdale 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

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

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

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