ValidatePayload should not accept nil or empty secretToken slice

[myitcv]

#1127 added support for calling ValidatePayload with a nil or empty secretToken slice. The justification for this change is explained in the current docs:

If your webhook does not contain a secret token, you can pass nil or an empty slice. This is intended for local development purposes only and all webhooks should ideally set up a secret token.

I think this leaves ValidatePayload with dangerous default behaviour. Because now every caller of this function has to check that they have a non-nil and non-empty secretToken slice if they want to validate a payload. If they don't, it's possible to accidentally call the function with a nil or empty slice (bad configuration, etc) and never know about it. This is not a safe default. Better would be to have the function panic or return an error in case secretToken is nil or empty - because such a value can never be used to validate a payload, which is obviously what the caller intended to do in calling the function.

The same applies to ValidatePayloadFromBody().

I haven't looked at other functions/methods.

Returning to the goal of #1126, I would instead have pushed back and suggested that developers who are in development mode change their calling code to simply not call ValidatePayload when they detect they are in such a mode.

[gmlewis]

So you disagree with the consensus reached on #1126 and wish to revert its changes, or just want to return a custom error value when nil is used?

[myitcv]

I defer to others on whether you revert or otherwise.

I am just trying to describe what I think is a safe default: ValidatePayload() should return an error (or panic) if the payload cannot be validated correctly.

[gmlewis]

I defer to others on whether you revert or otherwise.

I am just trying to describe what I think is a safe default: ValidatePayload() should return an error (or panic) if the payload cannot be validated correctly.

OK, I think I'll defer to @willnorris on this one. 😁

[willnorris]

In hindsight, we shouldn't have implemented it this way. It's still fine to have a webhook without a secret (and therefore without a signature) if that's what you really want, but to @myitcv's point, you probably shouldn't be able to circumvent signature verification on a webhook which does have a secret configured simply by passing a nil value.

What we probably should have done, and what I'm about to send a PR to update, is to check whether a non-empty secret was passed to the ValidatePayload function OR if the payload actually has a signature attached. If either of those are true, we should verify that they match.

Looking back at the original post in #1126, it sounds like the request was to support webhooks which don't have a secret, which will still be supported with my upcoming change. So I think we can safely make this change without changing the original intent of #1126.

[myitcv]

I make the following point humbly acknowledging I am neither a regular contributor nor a security expert!

What we probably should have done, and what I'm about to send a PR to update, is to check whether a non-empty secret was passed to the ValidatePayload function OR if the payload actually has a signature attached.

Not sure that I agree. If I make a call to ValidatePayload(), I'm clearly asserting that I want the payload of the request to be validated. If there are any bits missing in order to achieve a result of "validated successfully" I want to receive an error.

Imagine if the relevant header is missing for whatever reason (GitHub error): I would also want that to be caught. Similarly with a configuration error resulting in a nil of empty slice for the secret token.

Anything else just doesn't feel like a safe default to me.

[willnorris]

huh, I really thought this was doing more than JUST signature validation, which was my main argument for accepting this as a valid use case. It's been years since I've worked with this code regularly, so maybe it used to and doesn't any longer? I'll have to dig in more.

[willnorris]

The tests certainly make it look like it used to validate the event type and event ID as well, though those are no longer used apparently.

[willnorris]

okay, I was wrong about that. This was added in #294, and there was a decent amount of discussion there about what this is doing and the design. It looks like ValidatePayload has only ever handle signature checking, and the eventID present in the tests was just accidental and has never actually been used.

That said, I still think the change I described above is correct, and actually for the reasons you state:

If I make a call to ValidatePayload(), I'm clearly asserting that I want the payload of the request to be validated. If there are any bits missing in order to achieve a result of "validated successfully" I want to receive an error.

I would also extend to that to say that if I call ValidatePayload with a nil secretKey, then I'm explicitly stating that I do not expect a signature to be present, and if one IS present then something may be wrong. If we instruct callers to simply not call ValidatePayload at all for webhooks that don't have configured secrets, then they are missing that protection.

Imagine if the relevant header is missing for whatever reason (GitHub error): I would also want that to be caught. Similarly with a configuration error resulting in a nil of empty slice for the secret token.

Both scenarios would be caught in my proposed change. If you indicate that you are expecting a signature to be present, but it's not, then that's an error. If you pass a nil secret token (either because of a configuration error or intentionally), but a signature is present in the payload, then that's an error also.

The only scenario this would not handle is when both of the above are present at the same time... a GitHub error mistakenly omits the Signature header AND a configuration error passes a nil secret token. A call to ValidatePayload() would pass, since both sides match. Though that seems far-fetched enough to not worry about.

[myitcv]

I would also extend to that to say that if I call ValidatePayload with a nil secretKey, then I'm explicitly stating that I do not expect a signature to be present, and if one IS present then something may be wrong.

The problem is that as the authors of ValidatePayload() can't distinguish between those situations where the caller intended to pass in a nil or empty slice, or not (other than via static analysis to show that the nil identifier was used, or a constant representing the nil value, or an empty slice literal).

FWIW, even if I did make a call to ValidatePayload() with an explicitly nil secretToken, my intuition (without reading the docs) would be that the function should return an error.

Webhooks without a secret token are not secure and should be avoided.

We are agreed on this point. i.e. everyone should use secrets, even in development.

It therefore feels like we're bending over backwards too much for the case of a (development) mode where no secret is used.

If the call to ValidatePayload() does just that, unconditionally, then the users of webhooks where secrets are used can call that function safely, know exactly what it does, and get errors in all situations where that contract cannot be satisfied. The docs are very simple, and no caveats are required.

This rightly shifts the burden of working around this in cases where no secret is used onto the developer.

In fact I'd argue that doing so will actually increase the likelihood that people will shift to using secrets. Right now, the pattern of allowing nil or empty slice allows the developer to use webhooks without secrets at no cost.

The message then shifts to become "even in development mode you should set a secret".

Perhaps I'm missing something of the workflow/DX of this (development) mode where no secret is set. Can someone point me to an example of what requires people to not set a secret in such a mode? Why can they not just set a simple secret like testing?

[willnorris]

Sure, I completely see where you're coming from.

In some ways, this approaches a philosophical question of what the role of go-github is. In general, we've tried to simply provide an unopinionated, idiomatic Go library for the capabilities of the GitHub REST API. Maybe GitHub shouldn't allow creating webhooks without secrets, but since they do, I don't think it's unreasonable for the library to support that (and in this case, provide a helper function to validate the presence or absence of a payload signature, based on what the caller indicates is expected). There may be be cases where we have taken a more principled stance and not supported some capability of the GitHub API, but none immediately come to mind.

So on this particular point, we just disagree. But I think it's a relatively small difference in the grand scheme of things. And I also think this function is in a much better spot thanks to this issue (pointing out the mis-feature of being able to skip signature checks altogether), so honestly thanks for brining it up!