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

PhoneInfoga#995 #2107

Merged
merged 44 commits into from
Feb 21, 2024
Merged

PhoneInfoga#995 #2107

merged 44 commits into from
Feb 21, 2024

Conversation

g4ze
Copy link
Contributor

@g4ze g4ze commented Jan 31, 2024

closes #995

Description

Add phoneinfoga_scan analyzer

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist

  • I have read and understood the rules about how to Contribute to this project
  • The pull request is for the branch develop
  • A new plugin (analyzer, connector, visualizer, playbook, pivot or ingestor) was added or changed, in which case:
    • I strictly followed the documentation "How to create a Plugin"
    • Usage file was updated.
    • Advanced-Usage was updated (in case the plugin provides additional optional configuration).
    • If the plugin requires mocked testing, _monkeypatch() was used in its class to apply the necessary decorators.
    • I have dumped the configuration from Django Admin using the dumpplugin command and added it in the project as a data migration. ("How to share a plugin with the community")
    • If a File analyzer was added and it supports a mimetype which is not already supported, you added a sample of that type inside the archive test_files.zip and you added the default tests for that mimetype in test_classes.py.
    • If you created a new analyzer and it is free (does not require API keys), please add it in the FREE_TO_USE_ANALYZERS playbook by following this guide.
    • Check if it could make sense to add that analyzer/connector to other freely available playbooks.
    • I have provided the resulting raw JSON of a finished analysis and a screenshot of the results.
  • If external libraries/packages with restrictive licenses were used, they were added in the Legal Notice section.
  • Linters (Black, Flake, Isort) gave 0 errors. If you have correctly installed pre-commit, it does these checks and adjustments on your behalf.
  • I have added tests for the feature/bug I solved (see tests folder). All the tests (new and old ones) gave 0 errors.
  • If changes were made to an existing model/serializer/view, the docs were updated and regenerated (check CONTRIBUTE.md).
  • If the GUI has been modified:
    • I have a provided a screenshot of the result in the PR.
    • I have created new frontend tests for the new component or updated existing ones.

Important Rules

  • If you miss to compile the Checklist properly, your PR won't be reviewed by the maintainers.
  • If your changes decrease the overall tests coverage (you will know after the Codecov CI job is done), you should add the required tests to fix the problem
  • Everytime you make changes to the PR and you think the work is done, you should explicitly ask for a review. After being reviewed and received a "change request", you should explicitly ask for a review again once you have made the requested changes.
    Screenshot from 2024-01-31 21-10-25

raw json:
{"result":{"valid":true,"number":"33679368229","local_format":"0679368229","international_format":"+33679368229","country_prefix":"+33","country_code":"FR","country_name":"France","location":"","carrier":"Orange France SA","line_type":"mobile"}}

@g4ze
Copy link
Contributor Author

g4ze commented Jan 31, 2024

I still need to update docs and make migrations

@g4ze g4ze changed the title Phone infoga, closes #995 PhoneInfoga, closes #995 Jan 31, 2024
@g4ze
Copy link
Contributor Author

g4ze commented Jan 31, 2024

Docs done

@g4ze
Copy link
Contributor Author

g4ze commented Jan 31, 2024

Hello! @mlodic , this took me sometime to get done. The test is failing here. It pertains to the migration file. I used the dumpplugin comand to create the file. I've crossed checked it with the way previous migrations are written. Still can't seem to find out why this is happening.

Comment on lines 14 to 15
environment:
- GIN_MODE=release
Copy link
Member

Choose a reason for hiding this comment

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

reference to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should run the GIN framework used in release mode. It works fine even without it.

Comment on lines 1 to 13
version: '3.7'

services:
phoneinfoga:
container_name: phoneinfoga
restart: unless-stopped
image: sundowndev/phoneinfoga:latest
espose:
- "5000"
env_file:
- env_file_integrations
environment:
- GIN_MODE=release
Copy link
Member

Choose a reason for hiding this comment

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

copy/pastes are not required because the compose-tests.yml is an override of the compose.yml. Please use other docker analyzers as a reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so should this file even contain anything as we are using an external service; would it make sense to have an additional dockerfile that clones the repo and builds it for the compose-tests.yml?

Copy link
Member

Choose a reason for hiding this comment

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

you are right, we could basically leave this almost empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I just mention version: 3.8 in the test yml and rest empty?

integrations/phoneinfoga/compose.yml Outdated Show resolved Hide resolved
docs/source/Advanced-Usage.md Outdated Show resolved Hide resolved
@g4ze g4ze requested a review from mlodic February 1, 2024 20:52
@@ -0,0 +1,16 @@
version: '3.7'
Copy link
Member

Choose a reason for hiding this comment

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

use version 3.8 if you can, like we do in all other docker compose files in the project

@g4ze g4ze requested review from mlodic February 2, 2024 08:44
@mlodic
Copy link
Member

mlodic commented Feb 2, 2024

the migration error is here:

  File "/opt/deploy/intel_owl/api_app/analyzers_manager/migrations/0064_analyzer_config_phoneinfoga.py", line 118, in migrate
    _create_object(PluginConfig, value)

I think that there is some problem with the IDs

django.db.utils.IntegrityError: duplicate key value violates unique constraint "api_app_parameter_name_python_module_id_85f61e18_uniq"

