Opened 7 years ago

Closed 6 years ago

#2186 closed enhancement (fixed)

github change_hook needs security by default

Reported by: stefanha Owned by:
Priority: critical Milestone: 0.8.+
Version: 0.8.5 Keywords: github web sprint
Cc:

Description (last modified by dustin)

The github change_hook allows Github POST requests to trigger builds. It is part of WebStatus? and can be accessed via http://server/change_hook/github.

The buildbot documentation does not explain the security implications of enabling this change_hook. From what I can tell there are no checks in place to ensure the HTTP request is really from Github.

Anyone who pokes this URL will be able to trigger builds. It's also not clear to me whether builds are restricted to just the git repositories configured on the buildmaster or whether the repo URL from the HTTP request will be used.

Please document the security implications of the github change_hook.

I suggest supporting a secret token that can be configured both on the buildmaster and github side. If the HTTP request does not include the secret token then it will be denied. One way of doing this would be to customize the github change_hook URI, e.g. http://server/change_hook/1e505aa83c25910, so that it is not guessable.

Change History (13)

comment:1 Changed 7 years ago by Dustin J. Mitchell

Document the lack of auth in github hook

Refs #2186. This does not add authentication, but at least makes its lack clear.

Changeset: 41feadd8e4dd4a1c911ae82655c06be2f3191689

comment:2 Changed 7 years ago by dustin

  • Keywords sprint added
  • Milestone changed from undecided to 0.8.7
  • Priority changed from major to critical

comment:3 follow-up: Changed 7 years ago by tom.prince

I wonder if github hooks support HTTP basic auth? If they do, specifying a user/passwd in the url, and supporting it in buildbot is all that is required.

comment:4 Changed 6 years ago by dustin

  • Milestone changed from 0.8.+ to 0.8.7

Github now supports OAuth for HTTP requests *to* Github; but this is about requests *from* github, and ensuring that the URL provided to github can't be guessed.

I did some fanciness to support this in https://github.com/djmitche/highscore - that might be useful.

comment:5 Changed 6 years ago by dustin

  • Milestone changed from 0.8.7 to 0.8.+

comment:6 in reply to: ↑ 3 ; follow-up: Changed 6 years ago by clepple

Replying to tom.prince:

I wonder if github hooks support HTTP basic auth? If they do, specifying a user/passwd in the url, and supporting it in buildbot is all that is required.

It looks like they support HTTP basic auth now: https://help.github.com/articles/what-ip-addresses-does-github-use-that-i-should-whitelist#service-hook-ip-addresses

I'm interested in taking a crack at this, but I haven't delved into User objects yet. Are they a good match for verifying Basic auth credentials? Any other items I should be aware of?

comment:7 Changed 6 years ago by tom.prince

The proper way to handle basic auth is probably twisted.web.guard. I suspect that using user objects aren't appropriate, since github isn't a user.

comment:8 Changed 6 years ago by dustin

I'd like to have some code using twisted.web.guard in the codebase, too, so I can model the nine web service on it.

comment:9 in reply to: ↑ 6 ; follow-up: Changed 6 years ago by marchael

Replying to clepple:

I'm interested in taking a crack at this, but I haven't delved into User objects yet. Are they a good match for verifying Basic auth credentials? Any other items I should be aware of?

Hi! I want to fix this issue too! :) If you're already working on it, just let me know.

comment:10 in reply to: ↑ 9 Changed 6 years ago by clepple

Replying to marchael:

Replying to clepple:

I'm interested in taking a crack at this, but I haven't delved into User objects yet. Are they a good match for verifying Basic auth credentials? Any other items I should be aware of?

Hi! I want to fix this issue too! :) If you're already working on it, just let me know.

I don't have anything written yet.

comment:11 Changed 6 years ago by dustin

So, rather than both of you back off from implementing this, I'd prefer to err on the side of both working on it and sharing the results early and often. It will be a relatively small fix, and multiple implementations should lead to better code.

comment:12 Changed 6 years ago by marchael

I wrote simple patch for basic auth https://github.com/buildbot/buildbot/pull/657

It didn't merged for now, but I hope it will be.

comment:13 Changed 6 years ago by dustin

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from new to closed

Merged!

Note: See TracTickets for help on using tickets.