sammccall created this revision.
sammccall added reviewers: kadircet, kbobyrev.
Herald added subscribers: cfe-commits, usaxena95, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.
The index server has access to potentially-sensitive information, e.g. a
sequence of fuzzyFind requests reveals details about code completions in the
editor which in turn reveals details about the code being edited.
This information is necessary to provide the service, and our intention[1] is it
should never be retained beyond the scope of the request (e.g. not logged).
At the same time, some log messages should be exposed:
- server startup/index reloads etc that don't pertain to a user request
- basic request logs (method, latency, #results, error code) for monitoring
- errors while handling requests, without request-specific data
The -log=public design accommodates these by allowing three types of logs:
- those not associated with any user RPC request (via context-propagation)
- those explicitly tagged as [public] in the log line
- logging of format strings only, with no interpolated data (error level only)
[1] Specifically: Google is likely to run public instances of this server
for LLVM and potentially other projects, they will run in this configuration.
The details agreed in a Google-internal privacy review.
As clangd developers, we'd encourage others to use this configuration for public
instances too.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D90526
Files:
clang-tools-extra/clangd/index/remote/server/Server.cpp
clang-tools-extra/clangd/support/Logger.cpp
clang-tools-extra/clangd/support/Logger.h
clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -67,7 +67,8 @@
private:
// Color logs so we can distinguish them from test output.
- void log(Level L, const llvm::formatv_object_base &Message) override {
+ void log(Level L, const char *Fmt,
+ const llvm::formatv_object_base &Message) override {
raw_ostream::Colors Color;
switch (L) {
case Level::Verbose:
Index: clang-tools-extra/clangd/support/Logger.h
===================================================================
--- clang-tools-extra/clangd/support/Logger.h
+++ clang-tools-extra/clangd/support/Logger.h
@@ -24,16 +24,19 @@
public:
virtual ~Logger() = default;
- enum Level { Debug, Verbose, Info, Error };
+ /// The significance or severity of this message.
+ /// Typically used to filter the output to an interesting level.
+ enum Level : unsigned char { Debug, Verbose, Info, Error };
static char indicator(Level L) { return "DVIE"[L]; }
/// Implementations of this method must be thread-safe.
- virtual void log(Level, const llvm::formatv_object_base &Message) = 0;
+ virtual void log(Level, const char *Fmt,
+ const llvm::formatv_object_base &Message) = 0;
};
namespace detail {
const char *debugType(const char *Filename);
-void log(Logger::Level, const llvm::formatv_object_base &);
+void logImpl(Logger::Level, const char *Fmt, const llvm::formatv_object_base &);
// We often want to consume llvm::Errors by value when passing them to log().
// We automatically wrap them in llvm::fmt_consume() as formatv requires.
@@ -43,7 +46,8 @@
}
template <typename... Ts>
void log(Logger::Level L, const char *Fmt, Ts &&... Vals) {
- detail::log(L, llvm::formatv(Fmt, detail::wrap(std::forward<Ts>(Vals))...));
+ detail::logImpl(L, Fmt,
+ llvm::formatv(Fmt, detail::wrap(std::forward<Ts>(Vals))...));
}
llvm::Error error(std::error_code, std::string &&);
@@ -119,7 +123,8 @@
: MinLevel(MinLevel), Logs(Logs) {}
/// Write a line to the logging stream.
- void log(Level, const llvm::formatv_object_base &Message) override;
+ void log(Level, const char *Fmt,
+ const llvm::formatv_object_base &Message) override;
private:
Logger::Level MinLevel;
Index: clang-tools-extra/clangd/support/Logger.cpp
===================================================================
--- clang-tools-extra/clangd/support/Logger.cpp
+++ clang-tools-extra/clangd/support/Logger.cpp
@@ -28,10 +28,10 @@
LoggingSession::~LoggingSession() { L = nullptr; }
-void detail::log(Logger::Level Level,
- const llvm::formatv_object_base &Message) {
+void detail::logImpl(Logger::Level Level, const char *Fmt,
+ const llvm::formatv_object_base &Message) {
if (L)
- L->log(Level, Message);
+ L->log(Level, Fmt, Message);
else {
static std::mutex Mu;
std::lock_guard<std::mutex> Guard(Mu);
@@ -47,7 +47,7 @@
return Filename;
}
-void StreamLogger::log(Logger::Level Level,
+void StreamLogger::log(Logger::Level Level, const char *Fmt,
const llvm::formatv_object_base &Message) {
if (Level < MinLevel)
return;
Index: clang-tools-extra/clangd/index/remote/server/Server.cpp
===================================================================
--- clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ clang-tools-extra/clangd/index/remote/server/Server.cpp
@@ -12,6 +12,7 @@
#include "index/Serialization.h"
#include "index/Symbol.h"
#include "index/remote/marshalling/Marshalling.h"
+#include "support/Context.h"
#include "support/Logger.h"
#include "support/Shutdown.h"
#include "support/ThreadsafeFS.h"
@@ -23,6 +24,7 @@
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/VirtualFileSystem.h"
@@ -30,6 +32,7 @@
#include <chrono>
#include <grpc++/grpc++.h>
#include <grpc++/health_check_service_interface.h>
+#include <grpcpp/impl/codegen/server_context.h>
#include <memory>
#include <thread>
@@ -49,13 +52,26 @@
llvm::cl::opt<std::string> IndexRoot(llvm::cl::desc("<PROJECT ROOT>"),
llvm::cl::Positional, llvm::cl::Required);
-llvm::cl::opt<Logger::Level> LogLevel{
+// The RedactSensitiveInfo "log level" is peculiar to the remote index server.
+// When enabled, it ensures we don't log any potentially-sensitive user data.
+// (We don't disable logging entirely, so it still can be used for monitoring
+// and some limited debugging).
+enum ServerLogLevel {
+ verbose = Logger::Debug,
+ info = Logger::Info,
+ error = Logger::Error,
+ RedactSensitiveInfo,
+};
+llvm::cl::opt<ServerLogLevel> LogLevel{
"log",
llvm::cl::desc("Verbosity of log messages written to stderr"),
- values(clEnumValN(Logger::Error, "error", "Error messages only"),
- clEnumValN(Logger::Info, "info", "High level execution tracing"),
- clEnumValN(Logger::Debug, "verbose", "Low level details")),
- llvm::cl::init(Logger::Info),
+ values(
+ clEnumValN(RedactSensitiveInfo, "public",
+ "Avoid logging any potentially-sensitive request details."),
+ clEnumVal(error, "Error messages only"),
+ clEnumVal(info, "High level execution tracing"),
+ clEnumVal(verbose, "Low level details")),
+ llvm::cl::init(info),
};
llvm::cl::opt<std::string> TraceFile(
@@ -72,6 +88,8 @@
"server-address", llvm::cl::init("0.0.0.0:50051"),
llvm::cl::desc("Address of the invoked server. Defaults to 0.0.0.0:50051"));
+static Key<grpc::ServerContext *> CurrentRequest;
+
class RemoteIndexServer final : public v1::SymbolIndex::Service {
public:
RemoteIndexServer(clangd::SymbolIndex &Index, llvm::StringRef IndexRoot)
@@ -87,6 +105,7 @@
grpc::Status Lookup(grpc::ServerContext *Context,
const LookupRequest *Request,
grpc::ServerWriter<LookupReply> *Reply) override {
+ WithContextValue(CurrentRequest, Context);
trace::Span Tracer("LookupRequest");
auto Req = ProtobufMarshaller->fromProtobuf(Request);
if (!Req) {
@@ -119,6 +138,7 @@
grpc::Status FuzzyFind(grpc::ServerContext *Context,
const FuzzyFindRequest *Request,
grpc::ServerWriter<FuzzyFindReply> *Reply) override {
+ WithContextValue(CurrentRequest, Context);
trace::Span Tracer("FuzzyFindRequest");
auto Req = ProtobufMarshaller->fromProtobuf(Request);
if (!Req) {
@@ -151,6 +171,7 @@
grpc::Status Refs(grpc::ServerContext *Context, const RefsRequest *Request,
grpc::ServerWriter<RefsReply> *Reply) override {
+ WithContextValue(CurrentRequest, Context);
trace::Span Tracer("RefsRequest");
auto Req = ProtobufMarshaller->fromProtobuf(Request);
if (!Req) {
@@ -183,6 +204,7 @@
grpc::Status Relations(grpc::ServerContext *Context,
const RelationsRequest *Request,
grpc::ServerWriter<RelationsReply> *Reply) override {
+ WithContextValue(CurrentRequest, Context);
trace::Span Tracer("RelationsRequest");
auto Req = ProtobufMarshaller->fromProtobuf(Request);
if (!Req) {
@@ -229,8 +251,8 @@
LastStatus.getLastModificationTime() &&
Status->getSize() == LastStatus.getSize()))
return;
- vlog("Found different index version: existing index was modified at {0}, new "
- "index was modified at {1}. Attempting to reload.",
+ vlog("Found different index version: existing index was modified at "
+ "{0}, new index was modified at {1}. Attempting to reload.",
LastStatus.getLastModificationTime(), Status->getLastModificationTime());
LastStatus = *Status;
std::unique_ptr<clang::clangd::SymbolIndex> NewIndex = loadIndex(IndexPath);
@@ -266,6 +288,33 @@
ServerShutdownWatcher.join();
}
+std::unique_ptr<Logger> makeLogger(llvm::raw_ostream &OS,
+ ServerLogLevel Level) {
+ if (LogLevel != RedactSensitiveInfo) {
+ // Other values all correspond to Logger::Level directly.
+ Logger::Level StandandLevel = static_cast<Logger::Level>(int(Level));
+ return std::make_unique<StreamLogger>(OS, StandandLevel);
+ }
+ // Redacted mode:
+ // - messages outside the scope of a request: log fully
+ // - messages tagged [public]: log fully
+ // - errors: log the format string
+ // - others: drop
+ class RedactedLogger : public StreamLogger {
+ public:
+ RedactedLogger(llvm::raw_ostream &OS) : StreamLogger(OS, Debug) {}
+ void log(Level L, const char *Fmt,
+ const llvm::formatv_object_base &Message) override {
+ if (Context::current().get(CurrentRequest) == nullptr ||
+ llvm::StringRef(Fmt).startswith("[public]"))
+ return StreamLogger::log(L, Fmt, Message);
+ if (L >= Error)
+ return StreamLogger::log(L, Fmt, llvm::formatv("[redacted] {0}", Fmt));
+ }
+ };
+ return std::make_unique<RedactedLogger>(OS);
+}
+
} // namespace
} // namespace remote
} // namespace clangd
@@ -287,8 +336,8 @@
llvm::errs().SetBuffered();
// Don't flush stdout when logging for thread safety.
llvm::errs().tie(nullptr);
- clang::clangd::StreamLogger Logger(llvm::errs(), LogLevel);
- clang::clangd::LoggingSession LoggingSession(Logger);
+ auto Logger = makeLogger(llvm::errs(), LogLevel);
+ clang::clangd::LoggingSession LoggingSession(*Logger);
llvm::Optional<llvm::raw_fd_ostream> TracerStream;
std::unique_ptr<clang::clangd::trace::EventTracer> Tracer;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
