Repository: godaddy/kubernetes-external-secrets (js)

Issue:
Handling Vault token with Max TTL
Hello,

At the moment Kubernetes External Secrets is not able to handle tokens with [Max TTL](https://www.vaultproject.io/docs/concepts/tokens#the-general-case).

Based on [the code](https://github.com/godaddy/kubernetes-external-secrets/blob/a52987b7b4951dc7e6f50f92a8bd8a45aa6f8ef1/lib/backends/vault-backend.js#L42), KES only get new token if it's not there or renew the current token. Which doesn't work if the Vault role has Max TTL (which is a hard limit for token lifetime, even if it's renewed it cannot exceed the Max TTL).

After that Max TTL, KES stuck and tries to used that expired token: 
```
{"level":50,"time":1600252350767,"pid":17,"hostname":"kubernetes-external-secrets-5586c654c8-qnvvv","response":{"statusCode":403,"body":{"errors":["permission denied"]}},"stack":"Error: permission denied
    at handleVaultResponse (/app/node_modules/node-vault/src/index.js:58:21)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async VaultBackend._get (/app/lib/backends/vault-backend.js:52:27)
    at async /app/lib/backends/kv-backend.js:29:31
    at async Promise.all (index 0)
    at async Promise.all (index 1)
    at async VaultBackend.getSecretManifestData (/app/lib/backends/kv-backend.js:116:42)
    at async Poller._createSecretManifest (/app/lib/poller.js:93:18)
    at async Poller._upsertKubernetesSecret (/app/lib/poller.js:157:28)","type":"Error","msg":"failure while polling the secret default/secert-created-by-kes"}
```

The solution is to try to get a new token instead just renew.
I can implement a fix for this, but not sure what is the best way to make it since I'm not familiar with JS style and conventions.

Thanks

Pull request:
Handle token failures
(note this also includes PR #488 - will tidy up after that one is merged)

This fixes #484

Patch:
diff --git a/config/index.js b/config/index.js
index c9f5d7b..d047095 100644
--- a/config/index.js
+++ b/config/index.js
@@ -83,6 +83,7 @@ const vaultFactory = () => vault(vaultOptions)
 // expires and with at least one remaining poll opportunty to retry renewal if it fails.
 const vaultTokenRenewThreshold = envConfig.vaultTokenRenewThreshold
   ? Number(envConfig.vaultTokenRenewThreshold) : 3 * envConfig.pollerIntervalMilliseconds / 1000
+
 const vaultBackend = new VaultBackend({
   vaultFactory: vaultFactory,
   tokenRenewThreshold: vaultTokenRenewThreshold,
diff --git a/lib/backends/vault-backend.js b/lib/backends/vault-backend.js
index 4674bc3..73cff2c 100644
--- a/lib/backends/vault-backend.js
+++ b/lib/backends/vault-backend.js
@@ -53,6 +53,29 @@ class VaultBackend extends KVBackend {
       this._clients.set(clientCacheKey, client)
     }
 
+    // If we already have a cached token then inspect it...
+    if (client.token) {
+      try {
+        this._logger.debug(`checking vault token expiry for role ${vaultRoleGet} on ${vaultMountPointGet}`)
+        const tokenStatus = await client.tokenLookupSelf()
+        this._logger.debug(`vault token (role ${vaultRoleGet} on ${vaultMountPointGet}) valid for ${tokenStatus.data.ttl} seconds, renews at ${this._tokenRenewThreshold}`)
+
+        // If it needs renewing, renew it.
+        if (Number(tokenStatus.data.ttl) <= this._tokenRenewThreshold) {
+          this._logger.debug(`renewing role ${vaultRoleGet} on ${vaultMountPointGet} vault token`)
+          if (!(await client.tokenRenewSelf())) {
+            this._logger.debug(`cached token renewal failed.  Clearing cached token for role ${vaultRoleGet} on ${vaultMountPointGet}`)
+            client.token = null
+          }
+        }
+      } catch {
+        // If it can't be inspected/renewed, we clear the token.
+        this._logger.debug(`cached token operation failed.  Clearing cached token for role ${vaultRoleGet} on ${vaultMountPointGet}`)
+        client.token = null
+      }
+    }
+
+    // If we don't have a token here we either never had one or we just failed to renew it, so get a new one by logging-in
     if (!client.token) {
       const jwt = this._fetchServiceAccountToken()
       this._logger.debug(`fetching new token from vault for role ${vaultRoleGet} on ${vaultMountPointGet}`)
@@ -61,15 +84,6 @@ class VaultBackend extends KVBackend {
         role: vaultRoleGet,
         jwt: jwt
       })
-    } else {
-      this._logger.debug(`checking vault token expiry for role ${vaultRoleGet} on ${vaultMountPointGet}`)
-      const tokenStatus = await client.tokenLookupSelf()
-      this._logger.debug(`vault token (role ${vaultRoleGet} on ${vaultMountPointGet}) valid for ${tokenStatus.data.ttl} seconds, renews at ${this._tokenRenewThreshold}`)
-
-      if (Number(tokenStatus.data.ttl) <= this._tokenRenewThreshold) {
-        this._logger.debug(`renewing role ${vaultRoleGet} on ${vaultMountPointGet} vault token`)
-        await client.tokenRenewSelf()
-      }
     }
 
     this._logger.debug(`reading secret key ${key} from vault`)


Test patch:
diff --git a/lib/backends/vault-backend.test.js b/lib/backends/vault-backend.test.js
index 319fa7b..106a878 100644
--- a/lib/backends/vault-backend.test.js
+++ b/lib/backends/vault-backend.test.js
@@ -40,6 +40,7 @@ describe('VaultBackend', () => {
   }
 
   const vaultTokenRenewThreshold = 30
+
   const mockTokenLookupResultMustRenew = {
     data: {
       ttl: 15
@@ -213,6 +214,93 @@ describe('VaultBackend', () => {
       expect(secretPropertyValue).equals(quotedSecretValue)
     })
 
+    it('returns secret property value after failed renew token and then relogin', async () => {
+      clientMock.token = 'an-existing-token'
+      clientMock.tokenLookupSelf = sinon.stub().returns(mockTokenLookupResultMustRenew)
+      clientMock.tokenRenewSelf = sinon.stub().returns(false)
+
+      const secretPropertyValue = await vaultBackend._get({
+        specOptions: {
+          vaultMountPoint: mountPoint,
+          vaultRole: role
+        },
+        key: secretKey
+      })
+
+      // check the token happened ...
+      sinon.assert.calledOnce(clientMock.tokenLookupSelf)
+
+      // ... then try to renew the token ...
+      sinon.assert.calledOnce(clientMock.tokenRenewSelf)
+
+      // ... now relogin due to failure to renew ...
+      sinon.assert.calledOnce(clientMock.kubernetesLogin)
+
+      // ... then we fetch the secret ...
+      sinon.assert.calledWith(clientMock.read, secretKey)
+
+      // ... and expect to get its proper value
+      expect(secretPropertyValue).equals(quotedSecretValue)
+    })
+
+    it('returns secret property value after errored token inspection and then relogin', async () => {
+      clientMock.token = 'an-existing-token'
+      clientMock.tokenLookupSelf = sinon.stub().throws(new Error())
+      clientMock.tokenRenewSelf = sinon.stub().returns(false)
+
+      const secretPropertyValue = await vaultBackend._get({
+        specOptions: {
+          vaultMountPoint: mountPoint,
+          vaultRole: role
+        },
+        key: secretKey
+      })
+
+      // check the token happened ...
+      sinon.assert.calledOnce(clientMock.tokenLookupSelf)
+
+      // ... didn't try to renew the token ...
+      sinon.assert.notCalled(clientMock.tokenRenewSelf)
+
+      // ... but instead just relogin due to error upon inspection ...
+      sinon.assert.calledOnce(clientMock.kubernetesLogin)
+
+      // ... then we fetch the secret ...
+      sinon.assert.calledWith(clientMock.read, secretKey)
+
+      // ... and expect to get its proper value
+      expect(secretPropertyValue).equals(quotedSecretValue)
+    })
+
+    it('returns secret property value after errored token renewal and then relogin', async () => {
+      clientMock.token = 'an-existing-token'
+      clientMock.tokenLookupSelf = sinon.stub().returns(mockTokenLookupResultMustRenew)
+      clientMock.tokenRenewSelf = sinon.stub().throws(new Error())
+
+      const secretPropertyValue = await vaultBackend._get({
+        specOptions: {
+          vaultMountPoint: mountPoint,
+          vaultRole: role
+        },
+        key: secretKey
+      })
+
+      // check the token happened ...
+      sinon.assert.calledOnce(clientMock.tokenLookupSelf)
+
+      // ... didn't try to renew the token ...
+      sinon.assert.calledOnce(clientMock.tokenRenewSelf)
+
+      // ... but instead just relogin due to error upon renewal ...
+      sinon.assert.calledOnce(clientMock.kubernetesLogin)
+
+      // ... then we fetch the secret ...
+      sinon.assert.calledWith(clientMock.read, secretKey)
+
+      // ... and expect to get its proper value
+      expect(secretPropertyValue).equals(quotedSecretValue)
+    })
+
     it('returns secret property value if a token exists that does not need renewal', async () => {
       clientMock.token = 'an-existing-token'
       clientMock.tokenLookupSelf = sinon.stub().returns(mockTokenLookupResultNoRenew)