File scanning connector to handle multiple AV tools #1514

ARADDCC002 · 2024-09-11T16:09:00Z

No description provided.

JR40159 · 2024-10-02T07:23:38Z

backend/src/services/file.ts

-        avScan: { state: ScanState.Error },
-      })
+  if (config.ui.avScanning.enabled) {
+    const fileScanConnectors = getFileScanningConnectors()


It would be good if this new connector type could have the same interface as other connectors and this could just be scanners.scan(file). Ideally we want to encapsulate the initialisation and the calling of each individual scanner inside the connector logic, not here in the service layer every time we want to use the scanners

JR40159 · 2024-10-02T07:25:39Z

backend/src/connectors/fileScanning/clamAv.ts

+  }
+
+  async init() {
+    if (!av) {


I don't think this check is necessary because the init() function is only called to initialise the av scanner

JR40159 · 2024-10-02T07:26:42Z

backend/src/connectors/fileScanning/clamAv.ts

+      try {
+        av = await new NodeClam().init({ clamdscan: config.avScanning.clamdscan })
+      } catch (error) {
+        throw ConfigurationError('Cannot retrieve Clam AV config', {


Have you tested what happens if the Clam AV service isn't running? I assume this throw would be called. If so, the error message needs updating

JR40159 · 2024-10-02T07:28:13Z

backend/src/connectors/fileScanning/clamAv.ts

+  }
+
+  async scan(file: FileInterfaceDoc) {
+    const avStream = av.passthrough()


It's a bit of a pain but to be safe, you should probably check that av has been initialised at the start of functions like this when using the AV scanner and throw an error recommending that init() should be called first if av has not been assigned.

JR40159 · 2024-10-02T07:32:44Z

backend/src/connectors/fileScanning/clamAv.ts

+    if (!file.avScan.find((result) => result.toolName === toolName)) {
+      const updatedAvScanArray = [...file.avScan, { toolName: toolName, state: ScanState.InProgress }]
+      file.avScan = updatedAvScanArray
+      await FileModel.updateOne(


In my opinion, these scanners should not be responsible for storing the scan information in the DB, they should just be an interface with a particular scanning and this method should take in a file stream and return a scan result object. Ther service layer should be responsible for getting the file stream and processing the scan results.

This is a bit tricky as it's async and the service that calls these connectors already returns the response to the user. We'd need to call the scanners, wait for the results, store them, then return a 200 response to the user from the backend.

Just because we want a process to run asynchronously doesn't mean all the implementation needs to be in a single method. Use a .then() on the asynchronous call to provide the implementation for how we will process the results once the scanners are complete.

JR40159 · 2024-10-02T07:39:24Z

backend/src/models/File.ts

-      isInfected: { type: Boolean },
-      viruses: [{ type: String }],
-    },
+    avScan: [


Don't forget, this will require a migration. I don't think a migration was done when this field was originally added so this migration will have to account for when this field doesn't exist

I don't think it does but feel free to double check. From testing it looks like it will return single results as arrays anyway

From looking at this diff it looks like the previous type of avScan was an object and now avScan is an array? That's why I thought this would require a migration

It becomes an array of the same type of object as it was before and it seems to handle it automatically thankfully it seems

…d point for checkig what filescanners are active

… check for info insead of ui config for scanner list

JR40159 · 2024-10-14T15:59:52Z

backend/config/default.cjs

@@ -205,6 +204,10 @@ module.exports = {
    audit: {
      kind: 'silly',
    },
+
+    fileScanners: {
+      kinds: ['clamAV'],


By default, we shouldn't have any scanners configured. This instead should be done in the docker_compose.cjs

JR40159 · 2024-10-14T16:13:59Z

backend/src/connectors/fileScanning/index.ts

+        })
+    }
+  })
+


I would recommend call init() on all the connectors in fileScanConnectors here or in each case where you add a new connector to the list. This way, you can pick up on configuration errors early on

JR40159 · 2024-10-14T16:15:43Z

backend/src/connectors/fileScanning/index.ts

+    }
+  })
+
+  const wrapper = new FileScanningWrapper(fileScanConnectors)


In the other index.ts files for the other connectors we also have a caching mechanism we'll probably also want here. Something that checks if wrapper has already been assigned and returns it if it has

JR40159 · 2024-10-14T16:16:05Z

backend/src/connectors/fileScanning/clamAv.ts

+  }
+
+  async scan(file: FileInterfaceDoc): Promise<FileScanResult[]> {
+    await this.init()


See comment on index.ts about moving this to there

JR40159 · 2024-10-14T16:51:20Z

backend/src/services/file.ts

-        avScan: { state: ScanState.Error },
-      })
-    })
+  if (runFileScanners().info()) {


This isn't a major thing but could we get this to look more like the other connectors eg: const auth = await authorisation.file(user, model, file, FileAction.Upload) so this would become if (scanners.info()) {

JR40159 · 2024-10-14T16:53:03Z

backend/src/routes/v2/filescanning/getFilescanningInfo.ts

+export const getFilescanningInfo = [
+  bodyParser.json(),
+  async (req: Request, res: Response<GetFileScanningInfoResponse>) => {
+    return res.json({ scanners: runFileScanners().info() })


See comment on file.ts to make this be return res.json({ scanners: scanners.info() })

JR40159 · 2024-10-14T16:58:00Z

backend/src/services/file.ts

  }

  return file
 }

+export async function updateFileWithResults(file: FileInterface, results: FileScanResult[]) {
+  for (const result of results) {


I'd be interested to look to see if Mongo can handle this logic for you and add new elements to the array if a an element with a tool doesn't exist and update the existing element if a element with that tool does exist

JR40159 · 2024-10-14T16:58:50Z

backend/src/services/file.ts

+      await FileModel.updateOne(
+        { _id: file._id },
+        {
+          $set: { avScan: updatedAvScanArray },


Here you should use $push and then you won't need to have updatedAvScanArray

JR40159 · 2024-10-14T17:01:39Z

backend/src/services/mirroredModel.ts

+      } else if (
+        file.avScan.some(
+          (scanResult) =>
+            scanResult.state === ScanState.NotScanned ||


Can this not be scanResult.state !== ScanState.Complete?

…tion of the file scanning

JR40159

When setting the scanners connectors to an empty array, the information still appears in the UI?

JR40159 · 2024-10-22T17:53:46Z

backend/test/services/mirroredModel.spec.ts

@@ -27,16 +34,16 @@ vi.mock('../../src/connectors/authorisation/index.js', async () => ({
  default: authMock,
 }))

+// const fileScanConnectors: BaseFileScanningConnector[] = [new ClamAvFileScanningConnector()]


JR40159 · 2024-10-22T17:55:57Z

infrastructure/helm/bailo/templates/bailo/bailo.configmap.yaml

@@ -193,6 +192,10 @@ data:
        audit: {
          kind: '{{ .Values.connectors.audit.kind }}',
        },
+
+        fileScanners: {
+          kinds: '{{ .Values.connectors.fileScanners.kinds }}',


This should be an array?

This is just a reference though isn't it? I don't think we define if it's an array or not here

.

wip for file scanning connector

ac9e617

ARADDCC002 marked this pull request as draft September 11, 2024 16:09

ARADDCC002 added 13 commits September 18, 2024 12:39

wip for config changes for filescanning

0199b64

updated unit tests

cde5410

merged main and adjust av popover to handle multiple results

d947e29

updated av scanning config

93560e1

updated av scanning config

801b5a7

updated config to disable file scanning by default

a00b6c7

added check for required config

7244593

changes to file scanning connector

82a492e

added error checking for clamav config

48bc81c

updated docker compose prod file

dbe8651

added awaits for getfilescanconnectors call

57d1fb0

added awaits for getfilescanconnectors call

42fc570

reverted logic for checking if av is initialised

607bd6b

ARADDCC002 marked this pull request as ready for review September 27, 2024 12:59

ARADDCC002 added 2 commits September 27, 2024 13:33

added the file av results popover for successful av scans

5c5bcde

fixed an issue that was causing only one file scan to store results

358d87e

ARADDCC002 changed the title ~~wip for file scanning connector~~ File scanning connector to handle multiple AV tools Oct 1, 2024

JR40159 requested changes Oct 2, 2024

View reviewed changes

ARADDCC002 added 6 commits October 2, 2024 11:01

fixed some pr comments

c577734

changed structure of av file scan

46b6bba

various improvements to how we handle the AV connector. Added info en…

19abdc4

…d point for checkig what filescanners are active

added frontend actions file for filescanning info call. updated ui to…

fdfa056

… check for info insead of ui config for scanner list

updated config and unit tests

6dabbae

fixed a typo

c1c7548

JR40159 requested changes Oct 14, 2024

View reviewed changes

ARADDCC002 added 3 commits October 15, 2024 16:06

various improvements including change to how we manage the initialisa…

f1ccde5

…tion of the file scanning

updated ui to better reflect av results

bbdfe2b

Merge branch 'main' into feature/BAI-1458-add-file-scanning-connector

1bbaabb

ARADDCC002 and others added 4 commits October 16, 2024 12:41

added backed log

e9b6c42

awaited clam av init call

08b85d9

Update and simplify scanning process

5a18054

Fix config that was removed by mistake

ab64bd1

JR40159 previously requested changes Oct 22, 2024

View reviewed changes

ARADDCC002 added 5 commits October 29, 2024 16:19

removed uneeded config mocking

32e743d

updated bailo config map for filescanners

7e6b5f6

improved checking for scanners info before show av chip on file displays

62a66c2

readded clamav to config

610791b

updated test mocking

c3362fa

ARADDCC012 approved these changes Oct 31, 2024

View reviewed changes

ARADDCC002 merged commit 1692d87 into main Nov 1, 2024
15 checks passed

ARADDCC012 deleted the feature/BAI-1458-add-file-scanning-connector branch November 5, 2024 12:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File scanning connector to handle multiple AV tools #1514

File scanning connector to handle multiple AV tools #1514

ARADDCC002 commented Sep 11, 2024

JR40159 Oct 2, 2024

JR40159 Oct 2, 2024

JR40159 Oct 2, 2024

JR40159 Oct 2, 2024

JR40159 Oct 2, 2024

ARADDCC002 Oct 2, 2024

JR40159 Oct 2, 2024

JR40159 Oct 2, 2024

ARADDCC002 Oct 2, 2024

JR40159 Oct 2, 2024

ARADDCC002 Oct 2, 2024

JR40159 Oct 14, 2024

JR40159 Oct 14, 2024

JR40159 Oct 14, 2024

JR40159 Oct 14, 2024

JR40159 Oct 14, 2024

JR40159 Oct 14, 2024

JR40159 Oct 14, 2024

JR40159 Oct 14, 2024

JR40159 Oct 14, 2024

JR40159 left a comment

JR40159 Oct 22, 2024

JR40159 Oct 22, 2024

ARADDCC002 Oct 30, 2024

File scanning connector to handle multiple AV tools #1514

File scanning connector to handle multiple AV tools #1514

Conversation

ARADDCC002 commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JR40159 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment