Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adding google malware detection #36

Merged
merged 4 commits into from
Sep 12, 2022

Conversation

jsdevel
Copy link
Contributor

@jsdevel jsdevel commented Aug 28, 2022

  • Test ipfs malware sample from google with lookup and evaluate.

@jsdevel jsdevel force-pushed the adding-google-malware-detection branch from 0fe2ded to 1b9c179 Compare August 28, 2022 08:13
@jsdevel jsdevel changed the title Adding google malware detection. feat: Adding google malware detection. Aug 28, 2022
@jsdevel jsdevel changed the title feat: Adding google malware detection. feat: adding google malware detection Aug 28, 2022
@jsdevel jsdevel force-pushed the adding-google-malware-detection branch from 1b9c179 to e6f4227 Compare August 28, 2022 08:14
Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the first iteration @jsdevel

Let's please create a package from the start as previously talked and specified in https://hackmd.io/Y1vEmm9qTBmQU9Wzc3PY2w. Once we hook up tests, KV to store results / KV Locker it will be a bunch of logic that should be isolated. Also, we will want the logs for metrics + trigger badbits mail

For the package, we want a worker quite similar to this one with:

  • wrangler.toml
  • index.js
  • cors.js
  • error-handler.js
  • env.js

packages/edge-gateway/src/gateway.js Outdated Show resolved Hide resolved
packages/edge-gateway/src/gateway.js Outdated Show resolved Hide resolved
packages/edge-gateway/src/gateway.js Outdated Show resolved Hide resolved
Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the new iteration Joe. Added some initial feedback even though I know this is still WIP 😉

packages/cid-verifier/scripts/cli.js Outdated Show resolved Hide resolved
packages/cid-verifier/scripts/cli.js Outdated Show resolved Hide resolved
packages/cid-verifier/scripts/heartbeat.js Outdated Show resolved Hide resolved
packages/cid-verifier/package.json Outdated Show resolved Hide resolved
packages/cid-verifier/src/bindings.d.ts Outdated Show resolved Hide resolved
packages/cid-verifier/src/bindings.d.ts Show resolved Hide resolved
packages/cid-verifier/src/env.js Outdated Show resolved Hide resolved
packages/cid-verifier/src/env.js Outdated Show resolved Hide resolved
@jsdevel jsdevel force-pushed the adding-google-malware-detection branch 2 times, most recently from 9f4a736 to c3dd17c Compare September 1, 2022 09:13
Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the new iteration. Glad things are shaping up. A couple of general comments I could not have a place to add in review:

  • github actions workflow for cid verifier package (can be based on edge-gateway
  • CI is having a bad time, looks like pnpm lockfile should be commited? perhaps run on root dir pnpm clean && pnpm i

Also, this is first set of comments and I did not get into the actual code for CID verification. Just all the boilerplate around

packages/cid-verifier/package.json Outdated Show resolved Hide resolved
packages/cid-verifier/src/bindings.d.ts Outdated Show resolved Hide resolved
packages/cid-verifier/src/bindings.d.ts Outdated Show resolved Hide resolved
packages/cid-verifier/src/bindings.d.ts Outdated Show resolved Hide resolved
packages/cid-verifier/src/bindings.d.ts Outdated Show resolved Hide resolved
packages/cid-verifier/package.json Outdated Show resolved Hide resolved
packages/edge-gateway/wrangler.toml Outdated Show resolved Hide resolved
packages/edge-gateway/wrangler.toml Outdated Show resolved Hide resolved
packages/edge-gateway/src/utils/deny-list.js Show resolved Hide resolved
packages/edge-gateway/package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra thoughts, we should be good after this. Good job

packages/cid-verifier/wrangler.toml Outdated Show resolved Hide resolved
packages/cid-verifier/wrangler.toml Outdated Show resolved Hide resolved
packages/cid-verifier/wrangler.toml Outdated Show resolved Hide resolved
packages/cid-verifier/wrangler.toml Outdated Show resolved Hide resolved
packages/cid-verifier/test/utils/setup.js Outdated Show resolved Hide resolved
packages/cid-verifier/src/verification.js Outdated Show resolved Hide resolved
packages/cid-verifier/src/verification.js Outdated Show resolved Hide resolved
packages/cid-verifier/src/verification.js Outdated Show resolved Hide resolved
packages/cid-verifier/src/verification.js Outdated Show resolved Hide resolved
packages/cid-verifier/src/verification.js Outdated Show resolved Hide resolved
@jsdevel
Copy link
Contributor Author

jsdevel commented Sep 2, 2022

Thanks for the new iteration. Glad things are shaping up. A couple of general comments I could not have a place to add in review:

  • github actions workflow for cid verifier package (can be based on edge-gateway
  • CI is having a bad time, looks like pnpm lockfile should be commited? perhaps run on root dir pnpm clean && pnpm i

Also, this is first set of comments and I did not get into the actual code for CID verification. Just all the boilerplate around

i saw that error. i checked in the pnpm lock file. i think locally i'm using a later version of pnpm and that's causing it.

@jsdevel jsdevel force-pushed the adding-google-malware-detection branch 16 times, most recently from de4735a to 57a0953 Compare September 7, 2022 07:01
packages/cid-verifier/package.json Outdated Show resolved Hide resolved
packages/cid-verifier/src/verification.js Outdated Show resolved Hide resolved
@jsdevel jsdevel force-pushed the adding-google-malware-detection branch from 57a0953 to cc9b4f3 Compare September 7, 2022 22:20
@storacha storacha deleted a comment from vasco-santos Sep 7, 2022
@jsdevel jsdevel force-pushed the adding-google-malware-detection branch from cf8bef9 to 5412487 Compare September 8, 2022 22:38
@jsdevel
Copy link
Contributor Author

jsdevel commented Sep 8, 2022

New set of changes. Please also add README file, gather a copy from edge-gateway and basically need specific secrets for here specified there

i'll add a README once the approach is finalized

packages/cid-verifier/src/verification.js Outdated Show resolved Hide resolved
export interface EnvInput {
ENV: string;
DEBUG: string;
GOOGLE_EVALUATE_SAFE_CONFIDENCE_LEVELS: Array<string>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i had this in case there were other levels we'd want to configure other than SAFE

Comment on lines 20 to 21
.get('/verification', withCorsHeaders(verificationGet))
.post('/verification', withCorsHeaders(verificationPost))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can go over this in our call

packages/cid-verifier/src/verification.js Show resolved Hide resolved
packages/cid-verifier/src/verification.js Outdated Show resolved Hide resolved
@jsdevel jsdevel force-pushed the adding-google-malware-detection branch 2 times, most recently from 8833a3f to 3960f33 Compare September 9, 2022 04:05
export interface EnvInput {
ENV: string;
DEBUG: string;
GOOGLE_EVALUATE_SAFE_CONFIDENCE_LEVELS: Array<string>;
Copy link
Contributor

@vasco-santos vasco-santos Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I got it thanks, this also needs to go to docs. And added in wrangler toml for other environments

packages/cid-verifier/src/verification.js Show resolved Hide resolved
packages/cid-verifier/src/verification.js Show resolved Hide resolved
packages/cid-verifier/src/verification.js Outdated Show resolved Hide resolved
packages/cid-verifier/src/verification.js Outdated Show resolved Hide resolved
packages/cid-verifier/src/verification.js Outdated Show resolved Hide resolved
packages/edge-gateway/wrangler.toml Outdated Show resolved Hide resolved
packages/edge-gateway/wrangler.toml Outdated Show resolved Hide resolved
packages/cid-verifier/wrangler.toml Outdated Show resolved Hide resolved
packages/cid-verifier/src/verification.js Outdated Show resolved Hide resolved
@jsdevel jsdevel force-pushed the adding-google-malware-detection branch 2 times, most recently from bd6d31c to 7d51262 Compare September 9, 2022 19:15
* Moving DENYLIST logic to cid-verifier from edge-gateway
@jsdevel jsdevel force-pushed the adding-google-malware-detection branch from 7d51262 to 65a107e Compare September 10, 2022 02:48
Copy link
Contributor Author

@jsdevel jsdevel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hoping you can get this sample.html test to work.

@@ -101,3 +101,10 @@ test('Gets content with other base encodings', async (t) => {
await response.waitUntil()
t.is(await response.text(), 'Hello dot.storage! 😎')
})

test('Sends HTML files to cid-verifier', async (t) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vasco-santos i'm having trouble getting this to work. can you take a look?

Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants