From c4fdea666fc75eabdcdbf1a863902f295864b740 Mon Sep 17 00:00:00 2001 From: Derk Muenchhausen Date: Mon, 9 Dec 2019 21:01:41 +0100 Subject: [PATCH] feat: allow enforcing naming conventions for key names, limiting which keys can be fetched from backends (#230) * feat(naming-convention): enforcing naming conventions for AWS Secrets Manager entries closes #178 * chore: fix typo namspace to namespace * chore(naming-convention): early return on isPermitted also improved wording for naming permitted annotation related #178 * docs(naming-convention): initial documentation related #178 --- README.md | 26 +++++++++++++ bin/daemon.js | 4 +- config/environment.js | 2 + e2e/tests/secrets-manager.test.js | 2 +- e2e/tests/ssm.test.js | 2 +- lib/poller-factory.js | 3 ++ lib/poller.js | 36 ++++++++++++++++-- lib/poller.test.js | 62 ++++++++++++++++++++++++++++++- 8 files changed, 130 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 19028a7f..f851e352 100644 --- a/README.md +++ b/README.md @@ -184,6 +184,32 @@ data: password: MTIzNA== ``` +## Enforcing naming conventions for backend keys + +by default an `ExternalSecret` may access arbitrary keys from the backend e.g. + +```yml + data: + - key: /dev/cluster1/core-namespace/hello-service/password + name: password +``` + +An enforced naming convention helps to keep the structure tidy and limits the access according +to your naming schema. + +Configure the schema as regular expression in the namespace using an annotation. +This allows `ExternalSecrets` in `core-namespace` just to access secrets that start with +`/dev/cluster1/core-namespace/`: + +```yaml +kind: Namespace +metadata: + name: core-namespace + annotations: + # annotation key is configurable + externalsecrets.kubernetes-client.io/permitted-key-name: "/dev/cluster1/core-namespace/.*" +``` + ## Deprecations A few properties has changed name overtime, we still maintain backwards compatbility with these but they will eventually be removed, and they are not validated using the CRD validation. diff --git a/bin/daemon.js b/bin/daemon.js index ba3ee28e..b76064b5 100755 --- a/bin/daemon.js +++ b/bin/daemon.js @@ -23,7 +23,8 @@ const { metricsPort, pollerIntervalMilliseconds, pollingDisabled, - rolePermittedAnnotation + rolePermittedAnnotation, + namingPermittedAnnotation } = require('../config') async function main () { @@ -49,6 +50,7 @@ async function main () { metrics, pollerIntervalMilliseconds, rolePermittedAnnotation, + namingPermittedAnnotation, customResourceManifest, pollingDisabled, logger diff --git a/config/environment.js b/config/environment.js index ae745523..5435ee66 100644 --- a/config/environment.js +++ b/config/environment.js @@ -24,6 +24,7 @@ const logLevel = process.env.LOG_LEVEL || 'info' const pollingDisabled = 'DISABLE_POLLING' in process.env const rolePermittedAnnotation = process.env.ROLE_PERMITTED_ANNOTATION || 'iam.amazonaws.com/permitted' +const namingPermittedAnnotation = process.env.NAMING_PERMITTED_ANNOTATION || 'externalsecrets.kubernetes-client.io/permitted-key-name' const metricsPort = process.env.METRICS_PORT || 3001 @@ -33,6 +34,7 @@ module.exports = { pollerIntervalMilliseconds, metricsPort, rolePermittedAnnotation, + namingPermittedAnnotation, pollingDisabled, logLevel } diff --git a/e2e/tests/secrets-manager.test.js b/e2e/tests/secrets-manager.test.js index 3d378d19..24c978cb 100644 --- a/e2e/tests/secrets-manager.test.js +++ b/e2e/tests/secrets-manager.test.js @@ -200,7 +200,7 @@ describe('secretsmanager', async () => { .externalsecrets(`e2e-secretmanager-permitted-tls-${uuid}`) .get() expect(result).to.not.equal(undefined) - expect(result.body.status.status).to.contain('namspace does not allow to assume role let-me-be-root') + expect(result.body.status.status).to.contain('namespace does not allow to assume role let-me-be-root') }) }) }) diff --git a/e2e/tests/ssm.test.js b/e2e/tests/ssm.test.js index 48af00e8..a2bc137b 100644 --- a/e2e/tests/ssm.test.js +++ b/e2e/tests/ssm.test.js @@ -122,7 +122,7 @@ describe('ssm', async () => { .externalsecrets(`e2e-ssm-permitted-${uuid}`) .get() expect(result).to.not.equal(undefined) - expect(result.body.status.status).to.contain('namspace does not allow to assume role let-me-be-root') + expect(result.body.status.status).to.contain('namespace does not allow to assume role let-me-be-root') }) }) }) diff --git a/lib/poller-factory.js b/lib/poller-factory.js index d5c71b93..7b7f8eb2 100644 --- a/lib/poller-factory.js +++ b/lib/poller-factory.js @@ -19,6 +19,7 @@ class PollerFactory { metrics, pollerIntervalMilliseconds, rolePermittedAnnotation, + namingPermittedAnnotation, customResourceManifest, pollingDisabled, logger @@ -30,6 +31,7 @@ class PollerFactory { this._pollerIntervalMilliseconds = pollerIntervalMilliseconds this._customResourceManifest = customResourceManifest this._rolePermittedAnnotation = rolePermittedAnnotation + this._namingPermittedAnnotation = namingPermittedAnnotation this._pollingDisabled = pollingDisabled } @@ -46,6 +48,7 @@ class PollerFactory { metrics: this._metrics, customResourceManifest: this._customResourceManifest, rolePermittedAnnotation: this._rolePermittedAnnotation, + namingPermittedAnnotation: this._namingPermittedAnnotation, pollingDisabled: this._pollingDisabled, externalSecret }) diff --git a/lib/poller.js b/lib/poller.js index ebbf7db4..ea612518 100644 --- a/lib/poller.js +++ b/lib/poller.js @@ -38,6 +38,7 @@ class Poller { metrics, customResourceManifest, rolePermittedAnnotation, + namingPermittedAnnotation, pollingDisabled, externalSecret }) { @@ -49,6 +50,7 @@ class Poller { this._metrics = metrics this._pollingDisabled = pollingDisabled this._rolePermittedAnnotation = rolePermittedAnnotation + this._namingPermittedAnnotation = namingPermittedAnnotation this._customResourceManifest = customResourceManifest this._externalSecret = externalSecret @@ -187,21 +189,49 @@ class Poller { * @param {Object} descriptor secret descriptor */ _isPermitted (namespace, descriptor) { - const role = descriptor.roleArn let allowed = true let reason = '' - if (!namespace.metadata.annotations || !role) { + if (!namespace.metadata.annotations) { return { allowed, reason } } + + // 1. testing naming convention if defined in namespace + + const externalData = descriptor.data || descriptor.properties + const namingConvention = namespace.metadata.annotations[this._namingPermittedAnnotation] + + if (namingConvention) { + externalData.forEach((secretProperty, index) => { + const reNaming = new RegExp(namingConvention) + if (!reNaming.test(secretProperty.key)) { + allowed = false + reason = `key name does not match naming convention ${namingConvention}` + return { + allowed, reason + } + } + }) + } + + // 2. testing assume role if configured + + const role = descriptor.roleArn + + if (!role) { + return { + allowed, reason + } + } + // an empty annotation value allows access to all roles const re = new RegExp(namespace.metadata.annotations[this._rolePermittedAnnotation]) if (!re.test(role)) { allowed = false - reason = `namspace does not allow to assume role ${role}` + reason = `namespace does not allow to assume role ${role}` } return { diff --git a/lib/poller.test.js b/lib/poller.test.js index 8b917cd7..ce747eb6 100644 --- a/lib/poller.test.js +++ b/lib/poller.test.js @@ -26,6 +26,7 @@ describe('Poller', () => { }) const rolePermittedAnnotation = 'iam.amazonaws.com/permitted' + const namingPermittedAnnotation = 'externalsecrets.kubernetes-client.io/permitted-key-name' beforeEach(() => { backendMock = sinon.mock() @@ -93,6 +94,7 @@ describe('Poller', () => { logger: loggerMock, externalSecret: fakeExternalSecret, rolePermittedAnnotation, + namingPermittedAnnotation, customResourceManifest: fakeCustomResourceManifest }) } @@ -636,7 +638,7 @@ describe('Poller', () => { } expect(error).to.not.equal(undefined) - expect(error.message).equals('not allowed to fetch secret: fakeNamespace/fakeSecretName: namspace does not allow to assume role arn:aws:iam::123456789012:role/test-role') + expect(error.message).equals('not allowed to fetch secret: fakeNamespace/fakeSecretName: namespace does not allow to assume role arn:aws:iam::123456789012:role/test-role') }) it('fails storing secret', async () => { @@ -745,6 +747,64 @@ describe('Poller', () => { } ] + for (let i = 0; i < testcases.length; i++) { + const testcase = testcases[i] + const verdict = poller._isPermitted(testcase.ns, testcase.descriptor) + expect(verdict.allowed).to.equal(testcase.permitted) + } + }) + }) + describe('nameing conventions', () => { + let poller + beforeEach(() => { + poller = pollerFactory() + }) + it('should restrict access as defined in namespace naming convention ', () => { + const testcases = [ + { + // no annotations at all + ns: { metadata: {} }, + descriptor: {}, + permitted: true + }, + { + // empty annotation + ns: { metadata: { annotations: { [namingPermittedAnnotation]: '' } } }, + descriptor: {}, + permitted: true + }, + { + // test regex + ns: { metadata: { annotations: { [namingPermittedAnnotation]: '.*' } } }, + descriptor: { + data: [ + { key: 'whatever', name: 'somethingelse' } + ] + }, + permitted: true + }, + { + // test regex + ns: { metadata: { annotations: { [namingPermittedAnnotation]: 'dev/team-a/.*' } } }, + descriptor: { + data: [ + { key: 'dev/team-a/secret', name: 'somethingelse' } + ] + }, + permitted: true + }, + { + // test regex + ns: { metadata: { annotations: { [namingPermittedAnnotation]: 'dev/team-a/.*' } } }, + descriptor: { + data: [ + { key: 'dev/team-b/secret', name: 'somethingelse' } + ] + }, + permitted: false + } + ] + for (let i = 0; i < testcases.length; i++) { const testcase = testcases[i] const verdict = poller._isPermitted(testcase.ns, testcase.descriptor)