cudeso / misp2sentinel

MISP to Sentinel integration
MIT License
60 stars 20 forks source link

Add MSI support for function and updated doco #86

Closed jusso-dev closed 8 months ago

jusso-dev commented 8 months ago

Added MSI support for the function app to negate needing a separate identity in Azure to persist indicators and interact with Azure Key Vault. This is tested as working in my clients environment.

Addresses #80 and #3

lnfernux commented 8 months ago

I've been a bit busy lately, but the integration with Key Vault was already done via a managed identity using the application settings in the function app. This part of the code was created for a function app, not for running on a virtual machine.

The flow would be like this:

  1. Managed Identity is enabled for the Azure Function
  2. Managed Identity is given access to read secrets in the KV
  3. The relevant secrets are defined as key vault references in application settings
  4. You can now reference the KV-secrets as env-variables in the code

In my eyes, the past changes here have only contributed to make the code more busy. I don't see any reason define the code for interacting with the key vault in the config.py file, as this was already covered.

What this change seems to cover is the ability to use a MSI to interact with the KV to set values (this was already covered). If the idea is to allow for single-instance mode runs, you could simple define only one workspace in the tenants key vault secret, and it would only run for that single instance, instead of making the code more complex in my eyes.

@cudeso any thoughts? I'd appreciate the input.

Also @jusso-dev if this is for purposes of making it easier to run on virtual machines, then maybe we can create a fork or folder for the virtual machine version of this. Might be easier, since it's not (in my mind) smart to try and develop this for two different runtimes at once.

jusso-dev commented 8 months ago

Hey @lnfernux thanks for reaching out!

To clarify, this change supports the already existing KV integration yes, but also now allows you to not have to provision and manage a separate Entra ID identity to persist threat indicators via the "Threat Intelligence Ingestion Upload API".

So by adding this extra config check, we can save users the hassle of having to setup, manage, configure and deploy two identities to use this solution.

lnfernux commented 8 months ago

I see, that wasn't very clear to me. Either way, the requirements for KV integration in the code is still not there. For the multi-tenant flow, you'd use an Enterprise Application to push indicators, and the application settings + MSI for the actual secrets from the KV, while for the other flow (MSI directly to single host) you'd simple just use an MSI, no secrets, correct? :)

jusso-dev commented 8 months ago

No sorry that's not quite right. The single host variant still requires secrets in Key Vault and without this change, two identities.

The code wasn't update to support multi apps with a single identity because without something like Azure Lighthouse, it's not possible and we can't check for that programmatically unless we add another key, which isn't preferred from what I've read.

lnfernux commented 8 months ago

But, why would you need two identities without this change? The KV integration is already mentioned in the docs, you add an MSI to the function and you can add the key vault secrets as references in the application settings. This is native Azure. The same MSI can also be used to integrate to the workspace and push indicators. So one identity, no need for KV-integration in the script itself.

jusso-dev commented 8 months ago

The app has no context if a MSI is configured and if it doesn't have any context it will try and auth to endpoints like the indicators upload with client id and secret provided, you can't configure client secret on enterprise apps such as an MSI

EDIT - we could probably negate setting this env variable and instead check if "tenants" is set via Key Vault indicating single mode on functions and then assume MSI, but as someone who just had to install this fresh into my clients tenant it was hard enough to navigate the docs and code so I'm trying to be explicit and clean with what needs to be set.

lnfernux commented 8 months ago

I mean the app doesn't need the context if MSI is set or not, does it? You could simply check for the existence of the tenants variable, as you said, and if it's empty then try using the MSI instead?

My idea for the AzureFunction version of this script when I made it was to move all of the settings from config.py into the application settings of the function, either via key vault references or just values in plaintext (if they're not secrets), and have the script be as clean as possible. Which is also why the AzureFunction only supports Upload Indicators API, and not the Graph one.

I think for someone that's new, it's easier to understand the code if it's simple and allows for fewer mistakes in configuration. That's at least the case for me, and less moving parts means less likely to break.

jusso-dev commented 8 months ago

That makes sense, I think the reality is however there are lots of hard-coded configuration values and no mention that MSI is supported, only references that it only supports Azure VMs which is false.

Happy to just close this PR, no issues on my side.

lnfernux commented 8 months ago

I'm thinking we could jump on a call and work through the entire script + the docs, so everything is both easy in terms of the code and integration friendly, at some point. I'm leaving for a weeks on sunday, but when I get back we can look into it, if you'd like?