[JENKINS-48012] Require a X-Hub-Signature header when receiving a hook payload and if…

[silbernm]

… a secret is configured

As reported in JENKINS-48012, an attacker can currently circumvent the signature check of the GitHub plugin simply by omitting the X-Hub-Signature header.

---

This change is 

[daniel-beck]

@Restricted perhaps?

[oleg-nenashev]

It's in the Test scope

[oleg-nenashev]

Javadoc needs to be adjusted then. Currently it says "Checks that an incoming request has a valid signature, if there is specified a signature in the config." which is exactly what the method does.

For discussion: Maybe it worth to add a configuration option which would allow opting-out from enforced checks. I'd guess it's important for on-premise instances like GitHub Enterprise

[daniel-beck]

Seems to be introduced by #134:

also don't bother signature validation if no header from github with signature

Versions from v1.21.0 appear to be affected.

[daniel-beck]

@oleg-nenashev

Currently it says "Checks that an incoming request has a valid signature, if there is specified a signature in the config." which is exactly what the method does.

It only checks the signature validity if it's present. For there to be a valid signature, it needs:

• to have a signature
• the signature needs to be valid

So the Javadoc LGTM.

[silbernm]

@daniel-beck
@oleg-nenashev
I think we are on the same page, but the documentation is misleading. There is never a "signature" in the config, but a "hook secret". I understood the Javadoc comment as "if there is specified a secret in the config" (because that is the only thing that makes sense IMHO) and coded my fix according to that interpretation. I have tried coming up with a better wording and I have moved the line that reads the signature header further down, such that it is clear that the header is validated if and only if a hook secret is specified in the GitHub plugin config.

Regarding GitHub Enterprise on-premise: That is exactly the setup that we are using at my workplace. When not configuring a shared secret in the GitHub plugin configuration, the fixed method will continue not to verify the signature, even if one was provided. So we should be fine on that use-case.

[daniel-beck]

[ERROR] src/main/java/org/jenkinsci/plugins/github/webhook/RequirePostWithGHHookPayload.java[135] (sizes) LineLength: Line is longer than 120 characters (found 124).

[KostyaSha]

seems fine