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

Add the new Vault module to handle encrypted application secrets #1315

Merged
merged 25 commits into from Jun 11, 2019

Conversation

@Aschen
Copy link
Contributor

commented Jun 4, 2019

What does this PR do ?

Add an integrated Vault to securely store secrets (API key, credentials, ...) in project repositories.
This allows developers to encrypt a secrets file and then commit the encrypted version.
Then Kuzzle is able to decrypt the secrets file and expose it to plugin developers.

Secrets file are JSON file, only the string values are encrypted:

{
  "aws": {
    "key": "silmaril"
  },
  "secret": "ring"
}

You can use the CLI to encrypt the file, the secret key can be either passed as an argument or set as the KUZZLE_VAULT_KEY environment variable:

bin/kuzzle encryptSecrets config/secrets.json --vault-key verylongandstrongkey

Then a file named config/secrets.enc.json is generated:

{
  "aws": {
    "key": "536553f3181ada6f700cac98100f1266.f700cac98100f1266"
  },
  "secret": "3185620ca7e490a8d9dbf79c3e78c1a3.8d9dbf79c3e78c1a3"
}

Kuzzle will automatically try to decrypt this file if present, or you can specify another one using --secrets-file <file> with the bin/kuzzle start command.

Then the secrets will be exposed to plugins developers under context.secrets.

Doc kuzzleio/documentation#321

How should this be manually tested?

  • Step 1 : Encrypt a secrets file: ./bin/kuzzle encryptSecrets config/secrets.json --vault-key azerty
  • Step 2 : Decrypt it: ./bin/kuzzle decryptSecrets config/secrets..enc.json --vault-key azerty
  • Step 3 : Run Kuzzle with it KUZZLE_VAULT_KEY=azerty docker-compose -f docker-compose/dev.yml up
  • Step 4 : Access the secrets inside a plugin init method

Aschen added some commits Jun 4, 2019

@Aschen Aschen self-assigned this Jun 4, 2019

@Aschen Aschen changed the title Integrate a Vault to handle encrypted application secrets Add a Vault to handle encrypted application secrets Jun 5, 2019

@codecov

This comment has been minimized.

Copy link

commented Jun 5, 2019

Codecov Report

Merging #1315 into 1-dev will decrease coverage by 0.25%.
The diff coverage is 68.57%.

Impacted file tree graph

@@            Coverage Diff            @@
##           1-dev    #1315      +/-   ##
=========================================
- Coverage   93.8%   93.55%   -0.26%     
=========================================
  Files         98       99       +1     
  Lines       6991     7061      +70     
=========================================
+ Hits        6558     6606      +48     
- Misses       433      455      +22
Impacted Files Coverage Δ
lib/api/core/plugins/pluginContext.js 95.62% <100%> (+0.03%) ⬆️
lib/api/kuzzle.js 86.76% <100%> (+0.4%) ⬆️
lib/api/core/vault.js 66.15% <66.15%> (ø)

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 ea9205d...e734458. Read the comment docs.

Aschen added some commits Jun 5, 2019

Show resolved Hide resolved bin/commands/decryptSecrets.js Outdated
Show resolved Hide resolved bin/commands/decryptSecrets.js
Show resolved Hide resolved bin/commands/decryptSecrets.js Outdated
Show resolved Hide resolved bin/commands/encryptSecrets.js Outdated
Show resolved Hide resolved default.config.js Outdated
Show resolved Hide resolved lib/api/core/vault.js Outdated
Show resolved Hide resolved lib/api/core/vault.js Outdated
Show resolved Hide resolved lib/api/core/vault.js
Show resolved Hide resolved lib/api/core/vault.js Outdated

Aschen and others added some commits Jun 5, 2019

Apply suggestions from code review
Co-Authored-By: Sébastien Cottinet <scottinet@protonmail.com>
Show resolved Hide resolved bin/commands/decryptSecrets.js Outdated
Show resolved Hide resolved bin/commands/encryptSecrets.js Outdated
Show resolved Hide resolved lib/api/core/vault.js

Aschen and others added some commits Jun 5, 2019

Apply suggestions from code review
Co-Authored-By: Yoann-Abbes <44844010+Yoann-Abbes@users.noreply.github.com>

@Aschen Aschen requested review from scottinet and Yoann-Abbes and removed request for scottinet Jun 5, 2019

@Aschen Aschen changed the title Add a Vault to handle encrypted application secrets Add the Vault to handle encrypted application secrets Jun 5, 2019

Aschen added some commits Jun 5, 2019

nit
nit
@codecov

This comment has been minimized.

Copy link

commented Jun 5, 2019

Codecov Report

Merging #1315 into 1-dev will decrease coverage by 0.23%.
The diff coverage is 68.65%.

Impacted file tree graph

@@            Coverage Diff            @@
##           1-dev    #1315      +/-   ##
=========================================
- Coverage   93.8%   93.56%   -0.24%     
=========================================
  Files         98       99       +1     
  Lines       6992     7059      +67     
=========================================
+ Hits        6559     6605      +46     
- Misses       433      454      +21
Impacted Files Coverage Δ
lib/api/kuzzle.js 86.76% <100%> (+0.4%) ⬆️
lib/api/core/plugins/pluginContext.js 95.62% <100%> (+0.03%) ⬆️
lib/api/core/vault.js 66.12% <66.12%> (ø)

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 abf8974...fe1032e. Read the comment docs.

Show resolved Hide resolved .kuzzlerc.sample Outdated
Show resolved Hide resolved lib/api/core/vault.js Outdated
return null;
})
.catch(error => {
throw new Error(`Can not decrypt secrets: ${error.message}`);

This comment has been minimized.

Copy link
@scottinet

scottinet Jun 6, 2019

Member

(nitpicking) can you write Cannot instead of Can not? I find it easier to the eye :-)
(same goes for other error messages)

*/
_encryptData (data) {
const
IV = crypto.randomBytes(8).toString('hex'),

This comment has been minimized.

Copy link
@scottinet

scottinet Jun 6, 2019

Member

(nitpicking) names starting with an uppercase should be reserved to classes

Suggested change
IV = crypto.randomBytes(8).toString('hex'),
iv = crypto.randomBytes(8).toString('hex'),

(same thing in the _decryptData function below)

bindOption(
program
.command('encryptSecrets <file>')
.option(' --vault-key <vaultKey>', 'Vault key used to encrypt secrets')

This comment has been minimized.

Copy link
@scottinet

scottinet Jun 6, 2019

Member

I just spotted that during my review of this feature's documentation: you're mixing snake-case with camelCase.
These options should be named vaultKey, secretsFile, and so on, like all other commands and options of this CLI.

This comment has been minimized.

Copy link
@Aschen

Aschen Jun 6, 2019

Author Contributor

I do this because vaultKey will be the variable name used in the script but usually command line options are in snake-case

This comment has been minimized.

Copy link
@scottinet

scottinet Jun 6, 2019

Member

I know, and commands should be in snake-case too for the sake of convention... but it's too late now: we have --noColors and commands are all camelCased. :-(

This comment has been minimized.

Copy link
@scottinet

scottinet Jun 6, 2019

Member

I won't block that PR because of this, though

This comment has been minimized.

Copy link
@Aschen

Aschen Jun 6, 2019

Author Contributor

Oh.. I think we can get ride of it in v2

Aschen added some commits Jun 6, 2019

@scottinet scottinet changed the title Add the Vault to handle encrypted application secrets Add the new Vault module to handle encrypted application secrets Jun 11, 2019

@benoitvidis benoitvidis merged commit 5edcf31 into 1-dev Jun 11, 2019

4 checks passed

LGTM analysis: JavaScript No new or fixed alerts
Details
codecov/project 93.56% (-0.24%) compared to abf8974
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sonarqube SonarQube reported no issues

@benoitvidis benoitvidis deleted the KZL-1114-secrets-management branch Jun 11, 2019

scottinet added a commit to kuzzleio/documentation that referenced this pull request Jun 11, 2019

Add guide about secrets management with the Vault (#321)
## What does this PR do?

Document the secrets Vault.  
Also add a page for the new `context.secrets` object exposed in plugin context

kuzzleio/kuzzle#1315

### How should this be manually tested?

<!--
  Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
  Please also list any relevant details for your test configuration
-->
  - Step 1 :
  - Step 2 :
  - Step 3 :  
  ...

### Other changes

<!--
  Please describe here all changes not directly linked to the main issue, but made because of it.
  For instance: issues spotted during this PR and fixed on-the-fly, dependencies update, and so on
-->

### Boyscout

<!--
  Describe here minor improvements in the code base and not directly linked to the main changes:
  typos fixes, better/new comments, small code simplification, new debug messages, and so on.
-->

@Aschen Aschen referenced this pull request Jun 14, 2019

Merged

Release 1.8.0 #1322

Aschen added a commit that referenced this pull request Jun 14, 2019

Merge pull request #1322 from kuzzleio/1.8.0-proposal
Release 1.8.0

Bug fixes

    [ #1311 ] Fix promise leaks (scottinet)
    [ #1298 ] Fix disabled protocol initialization (Aschen)
    [ #1297 ] Fix timeouts on plugin action returing the request (benoitvidis)
    [ #1288 ] Fix an error when trying a non-partial bulk update (scottinet)
    [ #1286 ] Allows bulk inserts on aliases (benoitvidis)
    [ #1282 ] Scan keys on redis cluster (benoitvidis)
    [ #1279 ] Users must be authenticated to use auth:logout (scottinet)

New features

    [ #1315 ] Add the new Vault module to handle encrypted application secrets (Aschen)
    [ #1302 ] Add write and mWrite (Aschen)
    [ #1305 ] Add pipes & hooks wildcard event (thomasarbona)

Enhancements

    [ #1318 ] Add a maximum ttl to auth:login (benoitvidis)
    [ #1301 ] Upgrade the WebSocket libraries (scottinet)
    [ #1308 ] Events triggering refactor (scottinet)
    [ #1300 ] Collection specifications methods cloisoned to a collection (thomasarbona)
    [ #1295 ] Improve validation error messages (benoitvidis)
    [ #1292 ] Throw an error when the realtime controller is invoked by plugin developers (benoitvidis)
    [ #1257 ] Add ability to define mapping policy for new fields (Aschen)
    [ #1291 ] Fix --help on subcommands (Yoann-Abbes)
    [ #1289 ] Handle ping/pong packets (scottinet)
    [ #1273 ] Fix incomplete access logs (scottinet)

Others

    [ #1317 ] Add ps dependency to plugin-dev Docker image for pm2 (benoitvidis)
    [ #1312 ] Check that .kuzzlerc.sample is well-formed (scottinet)
    [ #1299 ] Add Kuzzle Nightly & Redis 3 and 4 test (alexandrebouthinon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.