Skip to content
This repository has been archived by the owner on Apr 26, 2021. It is now read-only.

Support for URLs analysis in cuckoo distributed (no db migration needed) #1700

Closed
wants to merge 1 commit into from
Closed

Conversation

fyhertz
Copy link

@fyhertz fyhertz commented Jul 6, 2017

Usage example:
curl http://localhost:9003/api/task -F url="http://www.perdu.com"

I think this is a better way to implement this feature than my previous pull request as no database migration is needed. The path field is used to store URLs in the task table.

@coveralls
Copy link

coveralls commented Jul 6, 2017

Coverage Status

Coverage decreased (-0.03%) to 58.544% when pulling 8e96c3b on fyhertz:urldistributed2 into 2611ef6 on cuckoosandbox:master.

@codecov-io
Copy link

Codecov Report

Merging #1700 into master will decrease coverage by 0.03%.
The diff coverage is 32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1700      +/-   ##
==========================================
- Coverage   58.57%   58.54%   -0.04%     
==========================================
  Files         147      147              
  Lines       14094    14104      +10     
==========================================
+ Hits         8256     8257       +1     
- Misses       5838     5847       +9
Impacted Files Coverage Δ
cuckoo/distributed/views/api.py 14.13% <0%> (-0.24%) ⬇️
cuckoo/distributed/api.py 80.35% <57.14%> (-9.44%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2611ef6...8e96c3b. Read the comment docs.

@jbremer
Copy link
Member

jbremer commented Jul 14, 2017

Although I mostly agree that this PR supersedes #1694, I somewhat also feel like adding a category field - much like we have in the Cuckoo Core - would make the changes in this PR slightly less hacky.
What are your thoughts?

@fyhertz
Copy link
Author

fyhertz commented Jul 18, 2017

It would be less hacky indeed! In the meantime, how about merging this cheap PR to enable distributed URLs analysis? Another later commit could add the category field and start using it to differentiate samples from URLs (instead of checking if filename is empty) while the path field would still be used for storing the URLs.

In other words, I think this PR brings distributed closer to a clean solution anyway. What do you think?

@jbremer
Copy link
Member

jbremer commented Jul 19, 2017

If I've learnt one thing then it's the fact that such 'temporary' fixes remain permanent ;-) As we have to maintain it it's much preferable to get it right the first time.
That said, and as you agreed the other option would be slightly better, would you be up for implementing it? Thanks!

@jbremer
Copy link
Member

jbremer commented Feb 17, 2018

I guess the answer is 'no' :-) We'll probably be implementing this ourself eventually (pinging @RicoVZ), closing PR. Feel free to reopen if you'd like to implement the proper approach, as discussed above.
Ah, just saw the other PR, we can do the category column addition on top of that PR.

@jbremer jbremer closed this Feb 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants