Skip to content

Commit 6baa281

Browse files
doggydogworlddoggydogworld
committed
Updating based on PR comments
1 parent 4ce637f commit 6baa281

File tree

4 files changed

+50
-17
lines changed

4 files changed

+50
-17
lines changed

‎libs/github/webhook/webhook.go

+19-8
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package webhook
22

33
import (
4-
"fmt"
54
"log/slog"
65
"net/http"
76

@@ -45,9 +44,9 @@ type Opt func(*Handler) error
4544
// If not set, the webhook will not verify the signature of the request.
4645
//
4746
// For more information, see: https://docs.github.com/en/webhooks/using-webhooks/validating-webhook-deliveries
48-
func WithSecretToken(secretToken []byte) Opt {
47+
func WithSecretToken(secretToken string) Opt {
4948
return func(p *Handler) error {
50-
p.secretToken = secretToken
49+
p.secretToken = []byte(secretToken)
5150
return nil
5251
}
5352
}
@@ -101,6 +100,7 @@ type Headers struct {
101100

102101
// ServeHTTP handles a webhook request.
103102
func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
103+
defer r.Body.Close()
104104
// Parse headers for debugging and audit purposes.
105105
var head Headers
106106
head.GithubHookID = r.Header.Get("X-GitHub-Hook-ID")
@@ -110,9 +110,15 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
110110
head.GitHubHookInstallationTargetID = r.Header.Get("X-GitHub-Hook-Installation-Target-ID")
111111
head.HubSignature256 = r.Header.Get("X-Hub-Signature-256")
112112

113+
if h.secretToken == nil && head.HubSignature256 != "" {
114+
h.log.Warn("received signature but no secret token is set", "github_headers", head)
115+
http.Error(w, "invalid request", http.StatusInternalServerError)
116+
return
117+
}
118+
113119
payload, err := github.ValidatePayload(r, h.secretToken) // If secretToken is empty, the signature will not be verified.
114120
if err != nil {
115-
h.log.Warn("webhook validation failed", "headers", head)
121+
h.log.Warn("webhook validation failed", "github_headers", head)
116122
http.Error(w, "invalid request", http.StatusBadRequest)
117123
return
118124
}
@@ -134,8 +140,13 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
134140
w.WriteHeader(http.StatusOK)
135141
}
136142

137-
// String returns a string representation of the Headers.
138-
func (h *Headers) String() string {
139-
return fmt.Sprintf("GithubHookID: %s\nGithubEvent: %s\nGithubDelivery: %s\nGitHubHookInstallationTargetType: %s\nGitHubHookInstallationTargetID: %s\nHubSignature256: %s\n",
140-
h.GithubHookID, h.GithubEvent, h.GithubDelivery, h.GitHubHookInstallationTargetType, h.GitHubHookInstallationTargetID, h.HubSignature256)
143+
func (h *Headers) LogValue() slog.Value {
144+
return slog.GroupValue(
145+
slog.String("github_hook_id", h.GithubHookID),
146+
slog.String("github_event", h.GithubEvent),
147+
slog.String("github_delivery", h.GithubDelivery),
148+
slog.String("github_hook_installation_target_type", h.GitHubHookInstallationTargetType),
149+
slog.String("github_hook_installation_target_id", h.GitHubHookInstallationTargetID),
150+
slog.String("hub_signature_256", h.HubSignature256),
151+
)
141152
}

‎tools/approval-service/cmd/approval-service.go

+11-5
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,14 @@ package main
22

33
import (
44
"context"
5+
"fmt"
56
"log"
67
"log/slog"
78
"net/http"
9+
"time"
810

911
"github.com/google/go-github/v69/github"
1012
"github.com/gravitational/shared-workflows/libs/github/webhook"
11-
"github.com/gravitational/trace"
1213
"golang.org/x/sync/errgroup"
1314
)
1415

@@ -144,12 +145,12 @@ func (ghes *GitHubEventSource) Setup() error {
144145
deployReviewChan <- event
145146
return nil
146147
default:
147-
return trace.Errorf("unknown event type: %T", event)
148+
return fmt.Errorf("unknown event type: %T", event)
148149
}
149150
})
150151
mux.Handle("/webhook", webhook.NewHandler(
151152
eventProcessor,
152-
webhook.WithSecretToken([]byte("secret-token")), // TODO: get from config
153+
webhook.WithSecretToken("secret-token"), // TODO: get from config
153154
webhook.WithLogger(logger),
154155
))
155156

@@ -193,11 +194,16 @@ func (ghes *GitHubEventSource) Run(ctx context.Context) error {
193194
case err = <-errc:
194195
ghes.srv.Shutdown(context.Background()) // Ignore error - we're already handling one
195196
case <-ctx.Done():
196-
err = ghes.srv.Shutdown(context.Background())
197+
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
198+
defer cancel()
199+
err = ghes.srv.Shutdown(ctx)
197200
<-errc // flush the error channel to avoid a goroutine leak
198201
}
199202

200-
return trace.Wrap(err)
203+
if err != nil {
204+
return fmt.Errorf("error enconutered while running GitHub event source: %w", err)
205+
}
206+
return nil
201207
}
202208

203209
func (ghes *GitHubEventSource) processDeploymentReviewEvent(payload *github.DeploymentReviewEvent) error {

‎tools/approval-service/go.mod

+8-2
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,13 @@ module github.com/gravitational/shared-workflows/tools/approval-service
33
go 1.23.5
44

55
require (
6-
github.com/google/go-github/v63 v63.0.0 // indirect
7-
github.com/google/go-github/v69 v69.1.0 // indirect
6+
github.com/google/go-github/v69 v69.1.0
7+
github.com/gravitational/trace v1.5.0
8+
)
9+
10+
require (
811
github.com/google/go-querystring v1.1.0 // indirect
12+
github.com/stretchr/testify v1.9.0 // indirect
13+
golang.org/x/net v0.34.0 // indirect
14+
golang.org/x/sync v0.11.0 // indirect
915
)

‎tools/approval-service/go.sum

+12-2
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,18 @@
1+
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
12
github.com/google/go-cmp v0.5.2/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
2-
github.com/google/go-github/v63 v63.0.0 h1:13xwK/wk9alSokujB9lJkuzdmQuVn2QCPeck76wR3nE=
3-
github.com/google/go-github/v63 v63.0.0/go.mod h1:IqbcrgUmIcEaioWrGYei/09o+ge5vhffGOcxrO0AfmA=
3+
github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
44
github.com/google/go-github/v69 v69.1.0 h1:ljzwzEsHsc4qUqyHEJCNA1dMqvoTK3YX2NAaK6iprDg=
55
github.com/google/go-github/v69 v69.1.0/go.mod h1:xne4jymxLR6Uj9b7J7PyTpkMYstEMMwGZa0Aehh1azM=
66
github.com/google/go-querystring v1.1.0 h1:AnCroh3fv4ZBgVIf1Iwtovgjaw/GiKJo8M8yD/fhyJ8=
77
github.com/google/go-querystring v1.1.0/go.mod h1:Kcdr2DB4koayq7X8pmAG4sNG59So17icRSOU623lUBU=
8+
github.com/gravitational/trace v1.5.0 h1:JbeL2HDGyzgy7G72Z2hP2gExEyA6Y2p7fCiSjyZwCJw=
9+
github.com/gravitational/trace v1.5.0/go.mod h1:dxezSkKm880IIDx+czWG8fq+pLnXjETBewMgN3jOBlg=
10+
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
11+
github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg=
12+
github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
13+
golang.org/x/net v0.34.0 h1:Mb7Mrk043xzHgnRM88suvJFwzVrRfHEHJEl5/71CKw0=
14+
golang.org/x/net v0.34.0/go.mod h1:di0qlW3YNM5oh6GqDGQr92MyTozJPmybPK4Ev/Gm31k=
15+
golang.org/x/sync v0.11.0 h1:GGz8+XQP4FvTTrjZPzNKTMFtSXH80RAzG+5ghFPgK9w=
16+
golang.org/x/sync v0.11.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
817
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
18+
gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=

0 commit comments

Comments
 (0)