-
Notifications
You must be signed in to change notification settings - Fork 446
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
[ProofPoint TAP] implement application #3343
[ProofPoint TAP] implement application #3343
Conversation
external-import/proofpoint-tap/proofpoint_tap/domain/models/octi/common.py
Outdated
Show resolved
Hide resolved
external-import/proofpoint-tap/proofpoint_tap/client_api/v2/threat.py
Outdated
Show resolved
Hide resolved
external-import/proofpoint-tap/proofpoint_tap/client_api/v2/campaign.py
Outdated
Show resolved
Hide resolved
external-import/proofpoint-tap/proofpoint_tap/client_api/common.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core of the PR
external-import/proofpoint-tap/tests/test_client_api/test_common.py
Outdated
Show resolved
Hide resolved
external-import/proofpoint-tap/tests/test_client_api/test_v2/test_campaign.py
Outdated
Show resolved
Hide resolved
external-import/proofpoint-tap/tests/test_client_api/test_v2/test_forensics.py
Outdated
Show resolved
Hide resolved
external-import/proofpoint-tap/tests/test_client_api/test_v2/test_threat.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some typos and missing decorators prevent the connector to launch (see comments).
I fixed these issues locally and was able to:
- make the tests pass
⚠️ (some counters need to be incremented by one since the TLP markings have been added to the bundle) - run connector locally with a
.config.yml
file ✅ - run connector locally with a
.env
file ✅ - build the docker image with a
.env
file ✅ - see no errors during ingestion ✅
Despite the complexity of the product, the code is pretty straightforward and each file's responsibility well defined 👏👏👏
@abstractmethod | ||
def _name(self) -> str: ... | ||
|
||
@_make_error_handler("Unable to retrieve connector name in config") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:+1
@abstractmethod | ||
def _scope(self) -> str: ... | ||
|
||
@_make_error_handler("Unable to retrieve connector scope in config") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:+1
|
||
@property | ||
@_make_error_handler("Unable to retrieve OpenCTI Token in config") | ||
def token(self) -> SecretStr: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for yu remark !
In my opinion SecretStr is used to remind the developper he/she is manipulating a secret and not a simple string and I should keep it this way here.
To fix the issue you highlighted, I've altered the to_dict method to handle this case if it is ok for you :)
== 1 | ||
) | ||
## Total 8 entities | ||
assert len(entities) == 8 # noqa: S101 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to add +1 for the TLP marking entity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:+1
== 1 | ||
) | ||
## Total 22 entities | ||
assert len(entities) == 22 # noqa: S101 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to add +1 for the TLP marking entity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:+1
CONNECTOR_NAME=ProofPointTAP | ||
CONNECTOR_SCOPE=report | ||
CONNECTOR_DURATION_PERIOD=PT12H | ||
CONNECTOR_LOG_LEVEL=warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be replaced by warn
, otherwise a ValueError is raised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:+1
TAP_MARKING_DEFINITION=white | ||
TAP_EXPORT_CAMPAIGNS=True | ||
TAP_EXPORT_EVENTS=True | ||
TAP_EVENTS_TYPES=issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: in config adapter, the expected variable is TAP_EVENTS_TYPES
(plural)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:+1
self._logger.warning( | ||
"[CONNECTOR] Connector acquisition SINCE parameter overwritten", | ||
{ | ||
# "previous": str(self.config.tap.export_since), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment to remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:+1
self._log_error(f"Data retrieval error: {str(e)}") | ||
return False | ||
|
||
def work(self) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good idea to call it work
instead of process
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you !
I still call _process the ETL steps though 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true 😅 maybe _send
or _import
/_export
could do the trick... but the most important IMHO is to not use process_message
method's name anymore, because the method wasn't processing any messages ^^
score=self.score, | ||
created_by_ref=self.author.id if self.author else None, | ||
), | ||
custom_properties=self.custom_properties_to_stix(), | ||
) | ||
|
||
def to_indicator( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember if I mentioned it in the previous PR but these are very convenient methods 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your remarks and tips @Powlinett !
I think I've fixed the issues you highlighted. Could you have a glance please ?
a002ffd
into
feat/1538-proofpoint-tap-connector
Proposed changes
Related issues
Checklist
Further comments
N/A