Skip to content
This repository has been archived by the owner on May 1, 2020. It is now read-only.

Bug 1220738: Create endpoint for credentials update. r=pmoore #14

Merged
merged 1 commit into from Jan 26, 2016
Merged

Bug 1220738: Create endpoint for credentials update. r=pmoore #14

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions README.md
View file
Open in desktop
Expand Up @@ -101,6 +101,15 @@ The proxy runs fine natively, but if you wish, you can also create a docker imag
./build.sh user/taskcluster-proxy
```

## Endpoints

#### Credentials Update

The proxy has the endpoint `/credentials` which accepts `PUT` request for
credentials update. The body is a
[Credentials](http://docs.taskcluster.net/queue/api-docs/#claimTask)
object in json format.


## Running tests

Expand Down
72 changes: 42 additions & 30 deletions authorization_test.go
View file
Open in desktop
Expand Up @@ -142,9 +142,11 @@ func TestBewit(t *testing.T) {
test := func(t *testing.T, creds *tcclient.Credentials) *httptest.ResponseRecorder {

// Test setup
routes := Routes(tcclient.ConnectionData{
Credentials: creds,
})
routes := Routes{
ConnectionData: tcclient.ConnectionData{
Credentials: creds,
},
}
req, err := http.NewRequest(
"POST",
"http://localhost:60024/bewit",
Expand Down Expand Up @@ -186,15 +188,17 @@ func TestAuthorizationDelegate(t *testing.T) {
test := func(name string, scopes []string) IntegrationTest {
return func(t *testing.T, creds *tcclient.Credentials) *httptest.ResponseRecorder {
// Test setup
routes := Routes(tcclient.ConnectionData{
Authenticate: true,
Credentials: &tcclient.Credentials{
ClientId: creds.ClientId,
AccessToken: creds.AccessToken,
Certificate: creds.Certificate,
AuthorizedScopes: scopes,
routes := Routes{
ConnectionData: tcclient.ConnectionData{
Authenticate: true,
Credentials: &tcclient.Credentials{
ClientId: creds.ClientId,
AccessToken: creds.AccessToken,
Certificate: creds.Certificate,
AuthorizedScopes: scopes,
},
},
})
}

// Requires scope "auth:azure-table-access:fakeaccount/DuMmYtAbLe"
req, err := http.NewRequest(
Expand Down Expand Up @@ -229,10 +233,12 @@ func TestAPICallWithPayload(t *testing.T) {
test := func(t *testing.T, creds *tcclient.Credentials) *httptest.ResponseRecorder {

// Test setup
routes := Routes(tcclient.ConnectionData{
Authenticate: true,
Credentials: creds,
})
routes := Routes{
ConnectionData: tcclient.ConnectionData{
Authenticate: true,
Credentials: creds,
},
}
taskId := slugid.Nice()
taskGroupId := slugid.Nice()
created := time.Now()
Expand Down Expand Up @@ -303,10 +309,12 @@ func TestNon200HasErrorBody(t *testing.T) {
test := func(t *testing.T, creds *tcclient.Credentials) *httptest.ResponseRecorder {

// Test setup
routes := Routes(tcclient.ConnectionData{
Authenticate: true,
Credentials: creds,
})
routes := Routes{
ConnectionData: tcclient.ConnectionData{
Authenticate: true,
Credentials: creds,
},
}
taskId := slugid.Nice()

req, err := http.NewRequest(
Expand Down Expand Up @@ -336,10 +344,12 @@ func TestOversteppedScopes(t *testing.T) {
test := func(t *testing.T, creds *tcclient.Credentials) *httptest.ResponseRecorder {

// Test setup
routes := Routes(tcclient.ConnectionData{
Authenticate: true,
Credentials: creds,
})
routes := Routes{
ConnectionData: tcclient.ConnectionData{
Authenticate: true,
Credentials: creds,
},
}

// This scope is not in the scopes of the temp credentials, which would
// happen if a task declares a scope that the provisioner does not
Expand Down Expand Up @@ -374,14 +384,16 @@ func TestOversteppedScopes(t *testing.T) {
}

func TestBadCredsReturns500(t *testing.T) {
routes := Routes(tcclient.ConnectionData{
Authenticate: true,
Credentials: &tcclient.Credentials{
ClientId: "abc",
AccessToken: "def",
Certificate: "ghi", // baaaad certificate
routes := Routes{
ConnectionData: tcclient.ConnectionData{
Authenticate: true,
Credentials: &tcclient.Credentials{
ClientId: "abc",
AccessToken: "def",
Certificate: "ghi", // baaaad certificate
},
},
})
}
req, err := http.NewRequest(
"GET",
"http://localhost:60024/secrets/v1/secret/garbage/pmoore/foo",
Expand Down
105 changes: 105 additions & 0 deletions credentials_update_test.go
View file
Open in desktop
@@ -0,0 +1,105 @@
package main

import (
"bytes"
"encoding/json"
"github.com/taskcluster/taskcluster-client-go/tcclient"
"io/ioutil"
"net/http"
"net/http/httptest"
"testing"
)

type RoutesTest struct {
Routes
t *testing.T
}

func TestCredentialsUpdate(t *testing.T) {
newCreds := CredentialsUpdate{
ClientId: "newClientId",
AccessToken: "newAccessToken",
Certificate: "newCertificate",
}

body, err := json.Marshal(&newCreds)

if err != nil {
t.Fatal(err)
}

routes := NewRoutesTest(t)

response := routes.request("POST", body)
if response.Code != 405 {
t.Errorf("Should return 405, but returned %d", response.Code)
}

response = routes.request("PUT", make([]byte, 0))
if response.Code != 400 {
t.Errorf("Should return 400, but returned %d", response.Code)
}

response = routes.request("PUT", body)
if response.Code != 200 {
content, _ := ioutil.ReadAll(response.Body)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: really we should report the error if it occurs. Not important though, you can leave it as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An error here is not important at all, as really the error is in the status code, that's why I am ignoring it.

t.Fatal("Request error %d: %s", response.Code, string(content))
}

if routes.Credentials.ClientId != newCreds.ClientId {
t.Errorf(
"ClientId should be \"%s\", but got \"%s\"",
newCreds.ClientId,
routes.Credentials.ClientId,
)
}

if routes.Credentials.AccessToken != newCreds.AccessToken {
t.Errorf(
"AccessToken should be \"%s\", but got \"%s\"",
newCreds.AccessToken,
routes.Credentials.AccessToken,
)
}

if routes.Credentials.Certificate != newCreds.Certificate {
t.Errorf(
"Certificate should be \"%s\", but got \"%s\"",
newCreds.Certificate,
routes.Credentials.Certificate,
)
}
}

func (self *RoutesTest) request(method string, content []byte) (res *httptest.ResponseRecorder) {
req, err := http.NewRequest(
method,
"http://localhost:8080/credentials",
bytes.NewBuffer(content),
)

if err != nil {
self.t.Fatal(err)
}

req.ContentLength = int64(len(content))
res = httptest.NewRecorder()
self.ServeHTTP(res, req)
return
}

func NewRoutesTest(t *testing.T) *RoutesTest {
return &RoutesTest{
Routes: Routes{
ConnectionData: tcclient.ConnectionData{
Authenticate: true,
Credentials: &tcclient.Credentials{
ClientId: "clientId",
AccessToken: "accessToken",
Certificate: "certificate",
},
},
},
t: t,
}
}
10 changes: 6 additions & 4 deletions main.go
View file
Open in desktop
Expand Up @@ -98,10 +98,12 @@ func main() {

log.Println("Proxy with scopes: ", creds.AuthorizedScopes)

routes := Routes(tcclient.ConnectionData{
Authenticate: true,
Credentials: creds,
})
routes := Routes{
ConnectionData: tcclient.ConnectionData{
Authenticate: true,
Credentials: creds,
},
}

startError := http.ListenAndServe(fmt.Sprintf(":%d", port), &routes)
if startError != nil {
Expand Down
52 changes: 49 additions & 3 deletions routes.go
View file
Open in desktop
Expand Up @@ -7,14 +7,24 @@ import (
"log"
"net/http"
"strings"
"sync"
"time"

"github.com/taskcluster/httpbackoff"
"github.com/taskcluster/taskcluster-client-go/tcclient"
tc "github.com/taskcluster/taskcluster-proxy/taskcluster"
)

type Routes tcclient.ConnectionData
type Routes struct {
tcclient.ConnectionData
lock sync.RWMutex
}

type CredentialsUpdate struct {
ClientId string `json:"clientId"`
AccessToken string `json:"accessToken"`
Certificate string `json:"certificate"`
}
Copy link
Member

Choose a reason for hiding this comment

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

It might be nice to tag the entries in the struct, e.g.

type CredentialsData struct {
    ClientId    string `json:"clientId"`
    AccessToken string `json:"accessToken"`
    Certificate string `json:"certificate"`
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, added. Just for curiosity, is this only for docs or is there any semantic meaning on it?

Copy link
Member

Choose a reason for hiding this comment

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

Nit: CredentialsUpdate might be a better name, since this type represents the json that is used for updating credentials, rather than the credetials per se which are stored in tcclient.Credentials (i.e. people might not realise this type only pertains to the credentials update API, and think it is used for storing the credentials). Not really important, you can leave it if you like.


var tcServices = tc.NewServices()
var httpClient = &http.Client{}
Expand All @@ -31,7 +41,7 @@ func (self *Routes) signUrl(res http.ResponseWriter, req *http.Request) {
}

urlString := strings.TrimSpace(string(body))
cd := tcclient.ConnectionData(*self)
cd := tcclient.ConnectionData(self.ConnectionData)
bewitUrl, err := (&cd).SignedURL(urlString, nil, time.Hour*1)

if err != nil {
Expand All @@ -46,8 +56,44 @@ func (self *Routes) signUrl(res http.ResponseWriter, req *http.Request) {
fmt.Fprintf(res, bewitUrl.String())
}

func (self *Routes) updateCredentials(res http.ResponseWriter, req *http.Request) {
if req.Method != "PUT" {
log.Printf("Invalid method %s\n", req.Method)
Copy link

Choose a reason for hiding this comment

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

Do we want to write this error back out to the client or just return a status code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, changed to return the string error to the client.

res.WriteHeader(405)
Copy link
Member

Choose a reason for hiding this comment

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

Nice! We should do this for our other entry points too (no need to do it in this PR).

return
}

decoder := json.NewDecoder(req.Body)

credentials := &CredentialsUpdate{}
err := decoder.Decode(credentials)

if err != nil {
log.Printf("Could not decode request: %v\n", err)
res.WriteHeader(400)
return
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think mutex locking only needs to happen at this point, because until now we are still validating parameters. I think we only need to lock once we've decided we definitely are going to change something.


self.lock.Lock()
defer self.lock.Unlock()
self.Credentials.ClientId = credentials.ClientId
Copy link

Choose a reason for hiding this comment

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

So I'm not sure about this and I'll let @petemoore weigh in, but are there issues with two requests racing (one to make a request to an api and one to update the credentials). Is there a way we could play a lock around updating the creds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right, my mind is still in nodejs async model, this runs in a go routine. Fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

Copy link
Member

Choose a reason for hiding this comment

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

@jonasfj I wonder if we should log a warning if the ClientId changes? It seems sane to allow the ClientId to be changed (future-proofing), but also could be concerning if it did. What do you think? An extreme position might be that ClientId should not be included in the task reclaim response schema, and not in the request body here either - but that also isn't so nice to sever it. Curious what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's overkill... I don't see any expoits from this...

Copy link
Member

Choose a reason for hiding this comment

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

@jonasfj I thought about this some more - would it not be simplest if the queue just issued temporary credentials that were valid for maxRunTime? Then reclaims would not need new credentials, we wouldn't need this endpoint, and there would be less functionality to implement and maintain.

At the moment we need to account for race conditions, congestion, exposure of PUT interface to tasks, then make sure we have working tests and documentation etc for this endpoint. If we delayed the expiry of the original credentials to maxRunTime, we wouldn't have these concerns. Is there a benefit to having new credentials with every reclaim? As I understand it, maxRunTime is typically an hour, and reclaims around 20 minutes, so there wouldn't be a huge difference. We could also enforce a maximum for maxRunTime in the schema to avoid abuse. Then all of the extra overhead from this PR would disappear, as well as the code on the worker side that would use this endpoint.

Copy link
Contributor

Choose a reason for hiding this comment

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

maxRunTime is a payload thing.
. not something the queue deals with...

We could extend it to be task.deadline but that is a long time...

IMO we can just leave docker-worker in its current state... But yes, this fix would be nice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding, concurrency just guard access to creds with a mutex..

self.Credentials.AccessToken = credentials.AccessToken
self.Credentials.Certificate = credentials.Certificate

res.WriteHeader(200)
}

// Routes implements the `http.Handler` interface
func (self *Routes) ServeHTTP(res http.ResponseWriter, req *http.Request) {
if req.URL.Path == "/credentials" {
log.Printf("Update credentials request %s\n", req.URL.String())
self.updateCredentials(res, req)
return
}

self.lock.RLock()
defer self.lock.RUnlock()

headersToSend := res.Header()
headersToSend.Set("X-Taskcluster-Proxy-Version", version)
cert, err := self.Credentials.Cert()
Expand Down Expand Up @@ -122,7 +168,7 @@ func (self *Routes) ServeHTTP(res http.ResponseWriter, req *http.Request) {
}
}

cd := tcclient.ConnectionData(*self)
cd := tcclient.ConnectionData(self.ConnectionData)
_, cs, err := (&cd).APICall(payload, req.Method, targetPath.String(), new(json.RawMessage), nil)
// If we fail to create a request notify the client.
if err != nil {
Expand Down