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

Jump to bottom

Add checks to determine if a user can trigger jobs #13197

Merged
merged 2 commits into from
Jul 3, 2019
Merged

Add checks to determine if a user can trigger jobs #13197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44c45cd
Add checks to determine if a user can trigger jobs
mirandachrist Jun 26, 2019
64c4e48
Modify handleRerun to support both rerunning from the command line an…
mirandachrist Jun 27, 2019
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
3 changes: 2 additions & 1 deletion prow/cluster/deck_rbac.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ rules:
- list
# Required when deck runs with `--rerun-creates-job=true`
# **Warning:** Only use this for non-public deck instances, this allows
# anyone with access to your Deck instance to create new Prowjobs
# anyone with access to your Deck instance to create new Prowjobs.
# Additionally, there is not yet protection against CSRF. Use with caution
# - create
---
kind: Role
Expand Down
5 changes: 5 additions & 0 deletions prow/cmd/deck/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,17 @@ go_test(
"//prow/client/clientset/versioned/fake:go_default_library",
"//prow/config:go_default_library",
"//prow/flagutil:go_default_library",
"//prow/githuboauth:go_default_library",
"//prow/pluginhelp:go_default_library",
"//prow/spyglass/lenses/buildlog:go_default_library",
"//prow/spyglass/lenses/junit:go_default_library",
"//prow/spyglass/lenses/metadata:go_default_library",
"//prow/tide:go_default_library",
"//prow/tide/history:go_default_library",
"//vendor/github.com/google/go-github/github:go_default_library",
"//vendor/github.com/gorilla/sessions:go_default_library",
"//vendor/github.com/sirupsen/logrus:go_default_library",
"//vendor/golang.org/x/oauth2:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/equality:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/labels:go_default_library",
Expand Down Expand Up @@ -105,6 +109,7 @@ go_library(
"//prow/errorutil:go_default_library",
"//prow/flagutil:go_default_library",
"//prow/gcsupload:go_default_library",
"//prow/github:go_default_library",
"//prow/githuboauth:go_default_library",
"//prow/kube:go_default_library",
"//prow/logrusutil:go_default_library",
Expand Down
76 changes: 59 additions & 17 deletions prow/cmd/deck/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import (
"k8s.io/test-infra/prow/config"
"k8s.io/test-infra/prow/deck/jobs"
prowflagutil "k8s.io/test-infra/prow/flagutil"
prowgithub "k8s.io/test-infra/prow/github"
"k8s.io/test-infra/prow/githuboauth"
"k8s.io/test-infra/prow/kube"
"k8s.io/test-infra/prow/logrusutil"
Expand Down Expand Up @@ -172,6 +173,8 @@ var (
}
)

type authCfgGetter func() *config.RerunAuthConfig

type traceResponseWriter struct {
http.ResponseWriter
statusCode int
Expand Down Expand Up @@ -389,12 +392,14 @@ func prodOnlyMain(cfg config.Getter, o options, mux *http.ServeMux) *http.ServeM
}, podLogClients, cfg)
ja.Start()

cfgGetter := func() *config.RerunAuthConfig { return &cfg().Deck.RerunAuthConfig }

// setup prod only handlers
mux.Handle("/data.js", gziphandler.GzipHandler(handleData(ja)))
mux.Handle("/prowjobs.js", gziphandler.GzipHandler(handleProwJobs(ja)))
mux.Handle("/badge.svg", gziphandler.GzipHandler(handleBadge(ja)))
mux.Handle("/log", gziphandler.GzipHandler(handleLog(ja)))
mux.Handle("/rerun", gziphandler.GzipHandler(handleRerun(prowJobClient, o.rerunCreatesJob)))

mux.Handle("/prowjob", gziphandler.GzipHandler(handleProwJob(prowJobClient)))

if o.spyglass {
Expand Down Expand Up @@ -489,6 +494,7 @@ func prodOnlyMain(cfg config.Getter, o options, mux *http.ServeMux) *http.ServeM
mux.Handle("/github-login", goa.HandleLogin(oauthClient))
// Handles redirect from GitHub OAuth server.
mux.Handle("/github-login/redirect", goa.HandleRedirect(oauthClient, githuboauth.NewGitHubClientGetter()))
mux.Handle("/rerun", gziphandler.GzipHandler(handleRerun(prowJobClient, o.rerunCreatesJob, cfgGetter, goa, githuboauth.NewGitHubClientGetter())))
}

// optionally inject http->https redirect handler when behind loadbalancer
Expand Down Expand Up @@ -1150,9 +1156,39 @@ func handleProwJob(prowJobClient prowv1.ProwJobInterface) http.HandlerFunc {
}
}

func handleRerun(prowJobClient prowv1.ProwJobInterface, createProwJob bool) http.HandlerFunc {
// canTriggerJob determines whether the given user can trigger any job.
func canTriggerJob(user string, cfg *config.RerunAuthConfig) bool {
if cfg.AllowAnyone {
return true
}
for _, u := range cfg.AuthorizedUsers {
if prowgithub.NormLogin(u) == prowgithub.NormLogin(user) {
return true
}
}
return false
}

func marshalJob(w http.ResponseWriter, pj prowapi.ProwJob, l *logrus.Entry) {
b, err := yaml.Marshal(&pj)
if err != nil {
http.Error(w, fmt.Sprintf("Error marshaling: %v", err), http.StatusInternalServerError)
l.WithError(err).Error("Error marshaling job.")
return
}
w.Header().Set("Content-Type", "application/x-yaml")
if _, err := w.Write(b); err != nil {
l.WithError(err).Error("Error writing log.")
}
}

// handleRerun triggers a rerun of the given job if that features is enabled, it receives a
// POST request, and the user has the necessary permissions. Otherwise, it writes the config
// for a new job but does not trigger it.
func handleRerun(prowJobClient prowv1.ProwJobInterface, createProwJob bool, cfg authCfgGetter, goa *githuboauth.Agent, ghc githuboauth.GitHubClientGetter) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
name := r.URL.Query().Get("prowjob")
mirandachrist marked this conversation as resolved.
Show resolved Hide resolved
l := logrus.WithField("prowjob", name)
if name == "" {
http.Error(w, "request did not provide the 'name' query parameter", http.StatusBadRequest)
return
Expand All @@ -1162,7 +1198,7 @@ func handleRerun(prowJobClient prowv1.ProwJobInterface, createProwJob bool) http
http.Error(w, fmt.Sprintf("ProwJob not found: %v", err), http.StatusNotFound)
if !kerrors.IsNotFound(err) {
// admins only care about errors other than not found
logrus.WithError(err).Warning("ProwJob not found.")
l.WithError(err).Warning("ProwJob not found.")
}
return
}
Expand All @@ -1172,29 +1208,35 @@ func handleRerun(prowJobClient prowv1.ProwJobInterface, createProwJob bool) http
// On Prow instances that require auth even for viewing Deck this is okayish, because the Prowjob UUID
// is hard to guess
// Ref: https://github.com/kubernetes/test-infra/pull/12827#issuecomment-502850414
if createProwJob {
if r.Method != http.MethodPost {
Copy link
Contributor

Choose a reason for hiding this comment

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

So previously this would allow only a POST if we were creating a new prowjob. Now, we do nothing if it isn't POST but we don't error. Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it isn't POST, we do the same thing /rerun does now (display the config YAML). This preserves the functionality for kubectl create -f "https://prow.k8s.io/rerun?prowjob=blahblah" and isn't necessarily an error. If someone tries to directly trigger a job but doesn't use a POST request, they'll just see the YAML which I think seems reasonable.

http.Error(w, "request must be of type POST", http.StatusMethodNotAllowed)
switch r.Method {
case http.MethodGet:
marshalJob(w, newPJ, l)
case http.MethodPost:
if !createProwJob {
http.Error(w, "Direct rerun feature is not yet enabled", http.StatusMethodNotAllowed)
return
}
login, err := goa.GetLogin(r, ghc)
if err != nil {
http.Error(w, fmt.Sprintf("Error retrieving GitHub login: %v", err), http.StatusInternalServerError)
l.WithError(err).Errorf("Error retrieving GitHub login")
return
}
if !canTriggerJob(login, cfg()) {
http.Redirect(w, r, "/?rerun=error", http.StatusFound)
return
}
if _, err := prowJobClient.Create(&newPJ); err != nil {
logrus.WithError(err).Error("Error creating job")
l.WithError(err).Error("Error creating job")
http.Error(w, fmt.Sprintf("Error creating job: %v", err), http.StatusInternalServerError)
return
}
w.WriteHeader(http.StatusNoContent)
http.Redirect(w, r, "/?rerun=success", http.StatusFound)
return
}
b, err := yaml.Marshal(&newPJ)
if err != nil {
http.Error(w, fmt.Sprintf("Error marshaling: %v", err), http.StatusInternalServerError)
logrus.WithError(err).Error("Error marshaling jobs.")
default:
http.Error(w, fmt.Sprintf("bad verb %v", r.Method), http.StatusMethodNotAllowed)
return
}
w.Header().Set("Content-Type", "application/json")
if _, err := w.Write(b); err != nil {
logrus.WithError(err).Error("Error writing log.")
}
}
}

Expand Down
117 changes: 103 additions & 14 deletions prow/cmd/deck/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,12 @@ import (
"errors"
"flag"
"fmt"
"github.com/gorilla/sessions"
"github.com/sirupsen/logrus"
"golang.org/x/oauth2"
"io"
"io/ioutil"
"k8s.io/test-infra/prow/githuboauth"
"net/http"
"net/http/httptest"
"net/url"
Expand All @@ -33,6 +37,8 @@ import (
"testing"
"time"

"github.com/google/go-github/github"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -253,20 +259,80 @@ func TestProwJob(t *testing.T) {
}
}

type mockGitHubConfigGetter struct {
githubLogin string
}

func (getter mockGitHubConfigGetter) GetGitHubClient(accessToken string, dryRun bool) githuboauth.GitHubClientWrapper {
return getter
}

func (getter mockGitHubConfigGetter) GetUser(login string) (*github.User, error) {
return &github.User{Login: &getter.githubLogin}, nil
}

// TestRerun just checks that the result can be unmarshaled properly, has an
// updated status, and has equal spec.
func TestRerun(t *testing.T) {
testCases := []struct {
name string
shouldCreateProwjob bool
login string
authorized []string
allowAnyone bool
rerunCreatesJob bool
shouldCreateProwJob bool
httpCode int
httpMethod string
}{
{
name: "Handler returns ProwJob",
shouldCreateProwjob: false,
login: "authorized",
authorized: []string{"authorized", "alsoauthorized"},
allowAnyone: false,
rerunCreatesJob: true,
shouldCreateProwJob: true,
httpCode: http.StatusFound,
httpMethod: http.MethodPost,
},
{
name: "User not authorized to create prow job",
login: "random-dude",
authorized: []string{"authorized", "alsoauthorized"},
allowAnyone: false,
rerunCreatesJob: true,
shouldCreateProwJob: false,
httpCode: http.StatusFound,
httpMethod: http.MethodPost,
},
{
name: "RerunCreatesJob set to false, should not create prow job",
login: "authorized",
authorized: []string{"authorized", "alsoauthorized"},
allowAnyone: true,
rerunCreatesJob: false,
shouldCreateProwJob: false,
httpCode: http.StatusOK,
httpMethod: http.MethodGet,
},
{
name: "Allow anyone set to true, creates job",
login: "ugh",
authorized: []string{"authorized", "alsoauthorized"},
allowAnyone: true,
rerunCreatesJob: true,
shouldCreateProwJob: true,
httpCode: http.StatusFound,
httpMethod: http.MethodPost,
},
{
name: "Handler creates ProwJob",
shouldCreateProwjob: true,
name: "Direct rerun disabled, post request",
login: "authorized",
authorized: []string{"authorized", "alsoauthorized"},
allowAnyone: true,
rerunCreatesJob: false,
shouldCreateProwJob: false,
httpCode: http.StatusMethodNotAllowed,
httpMethod: http.MethodPost,
},
}

Expand All @@ -292,18 +358,44 @@ func TestRerun(t *testing.T) {
State: prowapi.PendingState,
},
})
handler := handleRerun(fakeProwJobClient.ProwV1().ProwJobs("prowjobs"), tc.shouldCreateProwjob)
req, err := http.NewRequest(http.MethodPost, "/rerun?prowjob=wowsuch", nil)
configGetter := func() *config.RerunAuthConfig {
return &config.RerunAuthConfig{
AllowAnyone: tc.allowAnyone,
AuthorizedUsers: tc.authorized,
}
}

req, err := http.NewRequest(tc.httpMethod, "/rerun?prowjob=wowsuch", nil)
req.AddCookie(&http.Cookie{
Name: "github_login",
Value: tc.login,
Path: "/",
Expires: time.Now().Add(time.Hour * 24 * 30),
Secure: true,
})
mockCookieStore := sessions.NewCookieStore([]byte("secret-key"))
session, err := sessions.GetRegistry(req).Get(mockCookieStore, "access-token-session")
if err != nil {
t.Fatalf("Error making access token session: %v", err)
}
session.Values["access-token"] = &oauth2.Token{}

if err != nil {
t.Fatalf("Error making request: %v", err)
}
rr := httptest.NewRecorder()
mockConfig := &config.GitHubOAuthConfig{
CookieStore: mockCookieStore,
}
goa := githuboauth.NewAgent(mockConfig, &logrus.Entry{})
ghc := mockGitHubConfigGetter{githubLogin: tc.login}
handler := handleRerun(fakeProwJobClient.ProwV1().ProwJobs("prowjobs"), tc.rerunCreatesJob, configGetter, goa, ghc)
handler.ServeHTTP(rr, req)
if rr.Code != tc.httpCode {
t.Fatalf("Bad error code: %d", rr.Code)
}

if tc.shouldCreateProwjob {
if rr.Code != http.StatusNoContent {
t.Fatalf("Unexpected http status code: %d, expected 204", rr.Code)
}
if tc.shouldCreateProwJob {
pjs, err := fakeProwJobClient.ProwV1().ProwJobs("prowjobs").List(metav1.ListOptions{})
if err != nil {
t.Fatalf("failed to list prowjobs: %v", err)
Expand All @@ -312,10 +404,7 @@ func TestRerun(t *testing.T) {
t.Errorf("expected to get two prowjobs, got %d", numPJs)
}

} else {
if rr.Code != http.StatusOK {
t.Fatalf("Bad error code: %d", rr.Code)
}
} else if !tc.rerunCreatesJob && tc.httpCode == http.StatusOK {
resp := rr.Result()
defer resp.Body.Close()
body, err := ioutil.ReadAll(resp.Body)
Expand Down
14 changes: 10 additions & 4 deletions prow/cmd/deck/static/prow/prow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,9 +650,15 @@ function redraw(fz: FuzzySearch): void {
max = 2 * 3600;
}
drawJobHistogram(totalJob, jobHistogram, now - (12 * 3600), now, max);
if (rerunStatus != null) {
if (rerunStatus === "error") {
modal.style.display = "block";
rerunCommand.innerHTML = `Nice try! The direct rerun feature hasn't been implemented yet, so that button does nothing.`;
rerunCommand.innerHTML = "You don't have permission to rerun that job";
} else if (rerunStatus === "success") {
modal.style.display = "block";
rerunCommand.innerHTML = "Job successfully triggered. Wait 30 seconds and refresh the page for the job to show up";
} else if (rerunStatus != null) {
modal.style.display = "block";
rerunCommand.innerHTML = "Nice try! The direct rerun feature hasn't been implemented yet, so that button does nothing.";
}

}
Expand All @@ -661,7 +667,7 @@ function createRerunCell(modal: HTMLElement, rerunElement: HTMLElement, prowjob:
const url = `${location.protocol}//${location.host}/rerun?prowjob=${prowjob}`;
const c = document.createElement("td");
const i = icon.create("refresh", "Show instructions for rerunning this job");
const login = getCookieByName("github_login");
const login = getCookieByName("access-token-session");
i.onclick = () => {
modal.style.display = "block";
rerunElement.innerHTML = `kubectl create -f "<a href="${url}">${url}</a>"`;
Expand All @@ -671,7 +677,7 @@ function createRerunCell(modal: HTMLElement, rerunElement: HTMLElement, prowjob:
copyButton.innerHTML = "<i class='material-icons state triggered' style='color: gray'>file_copy</i>";
rerunElement.appendChild(copyButton);
const runButton = document.createElement('a');
runButton.innerHTML = "<button class='mdl-button mdl-js-button'>Run</button>";
runButton.innerHTML = "<button class='mdl-button mdl-js-button'>Rerun</button>";
if (login === "") {
runButton.href = `/github-login?dest=%2F?rerun=work_in_progress`;
} else {
Expand Down
Loading