Audit log in failed connection cases - #1281
Conversation
| public: | ||
| void Send(const absl::LogEntry& entry) override | ||
| { | ||
| if (!absl::StrContainsIgnoreCase(entry.text_message(), "handshake")) |
There was a problem hiding this comment.
I have 2 "thinking out loud" comments on this:
- Is it possible that grpc has improved or will improve support for connection logging as part of its own CRA compliance efforts? It's quite cool that we've managed to make this work in a pretty reasonable way in a difficult environment. But it's hard to imagine that every C++ grpc server will need to do all of this to comply with CRA.
- What are some interesting failure modes of this text match? Is it possible for a "failed connection" to be missing the
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.
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.
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 CertificateVerifier is probably the best way to do it in a less "hacky" way and would definitely be something to consider if gRPC stabilizes it.
|
Looks reasonable. Waiting for this to be rebased on the merged base commit (the diff is showing both for me) |
| { | ||
| static std::shared_ptr<spdlog::logger> audit_logger = []() { | ||
| auto sink = std::make_shared<spdlog::sinks::win_eventlog_sink_mt>("ni-grpc-device-server"); | ||
| auto sink = std::make_shared<spdlog::sinks::win_eventlog_sink_mt>("ni-grpc-device"); |
There was a problem hiding this comment.
Adding in a quick unrelated change to this PR as I'm going to be changing the Windows Event Log source name to prevent the logs from being redundant. Today they are prefixed with "[ni-grpc-device-server][Server]".
5 participants
What does this Pull Request accomplish?
This PR is a follow up to #1280 and is based on it. The relevant changes are mostly in
ClientConnectionLogger.cpp, and once #1280 is merged it will shrink considerably.This PR adds handshake failure logging to
grpc-device. Due to the limitations of gRPC, the mechanism that it uses to log differs slightly from the success logging, which fires once per RPC call. This new mechanism instead creates a sink usingabsl, which will catch all of gRPC's internal logging and bubble it up into a callback function.Since this naturally surfaces a very large number of logs (everything gRPC ever sends), I'm opting to filter for the word "handshake" when determining which logs to actually display to the user. This results in messages being logged on mTLS failures (such as when the client does not provide a certificate) and, from what I can tell, nowhere else.
Why should this Pull Request be merged?
Our goal is to add audit logging for the CRA. The server should log a message when clients succeed/fail when authenticating. This PR completes the implementation by adding an implementation for the failure scenario.
#1280 is the PR which laid the groundwork for this change and implemented the success scenario.
What testing has been done?
I tested this in a Windows -> Windows workflow and confirmed that the audit events appear in Windows Event Log as expected. Due to the difficulty of testing other workflows, I'll wait until this is merged to check Linux workflows.