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

Add missing security field to channelz Socket #25593

Merged
merged 5 commits into from
Mar 4, 2021

Conversation

yashykt
Copy link
Member

@yashykt yashykt commented Mar 2, 2021

As per

Security security = 5;
, Sockets should expose security information.

Currently, we are able to get the remote's peer certificate from grpc_auth_context once the security handshake is done, but we don't have any other information. This PR exposes the remote's peer certificates as part of the channelz's Socket information.

cc @markdroth @ZhenLian


This change is Reviewable

@yashykt yashykt closed this Mar 2, 2021
@yashykt yashykt reopened this Mar 2, 2021
Copy link
Contributor

@veblush veblush left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @yashykt)


src/core/ext/transport/chttp2/transport/chttp2_transport.cc, line 366 at r1 (raw file):

  }
  if (channelz_enabled) {
    auto sec = grpc_core::channelz::SocketNode::Security::GetFromChannelArgs(

is sec used here?

Copy link
Member Author

@yashykt yashykt left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @veblush)


src/core/ext/transport/chttp2/transport/chttp2_transport.cc, line 366 at r1 (raw file):

Previously, veblush (Esun Kim) wrote…

is sec used here?

Removed. Thanks for catching that! Looks like it got left over from testing.

@yashykt yashykt merged commit abf1e9a into grpc:master Mar 4, 2021
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! Just a couple of minor comments.

Reviewed 5 of 6 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @veblush and @yashykt)


src/core/lib/channel/channelz.cc, line 376 at r2 (raw file):

    case ModelType::kOther:
      if (other) {
        data["other"] = tls->RenderJson();

Shouldn't this use *other instead of tls->RenderJson()?


test/cpp/end2end/channelz_service_test.cc, line 145 at r2 (raw file):

  }
  args->SetSslTargetNameOverride("foo.test.google.fr");
  // TODO(yashykt): Switch to using C++ API once b/173823806 is fixed.

Is that bug a problem here? We're not using the custom verification config here, unless I'm missing something.

Copy link
Member Author

@yashykt yashykt left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @markdroth and @veblush)


src/core/lib/channel/channelz.cc, line 376 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Shouldn't this use *other instead of tls->RenderJson()?

Thanks for catching this. Making this change in #25624


test/cpp/end2end/channelz_service_test.cc, line 145 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Is that bug a problem here? We're not using the custom verification config here, unless I'm missing something.

You are right. I just copied over the code from existing tests but this should not be needed here. Making the change in #25624

@yashykt yashykt deleted the channelzsecurity branch May 18, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/core release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants