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

[ICAP] Stop reading the response after headers are read #445

Merged
merged 3 commits into from
May 28, 2021

Conversation

VicDeo
Copy link
Member

@VicDeo VicDeo commented May 25, 2021

Do not read the whole response as we need only ICAP headers and Encapsulated header.

So we read ICAP headers one by one until Encapsulated header is met.
Then we parse offsets provided by res-hdr and res-body of this header.

the content is considered to be infected if ANY of the following is true:

  • ICAP headers contain the header specified in av_response_header
  • HTTP header in res-hdr section has status code 403

the content is considered to be clean if the ICAP status code is 204

TL;DR

McAfee just hangs so if we want to read whole response we should wait until the network timeout happens
But the whole response is not needed for the virus detection as

  • 204 ICAP status means 'clean'
  • 200 ICAP status with encapsulated 403 HTTP status means 'infected'

Fixes #443 https://github.com/owncloud/enterprise/issues/4580
Obsoletes #444

@VicDeo VicDeo self-assigned this May 25, 2021
@VicDeo VicDeo force-pushed the bugfix/443 branch 3 times, most recently from d6d5907 to d910f33 Compare May 25, 2021 15:45
@VicDeo
Copy link
Member Author

VicDeo commented May 26, 2021

@jnweiger if it possible to test this patch before merge?

@VicDeo VicDeo changed the title Stop reading the response after headers are read [ICAP] Stop reading the response after headers are read May 26, 2021
@VicDeo VicDeo requested a review from micbar May 26, 2021 11:30
@VicDeo VicDeo changed the base branch from master to release-1.0.0 May 26, 2021 11:35
@VicDeo VicDeo added this to the QA milestone May 26, 2021
@micbar micbar requested a review from AlexAndBear May 26, 2021 13:24
@jnweiger
Copy link
Contributor

@jnweiger if it possible to test this patch before merge?

OK. I'll give it a test first, then we merge.

@jnweiger jnweiger self-requested a review May 26, 2021 14:23
Copy link
Contributor

@jnweiger jnweiger left a comment

Choose a reason for hiding this comment

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

With this patch, an infected file is reported immediately.
A clean file hangs for 60 seconds in 'processing file'.
Seen with McAfee via ICAP.

ClamAV and Kaspersky via ICAP work fine.

@VicDeo
Copy link
Member Author

VicDeo commented May 27, 2021

@jnweiger

With this patch, an infected file is reported immediately.
A clean file hangs for 60 seconds in 'processing file'.
Seen with McAfee via ICAP.

So is it acceptable or not?
If not I can't go further without access to McAfee.

@jnweiger
Copy link
Contributor

jnweiger commented May 27, 2021

@jnweiger
So is it acceptable or not?
If not I can't go further without access to McAfee.

A one minute delay per normal file is not acceptable. @VicDeo You now have access to my test system.

@CLAassistant
Copy link

CLAassistant commented May 27, 2021

CLA assistant check
All committers have signed the CLA.

@VicDeo
Copy link
Member Author

VicDeo commented May 27, 2021

@janackermann could you please sign the CLA for this repo?

@AlexAndBear
Copy link

Done

@sonarcloud
Copy link

sonarcloud bot commented May 27, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link

@AlexAndBear AlexAndBear left a comment

Choose a reason for hiding this comment

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

Codewise LGTM

@micbar micbar requested a review from jnweiger May 28, 2021 06:53
@VicDeo VicDeo merged commit ad499ee into release-1.0.0 May 28, 2021
@delete-merged-branch delete-merged-branch bot deleted the bugfix/443 branch May 28, 2021 10:22
jnweiger added a commit that referenced this pull request May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[QA] cannot upload files larger than ca. 20 MB
4 participants