alts: add gRPC TSI frame protector #3873

lizan · 2018-07-17T00:37:21Z

Signed-off-by: Lizan Zhou zlizan@google.com

Description:
2nd PR for #3429, add frame protector.

Risk Level: Low (not enabled in main)
Testing: bazel test //test/...
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Lizan Zhou <zlizan@google.com>

danzh2010

Looks good overall! Just a few nits. Thanks for this change!

danzh2010 · 2018-07-17T18:07:44Z

source/extensions/transport_sockets/alts/tsi_frame_protector.cc

+
+  // TODO(lizan): tune size later
+  unsigned char protected_buffer[16384];
+  size_t protected_buffer_size = sizeof(protected_buffer);


nits: make this a const and use it to initialize protected_buffer[]

danzh2010 · 2018-07-17T18:10:06Z

source/extensions/transport_sockets/alts/tsi_frame_protector.cc

+    input.drain(processed_message_size);
+  }
+
+  ASSERT(input.length() == 0);


Is this ASSERT redundant as the while condition guarantees input.length()==0 to exit the loop?

danzh2010 · 2018-07-17T18:11:30Z

source/extensions/transport_sockets/alts/tsi_frame_protector.cc

+
+  ASSERT(input.length() == 0);
+  size_t still_pending_size;
+  do {


nits: can you add some comments about the need of calling tsi_frame_protector_protect_flush()?

Is this addressed?

danzh2010 · 2018-07-17T18:13:34Z

source/extensions/transport_sockets/alts/tsi_frame_protector.cc

+
+  // TODO(lizan): Tune the buffer size.
+  unsigned char unprotected_buffer[16384];
+  size_t unprotected_buffer_size = sizeof(unprotected_buffer);


danzh2010 · 2018-07-17T18:17:18Z

source/extensions/transport_sockets/alts/tsi_frame_protector.cc

+
+  while (input.length() > 0) {
+    auto* message_bytes = reinterpret_cast<unsigned char*>(input.linearize(input.length()));
+    size_t unprotected_buffer_size_to_send = unprotected_buffer_size;


The name |unprotected_buffer_size_to_send| is a confusing. I think unprotect() should be called on receiving path.

danzh2010 · 2018-07-17T18:29:30Z

test/extensions/transport_sockets/alts/tsi_frame_protector_test.cc

+
+  {
+    Buffer::OwnedImpl input, encrypted;
+    input.add(std::string(20000, 'a'));


If you have a const for protected_buffer_size, you can use a relative larger number here instead of hard coding 20000.

It is a coincidence that the frame size generated by fake frame protector is max at 16K and the buffer size is 16K, the hard code here is not reflecting the buffer size.

I think this test case is testing protect() with |input| size larger than the buffer. So this line can be something more general, like: input.add(std::string(BUFFER_SIZE + 3620, 'a')); Am I misunderstanding?

The frame size of fake frame protector output is define here, it is coincidence that is same as BUFFER_SIZE, but the test should be larger than the frame size, it is not depends on BUFFER_SIZE.

Why is it testing against frame size which is not our implementation? I thought you meant to test protect() with input larger than |protected_buffer| which I think is worth to add if not covered yet.

I meant the expected output below (L63) is depends on frame size in fake frame protector, the input here is to exceed the BUFFER_SIZE and they are just happened to be same.

We don't have visibility of BUFFER_SIZE in test.cc so I'll have to define BUFFER_SIZE, input.add(std::string(BUFFER_SIZE + 3620, 'a')) won't work anyway.

danzh2010 · 2018-07-17T18:41:10Z

test/extensions/transport_sockets/alts/tsi_frame_protector_test.cc

+    input.add("foo");
+
+    EXPECT_EQ(TSI_OK, frame_protector_.protect(input, encrypted));
+    EXPECT_EQ("\x07\0\0\0foo"s, encrypted.toString());


nits: comment where the prefix "\x07\0\0\0" comes from? I guess fake_frame_protector has a contract about how to generate it.

There is comment at top of this file (L23)

Signed-off-by: Lizan Zhou <zlizan@google.com>

danzh2010

LGTM

alyssawilk

Sweeet, glad to see this progress! Just a few nits from me

alyssawilk · 2018-07-19T19:48:02Z

source/extensions/transport_sockets/alts/tsi_frame_protector.cc

+
+  // TSI may buffer some of the input in its internal, flushing its buffer to protected_buffer to
+  // avoid some of input is not written to socket.
+  size_t still_pending_size;


// TSI may buffer some of the input internally. Flush its buffer to protected_buffer.
?

alyssawilk · 2018-07-19T19:48:34Z

source/extensions/transport_sockets/alts/tsi_frame_protector.cc

+    auto* message_bytes = reinterpret_cast<unsigned char*>(input.linearize(input.length()));
+    size_t unprotected_buffer_size = BUFFER_SIZE;
+    size_t processed_message_size = input.length();
+    tsi_result result = tsi_frame_protector_unprotect(frame_protector_.get(), message_bytes,


Sanity check, there's no flushing problem in this direction?

No, there is no tsi_frame_protector_unprotect_flush.

alyssawilk · 2018-07-19T19:51:25Z

source/extensions/transport_sockets/alts/tsi_frame_protector.h

+
+  /**
+   * Wrapper for tsi_frame_protector_protect
+   * @param input supplies the input buffer, the method will drain it when it is processed.


input supplies the input buffer -> input supplies the data to protect.

ditto below for unprotect

alyssawilk · 2018-07-19T19:51:55Z

source/extensions/transport_sockets/alts/tsi_frame_protector.h

+  /**
+   * Wrapper for tsi_frame_protector_protect
+   * @param input supplies the input buffer, the method will drain it when it is processed.
+   * @param output supplies the output buffer


output supplies the buffer where the protected data will be stored.

Ditto for unprotect

alyssawilk · 2018-07-19T19:53:08Z

source/extensions/transport_sockets/alts/tsi_frame_protector.cc

+                                    protected_buffer, &protected_buffer_size);
+    if (result != TSI_OK) {
+      ASSERT(result != TSI_INVALID_ARGUMENT && result != TSI_UNIMPLEMENTED);
+      return result;


We should test error handling please.

https://73638-65214191-gh.circle-artifacts.com/0/build/envoy/generated/coverage/coverage.source_extensions_transport_sockets_alts_tsi_frame_protector.cc.html

Signed-off-by: Lizan Zhou <zlizan@google.com>

alts: add gRPC TSI frame protector

44108f3

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan self-assigned this Jul 17, 2018

lizan requested a review from danzh2010 July 17, 2018 00:37

danzh2010 reviewed Jul 17, 2018

View reviewed changes

lizan added 4 commits July 17, 2018 17:58

address comments, simplify code

a1a0577

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into alts_frame_protector

eae6944

fix format

115f4da

Signed-off-by: Lizan Zhou <zlizan@google.com>

address comments

52570fe

Signed-off-by: Lizan Zhou <zlizan@google.com>

danzh2010 previously approved these changes Jul 19, 2018

View reviewed changes

lizan requested a review from alyssawilk July 19, 2018 19:24

alyssawilk reviewed Jul 19, 2018

View reviewed changes

lizan added 2 commits July 19, 2018 14:29

update comments

9e2c420

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into alts_frame_protector

c0711c0

lizan dismissed danzh2010’s stale review via c0711c0 July 19, 2018 21:29

lizan added 2 commits July 19, 2018 14:43

typo

cb8bbd6

Signed-off-by: Lizan Zhou <zlizan@google.com>

test error handling

7c14028

Signed-off-by: Lizan Zhou <zlizan@google.com>

alyssawilk approved these changes Jul 23, 2018

View reviewed changes

danzh2010 approved these changes Jul 23, 2018

View reviewed changes

alyssawilk merged commit a5478ee into envoyproxy:master Jul 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

alts: add gRPC TSI frame protector #3873

alts: add gRPC TSI frame protector #3873

lizan commented Jul 17, 2018

danzh2010 left a comment

danzh2010 Jul 17, 2018

danzh2010 Jul 17, 2018

danzh2010 Jul 17, 2018

danzh2010 Jul 18, 2018

danzh2010 Jul 17, 2018

danzh2010 Jul 17, 2018

danzh2010 Jul 17, 2018

lizan Jul 18, 2018

danzh2010 Jul 18, 2018

lizan Jul 18, 2018

danzh2010 Jul 18, 2018

lizan Jul 18, 2018

danzh2010 Jul 17, 2018

lizan Jul 18, 2018

danzh2010 left a comment

alyssawilk left a comment

alyssawilk Jul 19, 2018

alyssawilk Jul 19, 2018

lizan Jul 19, 2018

alyssawilk Jul 19, 2018

alyssawilk Jul 19, 2018

alyssawilk Jul 19, 2018

lizan Jul 19, 2018

alts: add gRPC TSI frame protector #3873

alts: add gRPC TSI frame protector #3873

Conversation

lizan commented Jul 17, 2018

danzh2010 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

danzh2010 left a comment

Choose a reason for hiding this comment

alyssawilk 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