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

[Comlaude] improvements #3055

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[Comlaude] improvements #3055

wants to merge 3 commits into from

Conversation

yassine-ouaamou
Copy link
Member

This PR contains the code commited by @MohamedMerimi:

Proposed changes

  • I added the “_deserialize_json_string” function to convert the JSON-formatted text string into a python object, which eliminates json errors when executing on OpenCTI.
  • I Added “_validate_required_fields” function to check that all required data (mandatory fields) are present and not empty, which also prevents errors in OpenCTI executions
  • Addition of the “_ping_connector” function, which pings the connector every 5 minutes to keep it active (by modifying the “run” function).
  • Status tracking with “last_run” variable in format “{”last_run“: ‘2024-11-18T15:13:36Z’}”.

Related issues

@yassine-ouaamou yassine-ouaamou self-assigned this Dec 2, 2024
@yassine-ouaamou yassine-ouaamou added the community use to identify PR from community label Dec 2, 2024
@yassine-ouaamou yassine-ouaamou removed their assignment Dec 2, 2024
@Megafredo
Copy link
Member

Megafredo commented Dec 3, 2024

Hello @yassine-ouaamou, and @MohamedMerimi,
Since we don't have any identifying information to test this community connector, we can't test the quality of data ingestion from this connector, so we'll just skim the connector code to highlight best practices.

Suggestions for Code Improvement

During this PR, I took the time to examine the code as a whole, and here are a few points I think would be useful with best practices :

1. README

  • Add connector_duration_period variable to explain its configuration (see point 6)
  • Add connector_queue_threshold variable if relevant (default: 500MB) (see point 6)
  • Remove reference to interval (see point 6)
  • Delete connector_update_existing_data, as it is depreciated
  • Add missing variable comlaude_score
  • Correct a bug: it's not config_username, but comlaude_username

2. Docker Compose

  • Remove CONFIG_INTERVAL to avoid confusion
  • Add CONNECTOR_DURATION_PERIOD and CONNECTOR_QUEUE_THRESHOLD (if necessary)

3. config.yml.sample

  • Remove update_existing_data from the comlaude section.
  • Add score in the comlaude section.
  • Add duration_period in the connector section (for example, PT2H for a 2-hour period).
  • Include queue_threshold if necessary in the connector section.

4. Import management

  • When using isort, we recommend applying the --profil black to ensure consistent formatting. However, path-related problems can sometimes occur. Here's a suggestion for modifying imports to avoid ignoring the rules imposed by isort.
import datetime  
import json  
import os  
import sys  
import threading  
import time  

import comlaude  
import stix2  
import yaml  
from pycti import (  
    Identity,  
    Indicator,  
    OpenCTIConnectorHelper,  
    StixCoreRelationship,  
    get_config_variable,  
)  
from stix2 import TLP_AMBER, Bundle, DomainName  

5. requirements.txt

  • Explicitly add versions of dependencies used to ensure compatibility (e.g. pycti==6.4.2, requests==2.32.2)

6. Code cleaning

  1. Obsolete Variable and Method: interval
  • The following method, which I believe is never called, and any associated references can be deleted :
def _get_interval(self):
 """
 Get the interval of execution in seconds.
 :return: Interval in seconds.
 """
 return int(self.config_interval) * 60 * 60
def run(self):  
    self.helper.schedule_iso(  
        message_callback=self.your_old_process_run,  
        duration_period=self.duration_period  
    )
  1. Deprecated variable: update_existing_data
  • Delete this obsolete configuration:
self.update_existing_data = get_config_variable(
    "CONFIG_UPDATE_EXISTING_DATA",
    ["comlaude", "update_existing_data"],
    self.config,
    isNumber=True,
)
  • And also its reference in the call to send_stix2_bundle :
self.helper.send_stix2_bundle(
	bundle.serialize(),
	update=self.update_existing_data, # <<<<<< to be deleted
	work_id=self.work_id,
)
  1. Improving bundles:
  • To improve the quality of STIX bundles, use cleanup_inconsistent_bundle. Note that it is important to add all the markings used (e.g. TLP_AMBER):
self.helper.send_stix2_bundle(
    bundle.serialize(),
    cleanup_inconsistent_bundle=True,
    work_id=self.work_id,
)
  1. Modernised logger :
  • Replace the old self.helper.log_error() calls with the new format self.helper.connector_logger.error("Message", {"foo": foo, "bar": bar})
  1. Closing Work ID:
  • Remember to mark the work id as finished, even if you make a mistake, for example in the finally
finally:  
    if self.work_id is not None:  
        self.helper.api.work.to_processed(self.work_id, "Finished")
  1. Replacing utcnow() :
  • datetime.utcnow()` has been deprecated since Python 3.12. See Python Doc
  1. Adding a traceback
  • To improve exception handling in the main input of the script, add a traceback to the main block:
if __name__ == "__main__":
    """
    Entry point of the script.
    """
    import traceback

    try:
        connector = ComlaudeConnector()
        connector.run()
    except Exception:  
        traceback.print_exc()  
        sys.exit(1)

@yassine-ouaamou yassine-ouaamou added partner used to identify PR from patner and removed community use to identify PR from community labels Dec 5, 2024
if field not in domain_object or _is_empty(domain_object[field])
]
if missing_fields:
print(f"Skipping domain due to missing fields: {missing_fields}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please use the logger rather than a print please ? (and preferably when using the method rather than directly in it : this is not this method that skips but the code that uses its result)

@@ -84,13 +120,24 @@ def _generate_dynamic_custom_properties(helper, domain_object, score, author_ide
custom_properties = {
"x_opencti_score": score,
"x_opencti_description": "This domain is known infrastructure managed by Comlaude.",
"created_by_ref": author_identity.id, # Add the created_by_ref to custom properties
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning the code base 👍

@@ -255,7 +302,7 @@ def _load_config(self) -> dict:
)
return config
except Exception as e:
print(f"Error loading configuration: {str(e)}")
self.helper.log_error(f"Error loading configuration: {str(e)}")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
partner used to identify PR from patner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants