Skip to content

Commit 201cacc

Browse files
ivanstanevivanstanev
authored
Merge pull request #175 from snyk/fix/unique-tmp-files
fix: pull images to uniquely-named files
2 parents 19aea3a + 86a5483 commit 201cacc

File tree

6 files changed

+142
-40
lines changed

6 files changed

+142
-40
lines changed

‎src/images/index.ts‎

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,39 +3,56 @@ import { pull as dockerPull } from './docker';
33
import { pull as skopeoCopy, getDestinationForImage } from './skopeo';
44
import { unlink } from 'fs';
55
import { isStaticAnalysisEnabled } from '../common/features';
6+
import { IPullableImage } from './types';
67

7-
export { getDestinationForImage };
8-
9-
export async function pullImages(images: string[]): Promise<string[]> {
10-
const pulledImages: string[] = [];
8+
export async function pullImages(images: IPullableImage[]): Promise<IPullableImage[]> {
9+
const pulledImages: IPullableImage[] = [];
1110

1211
for (const image of images) {
12+
const {imageName, fileSystemPath} = image;
1313
try {
1414
if (isStaticAnalysisEnabled()) {
15-
await skopeoCopy(image);
15+
if (!fileSystemPath) {
16+
throw new Error('Missing required parameter fileSystemPath for static analysis');
17+
}
18+
await skopeoCopy(imageName, fileSystemPath);
1619
} else {
17-
await dockerPull(image);
20+
await dockerPull(imageName);
1821
}
1922
pulledImages.push(image);
2023
} catch (error) {
21-
logger.error({error, image}, 'failed to pull image');
24+
logger.error({error, image: imageName}, 'failed to pull image');
2225
}
2326
}
2427

2528
return pulledImages;
2629
}
2730

28-
export async function removePulledImages(images: string[]) {
31+
/**
32+
* TODO: For Docker (dynamic scanning) it returns the image but with an empty file system path
33+
* (because Docker does not pull images to a temporary directory). This will no longer be true
34+
* when static analysis becomes the only option, but worth nothing it here to avoid confusion!
35+
* @param images a list of images for which to generate a file system path
36+
*/
37+
export function getImagesWithFileSystemPath(images: string[]): IPullableImage[] {
38+
return isStaticAnalysisEnabled()
39+
? images.map((image) => ({ imageName: image, fileSystemPath: getDestinationForImage(image) }))
40+
: images.map((image) => ({ imageName: image }));
41+
}
42+
43+
export async function removePulledImages(images: IPullableImage[]) {
2944
if (!isStaticAnalysisEnabled()) {
3045
return;
3146
}
3247

33-
for (const image of images) {
48+
for (const {imageName, fileSystemPath} of images) {
3449
try {
35-
const destination = getDestinationForImage(image);
36-
await new Promise((resolve) => unlink(destination, resolve));
50+
if (!fileSystemPath) {
51+
throw new Error('Missing required parameter fileSystemPath for static analysis');
52+
}
53+
await new Promise((resolve) => unlink(fileSystemPath, resolve));
3754
} catch (error) {
38-
logger.warn({error, image}, 'failed to delete pulled image');
55+
logger.warn({error, image: imageName}, 'failed to delete pulled image');
3956
}
4057
}
4158
}

‎src/images/skopeo.ts‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,18 @@ import { SpawnPromiseResult } from 'child-process-promise';
33
import { exec } from '../cli/process';
44
import config = require('../common/config');
55

6+
function getUniqueIdentifier(): string {
7+
const [seconds, nanoseconds] = process.hrtime();
8+
return `${seconds}_${nanoseconds}`;
9+
}
10+
611
export function getDestinationForImage(image: string): string {
712
const normalisedImageName = image.replace(/\W/g, '_');
8-
return `${config.IMAGE_STORAGE_ROOT}/${normalisedImageName}.tar`;
13+
// If two workloads contain the same image and if the snyk-monitor attempts to pull the two images at the same time,
14+
// this can result in a problem where both actions try to work with the same file resulting in a nasty crash.
15+
// This is why we try to make the name of the temporary file unique for every workload analysis.
16+
const uniqueIdentifier = getUniqueIdentifier();
17+
return `${config.IMAGE_STORAGE_ROOT}/${normalisedImageName}_${uniqueIdentifier}.tar`;
918
}
1019

1120
function prefixRespository(target: string, type: SkopeoRepositoryType) {
@@ -21,12 +30,10 @@ function prefixRespository(target: string, type: SkopeoRepositoryType) {
2130

2231
export function pull(
2332
image: string,
33+
destination: string,
2434
): Promise<SpawnPromiseResult> {
25-
const source = image;
26-
const destination = getDestinationForImage(image);
27-
2835
return exec('skopeo', 'copy',
29-
prefixRespository(source, SkopeoRepositoryType.ImageRegistry),
36+
prefixRespository(image, SkopeoRepositoryType.ImageRegistry),
3037
prefixRespository(destination, SkopeoRepositoryType.DockerArchive),
3138
);
3239
}

‎src/images/types.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
export interface IPullableImage {
2+
imageName: string;
3+
fileSystemPath?: string;
4+
}

‎src/kube-scanner/image-scanner.ts‎

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
import * as plugin from 'snyk-docker-plugin';
22
import logger = require('../common/logger');
3-
import { getDestinationForImage } from '../images';
43
import { IStaticAnalysisOptions, StaticAnalysisImageType } from './types';
54
import { isStaticAnalysisEnabled } from '../common/features';
5+
import { IPullableImage } from '../images/types';
66

77
export interface IScanResult {
88
image: string;
@@ -23,44 +23,50 @@ function getImageTag(imageWithTag: string): string {
2323
return '';
2424
}
2525

26-
function constructStaticAnalysisOptions(image: string): { staticAnalysisOptions: IStaticAnalysisOptions } {
26+
function constructStaticAnalysisOptions(
27+
fileSystemPath: string | undefined,
28+
): { staticAnalysisOptions: IStaticAnalysisOptions } {
29+
if (!fileSystemPath) {
30+
throw new Error('Missing path for image for static analysis');
31+
}
32+
2733
return {
2834
staticAnalysisOptions: {
29-
imagePath: getDestinationForImage(image),
35+
imagePath: fileSystemPath,
3036
imageType: StaticAnalysisImageType.DockerArchive,
3137
},
3238
};
3339
}
3440

35-
export async function scanImages(images: string[]): Promise<IScanResult[]> {
41+
export async function scanImages(images: IPullableImage[]): Promise<IScanResult[]> {
3642
const scannedImages: IScanResult[] = [];
3743

3844
const dockerfile = undefined;
3945

40-
for (const image of images) {
46+
for (const {imageName, fileSystemPath} of images) {
4147
try {
4248
const options = isStaticAnalysisEnabled()
43-
? constructStaticAnalysisOptions(image)
49+
? constructStaticAnalysisOptions(fileSystemPath)
4450
: undefined;
4551

46-
const result = await plugin.inspect(image, dockerfile, options);
52+
const result = await plugin.inspect(imageName, dockerfile, options);
4753

4854
if (!result || !result.package || !result.package.dependencies) {
4955
throw Error('Unexpected empty result from docker-plugin');
5056
}
5157

5258
result.imageMetadata = {
53-
image: removeTagFromImage(image),
54-
imageTag: getImageTag(image),
59+
image: removeTagFromImage(imageName),
60+
imageTag: getImageTag(imageName),
5561
};
5662

5763
scannedImages.push({
58-
image: removeTagFromImage(image),
59-
imageWithTag: image,
64+
image: removeTagFromImage(imageName),
65+
imageWithTag: imageName,
6066
pluginResult: result,
6167
});
6268
} catch (error) {
63-
logger.warn({error, image}, 'failed to scan image');
69+
logger.warn({error, image: imageName}, 'failed to scan image');
6470
}
6571
}
6672

‎src/kube-scanner/index.ts‎

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,10 @@
1-
/***
2-
* consider using https://www.npmjs.com/package/kubernetes-client
3-
* currently it's better to not use this lib since version 9.0 is about to be
4-
* released and merged with oficial @kubernetes/client-node.
5-
* IMPORTANT:
6-
* see: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.14/#-strong-api-overview-strong-
7-
*/
81
import logger = require('../common/logger');
9-
import { pullImages, removePulledImages } from '../images';
2+
import { pullImages, removePulledImages, getImagesWithFileSystemPath } from '../images';
103
import { scanImages, IScanResult } from './image-scanner';
114
import { deleteHomebaseWorkload, sendDepGraph } from '../transmitter';
125
import { constructHomebaseDeleteWorkloadPayload, constructHomebaseDepGraphPayloads } from '../transmitter/payload';
136
import { IDepGraphPayload, IWorkload, ILocalWorkloadLocator } from '../transmitter/types';
7+
import { IPullableImage } from '../images/types';
148

159
export = class WorkloadWorker {
1610
private readonly name: string;
@@ -26,7 +20,8 @@ export = class WorkloadWorker {
2620
const uniqueImages = [...new Set<string>(allImages)];
2721

2822
logger.info({workloadName, imageCount: uniqueImages.length}, 'pulling unique images');
29-
const pulledImages = await pullImages(uniqueImages);
23+
const imagesWithFileSystemPath = getImagesWithFileSystemPath(uniqueImages);
24+
const pulledImages = await pullImages(imagesWithFileSystemPath);
3025
if (pulledImages.length === 0) {
3126
logger.info({workloadName}, 'no images were pulled, halting scanner process.');
3227
return;
@@ -48,7 +43,7 @@ export = class WorkloadWorker {
4843

4944
private async scanImagesAndSendResults(
5045
workloadName: string,
51-
pulledImages: string[],
46+
pulledImages: IPullableImage[],
5247
workloadMetadata: IWorkload[],
5348
): Promise<void> {
5449
const scannedImages: IScanResult[] = await scanImages(pulledImages);
@@ -63,8 +58,10 @@ export = class WorkloadWorker {
6358
const depGraphPayloads: IDepGraphPayload[] = constructHomebaseDepGraphPayloads(scannedImages, workloadMetadata);
6459
await sendDepGraph(...depGraphPayloads);
6560

61+
const pulledImagesNames = pulledImages.map((image) => image.imageName);
6662
const pulledImageMetadata = workloadMetadata.filter((meta) =>
67-
pulledImages.includes(meta.imageName));
63+
pulledImagesNames.includes(meta.imageName),
64+
);
6865

6966
logger.info({workloadName, imageCount: pulledImageMetadata.length}, 'processed images');
7067
}

‎test/unit/images.test.ts‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
import * as tap from 'tap';
2+
import { getImagesWithFileSystemPath, pullImages } from '../../src/images';
3+
const config = require('../../src/common/config');
4+
5+
tap.test('getImagesWithFileSystemPath()', async (t) => {
6+
const noImages: string[] = [];
7+
const noImagesResult = getImagesWithFileSystemPath(noImages);
8+
t.same(noImagesResult, [], 'correctly maps an empty array');
9+
10+
// Cache the last value of STATIC_ANALYSIS, we return it back to normal once the test completes
11+
const lastStaticAnalysisValue = config.STATIC_ANALYSIS;
12+
13+
// First try without static analysis set
14+
config.STATIC_ANALYSIS = false;
15+
const imageWithoutStaticAnalysis = ['redis:latest'];
16+
const imageWithoutStaticAnalysisResult = getImagesWithFileSystemPath(imageWithoutStaticAnalysis);
17+
t.same(
18+
imageWithoutStaticAnalysisResult,
19+
[{ imageName: 'redis:latest' }],
20+
'correctly returns an image without a file system path',
21+
);
22+
23+
// Next try with static analysis set
24+
config.STATIC_ANALYSIS = true;
25+
const imageWithStaticAnalysis = ['nginx:latest'];
26+
const imageWithStaticAnalysisResult = getImagesWithFileSystemPath(imageWithStaticAnalysis);
27+
t.same(imageWithStaticAnalysisResult.length, 1, 'expected 1 item');
28+
29+
const resultWithExpectedPath = imageWithStaticAnalysisResult[0];
30+
t.same(
31+
resultWithExpectedPath.imageName,
32+
'nginx:latest',
33+
'correctly returns an image without a file system path',
34+
);
35+
const fileSystemPath = resultWithExpectedPath.fileSystemPath!;
36+
t.ok(fileSystemPath, 'file system path exists on the result');
37+
t.ok(fileSystemPath.endsWith('.tar'), 'file system path ends in .tar');
38+
39+
const expectedPattern = fileSystemPath.indexOf(`${config.IMAGE_STORAGE_ROOT}/nginx_latest_`) !== -1;
40+
t.ok(expectedPattern, 'the file system path starts with an expected pattern');
41+
42+
// Finally ensure that two consecutive calls do not return the same file system path
43+
config.STATIC_ANALYSIS = true;
44+
const someImage = ['centos:latest'];
45+
const firstCallResult = getImagesWithFileSystemPath(someImage)[0];
46+
const secondCallResult = getImagesWithFileSystemPath(someImage)[0];
47+
t.ok(
48+
firstCallResult.fileSystemPath !== secondCallResult.fileSystemPath,
49+
'consecutive calls to the function with the same data return different file system paths',
50+
);
51+
52+
// Restore the old value
53+
config.STATIC_ANALYSIS = lastStaticAnalysisValue;
54+
});
55+
56+
tap.test('pullImages() skips on missing file system path in static analysis', async (t) => {
57+
// Cache the last value of STATIC_ANALYSIS, we return it back to normal once the test completes
58+
const lastStaticAnalysisValue = config.STATIC_ANALYSIS;
59+
60+
config.STATIC_ANALYSIS = true;
61+
const badStaticAnalysisImage = [
62+
{
63+
imageName: 'nginx:latest',
64+
},
65+
];
66+
const result = await pullImages(badStaticAnalysisImage);
67+
t.same(result, [], 'expect to skip images on static analysis set but missing file system path');
68+
69+
// Restore the old value
70+
config.STATIC_ANALYSIS = lastStaticAnalysisValue;
71+
});

0 commit comments

Comments
 (0)