From b9f4d16071cc05770f25c06069902ea654aae801 Mon Sep 17 00:00:00 2001 From: John-Michael Faircloth Date: Wed, 21 Jun 2023 11:55:50 -0500 Subject: [PATCH] fix secrets stored in json format (#466) * fix secrets in json format * fix actionlint * add more comments and docs * revert build.yml test * add test for json * fix selector * fix e2e test * fix e2e test 2 * remove test * remove isNaN check * update changelog --- .github/workflows/actionlint.yaml | 4 ++- .github/workflows/build.yml | 21 +++++++++++++ .github/workflows/local-test.yaml | 49 ++++++++++++++++++++++++----- CHANGELOG.md | 8 +++++ Makefile | 3 ++ README.md | 26 ++++++++-------- integrationTests/e2e/e2e.test.js | 5 +++ integrationTests/e2e/setup.js | 25 +++++++++++++++ src/action.test.js | 44 ++++++++++++++++++++++++-- src/retries.test.js | 2 +- src/secrets.js | 51 +++++++++++++++++++++++++++---- 11 files changed, 206 insertions(+), 32 deletions(-) create mode 100644 Makefile diff --git a/.github/workflows/actionlint.yaml b/.github/workflows/actionlint.yaml index ee79a646..b8c9d8e9 100644 --- a/.github/workflows/actionlint.yaml +++ b/.github/workflows/actionlint.yaml @@ -16,4 +16,6 @@ jobs: # in our e2e tests. # This error occurs because vault-action's outputs are dynamic but # actionlint expects action.yml to define them. - args: '-ignore "property \"othersecret\" is not defined in object type"' + args: > + -ignore "property \"othersecret\" is not defined in object type" + -ignore "property \"jsonstring\" is not defined in object type" diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 778f18c9..c4de30c9 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -153,6 +153,27 @@ jobs: my-secret/test altSecret | NAMED_ALTSECRET ; my-secret/nested/test otherAltSecret ; + # The ordering of these two Test Vault Action JSON String Format steps matters + - name: Test Vault Action JSON String Format (part 1/2) + id: import-secrets + uses: ./ + with: + url: http://localhost:8200 + token: testtoken + secrets: | + secret/data/test-json-string jsonString | JSON_STRING ; + secret/data/test-json-data jsonData | JSON_DATA ; + + - name: Test Vault Action JSON String Format (part 2/2) + run: | + echo "${{ steps.import-secrets.outputs.jsonString }}" > string.json + echo "${{ steps.import-secrets.outputs.jsonData }}" > data.json + cat string.json + cat data.json + # we should be able to parse the output as JSON + jq -c . < string.json + jq -c . < data.json + - name: Test Vault Action (cubbyhole) uses: ./ with: diff --git a/.github/workflows/local-test.yaml b/.github/workflows/local-test.yaml index fb3bce3b..4bb06131 100644 --- a/.github/workflows/local-test.yaml +++ b/.github/workflows/local-test.yaml @@ -18,11 +18,44 @@ jobs: name: local-test runs-on: ubuntu-latest steps: - - name: Import Secrets - uses: hashicorp/vault-action@YOUR_BRANCH_NAME - with: - url: http://localhost:8200 - method: token - token: testtoken - secrets: | - secret/data/test secret | SAMPLE_SECRET; + - uses: actions/checkout@8e5e7e5ab8b370d6c329ec480221332ada57f0ab # v3.5.2 + + - uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c # v3.6.0 + with: + node-version: '16.14.0' + + - name: NPM Install + run: npm ci + + - name: NPM Build + run: npm run build + + - name: Setup Vault + run: node ./integrationTests/e2e/setup.js + env: + VAULT_HOST: localhost + VAULT_PORT: 8200 + + - name: Import Secrets + id: import-secrets + # use the local changes + uses: ./ + # run against a specific version of vault-action + # uses: hashicorp/vault-action@v2.1.2 + with: + url: http://localhost:8200 + method: token + token: testtoken + secrets: | + secret/data/test-json-string jsonString; + + - name: Check Secrets + run: | + touch secrets.json + echo "${{ steps.import-secrets.outputs.jsonString }}" >> secrets.json + + - name: Check json file format + run: | + echo + cat secrets.json + jq -c . < secrets.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a530aed..cc60d6db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ ## Unreleased +Bugs: + +* Fix a regression that broke support for secrets in JSON format [GH-466](https://github.com/hashicorp/vault-action/pull/466) + +Improvements: + +* Fix a warning about outputToken being an unexpected input [GH-461](https://github.com/hashicorp/vault-action/pull/461) + ## 2.6.0 (June 7, 2023) Features: diff --git a/Makefile b/Makefile new file mode 100644 index 00000000..91758e68 --- /dev/null +++ b/Makefile @@ -0,0 +1,3 @@ +.PHONY: local-test +local-test: + docker compose down; docker-compose up -d vault && act workflow_dispatch -j local-test diff --git a/README.md b/README.md index 5613c61c..4dd5c489 100644 --- a/README.md +++ b/README.md @@ -547,24 +547,22 @@ $ npm run test:integration:basic # Choose one of: basic, enterprise, e2e, e2e-tl ### Running the action locally You can use the [act](https://github.com/nektos/act) command to test your -changes locally if desired. Unfortunately it is not currently possible to use -uncommitted local changes for a shared workfow. You will still need to push the -changes you would like to validate beforehand. Even if a commit is necessary, -this is still a more detailed and faster feedback loop than waiting for the -action to be executed by Github in a different repository. +changes locally. + +Edit the ./.github/workflows/local-test.yaml file and add any steps necessary +to test your changes. You may have to additionally edit the Vault url, token +and secret path if you are not using one of the provided containerized +instances. The `local-test` job will call the ./integrationTests/e2e/setup.js +script to bootstrap your local Vault instance with secrets. + +Run your feature branch locally: -Push your changes into a feature branch. ```sh -$ git checkout -b my-feature-branch -$ git commit -m "testing new changes" -$ git push +act workflow_dispatch -j local-test ``` -Edit the ./.github/workflows/local-test.yaml file to use your new feature -branch. You may have to additionally edit the vault url, token and secret path -if you are not using one of the provided containerized instance. Run your -feature branch locally. +Or use the provided make target which will also spin up a Vault container: ```sh -$ act workflow_dispatch -j local-test +make local-test ``` diff --git a/integrationTests/e2e/e2e.test.js b/integrationTests/e2e/e2e.test.js index 6495d14e..3e43490f 100644 --- a/integrationTests/e2e/e2e.test.js +++ b/integrationTests/e2e/e2e.test.js @@ -10,5 +10,10 @@ describe('e2e', () => { expect(process.env.FOO).toBe("bar"); expect(process.env.NAMED_CUBBYSECRET).toBe("zap"); expect(process.env.SUBSEQUENT_TEST_SECRET).toBe("SUBSEQUENT_TEST_SECRET"); + + const jsonString = '{"x":1,"y":"qux"}'; + let jsonResult = JSON.stringify(jsonString); + jsonResult = jsonResult.substring(1, jsonResult.length - 1); + expect(process.env.JSON_STRING).toBe(jsonResult); }); }); diff --git a/integrationTests/e2e/setup.js b/integrationTests/e2e/setup.js index 96f2295f..0766820f 100644 --- a/integrationTests/e2e/setup.js +++ b/integrationTests/e2e/setup.js @@ -3,6 +3,7 @@ const got = require('got'); const vaultUrl = `${process.env.VAULT_HOST}:${process.env.VAULT_PORT}`; const vaultToken = `${process.env.VAULT_TOKEN}` === undefined ? `${process.env.VAULT_TOKEN}` : "testtoken"; + (async () => { try { // Verify Connection @@ -36,6 +37,30 @@ const vaultToken = `${process.env.VAULT_TOKEN}` === undefined ? `${process.env.V } }); + await got(`http://${vaultUrl}/v1/secret/data/test-json-string`, { + method: 'POST', + headers: { + 'X-Vault-Token': vaultToken, + }, + json: { + data: { + jsonString: '{"x":1,"y":"qux"}', + }, + }, + }); + + await got(`http://${vaultUrl}/v1/secret/data/test-json-data`, { + method: 'POST', + headers: { + 'X-Vault-Token': vaultToken, + }, + json: { + data: { + jsonData: {"x":1,"y":"qux"}, + }, + }, + }); + await got(`http://${vaultUrl}/v1/sys/mounts/my-secret`, { method: 'POST', headers: { diff --git a/src/action.test.js b/src/action.test.js index 49c33cd3..f6304007 100644 --- a/src/action.test.js +++ b/src/action.test.js @@ -220,6 +220,22 @@ describe('exportSecrets', () => { expect(core.setOutput).toBeCalledWith('key', '1'); }); + it('json secret retrieval', async () => { + const jsonString = '{"x":1,"y":2}'; + let result = JSON.stringify(jsonString); + result = result.substring(1, result.length - 1); + + mockInput('test key'); + mockVaultData({ + key: jsonString, + }); + + await exportSecrets(); + + expect(core.exportVariable).toBeCalledWith('KEY', result); + expect(core.setOutput).toBeCalledWith('key', result); + }); + it('intl secret retrieval', async () => { mockInput('测试 测试'); mockVaultData({ @@ -334,7 +350,31 @@ describe('exportSecrets', () => { expect(core.setOutput).toBeCalledWith('key', 'secret'); }) - it('multi-line secret gets masked for each line', async () => { + it('multi-line secret', async () => { + const multiLineString = `ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAklOUpkDHrfHY17SbrmTIpNLTGK9Tjom/BWDSU +GPl+nafzlHDTYW7hdI4yZ5ew18JH4JW9jbhUFrviQzM7xlELEVf4h9lFX5QVkbPppSwg0cda3 +Pbv7kOdJ/MTyBlWXFCR+HAo3FXRitBqxiX1nKhXpHAZsMciLq8V6RjsNAQwdsdMFvSlVK/7XA +NrRFi9wrf+M7Q==`; + + mockInput('test key'); + mockVaultData({ + key: multiLineString + }); + mockExportToken("false") + + await exportSecrets(); + + expect(core.setSecret).toBeCalledTimes(5); // 1 for each non-empty line + VAULT_TOKEN + + expect(core.setSecret).toBeCalledWith("EXAMPLE"); // called for VAULT_TOKEN + expect(core.setSecret).toBeCalledWith("ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAklOUpkDHrfHY17SbrmTIpNLTGK9Tjom/BWDSU"); + expect(core.setSecret).toBeCalledWith("GPl+nafzlHDTYW7hdI4yZ5ew18JH4JW9jbhUFrviQzM7xlELEVf4h9lFX5QVkbPppSwg0cda3"); + expect(core.setSecret).toBeCalledWith("Pbv7kOdJ/MTyBlWXFCR+HAo3FXRitBqxiX1nKhXpHAZsMciLq8V6RjsNAQwdsdMFvSlVK/7XA"); + expect(core.setSecret).toBeCalledWith("NrRFi9wrf+M7Q=="); + expect(core.setOutput).toBeCalledWith('key', multiLineString); + }) + + it('multi-line secret gets masked for each non-empty line', async () => { const multiLineString = `a multi-line string with blank lines @@ -348,7 +388,7 @@ with blank lines await exportSecrets(); - expect(core.setSecret).toBeCalledTimes(3); // 1 for each non-empty line. + expect(core.setSecret).toBeCalledTimes(3); // 1 for each non-empty line + VAULT_TOKEN expect(core.setSecret).toBeCalledWith('a multi-line string'); expect(core.setSecret).toBeCalledWith('with blank lines'); diff --git a/src/retries.test.js b/src/retries.test.js index 132edd52..285a7f25 100644 --- a/src/retries.test.js +++ b/src/retries.test.js @@ -66,4 +66,4 @@ describe('exportSecrets retries', () => { done(); }); }); -}); \ No newline at end of file +}); diff --git a/src/secrets.js b/src/secrets.js index 45b26e0a..bc810a81 100644 --- a/src/secrets.js +++ b/src/secrets.js @@ -1,6 +1,5 @@ const jsonata = require("jsonata"); - /** * @typedef {Object} SecretRequest * @property {string} path @@ -67,12 +66,20 @@ async function getSecrets(secretRequests, client) { /** * Uses a Jsonata selector retrieve a bit of data from the result - * @param {object} data - * @param {string} selector + * @param {object} data + * @param {string} selector */ async function selectData(data, selector) { const ata = jsonata(selector); - let result = JSON.stringify(await ata.evaluate(data)); + let d = await ata.evaluate(data); + if (isJSON(d)) { + // If we already have JSON we will not "stringify" it yet so that we + // don't end up calling JSON.parse. This would break the secrets that + // are stored as JSON. See: https://github.com/hashicorp/vault-action/issues/194 + result = d; + } else { + result = JSON.stringify(d); + } // Compat for custom engines if (!result && ((ata.ast().type === "path" && ata.ast()['steps'].length === 1) || ata.ast().type === "string") && selector !== 'data' && 'data' in data) { result = JSON.stringify(await jsonata(`data.${selector}`).evaluate(data)); @@ -81,12 +88,44 @@ async function selectData(data, selector) { } if (result.startsWith(`"`)) { - result = JSON.parse(result); + // we need to strip the beginning and ending quotes otherwise it will + // always successfully parse as JSON + result = result.substring(1, result.length - 1); + if (!isJSON(result)) { + // add the quotes back so we can parse it into a Javascript object + // to allow support for multi-line secrets. See https://github.com/hashicorp/vault-action/issues/160 + result = `"${result}"` + result = JSON.parse(result); + } + } else if (isJSON(result)) { + // This is required to support secrets in JSON format. + // See https://github.com/hashicorp/vault-action/issues/194 and https://github.com/hashicorp/vault-action/pull/173 + result = JSON.stringify(result); + result = result.substring(1, result.length - 1); } return result; } +/** + * isJSON returns true if str parses as a valid JSON string + * @param {string} str + */ +function isJSON(str) { + if (typeof str !== "string"){ + return false; + } + + try { + JSON.parse(str); + } catch (e) { + return false; + } + + return true; +} + module.exports = { getSecrets, selectData -} \ No newline at end of file +} +