Skip to content
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

Open
6 tasks done
jcerjak opened this issue Nov 10, 2022 · 8 comments
Open
6 tasks done

Potentially dangerous and inconsistent behavior for secrets #689

jcerjak opened this issue Nov 10, 2022 · 8 comments
Labels

Comments

@jcerjak
Copy link

jcerjak commented Nov 10, 2022

Checklist

  • I have looked into the README and have not found a suitable solution or answer.
  • I have looked into the documentation and have not found a suitable solution or answer.
  • I have searched the issues and have not found a suitable solution or answer.
  • I have upgraded to the latest version of this tool and the issue still persists.
  • I have searched the Auth0 Community forums and have not found a suitable solution or answer.
  • I agree to the terms within the Auth0 Code of Conduct.

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 value
  • emailProvider.smtp_user contains actual value, emailProvider.smtp_pass contains sanitized value with keyword '##SMTP_PASS##'
  • logStreams.sink.httpAuthorization (for log stream of type http) contains sanitized value _VALUE_NOT_SHOWN_

Expectation

I would expect the following:

  • Behavior for secrets to be consistent across all resource types.
  • Behavior for secrets to be better documented. If exporting secrets is intentional, there should be a clear notice in the README warning people about this.
  • Safer default behavior for secrets.

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 using EXCLUDED_PROPS, add something like INCLUDED_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 on AUTH0_KEYWORD_REPLACE_MAPPINGS.

Reproduction

  1. Given Auth0 tenant with:
  • a social connection (e.g. Google)
  • Custom STMP Email Provider
  • Custom Webhook event stream, with Authorization token set
  1. When exporting the tenant config with a0deploy export --format=yaml --output_folder=config
  2. Then config/tenant.yaml contains:
  • connections.options.client_secret for social connection contains secret value
  • emailProvider.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

@jcerjak jcerjak added the bug label Nov 10, 2022
@jcerjak jcerjak changed the title Dangerous and inconsistent behavior for secrets Potentially dangerous and inconsistent behavior for secrets Nov 11, 2022
@jcerjak
Copy link
Author

jcerjak commented Nov 12, 2022

Related also to #72 and #601.

@willvedd
Copy link
Contributor

willvedd commented Dec 8, 2022

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.

@jcerjak
Copy link
Author

jcerjak commented Dec 12, 2022

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 emailProvider.smtp_user could be sanitized as well (similar to ##SMTP_PASS##). This would make the export/import roundtrip easier for us, because the value (we use ##SMTP_USER##) will not be replaced on each export . Unless this will already be handled in #688.

@jo-al-ra
Copy link

jo-al-ra commented Dec 12, 2022

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 AUTH0_KEYWORD_REPLACE_MAPPINGS in the config.json.

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 AUTH0_KEYWORD_REPLACE_MAPPINGS with environment variables is unfortunately not as easy as replacing top-level values such as the AUTH0_CLIENT_SECRET. Is there a way to replace single values of the AUTH0_KEYWORD_REPLACE_MAPPINGS without exporting the complete object as an environment variable as in the examples?
https://github.com/auth0/auth0-deploy-cli/blob/master/docs/configuring-the-deploy-cli.md#examples

# Non-primitive configuration values
export AUTH0_EXCLUDED='["actions","organizations"]'
export AUTH0_KEYWORD_REPLACE_MAPPINGS='{"ENVIRONMENT":"dev"}'
a0deploy export -c=config.json --format=yaml --output_folder=local

@jo-al-ra
Copy link

But replacing specific values of the AUTH0_KEYWORD_REPLACE_MAPPINGS with environment variables is unfortunately not as easy as replacing top-level values such as the AUTH0_CLIENT_SECRET.

My bad. I did not set up my environment variables correctly. Using "##MY_SECRET##" also works in the AUTH0_KEYWORD_REPLACE_MAPPINGS.

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 '##CUSTOM_CONNECTION_SECRET## could result in problems as well. As users might forget to replace it and will end up with the same secret for all connections.

@ghenry
Copy link

ghenry commented Mar 27, 2023

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 a0deploy supposed to be able to backup a whole auth0 tenant as-is?

Thanks.

@ghenry
Copy link

ghenry commented Apr 25, 2023

Nevermind, I've enabled the export of these fields using INCLUDED_PROPS

Thanks!

@philippefutureboy
Copy link

philippefutureboy commented Aug 24, 2024

Here's how I solved the issue on my end:

  1. tenant.yaml created by dump should not be under svc
  2. tenant.yaml managed by your team should be under svc

Assming you have pulled the remote tenant.yaml into tenant-remote.yaml, you can automatically replace secrets in the remote file with the keyword mappings using the following script:

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 tenant-remote.yaml and the tenant.yaml and rewrites any value in the remote that has a mapping in the tenant.yaml. That way you keep the updates in structure that can come from remote, but you also strip any secrets from the file. You can then overwrite your local tenant.yaml with the result.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants