Skip to content

Commit

Permalink
[BACKPORT 2.20][PLAT-15307]fix sensitive info leaks via Gflags
Browse files Browse the repository at this point in the history 
Summary:
Original commit: 79a00fd/D38034

We were leaking sensitive info via gflag values in YBA log. Changes in java and python layer to fix the leaks.

Also added changes to parse sensitive gflags from all db releases present locally.
The flags are parsed once per release and then cached in the yugaware pg for later use.

Test Plan:
Verified manually that all the occurences of the sensitive gflags in logs have value redacted.

Verified that sensitive gflags are updated for new releases upon import.

Reviewers: #yba-api-review!, amalyshev

Reviewed By: amalyshev

Subscribers: yugaware

Differential Revision: https://phorge.dev.yugabyte.com/D38708
  • Loading branch information
@asharma-yb
asharma-yb committed Oct 7, 2024
1 parent 010029e commit 3698da9
Show file tree
Hide file tree
Showing 11 changed files with 214 additions and 37 deletions.
4 changes: 3 additions & 1 deletion managed/devops/opscli/ybops/cloud/common/ansible.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def __init__(self):
self.connection_type = self.DEFAULT_SSH_CONNECTION_TYPE
self.connection_target = "localhost"
self.sensitive_data_keywords = ["KEY", "SECRET", "CREDENTIALS", "API", "POLICY",
"NODE_AGENT_AUTH_TOKEN"]
"NODE_AGENT_AUTH_TOKEN", "YCQL_LDAP", "YSQL_HBA_CONF"]

def set_connection_params(self, conn_type, target):
self.connection_type = conn_type
Expand All @@ -61,6 +61,8 @@ def redact_sensitive_data(self, playbook_args):
for key, value in playbook_args.items():
if self.is_sensitive(key.upper()):
playbook_args[key] = self.REDACT_STRING
elif isinstance(value, dict):
playbook_args[key] = self.redact_sensitive_data(value)
return playbook_args

def run(self, filename, extra_vars=None, host_info=None, print_output=True,
Expand Down
31 changes: 31 additions & 0 deletions managed/src/main/java/com/yugabyte/yw/common/AppInit.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.google.inject.Inject;
import com.google.inject.Singleton;
import com.google.inject.name.Named;
import com.jayway.jsonpath.JsonPath;
import com.typesafe.config.Config;
import com.yugabyte.yw.cloud.aws.AWSInitializer;
import com.yugabyte.yw.commissioner.BackupGarbageCollector;
Expand All @@ -24,6 +25,7 @@
import com.yugabyte.yw.commissioner.YbcUpgrade;
import com.yugabyte.yw.commissioner.tasks.subtasks.cloud.CloudImageBundleSetup;
import com.yugabyte.yw.common.ConfigHelper.ConfigType;
import com.yugabyte.yw.common.ReleaseManager.ReleaseMetadata;
import com.yugabyte.yw.common.alerts.AlertConfigurationService;
import com.yugabyte.yw.common.alerts.AlertConfigurationWriter;
import com.yugabyte.yw.common.alerts.AlertDestinationService;
Expand Down Expand Up @@ -51,8 +53,11 @@
import io.ebean.DB;
import io.prometheus.client.hotspot.DefaultExports;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import lombok.extern.slf4j.Slf4j;
import play.Application;
import play.Environment;
Expand Down Expand Up @@ -263,6 +268,8 @@ public AppInit(
});
armReleaseThread.start();

updateSensitiveGflagsforRedaction(releaseManager, gFlagsValidation, configHelper);

// initialize prometheus exports
DefaultExports.initialize();

Expand Down Expand Up @@ -347,4 +354,28 @@ public AppInit(
throw t;
}
}

private void updateSensitiveGflagsforRedaction(
ReleaseManager releaseManager, GFlagsValidation gFlagsValidation, ConfigHelper configHelper) {
Set<String> sensitiveGflags = new HashSet<>();
Map<String, Object> updatedReleases = new HashMap<>();
boolean update[] = {false};
releaseManager
.getReleaseMetadata()
.forEach(
(version, r) -> {
ReleaseMetadata rm = releaseManager.metadataFromObject(r);
if (rm.sensitiveGflags == null) {
update[0] = true;
rm.sensitiveGflags = gFlagsValidation.getSensitiveJsonPathsForVersion(version);
}
sensitiveGflags.addAll(rm.sensitiveGflags);
updatedReleases.put(version, rm);
});
if (update[0]) {
configHelper.loadConfigToDB(ConfigHelper.ConfigType.SoftwareReleases, updatedReleases);
}
RedactingService.SECRET_JSON_PATHS_LOGS.addAll(
sensitiveGflags.stream().map(JsonPath::compile).collect(Collectors.toList()));
}
}
20 changes: 8 additions & 12 deletions managed/src/main/java/com/yugabyte/yw/common/NodeManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,8 @@ private void processGFlags(
}
}

private List<String> getConfigureSubCommand(AnsibleConfigureServers.Params taskParam) {
private List<String> getConfigureSubCommand(
AnsibleConfigureServers.Params taskParam, Map<String, String> sensitiveData) {
Universe universe = Universe.getOrBadRequest(taskParam.getUniverseUUID());
Config config = runtimeConfigFactory.forUniverse(universe);
UserIntent userIntent = getUserIntentFromParams(universe, taskParam);
Expand Down Expand Up @@ -1046,8 +1047,7 @@ private List<String> getConfigureSubCommand(AnsibleConfigureServers.Params taskP
}
Map<String, String> gflags = new TreeMap<>(taskParam.gflags);
processGFlags(config, universe, node, taskParam, gflags, useHostname);
subcommand.add("--gflags");
subcommand.add(Json.stringify(Json.toJson(gflags)));
sensitiveData.put("--gflags", Json.stringify(Json.toJson(gflags)));
} else if (taskSubType.equals(
UpgradeTaskParams.UpgradeTaskSubType.YbcInstall.toString())) {
subcommand.add("--tags");
Expand Down Expand Up @@ -1120,8 +1120,7 @@ private List<String> getConfigureSubCommand(AnsibleConfigureServers.Params taskP
}
}
}
subcommand.add("--gflags");
subcommand.add(Json.stringify(Json.toJson(gflags)));
sensitiveData.put("--gflags", Json.stringify(Json.toJson(gflags)));

subcommand.add("--tags");
subcommand.add("override_gflags");
Expand Down Expand Up @@ -1233,8 +1232,7 @@ private List<String> getConfigureSubCommand(AnsibleConfigureServers.Params taskP
universe,
Arrays.asList(GFlagsUtil.CERTS_DIR, GFlagsUtil.CERTS_FOR_CLIENT_DIR)));
processGFlags(config, universe, node, taskParam, gflags, useHostname);
subcommand.add("--gflags");
subcommand.add(Json.stringify(Json.toJson(gflags)));
sensitiveData.put("--gflags", Json.stringify(Json.toJson(gflags)));
subcommand.add("--tags");
subcommand.add("override_gflags");
break;
Expand Down Expand Up @@ -1301,8 +1299,7 @@ private List<String> getConfigureSubCommand(AnsibleConfigureServers.Params taskP
gflags.putAll(filterCertsAndTlsGFlags(taskParam, universe, tlsGflagsToReplace));
}
processGFlags(config, universe, node, taskParam, gflags, useHostname, true);
subcommand.add("--gflags");
subcommand.add(Json.stringify(Json.toJson(gflags)));
sensitiveData.put("--gflags", Json.stringify(Json.toJson(gflags)));

subcommand.add("--tags");
subcommand.add("override_gflags");
Expand All @@ -1322,8 +1319,7 @@ private List<String> getConfigureSubCommand(AnsibleConfigureServers.Params taskP
LOG.warn("Round2 upgrade not required when there is no change in node-to-node");
}
processGFlags(config, universe, node, taskParam, gflags, useHostname);
subcommand.add("--gflags");
subcommand.add(Json.stringify(Json.toJson(gflags)));
sensitiveData.put("--gflags", Json.stringify(Json.toJson(gflags)));

