search

stacklok / minder

Software Supply Chain Security Platform
https://minder-docs.stacklok.dev/
Apache License 2.0
250 stars 35 forks source link

Don't use the same secret across all webhook-able provider instances #3235

Open jhrozek opened 4 months ago

jhrozek commented 4 months ago

Currently when we only support a single GitHub instance, it's probably not bad to use a single webhook secret to HMAC the messages sent by GH to our webhook handler - the attack surface is small, one would have to recover the shared secret from github which is currently not possible.

But as we add more providers, especially those where we have no idea who runs them and what their security are, we should use different webhook secrets per provider instance to make sure that compromising one provider doesn't mean that the attacker can impersonate the other provider instances.

Design page is available, although until we actually need this the design is a bit high level.

evankanderson commented 1 month ago

@dmjb -- I think you fixed this?

jhrozek commented 1 month ago

@dmjb -- I think you fixed this?

I don't think this is fixed. The problem that this issue describes is that we do have one config option webhook_secret_file that is used to create a webhook with the shared secret. Since all we support now is GitHub, this is not a problem, but it will be a problem once we support e.g. github where someone might enroll their own instance and see what secret is used to all provider instances.

jhrozek commented 1 month ago

cc @JAORMX this might be good to put into the GL work..

jhrozek commented 1 month ago

I moved the issue into the epic that tracks the webhook generalization. Right now as we only support the public github instance, we don't need to worry about this issue but it's going to be a problem once we support providers that can be self-hosted - in that case one instance of a gitlab provider would share its webhook secret with another instance.

We might want to approach this problem by treating the secret as a secret derivation material not the secret itself and then store the secret alongside the provider instance (which we might already do but I forgot)