server: Support a base_secret #78
Conversation
Conceptually now, the config file can be split into "secrets" and "configuration". I'd like to maintain the former in Kubernetes secrets, and the latter in ConfigMaps. Down the line also, this makes it easier to allow people to change their own repository configuration.
| # 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" |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Heads up before you invest more time into this: I'm not very involved with maintaining Homu so take this with a grain of salt, but I'm not yet convinced this is functionality that belongs inside Homu. |
|
☔ The latest upstream changes (presumably #114) made this pull request unmergeable. Please resolve the merge conflicts. |
Conceptually now, the config file can be split into "secrets" and
"configuration". I'd like to maintain the former in Kubernetes secrets, and the
latter in ConfigMaps.
Down the line also, this makes it easier to allow people to change their own
repository configuration.
This change is