Skip to content

Commit

Permalink
[BACKPORT 2.12][INCIDENT-19] YSQL: Redact auth key from logs on YSQL …
Browse files Browse the repository at this point in the history
…upgrade command

Summary:
The upgrade_ysql (via yb-admin) command uses yb-tserver-key auth method, and during upgrade, we log the postgres connection command to plaintext. The command contains a "password" field; however, as implemented in D9976, it is a 64 bit random integer and is not associated with any user credentials. As best security practices, similar to D13999, we should redact the field when logging it out.

Original diff / commit: D16149 / fd61376

Test Plan:
Run `./bin/yb-admin upgrade_ysql` and check `~/yugabyte-data` that the password field has been redacted. The text should look something like this:
`./node-1/disk-1/yb-data/tserver/logs/yb-tserver.INFO:I0323 00:46:36.154577 2306181 libpq_utils.cc:178] Connected to PG (user=postgres password=<REDACTED> host=/tmp/.yb.9427142463175692966 port=5433 dbname='system_platform'), time taken: 0.106s`

Reviewers: smishra, jason

Reviewed By: jason

Subscribers: yql, smishra

Differential Revision: https://phabricator.dev.yugabyte.com/D16209
  • Loading branch information
lnguyen-yugabyte committed Mar 28, 2022
1 parent 1b50b15 commit 3ad4697
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 6 deletions.
7 changes: 5 additions & 2 deletions src/yb/yql/pgwrapper/libpq_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,8 @@ Result<PGConn> PGConn::Connect(

Result<PGConn> PGConn::Connect(const std::string& conn_str,
CoarseTimePoint deadline,
bool simple_query_protocol) {
bool simple_query_protocol,
const boost::optional<std::string>& conn_str_for_log) {
auto start = CoarseMonoClock::now();
for (;;) {
PGConnPtr result(PQconnectdb(conn_str.c_str()));
Expand All @@ -169,7 +170,9 @@ Result<PGConn> PGConn::Connect(const std::string& conn_str,
}
auto status = PQstatus(result.get());
if (status == ConnStatusType::CONNECTION_OK) {
LOG(INFO) << "Connected to PG (" << conn_str << "), time taken: "
LOG(INFO) << "Connected to PG ("
<< (conn_str_for_log.has_value() ? conn_str_for_log.value() : conn_str)
<< "), time taken: "
<< MonoDelta(CoarseMonoClock::Now() - start);
return PGConn(std::move(result), simple_query_protocol);
}
Expand Down
13 changes: 10 additions & 3 deletions src/yb/yql/pgwrapper/libpq_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
#include "yb/util/net/net_fwd.h"
#include "yb/util/result.h"

#include <boost/optional.hpp>

namespace yb {
namespace pgwrapper {

Expand Down Expand Up @@ -97,17 +99,22 @@ class PGConn {
const std::string& db_name,
const std::string& user,
bool simple_query_protocol = false);
// Pass in an optional conn_str_for_log for logging purposes. This is used in case
// conn_str contains sensitive information (e.g. password).
static Result<PGConn> Connect(
const std::string& conn_str,
bool simple_query_protocol = false) {
bool simple_query_protocol = false,
const boost::optional<std::string>& conn_str_for_log = boost::none) {
return Connect(conn_str,
CoarseMonoClock::Now() + MonoDelta::FromSeconds(60) /* deadline */,
simple_query_protocol);
simple_query_protocol,
conn_str_for_log);
}
static Result<PGConn> Connect(
const std::string& conn_str,
CoarseTimePoint deadline,
bool simple_query_protocol = false);
bool simple_query_protocol = false,
const boost::optional<std::string>& conn_str_for_log = boost::none);

CHECKED_STATUS Execute(const std::string& command, bool show_query_in_error = true);

Expand Down
11 changes: 10 additions & 1 deletion src/yb/yql/pgwrapper/ysql_upgrade.cc
Original file line number Diff line number Diff line change
Expand Up @@ -236,8 +236,17 @@ Result<PGConn> YsqlUpgradeHelper::Connect(const std::string& database_name) {
PgDeriveSocketDir(ysql_proxy_addr_.host()),
ysql_proxy_addr_.port(),
pgwrapper::PqEscapeLiteral(database_name));
// Use the string with redacted password for logging purposes.
boost::optional<std::string> conn_str_for_log(Format(
"user=$0 password=$1 host=$2 port=$3 dbname=$4",
"postgres",
"<REDACTED>",
PgDeriveSocketDir(ysql_proxy_addr_.host()),
ysql_proxy_addr_.port(),
pgwrapper::PqEscapeLiteral(database_name)));

PGConn pgconn = VERIFY_RESULT(PGConn::Connect(conn_str));
PGConn pgconn = VERIFY_RESULT(
PGConn::Connect(conn_str, false /* simple_query_protocol */, conn_str_for_log));

RETURN_NOT_OK(pgconn.Execute("SET ysql_upgrade_mode TO true;"));

Expand Down

0 comments on commit 3ad4697

Please sign in to comment.