Multi-environment encryption key & team member separation

[joelgallant]

Provides a backwards compatible model for teamMembers and encryptionKeys that respects and segregates the current environment that's active.

Before landing:

• [x] Using / passing the environmentOptions defined by meta file through into encryption
• [x] Using parsing context in the encryption parsing extension
• [ ] Make encryption key IDs unambiguous across different environments - don't reuse the same IDs
• [ ] E2E encryption workflow tests for multi-environment setup
• [ ] Deciding the correct way to handle untrusting team members when using multi-env
• [ ] Updates for documentation to reflect this capability
• [ ] Unit testing the functions that accept environmentOptions
• [ ] Unit testing for encryption parsing extension using environment-specific team members
• [ ] Robust options in any related CLI commands
• [ ] Provide support for suffixed environment variable keys (APP_CONFIG_SECRETS_KEY_PROD)
• [ ] Beta testing w/ launchcode

Closes #105

[gregnr]

Awesome work, this is becoming a much-needed feature at LC.

Trying to piece together how this is implemented. Hard to see the full picture w/o an example or tests, so forgive me if some of these questions are off-base:

• Does this work by adding new properties under teamMembers and encryptionKeys which represent the environment, ie:

  .app-config.meta.yml:

  ...
  teamMembers:
    dev:
      - ...
      - ...
    qa:
      - ...
      - ...
    ...
  encryptionKeys:
    dev:
      - ...
      - ...
    qa:
      - ...
      - ...
    ...
  