subcommand.add("--tags");
subcommand.add("override_gflags");
Expand Down Expand Up @@ -1978,7 +1974,7 @@ && imdsv2required(arch, userIntent, provider)) {
throw new RuntimeException("NodeTaskParams is not AnsibleConfigureServers.Params");
}
AnsibleConfigureServers.Params taskParam = (AnsibleConfigureServers.Params) nodeTaskParam;
commandArgs.addAll(getConfigureSubCommand(taskParam));
commandArgs.addAll(getConfigureSubCommand(taskParam, sensitiveData));
if (taskParam.isSystemdUpgrade) {
// Cron to Systemd Upgrade
commandArgs.add("--tags");
Expand Down
10 changes: 7 additions & 3 deletions managed/src/main/java/com/yugabyte/yw/common/RedactingService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import com.jayway.jsonpath.JsonPath;
import com.yugabyte.yw.common.audit.AuditService;
import java.util.List;
import java.util.Set;
import java.util.concurrent.CopyOnWriteArraySet;
import java.util.stream.Collectors;
import javax.inject.Singleton;

Expand Down Expand Up @@ -91,16 +93,18 @@ public class RedactingService {
// LDAP - DB Universe Sync
.add("$..dbuserPassword")
.add("$..ldapBindPassword")
.add("$..ysql_hba_conf_csv")
// HA Config
.add("$..cluster_key")
.build();

// List of json paths to any secret fields we want to redact.
// More on json path format can be found here: https://goessner.net/articles/JsonPath/
public static final List<JsonPath> SECRET_JSON_PATHS_APIS =
SECRET_PATHS_FOR_APIS.stream().map(JsonPath::compile).collect(Collectors.toList());

public static final List<JsonPath> SECRET_JSON_PATHS_LOGS =
SECRET_PATHS_FOR_LOGS.stream().map(JsonPath::compile).collect(Collectors.toList());
public static Set<JsonPath> SECRET_JSON_PATHS_LOGS =
new CopyOnWriteArraySet<>(
SECRET_PATHS_FOR_LOGS.stream().map(JsonPath::compile).collect(Collectors.toList()));

public static JsonNode filterSecretFields(JsonNode input, RedactionTarget target) {
if (input == null) {
Expand Down
3 changes: 3 additions & 0 deletions managed/src/main/java/com/yugabyte/yw/common/ReleaseManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ public static class ReleaseMetadata {
@ApiModelProperty(value = "Release packages")
public List<Package> packages;

@ApiModelProperty(value = "List of sensitive gflags")
public Set<String> sensitiveGflags;

// Docker image tag corresponding to the release
@ApiModelProperty(value = "Release image tag")
public String imageTag;
Expand Down
24 changes: 24 additions & 0 deletions managed/src/main/java/com/yugabyte/yw/common/gflags/GFlagsValidation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,30 @@ public GFlagsValidation(
this.releaseManager = releaseManager;
}

/**
* Given a YBDB version, returns a set of JsonPaths containing the sensitive Master and Tserver
* Gflags.
*/
public Set<String> getSensitiveJsonPathsForVersion(String version) {
LOG.info("Parsing sensitive gflags for DB version " + version);
Set<String> sensitiveGflags = new HashSet<>();
for (ServerType server : ServerType.values()) {
if (!server.equals(ServerType.MASTER) && !server.equals(ServerType.TSERVER)) {
continue;
}
try {
for (GFlagDetails gflag : extractGFlags(version, server.name(), false)) {
if (gflag.tags.contains("sensitive_info")) {
sensitiveGflags.add("$.." + gflag.name);
}
}
} catch (Exception e) {
LOG.error("Error while fetching Gflags for db version " + version, e);
}
}
return sensitiveGflags;
}

public List<GFlagDetails> extractGFlags(String version, String serverType, boolean mostUsedGFlags)
throws IOException {
String releasesPath = confGetter.getStaticConf().getString(Util.YB_RELEASES_PATH);
Expand Down
11 changes: 10 additions & 1 deletion managed/src/main/java/com/yugabyte/yw/controllers/ReleaseController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.google.inject.Inject;
import com.jayway.jsonpath.JsonPath;
import com.yugabyte.yw.cloud.PublicCloudConstants.Architecture;
import com.yugabyte.yw.common.PlatformServiceException;
import com.yugabyte.yw.common.RedactingService;
import com.yugabyte.yw.common.ReleaseManager;
import com.yugabyte.yw.common.ReleaseManager.ReleaseMetadata;
import com.yugabyte.yw.common.Util;
Expand Down Expand Up @@ -35,10 +37,12 @@
import io.swagger.annotations.ApiOperation;
import io.swagger.annotations.Authorization;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import java.util.UUID;
import java.util.stream.Collectors;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -95,14 +99,19 @@ public Result create(UUID customerUUID, Http.Request request) {
}

try {
Set<String> sensitiveGflags = new HashSet<>();
Map<String, ReleaseMetadata> releases =
ReleaseManager.formDataToReleaseMetadata(versionDataList);
releases.forEach(
(version, metadata) -> {
releaseManager.addReleaseWithMetadata(version, metadata);
gFlagsValidation.addDBMetadataFiles(version, metadata);
metadata.sensitiveGflags = gFlagsValidation.getSensitiveJsonPathsForVersion(version);
sensitiveGflags.addAll(metadata.sensitiveGflags);
releaseManager.addReleaseWithMetadata(version, metadata);
});
releaseManager.updateCurrentReleases();
RedactingService.SECRET_JSON_PATHS_LOGS.addAll(
sensitiveGflags.stream().map(JsonPath::compile).collect(Collectors.toList()));
} catch (RuntimeException re) {
throw new PlatformServiceException(INTERNAL_SERVER_ERROR, re.getMessage());
}
Expand Down
8 changes: 8 additions & 0 deletions managed/src/main/resources/swagger-strict.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9218,6 +9218,14 @@
"$ref" : "#/definitions/S3Location",
"description" : "S3 link and credentials"
},
"sensitiveGflags" : {
"description" : "List of sensitive gflags",
"items" : {
"type" : "string"
},
"type" : "array",
"uniqueItems" : true
},
"state" : {
"description" : "Release state",
"enum" : [ "ACTIVE", "DISABLED", "DELETED" ],
Expand Down
8 changes: 8 additions & 0 deletions managed/src/main/resources/swagger.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9318,6 +9318,14 @@
"$ref" : "#/definitions/S3Location",
"description" : "S3 link and credentials"
},
"sensitiveGflags" : {
"description" : "List of sensitive gflags",
"items" : {
"type" : "string"
},
"type" : "array",
"uniqueItems" : true
},
"state" : {
"description" : "Release state",
"enum" : [ "ACTIVE", "DISABLED", "DELETED" ],
Expand Down
Loading

0 comments on commit 3698da9

Please sign in to comment.