Skip to content
This repository was archived by the owner on Mar 20, 2024. It is now read-only.

server: Support a base_secret #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

server: Support a base_secret #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions cfg.sample.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,23 @@ access_token = ""
app_client_id = ""
app_client_secret = ""

# A base shared secret; this can be used to generate the per-repo github secret,
# which is what's used to verify authenticity of github -> Homu webhooks. The
# per-repo secret can be computed as follows (you'll need to enter this in
# the repo webhook configuration):
#
# $ echo -n "$owner.$name" | openssl sha1 -hmac "$secret"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no cryptographer, but a (SHA1!!!!) HMAC seems like a) the wrong type of construct for this application and b) woefully insecure for any purpose. I believe this calls for a Key Derivation Function (KDF) instead, specifically a non-stretching KDF such as HKDF which supports adding a non-secret "salt" (here, the $owner.$name construct).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't claim deep crypto experience either, but I'm not a novice either 😄

What we're doing here is symmetric with how Github uses HMAC. The internet has text about HMAC - this stackoverflow seems decent.

I don't think we need a key here - we're not actually using this as a key for a cipher (whether symmetric or not). We simply need a shared secret between 3 parties - Homu, Github, and the repo owner. We don't want different repo owners to have the same secret.

Basically, why do you think we need a KDF here, but Github's use of HMAC to sign their requests is OK? Or do you disagree with that as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as using SHA1...sure, though the github shared secret is a maximum of 20...characters? UTF-8? I don't know. We could likely fit enough bits to do sha256 if we base64 encoded.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I overlooked that there's two HMAC uses here. The second (original) usage to compute computed_hmac as a signature over the data to compare to hmac_sig looks fine to me. The first usage is the one that's suspicious. We want to (deterministically) derive a repo_secret from a base_secret and additionally contextual info ($owner.$name), and this output needs to be a key, since we're passing it to the (second instance of) HMAC as such. This usage matches the intent of KDFs.

I've done some reading (the HKDF IETF spec, the HKDF paper, etc.) and I still think a KDF such as HDFK is preferable here. HKDF in particular uses HMAC as a primitive, but in a two phase scheme with some additional tricks (feedback to minimize correlation, a counter to prevent against cycle shortcuts, etc.). For example, HKDF can have variable sized output, not just whatever the output size of the underlying HMAC is.

Since we're deriving a key from another key, we should use a key derivation function. HKDF is fast and efficient anyways, so we don't lose anything over just HMAC. If we're really worried about performance impacts, we can precompute and/or cache the repo_secrets.

I also see after reading the paper that the $owner.$name is better suited for the contextual information of HKDF, not the salt.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I thought about this and I see the point. Still though, unlike a main motivation for HKDF, we're not trying to stretch weak key material. There's no reason not to make the base secret as strong as you want - the suggested 160 bits is by any reasonable measure quite strong.

The main thing that's having me hesitate still is it needs to be easy for users to script. It'd be easier if we shipped with Homu a script that idempotently sets the webhook configuration for a set of repos, given a config file. Then we wouldn't need to expose the user too much to our secret calculation scheme.

#
# Note this is new; Homu started with a per-repository `repo.github.secret`
# value, which if specified, overrides this. Hence, you can incrementally
# transition repositories.
#
# You can generate this value with $(openssl rand -hex 20) as per the final
# secret, but it could also be anything you like.
#
# base_secret = "<replace with secret>"


[git]

# Use the local Git command. Required to use some advanced features. It also
Expand Down
25 changes: 19 additions & 6 deletions homu/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,15 +233,28 @@ def github():

owner_info = info['repository']['owner']
owner = owner_info.get('login') or owner_info['name']
repo_label = g.repo_labels[owner, info['repository']['name']]
repo_name = info['repository']['name']
repo_label = g.repo_labels[owner, repo_name]
repo_cfg = g.repo_cfgs[repo_label]

# This HMAC authenticates that github knows our secret
hmac_method, hmac_sig = request.headers['X-Hub-Signature'].split('=')
if hmac_sig != hmac.new(
repo_cfg['github']['secret'].encode('utf-8'),
payload,
hmac_method,
).hexdigest():
# Originally, we only supported a per-repo secret
repo_secret = repo_cfg.get('github', {}).get('secret')
if repo_secret is None:
# But here we support automatically deriving a per-repo secret from a
# global secret, without having repo owners be able to compute the
# secret for a different repo. See the comment in cfg.sample.toml for
# more information.
base_secret = g.cfg['github'].get('base_secret')
if base_secret is None:
abort(500, "Repository {} has no secret specified, and no base_secret".format(repo_label))
repo_secret = hmac.new(base_secret.encode('utf-8'),
"{}.{}".format(owner, repo_name).encode('utf-8'),
'sha1').hexdigest()
computed_hmac = hmac.new(repo_secret.encode('utf-8'), payload, hmac_method).hexdigest()

if hmac_sig != computed_hmac:
abort(400, 'Invalid signature')

event_type = request.headers['X-Github-Event']
Expand Down