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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
May 28, 2021
Merged

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

merged 3 commits into from
May 28, 2021

Conversation

VicDeo
Copy link
Member

@VicDeo VicDeo commented May 25, 2021 •
edited
Loading

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 •
edited
Loading

@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 mentioned this pull request May 26, 2021
Open
@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
jnweiger
jnweiger requested changes May 26, 2021
View reviewed changes
Copy link
Contributor

@jnweiger jnweiger left a comment •
edited
Loading

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 •
edited
Loading

@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 •
edited
Loading

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

AlexAndBear
AlexAndBear approved these changes May 28, 2021
View reviewed changes
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
@VicDeo VicDeo mentioned this pull request May 28, 2021
Closed
jnweiger added a commit that referenced this pull request May 31, 2021
@jnweiger
added #445
7162a8e
@micbar micbar mentioned this pull request Jun 14, 2021
Closed
@jnweiger jnweiger mentioned this pull request Aug 9, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexAndBear AlexAndBear AlexAndBear approved these changes

@micbar micbar Awaiting requested review from micbar

@jnweiger jnweiger Awaiting requested review from jnweiger

Assignees

@VicDeo VicDeo

Labels
3 - To review
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.

[QA] cannot upload files larger than ca. 20 MB
4 participants
@VicDeo @jnweiger @CLAassistant @AlexAndBear