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

@@ -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"

.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:

.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

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:

Makefile

@@ -0,0 +1,3 @@
+.PHONY: local-test
+local-test:
+	docker compose down;  docker-compose up -d vault && act workflow_dispatch -j local-test

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
 ```

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);
     });
 });

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: {

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');

src/retries.test.js

@@ -66,4 +66,4 @@ describe('exportSecrets retries', () => {
             done();
         });
     });
-});
+});

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
-}
+}
+