-
Notifications
You must be signed in to change notification settings - Fork 159
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
Potentially dangerous and inconsistent behavior for secrets #689
Comments
I certainly agree that there are inconsistencies with how secret obfuscation is (or isn't) handled. I'm mostly concerned about the Google social connection exposing the client secret, I'll certainly look into that. |
Thanks. Maybe also consider some automated way of sanitizing secrets, since with manual whitelisting approach this could happen again (and it happened before, e.g. in #601). And please check if |
I have also encountered the same issue and accidentally pushed the secrets of several social connections (like Google Enterprise Connections or SIWE) to our repository. I then tried to replace the secrets using We usually push the config.json file for each tenant (dev, staging, prod) to our repository. Thus, we would like to replace those plain text secrets using environment variables, which will then be injected via bitbucket pipelines. But replacing specific values of the
|
My bad. I did not set up my environment variables correctly. Using I am sorry for hijacking this issue. Please be aware, that multiple connections with secrets can exist. Thus, sanitizing them all the same way using a constant like |
Hi all, I've come to this issue as I exported my whole config from a dev UK tenant into a new EU tenant. Export has no client ids or secrets and import created all new ones on the new tenant. Is Thanks. |
Nevermind, I've enabled the export of these fields using Thanks! |
Here's how I solved the issue on my end:
Assming you have pulled the remote tenant.yaml into const fs = require("node:fs")
const path = require("node:path")
const YAML = require('yaml')
function walkMutateMany(objects, callback) {
// Function to walk through the object
function walk(path, key, nodes, parents) {
// If all nodes are arrays, map over all indexes and reassign values
if (nodes.every(Array.isArray)) {
const maxLength = nodes.reduce(
(maxLength, node) =>
node.length > maxLength ? node.length : maxLength,
0
);
// If the value is an array, iterate through each element
let resultNodes = nodes;
for (let i = 0; i < maxLength; i += 1) {
const values = walk(
`${path}[${i}]`,
i,
nodes.map((node) => node[i]),
nodes
);
resultNodes.forEach((node, nodeIndex) => {
if (node.length > i) {
node[i] = values[nodeIndex];
}
});
}
return resultNodes;
// If all nodes are basic javascript objects, map over all keys and reassign values
} else if (
nodes.every(
(node) =>
typeof node === "object" &&
node !== null &&
node.constructor === Object
)
) {
// If the value is an object, iterate through its key-value pairs
const allKeys = Object.keys(nodes.reduce(
(keys, node) => {
Object.keys(node).forEach((k) => keys[k] = true)
return keys
},
{}
));
let resultNodes = nodes;
for (const key of allKeys) {
const values = walk(
`${path}.${key}`,
key,
nodes.map((node) => node[key]),
nodes
);
resultNodes.forEach((node, nodeIndex) => {
if (key in node && node.hasOwnProperty(key))
node[key] = values[nodeIndex];
});
}
return resultNodes;
// If the value is neither an array nor an object, invoke the callback
// This covers scalars as well as complex objects such as Date or objects created using a class
} else {
return callback(path, key, nodes, parents);
}
}
// Start the walk from the root object
return walk(
"$",
null,
objects,
objects.map(() => null)
);
}
const localTenantManifest = YAML.parse(fs.readFileSync(path.join(__dirname, "../tenants/my-tenant", "tenant.yaml")).toString('utf8'))
const remoteTenantManifest = YAML.parse(fs.readFileSync(path.join(__dirname, "../tenants/my-tenant", "tenant-remote.yaml")).toString('utf8'))
function isKeyWordMapping(value) {
return typeof value === "string" && /(##|@@)[A-Za-z0-9_]+(##|@@)/.test(value)
}
const results = walkMutateMany(
[
localTenantManifest,
remoteTenantManifest
],
(path, key, [local, remote], parents) => {
console.log(path, key, [local, remote])
return [local, isKeyWordMapping(local) ? local : remote]
}
)
fs.writeFileSync(
path.join(__dirname, "../tenants/my-tenant", "tenant-result.yaml"),
YAML.stringify(results[1])
) It basically walks through the WARNING: This assumes that the remote hasn't introduced secrets that aren't managed by the tooling in the repo. You should always check if the resulting file contains any new secrets before committing changes to svc. |
Checklist
Description
When running
export
, the exported files contain some secret values by default. This is dangerous, since users might accidentally push secrets to a code repository.Additionally, behavior is inconsistent between resource types. Some resource types provide sanitized values, but they are also sanitized differently.
I haven't done extensive testing, for now I've noticed the following in exported
tenant.yaml
:connections.options.client_secret
for social connections (e.g. Google) contains actual valueemailProvider.smtp_user
contains actual value,emailProvider.smtp_pass
contains sanitized value with keyword'##SMTP_PASS##'
logStreams.sink.httpAuthorization
(for log stream of typehttp
) contains sanitized value_VALUE_NOT_SHOWN_
Expectation
I would expect the following:
I think it would be best to not expose secrets by default. Currently, there is a way to exclude secret fields by using
EXCLUDED_PROPS
, however you need to carefully examine the exported data and manually check for potential secret values to exclude. Instead of blacklisting approach, I propose to sanitize all secrets by default. Then users can whitelist some properties if they want to get actual secret values. For example, instead of usingEXCLUDED_PROPS
, add something likeINCLUDED_SECRETS
.Regarding sanitization, secret values should be sanitized consistently, instead of using keywords (e.g.
'##SMTP_PASS##'
) and_VALUE_NOT_SHOWN_
. Keywords are probably more useful, since users could use https://github.com/auth0/auth0-deploy-cli/blob/master/docs/keyword-replacement.md to load secrets from environment variables. On other hand, this could be dangerous, if "reserved" Auth0 keywords (such as'##SMTP_PASS##'
) would collide with existing keyword mappings. Though this could probably be prevented by doing additional validation onAUTH0_KEYWORD_REPLACE_MAPPINGS
.Reproduction
Authorization token
seta0deploy export --format=yaml --output_folder=config
config/tenant.yaml
contains:connections.options.client_secret
for social connection contains secret valueemailProvider.smtp_pass
contains sanitized value'##SMTP_PASS##'
logStreams.sink.httpAuthorization
contains sanitized value_VALUE_NOT_SHOWN_
Deploy CLI version
7.15.1
Node version
v18.12.1
The text was updated successfully, but these errors were encountered: