Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix panic in github interceptor #357

Merged
merged 1 commit into from
Jan 22, 2020
+252 −75
Merged

fix panic in github interceptor #357

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
19 changes: 10 additions & 9 deletions pkg/interceptors/cel/cel.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package cel

import (
"bytes"
"encoding/json"
"fmt"
"io/ioutil"
Expand Down Expand Up @@ -58,15 +59,15 @@ func (w *Interceptor) ExecuteTrigger(request *http.Request) (*http.Response, err
return nil, fmt.Errorf("error creating cel environment: %w", err)
}

body, err := request.GetBody()
if err != nil {
return nil, fmt.Errorf("error getting request body: %w", err)
}
defer body.Close()
payload, err := ioutil.ReadAll(body)
if err != nil {
return nil, fmt.Errorf("error reading request body: %w", err)
var payload = []byte(`{}`)
Copy link
Member

Choose a reason for hiding this comment

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

I don't feel too strongly about this, but should we be consistent with the webhook handler and default to []byte{}?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, we'd have to do it the other way around though since makeCelEnv errors out if you pass it []byte{}

if request.Body != nil {
defer request.Body.Close()
payload, err = ioutil.ReadAll(request.Body)
if err != nil {
return nil, fmt.Errorf("error reading request body: %w", err)
}
}

evalContext, err := makeEvalContext(payload, request)
if err != nil {
return nil, fmt.Errorf("error making the evaluation context: %w", err)
Expand All @@ -84,7 +85,7 @@ func (w *Interceptor) ExecuteTrigger(request *http.Request) (*http.Response, err

return &http.Response{
Header: request.Header,
Body: request.Body,
Body: ioutil.NopCloser(bytes.NewBuffer(payload)),
}, nil
}

Expand Down
52 changes: 17 additions & 35 deletions pkg/interceptors/cel/cel_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,10 @@ import (
)

func TestInterceptor_ExecuteTrigger(t *testing.T) {
type args struct {
payload []byte
}
tests := []struct {
name string
CEL *triggersv1.CELInterceptor
args args
payload io.ReadCloser
want []byte
wantErr bool
}{
Expand All @@ -30,9 +27,7 @@ func TestInterceptor_ExecuteTrigger(t *testing.T) {
CEL: &triggersv1.CELInterceptor{
Filter: "body.value == 'testing'",
},
args: args{
payload: []byte(`{"value":"testing"}`),
},
payload: ioutil.NopCloser(bytes.NewBufferString(`{"value":"testing"}`)),
want: []byte(`{"value":"testing"}`),
wantErr: false,
},
Expand All @@ -41,19 +36,15 @@ func TestInterceptor_ExecuteTrigger(t *testing.T) {
CEL: &triggersv1.CELInterceptor{
Filter: "body.value == 'test'",
},
args: args{
payload: []byte(`{"value":"testing"}`),
},
payload: ioutil.NopCloser(bytes.NewBufferString(`{"value":"testing"}`)),
wantErr: true,
},
{
name: "simple header check with matching header",
CEL: &triggersv1.CELInterceptor{
Filter: "header['X-Test'][0] == 'test-value'",
},
args: args{
payload: []byte(`{}`),
},
payload: ioutil.NopCloser(bytes.NewBufferString(`{}`)),
want: []byte(`{}`),
wantErr: false,
},
Expand All @@ -62,29 +53,23 @@ func TestInterceptor_ExecuteTrigger(t *testing.T) {
CEL: &triggersv1.CELInterceptor{
Filter: "header['X-Test'][0] == 'unknown'",
},
args: args{
payload: []byte(`{}`),
},
payload: ioutil.NopCloser(bytes.NewBufferString(`{}`)),
wantErr: true,
},
{
name: "overloaded header check with case insensitive failed match",
CEL: &triggersv1.CELInterceptor{
Filter: "header.match('x-test', 'no-match')",
},
args: args{
payload: []byte(`{}`),
},
payload: ioutil.NopCloser(bytes.NewBufferString(`{}`)),
wantErr: true,
},
{
name: "overloaded header check with case insensitive matching",
CEL: &triggersv1.CELInterceptor{
Filter: "header.match('x-test', 'test-value')",
},
args: args{
payload: []byte(`{}`),
},
payload: ioutil.NopCloser(bytes.NewBufferString(`{}`)),
want: []byte(`{}`),
wantErr: false,
},
Expand All @@ -93,9 +78,7 @@ func TestInterceptor_ExecuteTrigger(t *testing.T) {
CEL: &triggersv1.CELInterceptor{
Filter: "header.match('x-test', 'test-value') && body.value == 'test'",
},
args: args{
payload: []byte(`{"value":"test"}`),
},
payload: ioutil.NopCloser(bytes.NewBufferString(`{"value":"test"}`)),
want: []byte(`{"value":"test"}`),
wantErr: false,
},
Expand All @@ -104,20 +87,22 @@ func TestInterceptor_ExecuteTrigger(t *testing.T) {
CEL: &triggersv1.CELInterceptor{
Filter: "header['X-Test",
},
args: args{
payload: []byte(`{"value":"test"}`),
},
payload: ioutil.NopCloser(bytes.NewBufferString(`{"value":"test"}`)),
wantErr: true,
},
{
name: "unable to parse the JSON body",
CEL: &triggersv1.CELInterceptor{
Filter: "body.value == 'test'",
},
args: args{
payload: []byte(`{]`),
},
payload: ioutil.NopCloser(bytes.NewBufferString(`{}`)),
wantErr: true,
}, {
name: "nil body does not panic",
CEL: &triggersv1.CELInterceptor{Filter: "header.match('x-test', 'test-value')"},
payload: nil,
want: []byte(`{}`),
wantErr: false,
},
}
for _, tt := range tests {
Expand All @@ -128,10 +113,7 @@ func TestInterceptor_ExecuteTrigger(t *testing.T) {
Logger: logger,
}
request := &http.Request{
Body: ioutil.NopCloser(bytes.NewReader(tt.args.payload)),
GetBody: func() (io.ReadCloser, error) {
return ioutil.NopCloser(bytes.NewReader(tt.args.payload)), nil
},
Body: tt.payload,
Header: http.Header{
"Content-Type": []string{"application/json"},
"X-Test": []string{"test-value"},
Expand Down
24 changes: 13 additions & 11 deletions pkg/interceptors/github/github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package github

import (
"bytes"
"errors"
"fmt"
"io/ioutil"
Expand Down Expand Up @@ -46,22 +47,23 @@ func NewInterceptor(gh *triggersv1.GitHubInterceptor, k kubernetes.Interface, ns
}

func (w *Interceptor) ExecuteTrigger(request *http.Request) (*http.Response, error) {
payload := []byte{}
var err error

if request.Body != nil {
defer request.Body.Close()
payload, err = ioutil.ReadAll(request.Body)
if err != nil {
return nil, fmt.Errorf("failed to read request body: %w", err)
}
}

// Validate secrets first before anything else, if set
if w.GitHub.SecretRef != nil {
header := request.Header.Get("X-Hub-Signature")
if header == "" {
return nil, errors.New("no X-Hub-Signature header set")
}

body, err := request.GetBody()
if err != nil {
return nil, err
}
defer body.Close()
payload, err := ioutil.ReadAll(body)
if err != nil {
return nil, err
}
secretToken, err := interceptors.GetSecretToken(w.KubeClientSet, w.GitHub.SecretRef, w.EventListenerNamespace)
if err != nil {
return nil, err
Expand All @@ -88,6 +90,6 @@ func (w *Interceptor) ExecuteTrigger(request *http.Request) (*http.Response, err

return &http.Response{
Header: request.Header,
Body: request.Body,
Body: ioutil.NopCloser(bytes.NewBuffer(payload)),
}, nil
}
32 changes: 19 additions & 13 deletions pkg/interceptors/github/github_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (

func TestInterceptor_ExecuteTrigger_Signature(t *testing.T) {
type args struct {
payload []byte
payload io.ReadCloser
secret *corev1.Secret
signature string
eventType string
Expand All @@ -34,7 +34,7 @@ func TestInterceptor_ExecuteTrigger_Signature(t *testing.T) {
name: "no secret",
GitHub: &triggersv1.GitHubInterceptor{},
args: args{
payload: []byte("somepayload"),
payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")),
signature: "foo",
},
want: []byte("somepayload"),
Expand All @@ -58,7 +58,7 @@ func TestInterceptor_ExecuteTrigger_Signature(t *testing.T) {
"token": []byte("secrettoken"),
},
},
payload: []byte("somepayload"),
payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")),
},
wantErr: true,
},
Expand All @@ -82,7 +82,7 @@ func TestInterceptor_ExecuteTrigger_Signature(t *testing.T) {
"token": []byte("secret"),
},
},
payload: []byte("somepayload"),
payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")),
},
wantErr: false,
want: []byte("somepayload"),
Expand All @@ -93,7 +93,7 @@ func TestInterceptor_ExecuteTrigger_Signature(t *testing.T) {
EventTypes: []string{"MY_EVENT", "YOUR_EVENT"},
},
args: args{
payload: []byte("somepayload"),
payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")),
eventType: "YOUR_EVENT",
},
wantErr: false,
Expand All @@ -105,7 +105,7 @@ func TestInterceptor_ExecuteTrigger_Signature(t *testing.T) {
EventTypes: []string{"MY_EVENT", "YOUR_EVENT"},
},
args: args{
payload: []byte("somepayload"),
payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")),
eventType: "OTHER_EVENT",
},
wantErr: true,
Expand All @@ -132,7 +132,7 @@ func TestInterceptor_ExecuteTrigger_Signature(t *testing.T) {
},
},
eventType: "MY_EVENT",
payload: []byte("somepayload"),
payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")),
},
wantErr: false,
want: []byte("somepayload"),
Expand All @@ -159,7 +159,7 @@ func TestInterceptor_ExecuteTrigger_Signature(t *testing.T) {
},
},
eventType: "OTHER_EVENT",
payload: []byte("somepayload"),
payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")),
},
wantErr: true,
},
Expand All @@ -183,9 +183,18 @@ func TestInterceptor_ExecuteTrigger_Signature(t *testing.T) {
},
},
eventType: "MY_EVENT",
payload: []byte("somepayload"),
payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")),
},
wantErr: true,
}, {
name: "nil body does not panic",
GitHub: &triggersv1.GitHubInterceptor{},
args: args{
payload: nil,
signature: "foo",
},
want: []byte{},
wantErr: false,
},
}
for _, tt := range tests {
Expand All @@ -194,10 +203,7 @@ func TestInterceptor_ExecuteTrigger_Signature(t *testing.T) {
logger, _ := logging.NewLogger("", "")
kubeClient := fakekubeclient.Get(ctx)
request := &http.Request{
Body: ioutil.NopCloser(bytes.NewReader(tt.args.payload)),
GetBody: func() (io.ReadCloser, error) {
return ioutil.NopCloser(bytes.NewReader(tt.args.payload)), nil
},
Body: tt.args.payload,
Header: http.Header{
"Content-Type": []string{"application/json"},
},
Expand Down
2 changes: 1 addition & 1 deletion pkg/resources/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func Create(logger *zap.SugaredLogger, rt json.RawMessage, triggerName, eventID,
if name == "" {
name = data.GetGenerateName()
}
logger.Infof("Generating resource: kind: %+v, name: %s", apiResource, name)
logger.Infof("Generating resource: kind: %s, name: %s", apiResource, name)

gvr := schema.GroupVersionResource{
Group: apiResource.Group,
Expand Down
11 changes: 7 additions & 4 deletions pkg/sink/sink.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package sink

import (
"bytes"
"encoding/json"
"errors"
"fmt"
Expand Down Expand Up @@ -72,7 +73,6 @@ func (r Sink) HandleEvent(response http.ResponseWriter, request *http.Request) {
response.WriteHeader(http.StatusInternalServerError)
return
}

event, err := ioutil.ReadAll(request.Body)
if err != nil {
r.Logger.Errorf("Error reading event body: %s", err)
Expand Down Expand Up @@ -158,7 +158,11 @@ func (r Sink) executeInterceptors(t *triggersv1.EventListenerTrigger, in *http.R
return event, in.Header, nil
}

request := in
// The request body to the first interceptor in the chain should be the received event body.
request := &http.Request{
Header: in.Header,
Body: ioutil.NopCloser(bytes.NewBuffer(event)),
}
var resp *http.Response
for _, i := range t.Interceptors {
var interceptor interceptors.Interceptor
Expand All @@ -174,7 +178,6 @@ func (r Sink) executeInterceptors(t *triggersv1.EventListenerTrigger, in *http.R
default:
return nil, nil, fmt.Errorf("unknown interceptor type: %v", i)
}

var err error
resp, err = interceptor.ExecuteTrigger(request)
if err != nil {
Expand All @@ -185,7 +188,7 @@ func (r Sink) executeInterceptors(t *triggersv1.EventListenerTrigger, in *http.R
// request chaining.
request = &http.Request{
Header: resp.Header,
Body: resp.Body,
Body: ioutil.NopCloser(resp.Body),
}
}
payload, err := ioutil.ReadAll(resp.Body)
Expand Down
Loading