cudeso / misp2sentinel

MISP to Sentinel integration
MIT License
52 stars 17 forks source link

Fix multi-single-tenant case, add error logging, kv integration as default method #63

Closed Kaloszer closed 8 months ago

Kaloszer commented 8 months ago

Change logic to no longer use tenantId as the key, tenants object now contains an object with all the info in it.

[
   { 
      "tenantId": "<TENANT_ID_WITH_APP_1>",
      "id": "<APP_ID>",
      "secret": "<APP_SECRET>",
      "workspaceid": "<WORKSPACE_ID>"
   }
   {
      "tenantId": "<TENANT_ID_WITH_APP_N>",
      "id": "<APP_ID>",
      "secret": "<APP_SECRET_N>",
      "workspaceid": "<WORKSPACE_ID_N>"
   }
]

Key vault defined as default in config.py

Application configuration items that are no longer needed as they are being parsed from KV using az module if kv integration is used

Due to that kvName now needs to be defined within Application Configuration items to know where to parse the mispkey/uri from

The same can be done for tenants, but haven't touched it yet for now. But should be pretty simple to change aswell.

Readme now also contains information regarding this change.

Test case: 2 workspaces within a single tenant:

image

lnfernux commented 8 months ago

Thanks for the PR @Kaloszer

The logging changes look good, so does the json change. Will review later.

As for KV integration, it's a roundabout way of doing things. Currently the kv integration is done using references to the key vault in the application settings. You can go to the readme for a reference on how to set that up, but it basically uses the managed identity and a reference to the secret in the kv to fetch the setting and allow us to reference the app setting from env variables. I personally think this is the easiest and best way to do that reference. Doing it in the configuration file is just something I added to the main script in order to support kv integration when you don't have the app setting option πŸ‘

One goal is to have as much of the settings reachable in the app settings to easily tune the configuration. I'm planning on moving a lot more of the settings to the app settings soon(Tm).

cudeso commented 8 months ago

@lnfernux Do you want me to already accept the PR or do I wait for your change for moving the settings to the app?

cudeso commented 8 months ago

@Kaloszer Thank you very much for the nice contribution; I wait for feedback from @lnfernux

lnfernux commented 8 months ago

I have tested the logging and json-changes now.

@Kaloszer There's one minor issue, which is the documentation shows the key for the item as workspaceid but init.py referes to workspaceId which throws a Result: Failure Exception: KeyError. Can be fixed by updating so the key is the same in documentation and script. Other than that, works fine.

Could you please remove the changes to config.py and update README.MD to only document the changes for the JSON-object? You might also want to remove the image from the commit πŸ‘πŸ»

I'll approve the PR once it's done, great job btw! πŸ’―

Kaloszer commented 8 months ago

I have tested the logging and json-changes now.

@Kaloszer There's one minor issue, which is the documentation shows the key for the item as workspaceid but init.py referes to workspaceId which throws a Result: Failure Exception: KeyError. Can be fixed by updating so the key is the same in documentation and script. Other than that, works fine.

Could you please remove the changes to config.py and update README.MD to only document the changes for the JSON-object? You might also want to remove the image from the commit πŸ‘πŸ»

I'll approve the PR once it's done, great job btw! πŸ’―

Rolled back changes in config.py to the current default config Readme.md/Readme.MD now contain correct case sensitivity