-
Notifications
You must be signed in to change notification settings - Fork 5.5k
alts: add gRPC TSI frame protector #3873
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
Changes from 1 commit
44108f3
a1a0577
eae6944
115f4da
52570fe
9e2c420
c0711c0
cb8bbd6
7c14028
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| #include "extensions/transport_sockets/alts/tsi_frame_protector.h" | ||
|
|
||
| #include "common/common/assert.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace TransportSockets { | ||
| namespace Alts { | ||
|
|
||
| TsiFrameProtector::TsiFrameProtector(CFrameProtectorPtr&& frame_protector) | ||
| : frame_protector_(std::move(frame_protector)) {} | ||
|
|
||
| tsi_result TsiFrameProtector::protect(Buffer::Instance& input, Buffer::Instance& output) { | ||
| ASSERT(frame_protector_); | ||
|
|
||
| // TODO(lizan): tune size later | ||
| unsigned char protected_buffer[16384]; | ||
| size_t protected_buffer_size = sizeof(protected_buffer); | ||
| while (input.length() > 0) { | ||
| auto* message_bytes = reinterpret_cast<unsigned char*>(input.linearize(input.length())); | ||
| size_t protected_buffer_size_to_send = protected_buffer_size; | ||
| size_t processed_message_size = input.length(); | ||
| tsi_result result = | ||
| tsi_frame_protector_protect(frame_protector_.get(), message_bytes, &processed_message_size, | ||
| protected_buffer, &protected_buffer_size_to_send); | ||
| if (result != TSI_OK) { | ||
| ASSERT(result != TSI_INVALID_ARGUMENT && result != TSI_UNIMPLEMENTED); | ||
| return result; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| } | ||
| output.add(protected_buffer, protected_buffer_size_to_send); | ||
| input.drain(processed_message_size); | ||
| } | ||
|
|
||
| ASSERT(input.length() == 0); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this ASSERT redundant as the while condition guarantees input.length()==0 to exit the loop? |
||
| size_t still_pending_size; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. // TSI may buffer some of the input internally. Flush its buffer to protected_buffer. |
||
| do { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nits: can you add some comments about the need of calling tsi_frame_protector_protect_flush()?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this addressed? |
||
| size_t protected_buffer_size_to_send = protected_buffer_size; | ||
| tsi_result result = | ||
| tsi_frame_protector_protect_flush(frame_protector_.get(), protected_buffer, | ||
| &protected_buffer_size_to_send, &still_pending_size); | ||
| if (result != TSI_OK) { | ||
| ASSERT(result != TSI_INVALID_ARGUMENT && result != TSI_UNIMPLEMENTED); | ||
| return result; | ||
| } | ||
| output.add(protected_buffer, protected_buffer_size_to_send); | ||
| } while (still_pending_size > 0); | ||
|
|
||
| return TSI_OK; | ||
| } | ||
|
|
||
| tsi_result TsiFrameProtector::unprotect(Buffer::Instance& input, Buffer::Instance& output) { | ||
| ASSERT(frame_protector_); | ||
|
|
||
| // TODO(lizan): Tune the buffer size. | ||
| unsigned char unprotected_buffer[16384]; | ||
| size_t unprotected_buffer_size = sizeof(unprotected_buffer); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto. |
||
|
|
||
| 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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name |unprotected_buffer_size_to_send| is a confusing. I think unprotect() should be called on receiving path. |
||
| size_t processed_message_size = input.length(); | ||
| tsi_result result = tsi_frame_protector_unprotect(frame_protector_.get(), message_bytes, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sanity check, there's no flushing problem in this direction?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, there is no |
||
| &processed_message_size, unprotected_buffer, | ||
| &unprotected_buffer_size_to_send); | ||
| if (result != TSI_OK) { | ||
| ASSERT(result != TSI_INVALID_ARGUMENT && result != TSI_UNIMPLEMENTED); | ||
| return result; | ||
| } | ||
| output.add(unprotected_buffer, unprotected_buffer_size_to_send); | ||
| input.drain(processed_message_size); | ||
| } | ||
|
|
||
| return TSI_OK; | ||
| } | ||
|
|
||
| } // namespace Alts | ||
| } // namespace TransportSockets | ||
| } // namespace Extensions | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| #pragma once | ||
|
|
||
| #include "envoy/buffer/buffer.h" | ||
|
|
||
| #include "extensions/transport_sockets/alts/grpc_tsi.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace TransportSockets { | ||
| namespace Alts { | ||
|
|
||
| /** | ||
| * A C++ wrapper for tsi_frame_protector interface. | ||
| * For detail of tsi_frame_protector, see | ||
| * https://github.com/grpc/grpc/blob/v1.10.0/src/core/tsi/transport_security_interface.h#L70 | ||
| * | ||
| * TODO(lizan): migrate to tsi_zero_copy_grpc_protector for further optimization | ||
| */ | ||
| class TsiFrameProtector final { | ||
| public: | ||
| explicit TsiFrameProtector(CFrameProtectorPtr&& frame_protector); | ||
|
|
||
| /** | ||
| * Wrapper for tsi_frame_protector_protect | ||
| * @param input supplies the input buffer, the method will drain it when it is processed. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. input supplies the input buffer -> input supplies the data to protect. ditto below for unprotect |
||
| * @param output supplies the output buffer | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. output supplies the buffer where the protected data will be stored. Ditto for unprotect |
||
| * @return tsi_result the status. | ||
| */ | ||
| tsi_result protect(Buffer::Instance& input, Buffer::Instance& output); | ||
|
|
||
| /** | ||
| * Wrapper for tsi_frame_protector_unprotect | ||
| * @param input supplies the input buffer, the method will drain it when it is processed. | ||
| * @param output supplies the output buffer | ||
| * @return tsi_result the status. | ||
| */ | ||
| tsi_result unprotect(Buffer::Instance& input, Buffer::Instance& output); | ||
|
|
||
| private: | ||
| CFrameProtectorPtr frame_protector_; | ||
| }; | ||
|
|
||
| typedef std::unique_ptr<TsiFrameProtector> TsiFrameProtectorPtr; | ||
|
|
||
| } // namespace Alts | ||
| } // namespace TransportSockets | ||
| } // namespace Extensions | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| #include "common/buffer/buffer_impl.h" | ||
|
|
||
| #include "extensions/transport_sockets/alts/tsi_frame_protector.h" | ||
|
|
||
| #include "gmock/gmock.h" | ||
| #include "gtest/gtest.h" | ||
| #include "src/core/tsi/fake_transport_security.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace TransportSockets { | ||
| namespace Alts { | ||
|
|
||
| using testing::InSequence; | ||
| using testing::Invoke; | ||
| using testing::NiceMock; | ||
| using testing::SaveArg; | ||
| using testing::Test; | ||
| using testing::_; | ||
| using namespace std::string_literals; | ||
|
|
||
| /** | ||
| * Test with fake frame protector. The protected frame header is 4 byte length (little endian, | ||
| * include header itself) and following the body. | ||
| */ | ||
| class TsiFrameProtectorTest : public Test { | ||
| public: | ||
| TsiFrameProtectorTest() : frame_protector_({tsi_create_fake_frame_protector(nullptr)}) {} | ||
|
|
||
| protected: | ||
| TsiFrameProtector frame_protector_; | ||
| }; | ||
|
|
||
| TEST_F(TsiFrameProtectorTest, Protect) { | ||
| { | ||
| Buffer::OwnedImpl input, encrypted; | ||
| input.add("foo"); | ||
|
|
||
| EXPECT_EQ(TSI_OK, frame_protector_.protect(input, encrypted)); | ||
| EXPECT_EQ("\x07\0\0\0foo"s, encrypted.toString()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nits: comment where the prefix "\x07\0\0\0" comes from? I guess fake_frame_protector has a contract about how to generate it.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is comment at top of this file (L23) |
||
| } | ||
|
|
||
| { | ||
| Buffer::OwnedImpl input, encrypted; | ||
| input.add("foo"); | ||
|
|
||
| EXPECT_EQ(TSI_OK, frame_protector_.protect(input, encrypted)); | ||
| EXPECT_EQ("\x07\0\0\0foo"s, encrypted.toString()); | ||
|
|
||
| input.add("bar"); | ||
| EXPECT_EQ(TSI_OK, frame_protector_.protect(input, encrypted)); | ||
| EXPECT_EQ("\x07\0\0\0foo\x07\0\0\0bar"s, encrypted.toString()); | ||
| } | ||
|
|
||
| { | ||
| Buffer::OwnedImpl input, encrypted; | ||
| input.add(std::string(20000, 'a')); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you have a const for protected_buffer_size, you can use a relative larger number here instead of hard coding 20000.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, |
||
|
|
||
| EXPECT_EQ(TSI_OK, frame_protector_.protect(input, encrypted)); | ||
|
|
||
| // fake frameprotector will split long buffer to 2 "encrypted" frames with length 16K. | ||
| std::string expected = | ||
| "\0\x40\0\0"s + std::string(16380, 'a') + "\x28\x0e\0\0"s + std::string(3620, 'a'); | ||
| EXPECT_EQ(expected, encrypted.toString()); | ||
| } | ||
| } | ||
|
|
||
| TEST_F(TsiFrameProtectorTest, Unprotect) { | ||
| { | ||
| Buffer::OwnedImpl input, decrypted; | ||
| input.add("\x07\0\0\0bar"s); | ||
|
|
||
| EXPECT_EQ(TSI_OK, frame_protector_.unprotect(input, decrypted)); | ||
| EXPECT_EQ("bar", decrypted.toString()); | ||
| } | ||
|
|
||
| { | ||
| Buffer::OwnedImpl input, decrypted; | ||
| input.add("\x0a\0\0\0foo"s); | ||
|
|
||
| EXPECT_EQ(TSI_OK, frame_protector_.unprotect(input, decrypted)); | ||
| EXPECT_EQ("", decrypted.toString()); | ||
|
|
||
| input.add("bar"); | ||
| EXPECT_EQ(TSI_OK, frame_protector_.unprotect(input, decrypted)); | ||
| EXPECT_EQ("foobar", decrypted.toString()); | ||
| } | ||
|
|
||
| { | ||
| Buffer::OwnedImpl input, decrypted; | ||
| input.add("\0\x40\0\0"s + std::string(16380, 'a')); | ||
| input.add("\x28\x0e\0\0"s + std::string(3620, 'a')); | ||
|
|
||
| EXPECT_EQ(TSI_OK, frame_protector_.unprotect(input, decrypted)); | ||
| EXPECT_EQ(std::string(20000, 'a'), decrypted.toString()); | ||
| } | ||
| } | ||
|
|
||
| } // namespace Alts | ||
| } // namespace TransportSockets | ||
| } // namespace Extensions | ||
| } // namespace Envoy | ||
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.
nits: make this a const and use it to initialize protected_buffer[]