• If the above is true, have we considered using the $env extension somehow to be more idiomatic with other app-config patterns? (asking this in full naivety - maybe this can't work - or maybe is already the case!):

  .app-config.meta.yml:

  ...
  teamMembers:
    $env:
      dev:
        - ...
        - ...
      qa:
        - ...
        - ...
      ...
  encryptionKeys:
    $env:
      dev:
        - ...
        - ...
      qa:
        - ...
        - ...
      ...
  

• Do we handle default or fallback environments for teamMembers/encryptionKeys? It would be handy to eg. have a default team member/encryption key for all environments except production, which has a separate set of team members/encryption keys.

  If we're using $env, I imagine this would "just work".

• For CLI, am I correct to say we have a new --environment-override flag when handling secrets? Would specifying the env via environment variable also work? eg:

  NODE_ENV=qa app-config secret encrypt
  

• For secret CLI, do we consider environmentAliases and environmentSourceNames overrides set in the meta file? (there might be a potential chicken-egg scenario here).

  Ref: https://app-config.dev/guide/intro/settings.html#parse-and-loading-configuration

• If a team member is trusted in both qa and prod (for example), does their public key exist twice in the meta file, once per environment?

  For the record, this is something I'm okay with and prefer - it keeps the implementation simple (vs. trying to normalize team member keys in one place, then reference them in multiple environments).

• Ideally we should provide a friendly "environment aware" error message in situations where app-config is trying to decrypt a config value that came from an environment that the current user isn't trusted in.

• I can predict us often needing to trust the "CI" team member in multiple environments. I imagine security best practice here would be to create separate CI keys per environment (in case one key is compromised, the scope is limited). This is going to make things tricky for CI's injected APP_CONFIG_SECRETS_KEY environment variable - how do we have multiple?

  Edit: In GitLab Premium, environment-specific variables can be set (ie. different APP_CONFIG_SECRETS_KEY for qa vs. prod). Not sure how easy this is to do in other CI systems.

• With regard to untrusting - this should be as simple as setting the correct environment in the CLI command as part of the untrust operation, correct? Or is there more to this that I'm missing?

[joelgallant]

If the above is true, have we considered using the $env extension somehow to be more idiomatic with other app-config patterns? (asking this in full naivety - maybe this can't work - or maybe is already the case!):

This could certainly be done (you're correct that I omitted $env because it has to be treated first-class in encryption stuff anyways). We have warnings when keys are $ prefixed and not processed by a parsing extension, although I don't think that applies here. I'm not strongly opinionated other than saying "$env isn't technically being used here", but that might be an implementation detail that's hidden (ie what if $env was extended to support new functionality but couldn't be reflected here).

Do we handle default or fallback environments for teamMembers/encryptionKeys? It would be handy to eg. have a default team member/encryption key for all environments except production, which has a separate set of team members/encryption keys.

Yup, this is built in to the PoC, ditto for none.

For CLI, am I correct to say we have a new --environment-override flag when handling secrets? Would specifying the env via environment variable also work?

Both ways are intended to work, for sure. The flags are there as a consistency measure, and largely for --environment-variable-name.

For secret CLI, do we consider environmentAliases and environmentSourceNames overrides set in the meta file? (there might be a potential chicken-egg scenario here).

This is (one of) the reason that I didn't actually use the $env parser. The environment resolving happens after meta file has been loaded. I think I missed actually passing those values through, but that's the spirit of why it's not quite as simple as it looks.

If a team member is trusted in both qa and prod (for example), does their public key exist twice in the meta file, once per environment?

Yup, this is the simplest way to handle it. Indeed, I've thought about similar extensions to $env for deduplication like this (#111). You could use YAML anchors, but they'd be erased if you use the CLI to write the meta file again.

Ideally we should provide a friendly "environment aware" error message in situations where app-config is trying to decrypt a config value that came from an environment that the current user isn't trusted in.

I put some messages in already to try and be specific, but they could be improved.

I can predict us often needing to trust the "CI" team member in multiple environments. I imagine security best practice here would be to create separate CI keys per environment (in case one key is compromised, the scope is limited). This is going to make things tricky for CI's injected APP_CONFIG_SECRETS_KEY environment variable - how do we have multiple?

Yeah, my answer here would default to "whatever CI providers give you" to define that variable differently per env. It's not too difficult to support a suffixed key though (APP_CONFIG_SECRETS_KEY_PROD). Think it's worth it?

With regard to untrusting - this should be as simple as setting the correct environment in the CLI command as part of the untrust operation, correct? Or is there more to this that I'm missing?

Yeah naturally, it will only remove that team member for the current environment (set by override or APP_CONFIG_ENV). Normally though, I would assume the use case for untrusting involves untrusting for all environments. Maybe it's as simple as an extra CLI flag for that, or a prompt.

[gregnr]

I'm not strongly opinionated other than saying "$env isn't technically being used here", but that might be an implementation detail that's hidden (ie what if $env was extended to support new functionality but couldn't be reflected here).

I agree 100%. If there's no way to fully support $env in the meta file, we shouldn't be calling it $env. The inconsistency will cause too much confusion.

Yup, this is built in to the PoC, ditto for none.

Thoughts on calling this default instead of none? Eg. in situations where we do set an environment, but that environment doesn't exist under teamMembers.

The environment resolving happens after meta file has been loaded.

Yeah, this makes sense.

Normally though, I would assume the use case for untrusting involves untrusting for all environments. Maybe it's as simple as an extra CLI flag for that, or a prompt.

Good point. I think those are both great options.

[joelgallant]

Thoughts on calling this default instead of none? Eg. in situations where we do set an environment, but that environment doesn't exist under teamMembers.

This is already the case, both are distinct and supported in $env and here now.

[gregnr]

Thoughts on calling this default instead of none? Eg. in situations where we do set an environment, but that environment doesn't exist under teamMembers.

This is already the case, both are distinct and supported in $env and here now.

Amazing.

This PR LGTM pending tests, documentation, etc.

[codecov-commenter]

Codecov Report

Merging #158 (6b7575a) into master (8e854db) will decrease coverage by 1.27%. The diff coverage is 41.60%.

@@            Coverage Diff             @@
##           master     #158      +/-   ##
==========================================
- Coverage   80.43%   79.16%   -1.28%     
==========================================
  Files          42       42              
  Lines        2453     2529      +76     
  Branches      586      608      +22     
==========================================
+ Hits         1973     2002      +29     
- Misses        480      527      +47     

Impacted Files	Coverage Δ
app-config-meta/src/index.ts	66.66% <ø> (ø)
app-config-cli/src/index.ts	50.78% <7.69%> (-2.31%)	:arrow_down:
app-config-encryption/src/secret-agent.ts	71.31% <33.33%> (-0.59%)	:arrow_down:
app-config-encryption/src/encryption.ts	70.43% <49.43%> (-5.71%)	:arrow_down:
app-config-encryption/src/index.ts	94.44% <100.00%> (+0.69%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8e854db...6b7575a. Read the comment docs.