diff --git a/api/webhook/taskcluster.go b/api/webhook/taskcluster.go index 19014de74a3..a9bd60d9497 100644 --- a/api/webhook/taskcluster.go +++ b/api/webhook/taskcluster.go @@ -6,9 +6,6 @@ package webhook import ( "context" - "crypto/hmac" - "crypto/sha1" - "encoding/hex" "encoding/json" "errors" "fmt" @@ -42,29 +39,14 @@ func tcWebhookHandler(w http.ResponseWriter, r *http.Request) { return } - ctx := shared.NewAppEngineContext(r) - log := shared.GetLogger(ctx) - - payload, err := ioutil.ReadAll(r.Body) - r.Body.Close() + payload, err := verifyAndGetPayload(r) if err != nil { - log.Errorf("%v", err) - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - - secret, err := getSecret(ctx) - if err != nil { - log.Errorf("%v", err) - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - - if !verifySignature(payload, r.Header.Get("X-Hub-Signature"), secret) { - http.Error(w, "HMAC verification failed", http.StatusUnauthorized) + http.Error(w, err.Error(), http.StatusUnauthorized) return } + ctx := appengine.NewContext(r) + log := shared.GetLogger(ctx) log.Debugf("GitHub Delivery: %s", r.Header.Get("X-GitHub-Delivery")) processed, err := handleStatusEvent(ctx, payload) @@ -267,26 +249,6 @@ func getAuth(ctx context.Context) (username string, password string, err error) return u.Username, u.Password, err } -func getSecret(ctx context.Context) (token string, err error) { - var t shared.Token - key := datastore.NewKey(ctx, "Token", "github-tc-webhook-secret", 0, nil) - err = datastore.Get(ctx, key, &t) - return t.Secret, err -} - -func verifySignature(message []byte, signature string, secret string) bool { - // https://developer.github.com/webhooks/securing/ - signature = strings.TrimPrefix(signature, "sha1=") - messageMAC, err := hex.DecodeString(signature) - if err != nil { - return false - } - mac := hmac.New(sha1.New, []byte(secret)) - mac.Write(message) - expectedMAC := mac.Sum(nil) - return hmac.Equal(messageMAC, expectedMAC) -} - func createAllRuns(log shared.Logger, client *http.Client, api, diff --git a/api/webhook/taskcluster_test.go b/api/webhook/taskcluster_test.go index 855fc1dc8eb..3fa32725b2f 100644 --- a/api/webhook/taskcluster_test.go +++ b/api/webhook/taskcluster_test.go @@ -112,23 +112,6 @@ func TestExtractResultURLs(t *testing.T) { }, urls) } -func TestVerifySignature(t *testing.T) { - message := []byte("test") - signature := "a053ee211b4693456ef071e336f74ab699250318" - secret := "95bfab9afa719185ee7c3658356b166b7f45349a" - assert.True(t, verifySignature( - message, signature, secret)) - assert.False(t, verifySignature( - []byte("foobar"), signature, secret)) - assert.False(t, verifySignature( - message, "875a5feef4cde4265d6d5d21c304d755903ccb60", secret)) - assert.False(t, verifySignature( - message, signature, "875a5feef4cde4265d6d5d21c304d755903ccb60")) - // Test an ill-formed (odd-length) signature. - assert.False(t, verifySignature( - message, "875a5feef4cde4265d6d5d21c304d755903ccb6", secret)) -} - func TestCreateAllRuns_success(t *testing.T) { var requested uint32 requested = 0 diff --git a/api/webhook/verify.go b/api/webhook/verify.go new file mode 100644 index 00000000000..63ab45dd195 --- /dev/null +++ b/api/webhook/verify.go @@ -0,0 +1,49 @@ +package webhook + +import ( + "crypto/hmac" + "crypto/sha1" + "encoding/hex" + "errors" + "io/ioutil" + "net/http" + "strings" + + "github.com/web-platform-tests/wpt.fyi/shared" +) + +func verifyAndGetPayload(r *http.Request) ([]byte, error) { + ctx := shared.NewAppEngineContext(r) + log := shared.GetLogger(ctx) + + payload, err := ioutil.ReadAll(r.Body) + r.Body.Close() + if err != nil { + log.Errorf("Failed to read request body: %s", err.Error()) + return nil, errors.New("Failed to read request body") + } + + secret, err := shared.GetSecret(ctx, "github-tc-webhook-secret") + if err != nil { + log.Errorf("Failed to get verification secret: %s", err.Error()) + return nil, errors.New("Internal error") + } + + if !verifySignature(payload, r.Header.Get("X-Hub-Signature"), secret) { + return nil, errors.New("HMAC verification failed") + } + return payload, nil +} + +func verifySignature(message []byte, signature string, secret string) bool { + // https://developer.github.com/webhooks/securing/ + signature = strings.TrimPrefix(signature, "sha1=") + messageMAC, err := hex.DecodeString(signature) + if err != nil { + return false + } + mac := hmac.New(sha1.New, []byte(secret)) + mac.Write(message) + expectedMAC := mac.Sum(nil) + return hmac.Equal(messageMAC, expectedMAC) +} diff --git a/api/webhook/verify_test.go b/api/webhook/verify_test.go new file mode 100644 index 00000000000..010c43bb59e --- /dev/null +++ b/api/webhook/verify_test.go @@ -0,0 +1,25 @@ +// +build small + +package webhook + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestVerifySignature(t *testing.T) { + message := []byte("test") + signature := "a053ee211b4693456ef071e336f74ab699250318" + secret := "95bfab9afa719185ee7c3658356b166b7f45349a" + assert.True(t, verifySignature(message, signature, secret)) + + assert.False(t, verifySignature([]byte("foobar"), signature, secret)) + assert.False(t, verifySignature( + message, "875a5feef4cde4265d6d5d21c304d755903ccb60", secret)) + assert.False(t, verifySignature( + message, signature, "875a5feef4cde4265d6d5d21c304d755903ccb60")) + // Test an ill-formed (odd-length) signature. + assert.False(t, verifySignature( + message, "875a5feef4cde4265d6d5d21c304d755903ccb6", secret)) +} diff --git a/shared/datastore.go b/shared/datastore.go index fb1b8f19b32..7ab702e34df 100644 --- a/shared/datastore.go +++ b/shared/datastore.go @@ -279,3 +279,14 @@ func IsFeatureEnabled(ctx context.Context, flagName string) bool { } return flag.Enabled } + +// GetSecret is a helper wrapper for loading a token's secret from the datastore +// by name. +func GetSecret(ctx context.Context, tokenName string) (string, error) { + key := datastore.NewKey(ctx, "Token", tokenName, 0, nil) + var token Token + if err := datastore.Get(ctx, key, &token); err != nil { + return "", err + } + return token.Secret, nil +} diff --git a/shared/datastore_medium_test.go b/shared/datastore_medium_test.go index 9fe148d634c..150cc526bb6 100644 --- a/shared/datastore_medium_test.go +++ b/shared/datastore_medium_test.go @@ -442,3 +442,22 @@ func TestIsFeatureEnabled(t *testing.T) { datastore.Put(ctx, key, &shared.Flag{Enabled: true}) assert.True(t, shared.IsFeatureEnabled(ctx, flagName)) } + +func TestGetSecret(t *testing.T) { + i, err := sharedtest.NewAEInstance(true) + assert.Nil(t, err) + defer i.Close() + r, err := i.NewRequest("GET", "/", nil) + assert.Nil(t, err) + tokenName := "foo" + ctx := shared.NewAppEngineContext(r) + key := datastore.NewKey(ctx, "Token", tokenName, 0, nil) + secret, err := shared.GetSecret(ctx, tokenName) + assert.NotNil(t, err) + assert.Equal(t, "", secret) + // Token. + datastore.Put(ctx, key, &shared.Token{Secret: "bar"}) + secret, err = shared.GetSecret(ctx, tokenName) + assert.Nil(t, err) + assert.Equal(t, "bar", secret) +}