I think that could be the error, I asking to @0ssigeno a second opinion

@0ssigeno
Copy link
Contributor

0ssigeno commented Feb 5, 2024

Checking

@g4ze
Copy link
Contributor Author

g4ze commented Feb 5, 2024

uhoh, i did it again; overlooked a checkpoint. Made required migrations(agn)

@g4ze
Copy link
Contributor Author

g4ze commented Feb 7, 2024

it worked lol. @mlodic any other changes?

@g4ze g4ze changed the title PhoneInfoga, closes #995 PhoneInfoga#995 Feb 8, 2024
Copy link
Member

@mlodic mlodic left a comment

Choose a reason for hiding this comment

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

you can ignore codecov errors, it's a problem of their server. We disabled it.

Considering that I merged Validin PR, could you please adjust the migration here again :P too many PR :P

Comment on lines 1 to 34
# This file is a part of IntelOwl https://github.com/intelowlproject/IntelOwl
# See the file 'LICENSE' for copying permission.


from django.db import migrations


def migrate(apps, schema_editor):
playbook_config = apps.get_model("playbooks_manager", "PlaybookConfig")
AnalyzerConfig = apps.get_model("analyzers_manager", "AnalyzerConfig")
pc = playbook_config.objects.get(name="FREE_TO_USE_ANALYZERS")
pc.analyzers.add(AnalyzerConfig.objects.get(name="Phoneinfoga").id)
pc.full_clean()
pc.save()


def reverse_migrate(apps, schema_editor):
playbook_config = apps.get_model("playbooks_manager", "PlaybookConfig")
AnalyzerConfig = apps.get_model("analyzers_manager", "AnalyzerConfig")
pc = playbook_config.objects.get(name="FREE_TO_USE_ANALYZERS")
pc.analyzers.remove(AnalyzerConfig.objects.get(name="Phoneinfoga").id)
pc.full_clean()
pc.save()


class Migration(migrations.Migration):
dependencies = [
("playbooks_manager", "0026_add_zippy_scan_to_free_to_use"),
("analyzers_manager", "0065_analyzer_config_phoneinfoga"),
]

operations = [
migrations.RunPython(migrate, reverse_migrate),
]
Copy link
Member

Choose a reason for hiding this comment

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

This is not required because this analyzer is "optional" and it requires to run an additional container.
To avoid to have it providing errors in the base installation, that is the most commonly used, I would not add it in the default playbook. Please remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noted. Shouldn't we also mention that such type of analyzers which are free but docker-based/optional can't go in default playbook in the docs or the PR checklist; to avoid such implementations in future.

Copy link
Member

Choose a reason for hiding this comment

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

sure it would make sense to explicitly say it in the PR template

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just one more concern, about another docker based analyzer, cyberchef which is in the default playbook
image

LOG_LEVEL=INFO
# For phoneinfoga analyzer
# NUMVERIFY_API_KEY=
Copy link
Member

Choose a reason for hiding this comment

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

isn't there a way to leverage classic Analyzer Secrets for this API key? In that way, every user can configure it and use its own API key.
I don't like to have it configured here because this API key would be shared among all the users. This could be an unwanted behavior.

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'll look into that. Will have to figure out a workaround :'')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi there! I took a look at their code base. The problem at hand involves the extraction of all the API keys from the from the integration .env file instead of directly using the key as authorization header when making calls. This is hard coded in their code base.
One work around could be making a new container everytime a new user selects the analyzer. This would have some infra implications but would allow us to have unique containers of the analyzer with its own env file for each individual user. (not a good approach tho)
I have raised an (issue)[https://github.com/sundowndev/phoneinfoga/issues/1393] for further enquiry.
In the meantime we could update the analyzer configs to take API keys from the user in the interface and implement that logic whenever they solve our problem, till then we might have to leverage integration env only.

Copy link
Member

Choose a reason for hiding this comment

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

thank you for the efforts. I would like to avoid to create complex things for this problem, I like to keep things simple if possible.
I suggest waiting for an answer from them. In case they do not answer within a week, we consider the problem unsolved and so we'll merge your actual PR even if there is that problem. In that case, we would need to specify more information about it in the docs, just to be explicit about this problem with the users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for sure! In the meantime can you guide me to another issue i can take up?

Copy link
Member

Choose a reason for hiding this comment

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

this is a sightly different analyzer: #1103.
This is a really important source for this project

g4ze and others added 2 commits February 8, 2024 16:01
code quality

Co-authored-by: Matteo Lodi <30625432+mlodic@users.noreply.github.com>
@g4ze g4ze requested a review from mlodic February 8, 2024 14:02
@mlodic
Copy link
Member

mlodic commented Feb 16, 2024

Just to keep a reference, they answered heresundowndev/phoneinfoga#1393)

We'll wait them to solve the issue regarding API keys than we'll finish this PR

@g4ze
Copy link
Contributor Author

g4ze commented Feb 21, 2024

image
works using new version of phoneinfoga

@g4ze
Copy link
Contributor Author

g4ze commented Feb 21, 2024

@mlodic kindly review.

@mlodic
Copy link
Member

mlodic commented Feb 21, 2024

let's wait for the tests. If they pass we can merge the PR. Ty!

@mlodic mlodic merged commit 5d6c04a into intelowlproject:develop Feb 21, 2024
11 checks passed
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.

None yet

3 participants