-
Notifications
You must be signed in to change notification settings - Fork 77
Audit log in failed connection cases #1281
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
Open
ryanwixon-emerson
wants to merge
20
commits into
ni:main
Choose a base branch
from
ryanwixon-emerson:failureLogging
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 12 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
f55be57
Log in successful connection cases
ryanwixon-emerson 6073f3b
Match source name from installer
ryanwixon-emerson c85a85b
Improve log message formatting and ditch set approach
ryanwixon-emerson e33fe95
Update README to document registry key requirement
ryanwixon-emerson 4f60dcd
Log on failure (not finished)
ryanwixon-emerson 1d2ce26
Review fixes
ryanwixon-emerson 635f5d7
Fix typo in README
ryanwixon-emerson 422f414
More review fixes (needs test)
ryanwixon-emerson cf4fd28
Documentation update for review
ryanwixon-emerson 3c77e46
Remove severity filter
ryanwixon-emerson 05ea291
Merge branch 'successLogging' of https://github.com/ryanwixon-emerson…
ryanwixon-emerson 59ee8ac
Fix typo from merge
ryanwixon-emerson 8068587
RAII wrapper for sink
ryanwixon-emerson 06656d5
Merge branch 'main' of https://github.com/ni/grpc-device into failure…
ryanwixon-emerson 09efcb4
Trim comment
ryanwixon-emerson ecf700d
Rearrange log initialization
ryanwixon-emerson 1ea4701
Empty commit, retry Windows build
ryanwixon-emerson 12e04a4
Empty commit, retry Windows build again
ryanwixon-emerson 8d39d5a
Addess review comments
ryanwixon-emerson 89e8bfc
Tweak sink source name
ryanwixon-emerson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| #include "client_connection_logger.h" | ||
| #include "logging.h" | ||
|
|
||
| #include <absl/log/globals.h> | ||
| #include <absl/log/initialize.h> | ||
| #include <absl/log/log_entry.h> | ||
| #include <absl/log/log_sink.h> | ||
| #include <absl/log/log_sink_registry.h> | ||
| #include <absl/strings/match.h> | ||
|
|
||
| #include <grpc/grpc_security_constants.h> | ||
|
|
||
| namespace nidevice_grpc { | ||
|
|
||
| namespace { | ||
|
|
||
| std::string describe_authentication(const grpc::AuthContext& auth_context) | ||
| { | ||
| if (!auth_context.IsPeerAuthenticated()) | ||
| return "Unauthenticated"; | ||
|
|
||
| const auto transport_type = auth_context.FindPropertyValues(GRPC_TRANSPORT_SECURITY_TYPE_PROPERTY_NAME); | ||
| const auto common_names = auth_context.FindPropertyValues(GRPC_X509_CN_PROPERTY_NAME); | ||
|
|
||
| std::string description = "Authenticated via "; | ||
| description += transport_type.empty() ? "unknown transport" : std::string(transport_type[0].data(), transport_type[0].size()); | ||
|
|
||
| if (!common_names.empty()) | ||
| description += ", client cert CN: " + std::string(common_names[0].data(), common_names[0].size()); | ||
|
|
||
| return description; | ||
| } | ||
|
|
||
| } | ||
|
|
||
| bool parse_peer(const std::string& peer, std::string& ip, std::string& port) | ||
| { | ||
| if (peer.rfind("ipv4:", 0) != 0 && peer.rfind("ipv6:", 0) != 0) | ||
| return false; | ||
|
|
||
| const auto scheme_end = peer.find(':'); | ||
| const auto port_pos = peer.rfind(':'); | ||
| if (port_pos <= scheme_end) | ||
| return false; | ||
|
|
||
| ip = peer.substr(scheme_end + 1, port_pos - scheme_end - 1); | ||
| port = peer.substr(port_pos + 1); | ||
|
|
||
| return !ip.empty() && !port.empty(); | ||
| } | ||
|
|
||
| // Converts absl's severity levels to our own. | ||
| logging::Level to_logging_level(absl::LogSeverity severity) | ||
| { | ||
| switch (severity) { | ||
| case absl::LogSeverity::kWarning: | ||
| return logging::Level_Warning; | ||
| case absl::LogSeverity::kError: | ||
| case absl::LogSeverity::kFatal: | ||
| return logging::Level_Error; | ||
| case absl::LogSeverity::kInfo: | ||
| default: | ||
| return logging::Level_Info; | ||
| } | ||
| } | ||
|
|
||
| // This sink captures gRPC's own internal logs and calls Send for each of them. We only use it to log handshake failures. | ||
| class AuditLogSink : public absl::LogSink { | ||
| public: | ||
| void Send(const absl::LogEntry& entry) override | ||
| { | ||
| if (!absl::StrContainsIgnoreCase(entry.text_message(), "handshake")) | ||
| return; | ||
|
|
||
| const auto message = std::string(entry.text_message()); | ||
| logging::log_to_audit_source(to_logging_level(entry.log_severity()), "%s", message.c_str()); | ||
| } | ||
| }; | ||
|
|
||
| void ClientConnectionLogger::PreSynchronousRequest(grpc::ServerContext* context) | ||
| { | ||
| std::string ip, port; | ||
| const auto peer = context->peer(); | ||
| const bool parsed = parse_peer(peer, ip, port); | ||
|
|
||
| // Only log the first connection seen from a given IP. | ||
| { | ||
| std::lock_guard<std::mutex> lock(seen_ips_mutex_); | ||
| if (!seen_ips_.insert(parsed ? ip : peer).second) | ||
| return; | ||
| } | ||
|
|
||
| const auto auth_description = describe_authentication(*context->auth_context()); | ||
|
|
||
| // Try to parse the IP + Port out of the peer uri so that the log message can be formatted nicely. If it fails, just log the raw string. | ||
| if (parsed) | ||
| nidevice_grpc::logging::log_to_audit_source(nidevice_grpc::logging::Level_Info, "Remote client successfully connected from %s:%s (%s)", ip.c_str(), port.c_str(), auth_description.c_str()); | ||
| else | ||
| nidevice_grpc::logging::log_to_audit_source(nidevice_grpc::logging::Level_Info, "Remote client successfully connected from %s (%s)", peer.c_str(), auth_description.c_str()); | ||
| } | ||
|
|
||
| void ClientConnectionLogger::PostSynchronousRequest(grpc::ServerContext*) | ||
| { | ||
| } | ||
|
|
||
| void register_client_connection_logger() | ||
| { | ||
| // gRPC stores this in an owning shared_ptr (see Server::SetGlobalCallbacks inserver_cc.cc) and deletes it at static destruction. Even | ||
| // if that changes to non-owning, intentionally leaking one process-wide object is correct and avoids a static-destruction-order hazard | ||
| // against grpc::Server. | ||
| grpc::Server::SetGlobalCallbacks(new ClientConnectionLogger()); | ||
| } | ||
|
|
||
| void register_grpc_log_sink() | ||
| { | ||
| absl::InitializeLog(); | ||
|
|
||
| // Abseil does not take ownership of the sink, so it will be a static object. | ||
| static AuditLogSink sink; | ||
| absl::AddLogSink(&sink); | ||
|
ryanwixon-emerson marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| } // namespace nidevice_grpc | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| #ifndef NIDEVICE_GRPC_CLIENT_CONNECTION_LOGGER_H | ||
| #define NIDEVICE_GRPC_CLIENT_CONNECTION_LOGGER_H | ||
|
|
||
| #include <grpcpp/grpcpp.h> | ||
|
|
||
| #include <mutex> | ||
| #include <string> | ||
| #include <unordered_set> | ||
|
|
||
| namespace nidevice_grpc { | ||
|
|
||
| // Parses a gRPC peer URI (e.g. "ipv4:127.0.0.1:12345" or "ipv6:[::1]:12345") into its ip and port components. | ||
| // Returns false if the peer string doesn't match a recognized format. | ||
| bool parse_peer(const std::string& peer, std::string& ip, std::string& port); | ||
|
|
||
| // Implementation of callbacks that the grpc::Server will invoke for individual client RPC calls. | ||
| class ClientConnectionLogger : public grpc::Server::GlobalCallbacks { | ||
| public: | ||
| void PreSynchronousRequest(grpc::ServerContext* context) override; | ||
| void PostSynchronousRequest(grpc::ServerContext* context) override; | ||
|
|
||
| private: | ||
| std::mutex seen_ips_mutex_; | ||
| std::unordered_set<std::string> seen_ips_; | ||
| }; | ||
|
|
||
| // Registers a process-wide ClientConnectionLogger with gRPC, must be called before any grpc::Server is built. | ||
| void register_client_connection_logger(); | ||
|
|
||
| // Registers an absl::LogSink that captures all of gRPC's internal log messages; used for logging connection failures | ||
| void register_grpc_log_sink(); | ||
|
|
||
| } // namespace nidevice_grpc | ||
|
|
||
| #endif // NIDEVICE_GRPC_CLIENT_CONNECTION_LOGGER_H |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I have 2 "thinking out loud" comments on this:
handshaketoken? Is it possible for a "normal message" to include thehandshaketoken? Is it possible for a client (unauthenticated client?) to influence either failure? Would using an additional/different filter (like "severity"?) improve it?Most likely what you're doing is about right. Just checking.
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.
I chatted with Ryan about this a bit and I believe the preferred mechanism for authentication logging moving forward will be to use https://grpc.io/docs/guides/interceptors/. Currently these are under the experimental namespace so we did not want to use these in their current state. Mainly to avoid any API friction that could be incurred when upgrading to newer versions of gRPC in grpc-device. Once these have stabilized and have been moved out of experimental it's definitely worth looking into.
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.
Yes, gRPC has several experimental options that would likely streamline the logging process. They've existed for a while but have not been stabilized yet. I'm not sure if there are plans on their end to do this at the moment, but maybe the CRA deadline will provide some motivation. In particular, there's a CertificateVerifier which seems to have the capability to provide function hooks that will fire during a handshake itself.
From what I can tell throughout my testing the "handshake" token seems to be sufficient as a filtering method. I saw the correct message in all failure cases and did not notice any extraneous error messages appearing at any other point either. Exploring the gRPC source with Copilot seems to align with this behavior; I found only 7 error messages that include it, including the one I saw in testing. It is possible for a failed connection to not include the token, but the scope of this addition is only to log authentication failures, not any kind of failure.
I think that the current state is probably the best solution we have for today; the aforementioned
CertificateVerifieris probably the best way to do it in a less "hacky" way and would definitely be something to consider if gRPC stabilizes it.