Merge commit from fork
* Fix secret redaction bypass via Nunjucks filters * Address PR feedback on secret redaction helpers
This commit is contained in:
Ben Lambert
@@ -0,0 +1,5 @@
|
||||
---
|
||||
'@backstage/plugin-scaffolder-backend': patch
|
||||
---
|
||||
|
||||
Fixed a security vulnerability where secrets could bypass log redaction when transformed through Nunjucks filters in scaffolder templates.
|
||||
@@ -896,6 +896,405 @@ describe('NunjucksWorkflowRunner', () => {
|
||||
|
||||
expectTaskLog('info: *** {"thing":"***"}');
|
||||
});
|
||||
|
||||
// eslint-disable-next-line jest/expect-expect
|
||||
it('should redact secrets that have been transformed with a replace filter', async () => {
|
||||
actionRegistry.register({
|
||||
id: 'log-secret',
|
||||
description: 'Mock action for testing',
|
||||
supportsDryRun: true,
|
||||
handler: async ctx => {
|
||||
ctx.logger.info(ctx.input.secret);
|
||||
},
|
||||
schema: {
|
||||
input: {
|
||||
type: 'object',
|
||||
required: ['secret'],
|
||||
properties: {
|
||||
secret: {
|
||||
type: 'string',
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const task = createMockTaskWithSpec(
|
||||
{
|
||||
steps: [
|
||||
{
|
||||
id: 'test',
|
||||
name: 'name',
|
||||
action: 'log-secret',
|
||||
input: {
|
||||
secret: "${{ secrets.backstageToken | replace('.', '_DOT_') }}",
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
{ backstageToken: 'header.payload.signature' },
|
||||
);
|
||||
|
||||
await runner.execute(task);
|
||||
|
||||
expectTaskLog('info: ***');
|
||||
});
|
||||
|
||||
// eslint-disable-next-line jest/expect-expect
|
||||
it('should redact secrets transformed with the upper filter', async () => {
|
||||
actionRegistry.register({
|
||||
id: 'log-secret',
|
||||
description: 'Mock action for testing',
|
||||
supportsDryRun: true,
|
||||
handler: async ctx => {
|
||||
ctx.logger.info(ctx.input.secret);
|
||||
},
|
||||
schema: {
|
||||
input: {
|
||||
type: 'object',
|
||||
required: ['secret'],
|
||||
properties: {
|
||||
secret: {
|
||||
type: 'string',
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const task = createMockTaskWithSpec(
|
||||
{
|
||||
steps: [
|
||||
{
|
||||
id: 'test',
|
||||
name: 'name',
|
||||
action: 'log-secret',
|
||||
input: {
|
||||
secret: '${{ secrets.mySecret | upper }}',
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
{ mySecret: 'super-secret-token' },
|
||||
);
|
||||
|
||||
await runner.execute(task);
|
||||
|
||||
expectTaskLog('info: ***');
|
||||
});
|
||||
|
||||
// eslint-disable-next-line jest/expect-expect
|
||||
it('should redact secrets embedded in a larger string with other text', async () => {
|
||||
actionRegistry.register({
|
||||
id: 'log-secret',
|
||||
description: 'Mock action for testing',
|
||||
supportsDryRun: true,
|
||||
handler: async ctx => {
|
||||
ctx.logger.info(ctx.input.message);
|
||||
},
|
||||
schema: {
|
||||
input: {
|
||||
type: 'object',
|
||||
required: ['message'],
|
||||
properties: {
|
||||
message: {
|
||||
type: 'string',
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const task = createMockTaskWithSpec(
|
||||
{
|
||||
steps: [
|
||||
{
|
||||
id: 'test',
|
||||
name: 'name',
|
||||
action: 'log-secret',
|
||||
input: {
|
||||
message:
|
||||
"scaffold-init:${{ secrets.backstageToken | replace('.', '_DOT_') }}",
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
{ backstageToken: 'header.payload.signature' },
|
||||
);
|
||||
|
||||
await runner.execute(task);
|
||||
|
||||
expectTaskLog('info: ***');
|
||||
});
|
||||
|
||||
// eslint-disable-next-line jest/expect-expect
|
||||
it('should redact environment secrets that have been transformed', async () => {
|
||||
actionRegistry.register({
|
||||
id: 'log-secret',
|
||||
description: 'Mock action for testing',
|
||||
supportsDryRun: true,
|
||||
handler: async ctx => {
|
||||
ctx.logger.info(ctx.input.secret);
|
||||
},
|
||||
schema: {
|
||||
input: {
|
||||
type: 'object',
|
||||
required: ['secret'],
|
||||
properties: {
|
||||
secret: {
|
||||
type: 'string',
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const task = createMockTaskWithSpec(
|
||||
{
|
||||
steps: [
|
||||
{
|
||||
id: 'test',
|
||||
name: 'name',
|
||||
action: 'log-secret',
|
||||
input: {
|
||||
secret: '${{ environment.secrets.AWS_ACCESS_KEY | upper }}',
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
{},
|
||||
);
|
||||
|
||||
await runner.execute(task);
|
||||
|
||||
expectTaskLog('info: ***');
|
||||
});
|
||||
|
||||
it('should not redact non-secret values in rendered input', async () => {
|
||||
actionRegistry.register({
|
||||
id: 'log-secret',
|
||||
description: 'Mock action for testing',
|
||||
supportsDryRun: true,
|
||||
handler: async ctx => {
|
||||
ctx.logger.info(ctx.input.name);
|
||||
},
|
||||
schema: {
|
||||
input: {
|
||||
type: 'object',
|
||||
required: ['name'],
|
||||
properties: {
|
||||
name: {
|
||||
type: 'string',
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const task = createMockTaskWithSpec({
|
||||
steps: [
|
||||
{
|
||||
id: 'test',
|
||||
name: 'name',
|
||||
action: 'log-secret',
|
||||
input: {
|
||||
name: '${{ parameters.serviceName }}',
|
||||
},
|
||||
},
|
||||
],
|
||||
parameters: { serviceName: 'my-service' },
|
||||
});
|
||||
|
||||
await runner.execute(task);
|
||||
|
||||
expectTaskLog('info: my-service');
|
||||
});
|
||||
|
||||
// eslint-disable-next-line jest/expect-expect
|
||||
it('should redact secrets in deeply nested input objects', async () => {
|
||||
actionRegistry.register({
|
||||
id: 'log-secret',
|
||||
description: 'Mock action for testing',
|
||||
supportsDryRun: true,
|
||||
handler: async ctx => {
|
||||
ctx.logger.info(ctx.input.nested.deep.secret);
|
||||
},
|
||||
schema: {
|
||||
input: {
|
||||
type: 'object',
|
||||
required: ['nested'],
|
||||
properties: {
|
||||
nested: {
|
||||
type: 'object',
|
||||
properties: {
|
||||
deep: {
|
||||
type: 'object',
|
||||
properties: {
|
||||
secret: { type: 'string' },
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const task = createMockTaskWithSpec(
|
||||
{
|
||||
steps: [
|
||||
{
|
||||
id: 'test',
|
||||
name: 'name',
|
||||
action: 'log-secret',
|
||||
input: {
|
||||
nested: {
|
||||
deep: {
|
||||
secret: "${{ secrets.token | replace('.', '-') }}",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
{ token: 'aaa.bbb.ccc' },
|
||||
);
|
||||
|
||||
await runner.execute(task);
|
||||
|
||||
expectTaskLog('info: ***');
|
||||
});
|
||||
|
||||
// eslint-disable-next-line jest/expect-expect
|
||||
it('should redact secrets in arrays within input', async () => {
|
||||
actionRegistry.register({
|
||||
id: 'log-secret',
|
||||
description: 'Mock action for testing',
|
||||
supportsDryRun: true,
|
||||
handler: async ctx => {
|
||||
ctx.logger.info(ctx.input.items[0]);
|
||||
},
|
||||
schema: {
|
||||
input: {
|
||||
type: 'object',
|
||||
required: ['items'],
|
||||
properties: {
|
||||
items: {
|
||||
type: 'array',
|
||||
items: { type: 'string' },
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const task = createMockTaskWithSpec(
|
||||
{
|
||||
steps: [
|
||||
{
|
||||
id: 'test',
|
||||
name: 'name',
|
||||
action: 'log-secret',
|
||||
input: {
|
||||
items: ['${{ secrets.token | upper }}', 'not-a-secret'],
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
{ token: 'my-secret' },
|
||||
);
|
||||
|
||||
await runner.execute(task);
|
||||
|
||||
expectTaskLog('info: ***');
|
||||
});
|
||||
|
||||
// eslint-disable-next-line jest/expect-expect
|
||||
it('should redact multiple different transformed secrets in the same step', async () => {
|
||||
actionRegistry.register({
|
||||
id: 'log-secret',
|
||||
description: 'Mock action for testing',
|
||||
supportsDryRun: true,
|
||||
handler: async ctx => {
|
||||
ctx.logger.info(`${ctx.input.a} ${ctx.input.b}`);
|
||||
},
|
||||
schema: {
|
||||
input: {
|
||||
type: 'object',
|
||||
required: ['a', 'b'],
|
||||
properties: {
|
||||
a: { type: 'string' },
|
||||
b: { type: 'string' },
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const task = createMockTaskWithSpec(
|
||||
{
|
||||
steps: [
|
||||
{
|
||||
id: 'test',
|
||||
name: 'name',
|
||||
action: 'log-secret',
|
||||
input: {
|
||||
a: '${{ secrets.s1 | upper }}',
|
||||
b: "${{ secrets.s2 | replace('.', '_') }}",
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
{ s1: 'first-secret', s2: 'second.secret' },
|
||||
);
|
||||
|
||||
await runner.execute(task);
|
||||
|
||||
expectTaskLog('info: *** ***');
|
||||
});
|
||||
|
||||
it('should still pass the correct transformed value to the action input', async () => {
|
||||
actionRegistry.register({
|
||||
id: 'log-secret',
|
||||
description: 'Mock action for testing',
|
||||
supportsDryRun: true,
|
||||
handler: fakeActionHandler,
|
||||
schema: {
|
||||
input: {
|
||||
type: 'object',
|
||||
required: ['secret'],
|
||||
properties: {
|
||||
secret: {
|
||||
type: 'string',
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const task = createMockTaskWithSpec(
|
||||
{
|
||||
steps: [
|
||||
{
|
||||
id: 'test',
|
||||
name: 'name',
|
||||
action: 'log-secret',
|
||||
input: {
|
||||
secret: "${{ secrets.backstageToken | replace('.', '_DOT_') }}",
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
{ backstageToken: 'header.payload.signature' },
|
||||
);
|
||||
|
||||
await runner.execute(task);
|
||||
|
||||
expect(fakeActionHandler).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
input: { secret: 'header_DOT_payload_DOT_signature' },
|
||||
}),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('each', () => {
|
||||
|
||||
@@ -129,6 +129,53 @@ const createStepLogger = ({
|
||||
return { taskLogger };
|
||||
};
|
||||
|
||||
/**
|
||||
* Recursively compares two rendered objects and returns string values from
|
||||
* `withSecrets` that differ from their counterpart in `withoutSecrets`.
|
||||
* These are values that were influenced by secret interpolation and should
|
||||
* be added as log redactions.
|
||||
*/
|
||||
function collectSecretRedactions(
|
||||
withSecrets: unknown,
|
||||
withoutSecrets: unknown,
|
||||
): string[] {
|
||||
if (typeof withSecrets === 'string') {
|
||||
return withSecrets !== withoutSecrets ? [withSecrets] : [];
|
||||
}
|
||||
if (Array.isArray(withSecrets)) {
|
||||
const other = Array.isArray(withoutSecrets) ? withoutSecrets : [];
|
||||
return withSecrets.flatMap((val, i) =>
|
||||
collectSecretRedactions(val, other[i]),
|
||||
);
|
||||
}
|
||||
if (withSecrets && typeof withSecrets === 'object') {
|
||||
const other =
|
||||
withoutSecrets && typeof withoutSecrets === 'object'
|
||||
? (withoutSecrets as Record<string, unknown>)
|
||||
: {};
|
||||
return Object.entries(withSecrets as Record<string, unknown>).flatMap(
|
||||
([key, val]) => collectSecretRedactions(val, other[key]),
|
||||
);
|
||||
}
|
||||
return [];
|
||||
}
|
||||
|
||||
/**
|
||||
* Extracts all string values from a nested object structure.
|
||||
* Used as a fallback when the comparison render fails.
|
||||
*/
|
||||
function extractStringValues(obj: unknown): string[] {
|
||||
if (typeof obj === 'string') return [obj];
|
||||
if (Array.isArray(obj)) return obj.flatMap(extractStringValues);
|
||||
if (obj && typeof obj === 'object') {
|
||||
return Object.entries(obj).flatMap(([key, val]) => [
|
||||
key,
|
||||
...extractStringValues(val),
|
||||
]);
|
||||
}
|
||||
return [];
|
||||
}
|
||||
|
||||
const isActionAuthorized = createConditionAuthorizer(
|
||||
Object.values(scaffolderActionRules),
|
||||
);
|
||||
@@ -445,6 +492,39 @@ export class NunjucksWorkflowRunner implements WorkflowRunner {
|
||||
);
|
||||
}
|
||||
|
||||
// Redact any rendered values that were influenced by secrets.
|
||||
// Re-render the input without secrets and diff against the real render
|
||||
// to find values that changed due to secret interpolation.
|
||||
if (step.input) {
|
||||
const hasSecrets =
|
||||
Object.keys(task.secrets ?? {}).length > 0 ||
|
||||
Object.keys(this.environment?.secrets ?? {}).length > 0;
|
||||
|
||||
if (hasSecrets) {
|
||||
try {
|
||||
const contextNoSecrets = {
|
||||
...preIterationContext,
|
||||
...(iteration.each ? { each: iteration.each } : {}),
|
||||
secrets: {},
|
||||
environment: {
|
||||
...preIterationContext.environment,
|
||||
secrets: {},
|
||||
},
|
||||
};
|
||||
const inputWithoutSecrets = this.render(
|
||||
step.input,
|
||||
contextNoSecrets,
|
||||
renderTemplate,
|
||||
);
|
||||
taskLogger.addRedactions(
|
||||
collectSecretRedactions(iteration.input, inputWithoutSecrets),
|
||||
);
|
||||
} catch {
|
||||
taskLogger.addRedactions(extractStringValues(iteration.input));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
await action.handler({
|
||||
input: iteration.input,
|
||||
task: {
|
||||
|
||||
Reference in New Issue
Block a user