minder icon indicating copy to clipboard operation
minder copied to clipboard

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

Open jhrozek opened this issue 1 year ago • 5 comments

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.

jhrozek avatar May 03 '24 14:05 jhrozek

@dmjb -- I think you fixed this?

evankanderson avatar Jul 30 '24 13:07 evankanderson

@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 avatar Aug 13 '24 17:08 jhrozek

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

jhrozek avatar Aug 13 '24 17:08 jhrozek

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)

jhrozek avatar Aug 15 '24 12:08 jhrozek

Did we fix this since August, or is this still TODO?

evankanderson avatar Dec 17 '24 14:12 evankanderson