Skip to content
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

security: Refactor some utility functions in TSI for better internal use #29728

Merged
merged 25 commits into from Nov 15, 2022

Conversation

zeromath
Copy link
Contributor

Factor out the function body of ssl_protector_protect, ssl_protector_protect_flush, and ssl_protector_unprotect to utility functions for internal use.

@ZhenLian

…r_protect_flush`, and `ssl_protector_unprotect` to utility functions for internal use.
@ZhenLian ZhenLian self-requested a review May 19, 2022 20:45
@ZhenLian ZhenLian assigned ZhenLian and unassigned markdroth May 19, 2022
@ZhenLian ZhenLian changed the title Refactor ssl security: Refactor some utility functions in TSI for better internal use May 19, 2022
src/core/tsi/ssl_transport_security.cc Outdated Show resolved Hide resolved
@@ -1053,6 +1053,272 @@ void ssl_tsi_test_do_handshake_with_custom_bio_pair() {
tsi_test_fixture_destroy(fixture);
}

absl::Status void do_handshake_helper(SSL** out_client, SSL** out_server) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this function used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only used in ssl_transport_security_util_test.cc.

test/core/tsi/ssl_transport_security_test.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@ZhenLian ZhenLian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making the changes, and apologize for the delay(sorry, really too many things on my plate now).
It looks good overall, except some minor items. I've made some comments on the test structure. Once that's solved, I can enable the tests for you :)

src/core/tsi/ssl_transport_security_util.cc Outdated Show resolved Hide resolved
src/core/tsi/ssl_transport_security_util.h Outdated Show resolved Hide resolved
src/core/tsi/ssl_transport_security_util.h Outdated Show resolved Hide resolved
src/core/tsi/ssl_transport_security_util.h Outdated Show resolved Hide resolved
test/core/tsi/ssl_transport_security_util_test.cc Outdated Show resolved Hide resolved
test/core/tsi/ssl_transport_security_util_test.cc Outdated Show resolved Hide resolved
test/core/tsi/ssl_transport_security_util_test.cc Outdated Show resolved Hide resolved
std::size_t server_buffer_offset;
};

TEST_P(FlowTest, TestWorkFlows) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The names could be more specific here...what kind of flow are we testing? Roughly how do we execute our tests? You can put some comments above to make it more clear...

test/core/tsi/ssl_transport_security_util_test.cc Outdated Show resolved Hide resolved
test/core/tsi/ssl_transport_security_util_test.cc Outdated Show resolved Hide resolved
src/core/tsi/ssl_transport_security_util.h Outdated Show resolved Hide resolved
src/core/tsi/ssl_transport_security_util.h Outdated Show resolved Hide resolved
src/core/tsi/ssl_transport_security_util.h Outdated Show resolved Hide resolved
src/core/tsi/ssl_transport_security_util.h Outdated Show resolved Hide resolved
src/core/tsi/ssl_transport_security_util.h Outdated Show resolved Hide resolved
test/core/tsi/ssl_transport_security_util_test.cc Outdated Show resolved Hide resolved
test/core/tsi/ssl_transport_security_util_test.cc Outdated Show resolved Hide resolved
/*writebuf2=*/0),
1);
SSL_set_bio(server_ssl, server_ssl_bio, server_ssl_bio);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one small nit: can we please remove all the unnecessary blank spaces in this PR? If you intend the blank space to be a "logical breakpoint", you can add a comment explaining what the next block will do. Otherwise I think it's safe to just remove these blank spaces, and same thing for all the other files.

@ZhenLian
Copy link
Contributor

ZhenLian commented Jul 6, 2022

@markdroth Hi Mark, would you mind taking a second look at the style&semantics of this change? Thank you so much!

@stale stale bot removed the disposition/stale label Nov 9, 2022
@zeromath
Copy link
Contributor Author

zeromath commented Nov 9, 2022

Looks like this is accidentally changing the version of the third_party/envoy-api submodule. You'll need to fix that before this can be merged.

They should be fixed now.

Copy link
Contributor

@ZhenLian ZhenLian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zeromath Thank you so much for the great work! The failing test seems irrelevant.
Merging the pull request now...

@ZhenLian ZhenLian merged commit 8984a26 into grpc:master Nov 15, 2022
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Nov 15, 2022
yijiem added a commit that referenced this pull request Nov 17, 2022
yijiem added a commit that referenced this pull request Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security bloat/none imported Specifies if the PR has been imported to the internal repository lang/c++ lang/core per-call-memory/neutral per-channel-memory/neutral release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants