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.

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

Add buildkite-agent oidc request-token command #1827

Merged
merged 38 commits into from
Nov 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
7c9e4dd
Create method to call API method :jobid/oidc/tokens
triarius Nov 8, 2022
2946a0f
Create command for oidc test
triarius Nov 9, 2022
1c1b516
Add support for 0 or 1 length audience
triarius Nov 10, 2022
3766723
Fix http client does not require explict unmarshal
triarius Nov 10, 2022
53b4098
Convert api tests to be blackbox
triarius Nov 10, 2022
892baee
Add test for oidc token
triarius Nov 10, 2022
f947ad1
Add table test for 0, 1 and 2 audiences
triarius Nov 10, 2022
7f7cf5d
Factor out oidc token into test table
triarius Nov 10, 2022
b11ea30
Make OidcTokenRequest a local struct
triarius Nov 10, 2022
d9f9fb1
Add a comment about audience
triarius Nov 10, 2022
3a7e5dd
Use exp backoff
triarius Nov 10, 2022
52995c2
Adjust help text and comments
triarius Nov 10, 2022
e6d7ff4
Fix handle error from roko
triarius Nov 10, 2022
fcbff6f
Fix retry on errors that should not trigger retry
triarius Nov 10, 2022
301bd19
Fix empty object instead of no body for default audience
triarius Nov 10, 2022
8753f8a
Test the request body created for Oidc Token API calls
triarius Nov 10, 2022
2c5170b
Add skipping of retry for 400 errors
triarius Nov 10, 2022
778010d
Remove a debug print
triarius Nov 10, 2022
f2ebe2b
Update main.go
triarius Nov 11, 2022
918ca9b
Fix up help text
triarius Nov 11, 2022
5a10ef1
Use a struct as the only argument to the OidcTokenMethod
triarius Nov 11, 2022
b265e4a
Fix test does not close server until after all iteration
triarius Nov 11, 2022
49065c0
Use cmp.Equals
triarius Nov 11, 2022
705e40f
Change subcommand name to request-token
triarius Nov 11, 2022
863e572
Change sub-command to request-token
triarius Nov 11, 2022
3ec96cc
Merge branch 'triarius/oidc-token-command' of github.com:buildkite/ag…
triarius Nov 11, 2022
1a9d87e
Refactor oidc to take advantage of omitempty
triarius Nov 13, 2022
9395a9e
Capitalise OIDC initialism
triarius Nov 14, 2022
5ddef48
Change OIDCToken method to not return a nil pointer in case of error
triarius Nov 14, 2022
fb2dca5
Fix name of a file
triarius Nov 14, 2022
82dc5fe
Change test to declare table separately from iteration
triarius Nov 14, 2022
e22417c
Change test to have single source of truth for expected audience
triarius Nov 14, 2022
a3418bb
Fix an overzealous use of else if and e.Fatal
triarius Nov 14, 2022
0a03238
Fix tests were failing too fast
triarius Nov 14, 2022
0e4e15a
Rewrite tests to have separate test tables for success and error
triarius Nov 14, 2022
bc55667
Use interface to stderr instead of it directly
triarius Nov 14, 2022
5b860fb
Fix a comment
triarius Nov 14, 2022
15bc08d
Scope the response to be within the retry loop
triarius Nov 14, 2022
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
7 changes: 4 additions & 3 deletions api/client_test.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package api
package api_test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to use a private method in this file in a black box test, so I made this file a black box test too.


import (
"fmt"
Expand All @@ -7,6 +7,7 @@ import (
"strings"
"testing"

"github.com/buildkite/agent/v3/api"
"github.com/buildkite/agent/v3/logger"
)

Expand Down Expand Up @@ -36,13 +37,13 @@ func TestRegisteringAndConnectingClient(t *testing.T) {
defer server.Close()

// Initial client with a registration token
c := NewClient(logger.Discard, Config{
c := api.NewClient(logger.Discard, api.Config{
Endpoint: server.URL,
Token: "llamas",
})

// Check a register works
regResp, _, err := c.Register(&AgentRegisterRequest{})
regResp, _, err := c.Register(&api.AgentRegisterRequest{})
if err != nil {
t.Fatalf("c.Register(&AgentRegisterRequest{}) error = %v", err)
}
Expand Down
36 changes: 36 additions & 0 deletions api/oidc.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package api

import (
"fmt"
)

type OIDCToken struct {
Token string `json:"token"`
}

type OIDCTokenRequest struct {
Job string
Audience string
}

func (c *Client) OIDCToken(methodReq *OIDCTokenRequest) (*OIDCToken, *Response, error) {
m := &struct {
Audience string `json:"audience,omitempty"`
}{
Audience: methodReq.Audience,
}

u := fmt.Sprintf("jobs/%s/oidc/tokens", methodReq.Job)
httpReq, err := c.newRequest("POST", u, m)
if err != nil {
return nil, nil, err
}

t := &OIDCToken{}
resp, err := c.doRequest(httpReq, t)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the token and response ever useful if err != nil? If so that would make using this method more complicated.
Otherwise to avoid potential confusion ("I called OIDCToken and got both an error and some other stuff?") I would insert an if err != nil { return nil, nil, err } in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doRequest returns the response in cases where err != nil. The rationale seems to be that it allows inspection of the status code and retrying if necessary. See

agent/api/client.go

Lines 260 to 261 in 1a9d87e

// even though there was an error, we still return the response
// in case the caller wants to inspect it further

The practice in this codebase seems to be to do the retrying in the clicommand method and not the api one, and inspecting the status code from the bubbled up response. See

agent/api/meta_data.go

Lines 60 to 65 in 1a9d87e

resp, err := c.doRequest(req, e)
if err != nil {
return nil, resp, err
}
return e, resp, err
and
).Do(func(r *roko.Retrier) error {
metaData, resp, err = client.GetMetaData(cfg.Job, cfg.Key)
// Don't bother retrying if the response was one of these statuses
if resp != nil && (resp.StatusCode == 401 || resp.StatusCode == 404 || resp.StatusCode == 400) {
r.Break()
return err
}
if err != nil {
l.Warn("%s (%s)", err, r)
}
return err
})

So the respone SHOULD be passed up or the retry won't work as expected. The token is not needed not however, so I have changed it to pass a nil pointer on error now.

I don't really think this is a good practice though (see below). However, I do think it's better to consistently follow a bad practice than to mix different practices. So I think we should follow this for now, and have a piece of work to unravel http.doRequest. I haven't thought too deeply about what that should be, but here are some initial thoughts:

I think there is no good reason for api.doRequest to return an http.Response. Instead, I think the relavant parts of the response that are needed for retry handling or otherwise should be returned as part of a structured error. Indeed, there already is a error type that purports to do this: api.ErrorReponse, although it contains a pointer to http.Response 🤷‍♂️. Either way returning a pointer to http.Response is potentially misleading as the body of the http.Response is closed in api.doRequest, so it will look like there is no body if we try to use it outside the api.doRequest method.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fair. net/http itself does this sort of thing.

if err != nil {
return nil, resp, err
}

return t, resp, nil
}
237 changes: 237 additions & 0 deletions api/oidc_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,237 @@
package api_test

import (
"bytes"
"fmt"
"io"
"net/http"
"net/http/httptest"
"testing"

"github.com/buildkite/agent/v3/api"
"github.com/buildkite/agent/v3/logger"
"github.com/google/go-cmp/cmp"
)

type testOIDCTokenServer struct {
accessToken string
oidcToken string
jobID string
forbiddenJobID string
expectedBody []byte
}

func (s *testOIDCTokenServer) New(t *testing.T) *httptest.Server {
t.Helper()
path := fmt.Sprintf("/jobs/%s/oidc/tokens", s.jobID)
forbiddenPath := fmt.Sprintf("/jobs/%s/oidc/tokens", s.forbiddenJobID)
return httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
if got, want := authToken(req), s.accessToken; got != want {
http.Error(
rw,
fmt.Sprintf("authToken(req) = %q, want %q", got, want),
http.StatusUnauthorized,
)
return
}

switch req.URL.Path {
case path:
body, err := io.ReadAll(req.Body)
if err != nil {
http.Error(
rw,
fmt.Sprintf(`{"message:"Internal Server Error: %s"}`, err),
http.StatusInternalServerError,
)
return
}

if !bytes.Equal(body, s.expectedBody) {
t.Errorf("wanted = %q, got = %q", s.expectedBody, body)
http.Error(
rw,
fmt.Sprintf(`{"message:"Bad Request: wanted = %q, got = %q"}`, s.expectedBody, body),
http.StatusBadRequest,
)
return
}

io.WriteString(rw, fmt.Sprintf(`{"token":"%s"}`, s.oidcToken))

case forbiddenPath:
http.Error(
rw,
fmt.Sprintf(`{"message":"Forbidden; method = %q, path = %q"}`, req.Method, req.URL.Path),
http.StatusForbidden,
)

default:
http.Error(
rw,
fmt.Sprintf(
`{"message":"Not Found; method = %q, path = %q"}`,
req.Method,
req.URL.Path,
),
http.StatusNotFound,
)
}
}))
}

func TestOIDCToken(t *testing.T) {
const jobID = "b078e2d2-86e9-4c12-bf3b-612a8058d0a4"
const unauthorizedJobID = "a078e2d2-86e9-4c12-bf3b-612a8058d0a4"
const oidcToken = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWUsImlhdCI6MTUxNjIzOTAyMn0.NHVaYe26MbtOYhSKkoKYdFVomg4i8ZJd8_-RU8VNbftc4TSMb4bXP3l3YlNWACwyXPGffz5aXHc6lty1Y2t4SWRqGteragsVdZufDn5BlnJl9pdR_kdVFUsra2rWKEofkZeIC4yWytE58sMIihvo9H1ScmmVwBcQP6XETqYd0aSHp1gOa9RdUPDvoXQ5oqygTqVtxaDr6wUFKrKItgBMzWIdNZ6y7O9E0DhEPTbE9rfBo6KTFsHAZnMg4k68CDp2woYIaXbmYTWcvbzIuHO7_37GT79XdIwkm95QJ7hYC9RiwrV7mesbY4PAahERJawntho0my942XheVLmGwLMBkQ"
const accessToken = "llamas"
const audience = "sts.amazonaws.com"

tests := []struct {
OIDCTokenRequest *api.OIDCTokenRequest
AccessToken string
ExpectedBody []byte
OIDCToken *api.OIDCToken
}{
{
AccessToken: accessToken,
OIDCTokenRequest: &api.OIDCTokenRequest{
Job: jobID,
},
ExpectedBody: []byte("{}\n"),
OIDCToken: &api.OIDCToken{Token: oidcToken},
},
{
AccessToken: accessToken,
OIDCTokenRequest: &api.OIDCTokenRequest{
Job: jobID,
Audience: audience,
},
ExpectedBody: []byte(fmt.Sprintf(`{"audience":%q}`+"\n", audience)),
OIDCToken: &api.OIDCToken{Token: oidcToken},
},
}

for _, test := range tests {
func() { // this exists to allow closing the server on each iteration
server := (&testOIDCTokenServer{
accessToken: test.AccessToken,
oidcToken: test.OIDCToken.Token,
jobID: jobID,
forbiddenJobID: unauthorizedJobID,
expectedBody: test.ExpectedBody,
}).New(t)
defer server.Close()

// Initial client with a registration token
client := api.NewClient(logger.Discard, api.Config{
UserAgent: "Test",
Endpoint: server.URL,
Token: accessToken,
DebugHTTP: true,
})

token, resp, err := client.OIDCToken(test.OIDCTokenRequest)
if err != nil {
t.Errorf(
"OIDCToken(%v) got error = %v",
test.OIDCTokenRequest,
err,
)
return
}

if !cmp.Equal(token, test.OIDCToken) {
t.Errorf(
"OIDCToken(%v) got token = %v, want %v",
test.OIDCTokenRequest,
token,
test.OIDCToken,
)
}

if resp.StatusCode != http.StatusOK {
t.Errorf(
"OIDCToken(%v) got StatusCode = %v, want %v",
test.OIDCTokenRequest,
resp.StatusCode,
http.StatusOK,
)
}
}()
}
}

func TestOIDCTokenError(t *testing.T) {
const jobID = "b078e2d2-86e9-4c12-bf3b-612a8058d0a4"
const unauthorizedJobID = "a078e2d2-86e9-4c12-bf3b-612a8058d0a4"
const accessToken = "llamas"
const audience = "sts.amazonaws.com"

tests := []struct {
OIDCTokenRequest *api.OIDCTokenRequest
AccessToken string
ExpectedStatus int
// TODO: make api.ErrorReponse a serializable type and populate this field
// ExpectedErr error
}{
{
AccessToken: "camels",
OIDCTokenRequest: &api.OIDCTokenRequest{
Job: jobID,
Audience: audience,
},
ExpectedStatus: http.StatusUnauthorized,
},
{
AccessToken: accessToken,
OIDCTokenRequest: &api.OIDCTokenRequest{
Job: unauthorizedJobID,
Audience: audience,
},
ExpectedStatus: http.StatusForbidden,
},
{
AccessToken: accessToken,
OIDCTokenRequest: &api.OIDCTokenRequest{
Job: "2",
Audience: audience,
},
ExpectedStatus: http.StatusNotFound,
},
}

for _, test := range tests {
func() { // this exists to allow closing the server on each iteration
server := (&testOIDCTokenServer{
accessToken: test.AccessToken,
jobID: jobID,
forbiddenJobID: unauthorizedJobID,
}).New(t)
defer server.Close()

// Initial client with a registration token
client := api.NewClient(logger.Discard, api.Config{
UserAgent: "Test",
Endpoint: server.URL,
Token: accessToken,
DebugHTTP: true,
})

_, resp, err := client.OIDCToken(test.OIDCTokenRequest)
// TODO: make api.ErrorReponse a serializable type and test that the right error type is returned here
if err == nil {
t.Errorf("OIDCToken(%v) did not return an error as expected", test.OIDCTokenRequest)
}

if resp.StatusCode != test.ExpectedStatus {
t.Errorf(
"OIDCToken(%v) got StatusCode = %v, want %v",
test.OIDCTokenRequest,
resp.StatusCode,
test.ExpectedStatus,
)
}
}()
}
}
Loading