From 7c9e4dd35ce95cfb7ae0ca2a1910899ca4f5ddca Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Wed, 9 Nov 2022 10:44:28 +1100 Subject: [PATCH 01/37] Create method to call API method :jobid/oidc/tokens --- api/oidc.go | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 api/oidc.go diff --git a/api/oidc.go b/api/oidc.go new file mode 100644 index 0000000000..e220f4bba8 --- /dev/null +++ b/api/oidc.go @@ -0,0 +1,35 @@ +package api + +import ( + "encoding/json" + "fmt" +) + +type OidcTokenRequest struct { + Audience string `json:"audience"` +} + +type OidcToken struct { + Token string `json:"token"` +} + +func (c *Client) OidcToken(jobId, audience string) (*OidcToken, *Response, error) { + u := fmt.Sprintf("jobs/%s/oidc/tokens", jobId) + m := &OidcTokenRequest{Audience: audience} + req, err := c.newRequest("POST", u, m) + if err != nil { + return nil, nil, err + } + + resp, err := c.doRequest(req, m) + if err != nil { + return nil, nil, err + } + + t := &OidcToken{} + if err := json.NewDecoder(resp.Body).Decode(t); err != nil { + return nil, resp, err + } + + return t, resp, err +} From 2946a0f94fff3d03e2c277b5da88589ae45b301b Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Thu, 10 Nov 2022 08:46:41 +1100 Subject: [PATCH 02/37] Create command for oidc test --- clicommand/oidc_token.go | 128 +++++++++++++++++++++++++++++++++++++++ main.go | 7 +++ 2 files changed, 135 insertions(+) create mode 100644 clicommand/oidc_token.go diff --git a/clicommand/oidc_token.go b/clicommand/oidc_token.go new file mode 100644 index 0000000000..a77d0caf47 --- /dev/null +++ b/clicommand/oidc_token.go @@ -0,0 +1,128 @@ +package clicommand + +import ( + "fmt" + "net/http" + "os" + "time" + + "github.com/buildkite/agent/v3/api" + "github.com/buildkite/agent/v3/cliconfig" + "github.com/buildkite/roko" + "github.com/urfave/cli" +) + +type OidcTokenConfig struct { + Audience string `cli:"audience"` + Job string `cli:"job" validate:"required"` + + // Global flags + Debug bool `cli:"debug"` + LogLevel string `cli:"log-level"` + NoColor bool `cli:"no-color"` + Experiments []string `cli:"experiment" normalize:"list"` + Profile string `cli:"profile"` + + // API config + DebugHTTP bool `cli:"debug-http"` + AgentAccessToken string `cli:"agent-access-token" validate:"required"` + Endpoint string `cli:"endpoint" validate:"required"` + NoHTTP2 bool `cli:"no-http2"` +} + +const oidcTokenDescription = `Usage: + + buildkite-agent oidc token [options] + +Description: + Requests and prints an OIDC token from Buildkite with the specified audience. + +Example: + $ buildkite-agent oidc token --audience sts.amazonaws.com + + Prints the environment passed into the process +` + +var OidcTokenCommand = cli.Command{ + Name: "token", + Usage: "Requests and prints an OIDC token from Buildkite with the specified audience,", + Description: oidcTokenDescription, + Flags: []cli.Flag{ + cli.StringFlag{ + Name: "audience", + Value: "", + Usage: "The audience that will consume the OIDC token", + }, + cli.StringFlag{ + Name: "job", + Value: "", + Usage: "Buildkite Job ID", + EnvVar: "BUILDKITE_JOB_ID", + }, + + // API Flags + AgentAccessTokenFlag, + EndpointFlag, + NoHTTP2Flag, + DebugHTTPFlag, + + // Global flags + NoColorFlag, + DebugFlag, + LogLevelFlag, + ExperimentsFlag, + ProfileFlag, + }, + Action: func(c *cli.Context) error { + // The configuration will be loaded into this struct + cfg := OidcTokenConfig{} + + loader := cliconfig.Loader{CLI: c, Config: &cfg} + warnings, err := loader.Load() + if err != nil { + fmt.Fprintf(os.Stderr, "%s\n", err) + os.Exit(1) + } + + l := CreateLogger(&cfg) + + // Now that we have a logger, log out the warnings that loading config generated + for _, warning := range warnings { + l.Warn("%s", warning) + } + + // Setup any global configuration options + done := HandleGlobalFlags(l, cfg) + defer done() + + // Create the API client + client := api.NewClient(l, loadAPIClientConfig(cfg, "AgentAccessToken")) + + // Find the meta data value + var token *api.OidcToken + var resp *api.Response + + err = roko.NewRetrier( + roko.WithMaxAttempts(10), + roko.WithStrategy(roko.Constant(5*time.Second)), + ).Do(func(r *roko.Retrier) error { + token, resp, err = client.OidcToken(cfg.Job, cfg.Audience) + // Don't bother retrying if the response was one of these statuses + if resp != nil { + switch resp.StatusCode { + case http.StatusUnauthorized | http.StatusForbidden | http.StatusUnprocessableEntity: + r.Break() + return err + } + } + if err != nil { + l.Warn("%s (%s)", err, r) + } + + return err + }) + + fmt.Println(token.Token) + return nil + }, +} diff --git a/main.go b/main.go index 595e93be47..8e1bd86068 100644 --- a/main.go +++ b/main.go @@ -90,6 +90,13 @@ func main() { clicommand.MetaDataKeysCommand, }, }, + { + Name: "oidc", + Usage: "Obtains OIDC information from Buildkite", + Subcommands: []cli.Command{ + clicommand.OidcTokenCommand, + }, + }, { Name: "pipeline", Usage: "Make changes to the pipeline of the currently running build", From 1c1b516e18499b0b756aba365c47792602c80ac1 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Thu, 10 Nov 2022 13:35:38 +1100 Subject: [PATCH 03/37] Add support for 0 or 1 length audience --- api/oidc.go | 16 ++++++++++++++-- clicommand/oidc_token.go | 8 +++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/api/oidc.go b/api/oidc.go index e220f4bba8..1b1e4007b6 100644 --- a/api/oidc.go +++ b/api/oidc.go @@ -2,9 +2,12 @@ package api import ( "encoding/json" + "errors" "fmt" ) +var ErrAudienceTooLong = errors.New("the API only supports at most one element in the audience") + type OidcTokenRequest struct { Audience string `json:"audience"` } @@ -13,9 +16,18 @@ type OidcToken struct { Token string `json:"token"` } -func (c *Client) OidcToken(jobId, audience string) (*OidcToken, *Response, error) { +func (c *Client) OidcToken(jobId string, audience ...string) (*OidcToken, *Response, error) { + var m *OidcTokenRequest + switch len(audience) { + case 0: + m = nil + case 1: + m = &OidcTokenRequest{Audience: audience[0]} + default: + return nil, nil, ErrAudienceTooLong + } + u := fmt.Sprintf("jobs/%s/oidc/tokens", jobId) - m := &OidcTokenRequest{Audience: audience} req, err := c.newRequest("POST", u, m) if err != nil { return nil, nil, err diff --git a/clicommand/oidc_token.go b/clicommand/oidc_token.go index a77d0caf47..72d737e95a 100644 --- a/clicommand/oidc_token.go +++ b/clicommand/oidc_token.go @@ -106,7 +106,12 @@ var OidcTokenCommand = cli.Command{ roko.WithMaxAttempts(10), roko.WithStrategy(roko.Constant(5*time.Second)), ).Do(func(r *roko.Retrier) error { - token, resp, err = client.OidcToken(cfg.Job, cfg.Audience) + var audience []string + if len(cfg.Audience) > 0 { + audience = []string{cfg.Audience} + } + + token, resp, err = client.OidcToken(cfg.Job, audience...) // Don't bother retrying if the response was one of these statuses if resp != nil { switch resp.StatusCode { @@ -115,6 +120,7 @@ var OidcTokenCommand = cli.Command{ return err } } + if err != nil { l.Warn("%s (%s)", err, r) } From 37667238171d53e2f769de9c30930cb47dfbfacc Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Thu, 10 Nov 2022 16:44:18 +1100 Subject: [PATCH 04/37] Fix http client does not require explict unmarshal --- api/oidc.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/api/oidc.go b/api/oidc.go index 1b1e4007b6..6b71223ac0 100644 --- a/api/oidc.go +++ b/api/oidc.go @@ -1,7 +1,6 @@ package api import ( - "encoding/json" "errors" "fmt" ) @@ -33,15 +32,11 @@ func (c *Client) OidcToken(jobId string, audience ...string) (*OidcToken, *Respo return nil, nil, err } - resp, err := c.doRequest(req, m) + t := &OidcToken{} + resp, err := c.doRequest(req, t) if err != nil { return nil, nil, err } - t := &OidcToken{} - if err := json.NewDecoder(resp.Body).Decode(t); err != nil { - return nil, resp, err - } - return t, resp, err } From 53b409859847a1e102eac4b5d97f3d8611274b91 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Thu, 10 Nov 2022 16:45:00 +1100 Subject: [PATCH 05/37] Convert api tests to be blackbox --- api/client_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/api/client_test.go b/api/client_test.go index 51b546483a..3b51080c39 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -1,4 +1,4 @@ -package api +package api_test import ( "fmt" @@ -7,6 +7,7 @@ import ( "strings" "testing" + "github.com/buildkite/agent/v3/api" "github.com/buildkite/agent/v3/logger" ) @@ -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) } From 892baee643198b037906909d6c63f20c1f098af9 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Thu, 10 Nov 2022 16:45:39 +1100 Subject: [PATCH 06/37] Add test for oidc token --- api/oidc_test.go | 62 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 api/oidc_test.go diff --git a/api/oidc_test.go b/api/oidc_test.go new file mode 100644 index 0000000000..c4b3697c3a --- /dev/null +++ b/api/oidc_test.go @@ -0,0 +1,62 @@ +package api_test + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/buildkite/agent/v3/api" + "github.com/buildkite/agent/v3/logger" +) + +func TestOidcTokenNoAudience(t *testing.T) { + const jobId = "b078e2d2-86e9-4c12-bf3b-612a8058d0a4" + const oidcToken = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWUsImlhdCI6MTUxNjIzOTAyMn0.NHVaYe26MbtOYhSKkoKYdFVomg4i8ZJd8_-RU8VNbftc4TSMb4bXP3l3YlNWACwyXPGffz5aXHc6lty1Y2t4SWRqGteragsVdZufDn5BlnJl9pdR_kdVFUsra2rWKEofkZeIC4yWytE58sMIihvo9H1ScmmVwBcQP6XETqYd0aSHp1gOa9RdUPDvoXQ5oqygTqVtxaDr6wUFKrKItgBMzWIdNZ6y7O9E0DhEPTbE9rfBo6KTFsHAZnMg4k68CDp2woYIaXbmYTWcvbzIuHO7_37GT79XdIwkm95QJ7hYC9RiwrV7mesbY4PAahERJawntho0my942XheVLmGwLMBkQ" + const accessToken = "llamas" + + path := fmt.Sprintf("/jobs/%s/oidc/tokens", jobId) + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + switch req.URL.Path { + case path: + if got, want := authToken(req), accessToken; got != want { + http.Error( + rw, + fmt.Sprintf("authToken(req) = %q, want %q", got, want), + http.StatusUnauthorized, + ) + return + } + rw.WriteHeader(http.StatusOK) + fmt.Fprint(rw, fmt.Sprintf(`{"token":"%s"}`, oidcToken)) + + default: + http.Error( + rw, + fmt.Sprintf( + `{"message": "not found; method = %q, path = %q"}`, + req.Method, + req.URL.Path, + ), + http.StatusNotFound, + ) + } + })) + 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, + }) + + if token, resp, err := client.OidcToken(jobId); err != nil { + t.Fatalf("OidcToken(%v) error = %# v", jobId, err) + } else if token.Token != oidcToken { + t.Fatalf("OidcToken(%v) got token = %# v, want %# v", jobId, token, &api.OidcToken{Token: oidcToken}) + } else if resp.StatusCode != http.StatusOK { + t.Fatalf("OidcToken(%v) got StatusCode = %# v, want %# v", jobId, resp.StatusCode, http.StatusOK) + } +} From f947ad173507aec6de5f726f07fb49b497682993 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Thu, 10 Nov 2022 17:09:12 +1100 Subject: [PATCH 07/37] Add table test for 0, 1 and 2 audiences --- api/oidc_test.go | 41 ++++++++++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/api/oidc_test.go b/api/oidc_test.go index c4b3697c3a..25f82f1cca 100644 --- a/api/oidc_test.go +++ b/api/oidc_test.go @@ -1,6 +1,7 @@ package api_test import ( + "errors" "fmt" "net/http" "net/http/httptest" @@ -10,7 +11,7 @@ import ( "github.com/buildkite/agent/v3/logger" ) -func TestOidcTokenNoAudience(t *testing.T) { +func TestOidcToken(t *testing.T) { const jobId = "b078e2d2-86e9-4c12-bf3b-612a8058d0a4" const oidcToken = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWUsImlhdCI6MTUxNjIzOTAyMn0.NHVaYe26MbtOYhSKkoKYdFVomg4i8ZJd8_-RU8VNbftc4TSMb4bXP3l3YlNWACwyXPGffz5aXHc6lty1Y2t4SWRqGteragsVdZufDn5BlnJl9pdR_kdVFUsra2rWKEofkZeIC4yWytE58sMIihvo9H1ScmmVwBcQP6XETqYd0aSHp1gOa9RdUPDvoXQ5oqygTqVtxaDr6wUFKrKItgBMzWIdNZ6y7O9E0DhEPTbE9rfBo6KTFsHAZnMg4k68CDp2woYIaXbmYTWcvbzIuHO7_37GT79XdIwkm95QJ7hYC9RiwrV7mesbY4PAahERJawntho0my942XheVLmGwLMBkQ" const accessToken = "llamas" @@ -52,11 +53,37 @@ func TestOidcTokenNoAudience(t *testing.T) { DebugHTTP: true, }) - if token, resp, err := client.OidcToken(jobId); err != nil { - t.Fatalf("OidcToken(%v) error = %# v", jobId, err) - } else if token.Token != oidcToken { - t.Fatalf("OidcToken(%v) got token = %# v, want %# v", jobId, token, &api.OidcToken{Token: oidcToken}) - } else if resp.StatusCode != http.StatusOK { - t.Fatalf("OidcToken(%v) got StatusCode = %# v, want %# v", jobId, resp.StatusCode, http.StatusOK) + for _, testData := range []struct { + JobId string + AccessToken string + Audience []string + Error error + }{ + { + JobId: jobId, + AccessToken: accessToken, + Audience: []string{}, + }, + { + JobId: jobId, + AccessToken: accessToken, + Audience: []string{"sts.amazonaws.com"}, + }, + { + JobId: jobId, + AccessToken: accessToken, + Audience: []string{"sts.amazonaws.com", "buildkite.com"}, + Error: api.ErrAudienceTooLong, + }, + } { + if token, resp, err := client.OidcToken(testData.JobId, testData.Audience...); err != nil { + if !errors.Is(err, testData.Error) { + t.Fatalf("OidcToken(%v, %v) got error = %# v, want error = %# v", testData.JobId, testData.Audience, err, testData.Error) + } + } else if token.Token != oidcToken { + t.Fatalf("OidcToken(%v, %v) got token = %# v, want %# v", testData.JobId, testData.Audience, token, &api.OidcToken{Token: oidcToken}) + } else if resp.StatusCode != http.StatusOK { + t.Fatalf("OidcToken(%v, %v) got StatusCode = %# v, want %# v", testData.JobId, testData.Audience, resp.StatusCode, http.StatusOK) + } } } From 7f7cf5dfad34fe7058d8e9bc9f4be046b87e9c7c Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Thu, 10 Nov 2022 17:13:53 +1100 Subject: [PATCH 08/37] Factor out oidc token into test table --- api/oidc_test.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/api/oidc_test.go b/api/oidc_test.go index 25f82f1cca..3092aa5b05 100644 --- a/api/oidc_test.go +++ b/api/oidc_test.go @@ -56,17 +56,20 @@ func TestOidcToken(t *testing.T) { for _, testData := range []struct { JobId string AccessToken string + OidcToken *api.OidcToken Audience []string Error error }{ { JobId: jobId, AccessToken: accessToken, + OidcToken: &api.OidcToken{Token: oidcToken}, Audience: []string{}, }, { JobId: jobId, AccessToken: accessToken, + OidcToken: &api.OidcToken{Token: oidcToken}, Audience: []string{"sts.amazonaws.com"}, }, { @@ -78,10 +81,16 @@ func TestOidcToken(t *testing.T) { } { if token, resp, err := client.OidcToken(testData.JobId, testData.Audience...); err != nil { if !errors.Is(err, testData.Error) { - t.Fatalf("OidcToken(%v, %v) got error = %# v, want error = %# v", testData.JobId, testData.Audience, err, testData.Error) + t.Fatalf( + "OidcToken(%v, %v) got error = %# v, want error = %# v", + testData.JobId, + testData.Audience, + err, + testData.Error, + ) } } else if token.Token != oidcToken { - t.Fatalf("OidcToken(%v, %v) got token = %# v, want %# v", testData.JobId, testData.Audience, token, &api.OidcToken{Token: oidcToken}) + t.Fatalf("OidcToken(%v, %v) got token = %# v, want %# v", testData.JobId, testData.Audience, token, testData.OidcToken) } else if resp.StatusCode != http.StatusOK { t.Fatalf("OidcToken(%v, %v) got StatusCode = %# v, want %# v", testData.JobId, testData.Audience, resp.StatusCode, http.StatusOK) } From b11ea30dd5352e96e5de46f8960895d62478fa95 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Thu, 10 Nov 2022 17:38:07 +1100 Subject: [PATCH 09/37] Make OidcTokenRequest a local struct --- api/oidc.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/api/oidc.go b/api/oidc.go index 6b71223ac0..957120dc7b 100644 --- a/api/oidc.go +++ b/api/oidc.go @@ -7,21 +7,21 @@ import ( var ErrAudienceTooLong = errors.New("the API only supports at most one element in the audience") -type OidcTokenRequest struct { - Audience string `json:"audience"` -} - type OidcToken struct { Token string `json:"token"` } func (c *Client) OidcToken(jobId string, audience ...string) (*OidcToken, *Response, error) { - var m *OidcTokenRequest + type oidcTokenRequest struct { + Audience string `json:"audience"` + } + + var m *oidcTokenRequest switch len(audience) { case 0: m = nil case 1: - m = &OidcTokenRequest{Audience: audience[0]} + m = &oidcTokenRequest{Audience: audience[0]} default: return nil, nil, ErrAudienceTooLong } From d9f9fb14a7bb6fec351384a3cce1e7495b17da31 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Thu, 10 Nov 2022 17:44:02 +1100 Subject: [PATCH 10/37] Add a comment about audience --- api/oidc.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/api/oidc.go b/api/oidc.go index 957120dc7b..ef487bfe9c 100644 --- a/api/oidc.go +++ b/api/oidc.go @@ -23,6 +23,9 @@ func (c *Client) OidcToken(jobId string, audience ...string) (*OidcToken, *Respo case 1: m = &oidcTokenRequest{Audience: audience[0]} default: + // While the spec supports multiple audiences in an Id JWT, our API does + // not support issuing them. + // See: https://openid.net/specs/openid-connect-core-1_0.html#IDToken. return nil, nil, ErrAudienceTooLong } From 3a7e5dd73a36cc4cd1b00962eba736178812b41e Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Thu, 10 Nov 2022 17:54:41 +1100 Subject: [PATCH 11/37] Use exp backoff --- clicommand/oidc_token.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/clicommand/oidc_token.go b/clicommand/oidc_token.go index 72d737e95a..17e09c3d45 100644 --- a/clicommand/oidc_token.go +++ b/clicommand/oidc_token.go @@ -30,7 +30,8 @@ type OidcTokenConfig struct { NoHTTP2 bool `cli:"no-http2"` } -const oidcTokenDescription = `Usage: +const ( + oidcTokenDescription = `Usage: buildkite-agent oidc token [options] @@ -42,6 +43,9 @@ Example: Prints the environment passed into the process ` + backoffSeconds = 2 + maxAttempts = 5 +) var OidcTokenCommand = cli.Command{ Name: "token", @@ -103,8 +107,8 @@ var OidcTokenCommand = cli.Command{ var resp *api.Response err = roko.NewRetrier( - roko.WithMaxAttempts(10), - roko.WithStrategy(roko.Constant(5*time.Second)), + roko.WithMaxAttempts(maxAttempts), + roko.WithStrategy(roko.Exponential(backoffSeconds*time.Second, 0)), ).Do(func(r *roko.Retrier) error { var audience []string if len(cfg.Audience) > 0 { From 52995c2051953564038cf4346f00784a56e6afc8 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Thu, 10 Nov 2022 17:54:53 +1100 Subject: [PATCH 12/37] Adjust help text and comments --- clicommand/oidc_token.go | 10 ++++++---- main.go | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/clicommand/oidc_token.go b/clicommand/oidc_token.go index 17e09c3d45..24eb41bfd3 100644 --- a/clicommand/oidc_token.go +++ b/clicommand/oidc_token.go @@ -33,10 +33,12 @@ type OidcTokenConfig struct { const ( oidcTokenDescription = `Usage: - buildkite-agent oidc token [options] + buildkite-agent oidc token [options...] Description: - Requests and prints an OIDC token from Buildkite with the specified audience. + Requests and prints an OIDC token from Buildkite that claims the Job ID and + the specified audience. If no audience is specified, the endpoint's default + audience will be claimed. Example: $ buildkite-agent oidc token --audience sts.amazonaws.com @@ -60,7 +62,7 @@ var OidcTokenCommand = cli.Command{ cli.StringFlag{ Name: "job", Value: "", - Usage: "Buildkite Job ID", + Usage: "Buildkite Job Id to claim in the OIDC token", EnvVar: "BUILDKITE_JOB_ID", }, @@ -116,9 +118,9 @@ var OidcTokenCommand = cli.Command{ } token, resp, err = client.OidcToken(cfg.Job, audience...) - // Don't bother retrying if the response was one of these statuses if resp != nil { switch resp.StatusCode { + // Don't bother retrying if the response was one of these statuses case http.StatusUnauthorized | http.StatusForbidden | http.StatusUnprocessableEntity: r.Break() return err diff --git a/main.go b/main.go index 8e1bd86068..c401fa36bc 100644 --- a/main.go +++ b/main.go @@ -92,7 +92,7 @@ func main() { }, { Name: "oidc", - Usage: "Obtains OIDC information from Buildkite", + Usage: "Interact with Buildkite OIDC", Subcommands: []cli.Command{ clicommand.OidcTokenCommand, }, From e6d7ff488c97c5436841ab962f7e905db75079e4 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Thu, 10 Nov 2022 18:16:49 +1100 Subject: [PATCH 13/37] Fix handle error from roko --- clicommand/oidc_token.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/clicommand/oidc_token.go b/clicommand/oidc_token.go index 24eb41bfd3..34cd9aa20c 100644 --- a/clicommand/oidc_token.go +++ b/clicommand/oidc_token.go @@ -108,7 +108,7 @@ var OidcTokenCommand = cli.Command{ var token *api.OidcToken var resp *api.Response - err = roko.NewRetrier( + if err := roko.NewRetrier( roko.WithMaxAttempts(maxAttempts), roko.WithStrategy(roko.Exponential(backoffSeconds*time.Second, 0)), ).Do(func(r *roko.Retrier) error { @@ -119,6 +119,8 @@ var OidcTokenCommand = cli.Command{ token, resp, err = client.OidcToken(cfg.Job, audience...) if resp != nil { + fmt.Println(resp.StatusCode, r.ShouldGiveUp()) + switch resp.StatusCode { // Don't bother retrying if the response was one of these statuses case http.StatusUnauthorized | http.StatusForbidden | http.StatusUnprocessableEntity: @@ -132,7 +134,14 @@ var OidcTokenCommand = cli.Command{ } return err - }) + }); err != nil { + if len(cfg.Audience) > 0 { + l.Error("Could not obtain OIDC token for audience %s", cfg.Audience) + } else { + l.Error("Could not obtain OIDC token for default audience") + } + return err + } fmt.Println(token.Token) return nil From fcbff6f8c699d0d0ca09948c9d844f56a0cfd6c7 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Thu, 10 Nov 2022 18:28:49 +1100 Subject: [PATCH 14/37] Fix retry on errors that should not trigger retry --- api/oidc.go | 4 ---- clicommand/oidc_token.go | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/api/oidc.go b/api/oidc.go index ef487bfe9c..b7c4e07de0 100644 --- a/api/oidc.go +++ b/api/oidc.go @@ -37,9 +37,5 @@ func (c *Client) OidcToken(jobId string, audience ...string) (*OidcToken, *Respo t := &OidcToken{} resp, err := c.doRequest(req, t) - if err != nil { - return nil, nil, err - } - return t, resp, err } diff --git a/clicommand/oidc_token.go b/clicommand/oidc_token.go index 34cd9aa20c..9f7d0dfa8f 100644 --- a/clicommand/oidc_token.go +++ b/clicommand/oidc_token.go @@ -123,7 +123,7 @@ var OidcTokenCommand = cli.Command{ switch resp.StatusCode { // Don't bother retrying if the response was one of these statuses - case http.StatusUnauthorized | http.StatusForbidden | http.StatusUnprocessableEntity: + case http.StatusUnauthorized, http.StatusForbidden, http.StatusUnprocessableEntity: r.Break() return err } From 301bd192d1f81ed0eb95d86f8eb028586e8dbf6b Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Thu, 10 Nov 2022 23:34:30 +1100 Subject: [PATCH 15/37] Fix empty object instead of no body for default audience The ticket seemed to indicate that if there is no audience specified, then the request was to have an empty body. However, that leads to a 500 from the API. Reading the associated plugin show that an empty object is expected in that case. --- api/oidc.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/api/oidc.go b/api/oidc.go index b7c4e07de0..02667983ea 100644 --- a/api/oidc.go +++ b/api/oidc.go @@ -13,15 +13,14 @@ type OidcToken struct { func (c *Client) OidcToken(jobId string, audience ...string) (*OidcToken, *Response, error) { type oidcTokenRequest struct { - Audience string `json:"audience"` + Audience string `json:"audience,omitempty"` } - var m *oidcTokenRequest + m := &oidcTokenRequest{} switch len(audience) { case 0: - m = nil case 1: - m = &oidcTokenRequest{Audience: audience[0]} + m.Audience = audience[0] default: // While the spec supports multiple audiences in an Id JWT, our API does // not support issuing them. From 8753f8af6d58294682ef3b05f53777dcb4d01186 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Fri, 11 Nov 2022 00:25:42 +1100 Subject: [PATCH 16/37] Test the request body created for Oidc Token API calls --- api/oidc_test.go | 108 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 78 insertions(+), 30 deletions(-) diff --git a/api/oidc_test.go b/api/oidc_test.go index 3092aa5b05..dc885e34fe 100644 --- a/api/oidc_test.go +++ b/api/oidc_test.go @@ -1,8 +1,10 @@ package api_test import ( + "bytes" "errors" "fmt" + "io" "net/http" "net/http/httptest" "testing" @@ -11,13 +13,14 @@ import ( "github.com/buildkite/agent/v3/logger" ) -func TestOidcToken(t *testing.T) { - const jobId = "b078e2d2-86e9-4c12-bf3b-612a8058d0a4" - const oidcToken = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWUsImlhdCI6MTUxNjIzOTAyMn0.NHVaYe26MbtOYhSKkoKYdFVomg4i8ZJd8_-RU8VNbftc4TSMb4bXP3l3YlNWACwyXPGffz5aXHc6lty1Y2t4SWRqGteragsVdZufDn5BlnJl9pdR_kdVFUsra2rWKEofkZeIC4yWytE58sMIihvo9H1ScmmVwBcQP6XETqYd0aSHp1gOa9RdUPDvoXQ5oqygTqVtxaDr6wUFKrKItgBMzWIdNZ6y7O9E0DhEPTbE9rfBo6KTFsHAZnMg4k68CDp2woYIaXbmYTWcvbzIuHO7_37GT79XdIwkm95QJ7hYC9RiwrV7mesbY4PAahERJawntho0my942XheVLmGwLMBkQ" - const accessToken = "llamas" +func newOidcTokenServer( + t *testing.T, + accessToken, oidcToken, path string, + expectedBody []byte, +) *httptest.Server { + t.Helper() - path := fmt.Sprintf("/jobs/%s/oidc/tokens", jobId) - server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + return httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { switch req.URL.Path { case path: if got, want := authToken(req), accessToken; got != want { @@ -28,14 +31,38 @@ func TestOidcToken(t *testing.T) { ) return } - rw.WriteHeader(http.StatusOK) - fmt.Fprint(rw, fmt.Sprintf(`{"token":"%s"}`, oidcToken)) + + body, err := io.ReadAll(req.Body) + if err != nil { + http.Error( + rw, + fmt.Sprintf(`{"message:"Internal Server Error: %q"}`, err), + http.StatusInternalServerError, + ) + return + } + + if !bytes.Equal(body, expectedBody) { + t.Errorf("wanted = %q, got = %q", expectedBody, body) + http.Error( + rw, + fmt.Sprintf( + `{"message:"Bad Request: wanted = %q, got = %q"}`, + expectedBody, + body, + ), + http.StatusBadRequest, + ) + return + } + + io.WriteString(rw, fmt.Sprintf(`{"token":"%s"}`, oidcToken)) default: http.Error( rw, fmt.Sprintf( - `{"message": "not found; method = %q, path = %q"}`, + `{"message":"Not Found; method = %q, path = %q"}`, req.Method, req.URL.Path, ), @@ -43,46 +70,67 @@ func TestOidcToken(t *testing.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, - }) +func TestOidcToken(t *testing.T) { + const jobId = "b078e2d2-86e9-4c12-bf3b-612a8058d0a4" + const oidcToken = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWUsImlhdCI6MTUxNjIzOTAyMn0.NHVaYe26MbtOYhSKkoKYdFVomg4i8ZJd8_-RU8VNbftc4TSMb4bXP3l3YlNWACwyXPGffz5aXHc6lty1Y2t4SWRqGteragsVdZufDn5BlnJl9pdR_kdVFUsra2rWKEofkZeIC4yWytE58sMIihvo9H1ScmmVwBcQP6XETqYd0aSHp1gOa9RdUPDvoXQ5oqygTqVtxaDr6wUFKrKItgBMzWIdNZ6y7O9E0DhEPTbE9rfBo6KTFsHAZnMg4k68CDp2woYIaXbmYTWcvbzIuHO7_37GT79XdIwkm95QJ7hYC9RiwrV7mesbY4PAahERJawntho0my942XheVLmGwLMBkQ" + const accessToken = "llamas" for _, testData := range []struct { - JobId string - AccessToken string - OidcToken *api.OidcToken - Audience []string - Error error + JobId string + AccessToken string + Audience []string + ExpectedBody []byte + OidcToken *api.OidcToken + Error error }{ { - JobId: jobId, - AccessToken: accessToken, - OidcToken: &api.OidcToken{Token: oidcToken}, - Audience: []string{}, + JobId: jobId, + AccessToken: accessToken, + Audience: []string{}, + ExpectedBody: []byte("{}\n"), + OidcToken: &api.OidcToken{Token: oidcToken}, }, { JobId: jobId, AccessToken: accessToken, - OidcToken: &api.OidcToken{Token: oidcToken}, Audience: []string{"sts.amazonaws.com"}, + ExpectedBody: []byte(`{"audience":"sts.amazonaws.com"} +`), + OidcToken: &api.OidcToken{Token: oidcToken}, }, { JobId: jobId, AccessToken: accessToken, Audience: []string{"sts.amazonaws.com", "buildkite.com"}, + OidcToken: &api.OidcToken{Token: oidcToken}, Error: api.ErrAudienceTooLong, }, } { + path := fmt.Sprintf("/jobs/%s/oidc/tokens", testData.JobId) + + server := newOidcTokenServer( + t, + testData.AccessToken, + testData.OidcToken.Token, + path, + testData.ExpectedBody, + ) + 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, + }) + if token, resp, err := client.OidcToken(testData.JobId, testData.Audience...); err != nil { if !errors.Is(err, testData.Error) { t.Fatalf( - "OidcToken(%v, %v) got error = %# v, want error = %# v", + "OidcToken(%v, %v) got error = %v, want error = %v", testData.JobId, testData.Audience, err, @@ -90,9 +138,9 @@ func TestOidcToken(t *testing.T) { ) } } else if token.Token != oidcToken { - t.Fatalf("OidcToken(%v, %v) got token = %# v, want %# v", testData.JobId, testData.Audience, token, testData.OidcToken) + t.Fatalf("OidcToken(%v, %v) got token = %v, want %v", testData.JobId, testData.Audience, token, testData.OidcToken) } else if resp.StatusCode != http.StatusOK { - t.Fatalf("OidcToken(%v, %v) got StatusCode = %# v, want %# v", testData.JobId, testData.Audience, resp.StatusCode, http.StatusOK) + t.Fatalf("OidcToken(%v, %v) got StatusCode = %v, want %v", testData.JobId, testData.Audience, resp.StatusCode, http.StatusOK) } } } From 2c5170b1f5b6941a1f924ded99b178374c9baa42 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Fri, 11 Nov 2022 00:26:12 +1100 Subject: [PATCH 17/37] Add skipping of retry for 400 errors --- clicommand/oidc_token.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clicommand/oidc_token.go b/clicommand/oidc_token.go index 9f7d0dfa8f..611faa0d10 100644 --- a/clicommand/oidc_token.go +++ b/clicommand/oidc_token.go @@ -123,7 +123,7 @@ var OidcTokenCommand = cli.Command{ switch resp.StatusCode { // Don't bother retrying if the response was one of these statuses - case http.StatusUnauthorized, http.StatusForbidden, http.StatusUnprocessableEntity: + case http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden, http.StatusUnprocessableEntity: r.Break() return err } From 778010d4b84890c2a62fe9ec98015f80f0292922 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Fri, 11 Nov 2022 00:41:42 +1100 Subject: [PATCH 18/37] Remove a debug print --- clicommand/oidc_token.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/clicommand/oidc_token.go b/clicommand/oidc_token.go index 611faa0d10..32560f7799 100644 --- a/clicommand/oidc_token.go +++ b/clicommand/oidc_token.go @@ -119,8 +119,6 @@ var OidcTokenCommand = cli.Command{ token, resp, err = client.OidcToken(cfg.Job, audience...) if resp != nil { - fmt.Println(resp.StatusCode, r.ShouldGiveUp()) - switch resp.StatusCode { // Don't bother retrying if the response was one of these statuses case http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden, http.StatusUnprocessableEntity: From f2ebe2b31402febaae4869c660963cedf77f3de3 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Fri, 11 Nov 2022 12:22:03 +1100 Subject: [PATCH 19/37] Update main.go Co-authored-by: Paul Annesley --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index c401fa36bc..f56780ad26 100644 --- a/main.go +++ b/main.go @@ -92,7 +92,7 @@ func main() { }, { Name: "oidc", - Usage: "Interact with Buildkite OIDC", + Usage: "Interact with Buildkite OpenID Connect (OIDC)", Subcommands: []cli.Command{ clicommand.OidcTokenCommand, }, From 918ca9b47b8b680870cd0b48cd313bc50404803f Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Fri, 11 Nov 2022 12:04:27 +1100 Subject: [PATCH 20/37] Fix up help text --- clicommand/oidc_token.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/clicommand/oidc_token.go b/clicommand/oidc_token.go index 32560f7799..6e278990c3 100644 --- a/clicommand/oidc_token.go +++ b/clicommand/oidc_token.go @@ -36,14 +36,15 @@ const ( buildkite-agent oidc token [options...] Description: - Requests and prints an OIDC token from Buildkite that claims the Job ID and - the specified audience. If no audience is specified, the endpoint's default - audience will be claimed. + Requests and prints an OIDC token from Buildkite that claims the Job ID + (amongst other things) and the specified audience. If no audience is + specified, the endpoint's default audience will be claimed. Example: $ buildkite-agent oidc token --audience sts.amazonaws.com - Prints the environment passed into the process + Requests and prints an OIDC token from Buildkite that claims the Job ID + (amongst other things) and the audience "sts.amazonaws.com". ` backoffSeconds = 2 maxAttempts = 5 From 5a10ef15826213dca5d3f0d8517728b16a97a1cd Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Fri, 11 Nov 2022 14:06:26 +1100 Subject: [PATCH 21/37] Use a struct as the only argument to the OidcTokenMethod This is more extensible. --- api/oidc.go | 32 +++++++++++------------------ api/oidc_test.go | 44 +++++++++++++++++----------------------- clicommand/oidc_token.go | 8 +++++--- 3 files changed, 36 insertions(+), 48 deletions(-) diff --git a/api/oidc.go b/api/oidc.go index 02667983ea..6671461e05 100644 --- a/api/oidc.go +++ b/api/oidc.go @@ -1,40 +1,32 @@ package api import ( - "errors" "fmt" ) -var ErrAudienceTooLong = errors.New("the API only supports at most one element in the audience") - type OidcToken struct { Token string `json:"token"` } -func (c *Client) OidcToken(jobId string, audience ...string) (*OidcToken, *Response, error) { - type oidcTokenRequest struct { - Audience string `json:"audience,omitempty"` - } +type OidcTokenRequest struct { + JobId string + Audience string +} - m := &oidcTokenRequest{} - switch len(audience) { - case 0: - case 1: - m.Audience = audience[0] - default: - // While the spec supports multiple audiences in an Id JWT, our API does - // not support issuing them. - // See: https://openid.net/specs/openid-connect-core-1_0.html#IDToken. - return nil, nil, ErrAudienceTooLong +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", jobId) - req, err := c.newRequest("POST", u, m) + u := fmt.Sprintf("jobs/%s/oidc/tokens", methodReq.JobId) + httpReq, err := c.newRequest("POST", u, m) if err != nil { return nil, nil, err } t := &OidcToken{} - resp, err := c.doRequest(req, t) + resp, err := c.doRequest(httpReq, t) return t, resp, err } diff --git a/api/oidc_test.go b/api/oidc_test.go index dc885e34fe..6abc23d6ae 100644 --- a/api/oidc_test.go +++ b/api/oidc_test.go @@ -78,37 +78,32 @@ func TestOidcToken(t *testing.T) { const accessToken = "llamas" for _, testData := range []struct { - JobId string - AccessToken string - Audience []string - ExpectedBody []byte - OidcToken *api.OidcToken - Error error + OidcTokenRequest *api.OidcTokenRequest + AccessToken string + ExpectedBody []byte + OidcToken *api.OidcToken + Error error }{ { - JobId: jobId, - AccessToken: accessToken, - Audience: []string{}, + AccessToken: accessToken, + OidcTokenRequest: &api.OidcTokenRequest{ + JobId: jobId, + }, ExpectedBody: []byte("{}\n"), OidcToken: &api.OidcToken{Token: oidcToken}, }, { - JobId: jobId, AccessToken: accessToken, - Audience: []string{"sts.amazonaws.com"}, + OidcTokenRequest: &api.OidcTokenRequest{ + JobId: jobId, + Audience: "sts.amazonaws.com", + }, ExpectedBody: []byte(`{"audience":"sts.amazonaws.com"} `), OidcToken: &api.OidcToken{Token: oidcToken}, }, - { - JobId: jobId, - AccessToken: accessToken, - Audience: []string{"sts.amazonaws.com", "buildkite.com"}, - OidcToken: &api.OidcToken{Token: oidcToken}, - Error: api.ErrAudienceTooLong, - }, } { - path := fmt.Sprintf("/jobs/%s/oidc/tokens", testData.JobId) + path := fmt.Sprintf("/jobs/%s/oidc/tokens", testData.OidcTokenRequest.JobId) server := newOidcTokenServer( t, @@ -127,20 +122,19 @@ func TestOidcToken(t *testing.T) { DebugHTTP: true, }) - if token, resp, err := client.OidcToken(testData.JobId, testData.Audience...); err != nil { + if token, resp, err := client.OidcToken(testData.OidcTokenRequest); err != nil { if !errors.Is(err, testData.Error) { t.Fatalf( - "OidcToken(%v, %v) got error = %v, want error = %v", - testData.JobId, - testData.Audience, + "OidcToken(%v) got error = %v, want error = %v", + testData.OidcTokenRequest, err, testData.Error, ) } } else if token.Token != oidcToken { - t.Fatalf("OidcToken(%v, %v) got token = %v, want %v", testData.JobId, testData.Audience, token, testData.OidcToken) + t.Fatalf("OidcToken(%v) got token = %v, want %v", testData.OidcTokenRequest, token, testData.OidcToken) } else if resp.StatusCode != http.StatusOK { - t.Fatalf("OidcToken(%v, %v) got StatusCode = %v, want %v", testData.JobId, testData.Audience, resp.StatusCode, http.StatusOK) + t.Fatalf("OidcToken(%v) got StatusCode = %v, want %v", testData.OidcTokenRequest, resp.StatusCode, http.StatusOK) } } } diff --git a/clicommand/oidc_token.go b/clicommand/oidc_token.go index 6e278990c3..704f9f6e26 100644 --- a/clicommand/oidc_token.go +++ b/clicommand/oidc_token.go @@ -113,12 +113,14 @@ var OidcTokenCommand = cli.Command{ roko.WithMaxAttempts(maxAttempts), roko.WithStrategy(roko.Exponential(backoffSeconds*time.Second, 0)), ).Do(func(r *roko.Retrier) error { - var audience []string + req := &api.OidcTokenRequest{ + JobId: cfg.Job, + } if len(cfg.Audience) > 0 { - audience = []string{cfg.Audience} + req.Audience = cfg.Audience } - token, resp, err = client.OidcToken(cfg.Job, audience...) + token, resp, err = client.OidcToken(req) if resp != nil { switch resp.StatusCode { // Don't bother retrying if the response was one of these statuses From b265e4a90829198bbe7d741fd5a0c92f79b680b2 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Fri, 11 Nov 2022 14:30:19 +1100 Subject: [PATCH 22/37] Fix test does not close server until after all iteration --- api/oidc_test.go | 60 +++++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 29 deletions(-) diff --git a/api/oidc_test.go b/api/oidc_test.go index 6abc23d6ae..443f9f7733 100644 --- a/api/oidc_test.go +++ b/api/oidc_test.go @@ -103,38 +103,40 @@ func TestOidcToken(t *testing.T) { OidcToken: &api.OidcToken{Token: oidcToken}, }, } { - path := fmt.Sprintf("/jobs/%s/oidc/tokens", testData.OidcTokenRequest.JobId) + func() { // this exists to allow closing the server on each iteration + path := fmt.Sprintf("/jobs/%s/oidc/tokens", testData.OidcTokenRequest.JobId) - server := newOidcTokenServer( - t, - testData.AccessToken, - testData.OidcToken.Token, - path, - testData.ExpectedBody, - ) - defer server.Close() + server := newOidcTokenServer( + t, + testData.AccessToken, + testData.OidcToken.Token, + path, + testData.ExpectedBody, + ) + 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, - }) + // Initial client with a registration token + client := api.NewClient(logger.Discard, api.Config{ + UserAgent: "Test", + Endpoint: server.URL, + Token: accessToken, + DebugHTTP: true, + }) - if token, resp, err := client.OidcToken(testData.OidcTokenRequest); err != nil { - if !errors.Is(err, testData.Error) { - t.Fatalf( - "OidcToken(%v) got error = %v, want error = %v", - testData.OidcTokenRequest, - err, - testData.Error, - ) + if token, resp, err := client.OidcToken(testData.OidcTokenRequest); err != nil { + if !errors.Is(err, testData.Error) { + t.Fatalf( + "OidcToken(%v) got error = %v, want error = %v", + testData.OidcTokenRequest, + err, + testData.Error, + ) + } + } else if token.Token != oidcToken { + t.Fatalf("OidcToken(%v) got token = %v, want %v", testData.OidcTokenRequest, token, testData.OidcToken) + } else if resp.StatusCode != http.StatusOK { + t.Fatalf("OidcToken(%v) got StatusCode = %v, want %v", testData.OidcTokenRequest, resp.StatusCode, http.StatusOK) } - } else if token.Token != oidcToken { - t.Fatalf("OidcToken(%v) got token = %v, want %v", testData.OidcTokenRequest, token, testData.OidcToken) - } else if resp.StatusCode != http.StatusOK { - t.Fatalf("OidcToken(%v) got StatusCode = %v, want %v", testData.OidcTokenRequest, resp.StatusCode, http.StatusOK) - } + }() } } From 49065c0905165a63dc0554b514aa03129f69a290 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Fri, 11 Nov 2022 14:40:39 +1100 Subject: [PATCH 23/37] Use cmp.Equals As per https://github.com/golang/go/wiki/TestComments#equality-comparison-and-diffs --- api/oidc_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/oidc_test.go b/api/oidc_test.go index 443f9f7733..9dd978dc66 100644 --- a/api/oidc_test.go +++ b/api/oidc_test.go @@ -11,6 +11,7 @@ import ( "github.com/buildkite/agent/v3/api" "github.com/buildkite/agent/v3/logger" + "github.com/google/go-cmp/cmp" ) func newOidcTokenServer( @@ -132,7 +133,7 @@ func TestOidcToken(t *testing.T) { testData.Error, ) } - } else if token.Token != oidcToken { + } else if !cmp.Equal(token, testData.OidcToken) { t.Fatalf("OidcToken(%v) got token = %v, want %v", testData.OidcTokenRequest, token, testData.OidcToken) } else if resp.StatusCode != http.StatusOK { t.Fatalf("OidcToken(%v) got StatusCode = %v, want %v", testData.OidcTokenRequest, resp.StatusCode, http.StatusOK) From 705e40f119729a745703cf5ddb2e87193a2130d0 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Fri, 11 Nov 2022 14:47:13 +1100 Subject: [PATCH 24/37] Change subcommand name to request-token --- clicommand/oidc_token.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clicommand/oidc_token.go b/clicommand/oidc_token.go index 704f9f6e26..73a6dc5bd5 100644 --- a/clicommand/oidc_token.go +++ b/clicommand/oidc_token.go @@ -51,7 +51,7 @@ Example: ) var OidcTokenCommand = cli.Command{ - Name: "token", + Name: "request-token", Usage: "Requests and prints an OIDC token from Buildkite with the specified audience,", Description: oidcTokenDescription, Flags: []cli.Flag{ From 863e57244e9a2a04725cba4c85981f700d712e09 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Fri, 11 Nov 2022 15:35:00 +1100 Subject: [PATCH 25/37] Change sub-command to request-token --- clicommand/{oidc_token.go => odic_request_token.go} | 6 +++--- main.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) rename clicommand/{oidc_token.go => odic_request_token.go} (95%) diff --git a/clicommand/oidc_token.go b/clicommand/odic_request_token.go similarity index 95% rename from clicommand/oidc_token.go rename to clicommand/odic_request_token.go index 73a6dc5bd5..0293fd57fa 100644 --- a/clicommand/oidc_token.go +++ b/clicommand/odic_request_token.go @@ -33,7 +33,7 @@ type OidcTokenConfig struct { const ( oidcTokenDescription = `Usage: - buildkite-agent oidc token [options...] + buildkite-agent oidc request-token [options...] Description: Requests and prints an OIDC token from Buildkite that claims the Job ID @@ -41,7 +41,7 @@ Description: specified, the endpoint's default audience will be claimed. Example: - $ buildkite-agent oidc token --audience sts.amazonaws.com + $ buildkite-agent oidc request-token --audience sts.amazonaws.com Requests and prints an OIDC token from Buildkite that claims the Job ID (amongst other things) and the audience "sts.amazonaws.com". @@ -50,7 +50,7 @@ Example: maxAttempts = 5 ) -var OidcTokenCommand = cli.Command{ +var OidcRequestTokenCommand = cli.Command{ Name: "request-token", Usage: "Requests and prints an OIDC token from Buildkite with the specified audience,", Description: oidcTokenDescription, diff --git a/main.go b/main.go index c401fa36bc..013f11507e 100644 --- a/main.go +++ b/main.go @@ -94,7 +94,7 @@ func main() { Name: "oidc", Usage: "Interact with Buildkite OIDC", Subcommands: []cli.Command{ - clicommand.OidcTokenCommand, + clicommand.OidcRequestTokenCommand, }, }, { From 1a9d87e123f3b2c07c86d763a05e870a6395da0c Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Sun, 13 Nov 2022 23:54:11 +1100 Subject: [PATCH 26/37] Refactor oidc to take advantage of omitempty --- clicommand/odic_request_token.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/clicommand/odic_request_token.go b/clicommand/odic_request_token.go index 0293fd57fa..39faa25ae5 100644 --- a/clicommand/odic_request_token.go +++ b/clicommand/odic_request_token.go @@ -114,10 +114,8 @@ var OidcRequestTokenCommand = cli.Command{ roko.WithStrategy(roko.Exponential(backoffSeconds*time.Second, 0)), ).Do(func(r *roko.Retrier) error { req := &api.OidcTokenRequest{ - JobId: cfg.Job, - } - if len(cfg.Audience) > 0 { - req.Audience = cfg.Audience + JobId: cfg.Job, + Audience: cfg.Audience, } token, resp, err = client.OidcToken(req) From 9395a9e5d9e841108b8c613c41c241394f208049 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Mon, 14 Nov 2022 13:16:03 +1100 Subject: [PATCH 27/37] Capitalise OIDC initialism As per https://github.com/golang/go/wiki/CodeReviewComments#initialisms --- api/oidc.go | 8 ++++---- api/oidc_test.go | 34 ++++++++++++++++---------------- clicommand/odic_request_token.go | 12 +++++------ main.go | 2 +- 4 files changed, 28 insertions(+), 28 deletions(-) diff --git a/api/oidc.go b/api/oidc.go index 6671461e05..055eaf294a 100644 --- a/api/oidc.go +++ b/api/oidc.go @@ -4,16 +4,16 @@ import ( "fmt" ) -type OidcToken struct { +type OIDCToken struct { Token string `json:"token"` } -type OidcTokenRequest struct { +type OIDCTokenRequest struct { JobId string Audience string } -func (c *Client) OidcToken(methodReq *OidcTokenRequest) (*OidcToken, *Response, error) { +func (c *Client) OIDCToken(methodReq *OIDCTokenRequest) (*OIDCToken, *Response, error) { m := &struct { Audience string `json:"audience,omitempty"` }{ @@ -26,7 +26,7 @@ func (c *Client) OidcToken(methodReq *OidcTokenRequest) (*OidcToken, *Response, return nil, nil, err } - t := &OidcToken{} + t := &OIDCToken{} resp, err := c.doRequest(httpReq, t) return t, resp, err } diff --git a/api/oidc_test.go b/api/oidc_test.go index 9dd978dc66..6c0ce3d8f2 100644 --- a/api/oidc_test.go +++ b/api/oidc_test.go @@ -14,7 +14,7 @@ import ( "github.com/google/go-cmp/cmp" ) -func newOidcTokenServer( +func newOIDCTokenServer( t *testing.T, accessToken, oidcToken, path string, expectedBody []byte, @@ -73,44 +73,44 @@ func newOidcTokenServer( })) } -func TestOidcToken(t *testing.T) { +func TestOIDCToken(t *testing.T) { const jobId = "b078e2d2-86e9-4c12-bf3b-612a8058d0a4" const oidcToken = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWUsImlhdCI6MTUxNjIzOTAyMn0.NHVaYe26MbtOYhSKkoKYdFVomg4i8ZJd8_-RU8VNbftc4TSMb4bXP3l3YlNWACwyXPGffz5aXHc6lty1Y2t4SWRqGteragsVdZufDn5BlnJl9pdR_kdVFUsra2rWKEofkZeIC4yWytE58sMIihvo9H1ScmmVwBcQP6XETqYd0aSHp1gOa9RdUPDvoXQ5oqygTqVtxaDr6wUFKrKItgBMzWIdNZ6y7O9E0DhEPTbE9rfBo6KTFsHAZnMg4k68CDp2woYIaXbmYTWcvbzIuHO7_37GT79XdIwkm95QJ7hYC9RiwrV7mesbY4PAahERJawntho0my942XheVLmGwLMBkQ" const accessToken = "llamas" for _, testData := range []struct { - OidcTokenRequest *api.OidcTokenRequest + OIDCTokenRequest *api.OIDCTokenRequest AccessToken string ExpectedBody []byte - OidcToken *api.OidcToken + OIDCToken *api.OIDCToken Error error }{ { AccessToken: accessToken, - OidcTokenRequest: &api.OidcTokenRequest{ + OIDCTokenRequest: &api.OIDCTokenRequest{ JobId: jobId, }, ExpectedBody: []byte("{}\n"), - OidcToken: &api.OidcToken{Token: oidcToken}, + OIDCToken: &api.OIDCToken{Token: oidcToken}, }, { AccessToken: accessToken, - OidcTokenRequest: &api.OidcTokenRequest{ + OIDCTokenRequest: &api.OIDCTokenRequest{ JobId: jobId, Audience: "sts.amazonaws.com", }, ExpectedBody: []byte(`{"audience":"sts.amazonaws.com"} `), - OidcToken: &api.OidcToken{Token: oidcToken}, + OIDCToken: &api.OIDCToken{Token: oidcToken}, }, } { func() { // this exists to allow closing the server on each iteration - path := fmt.Sprintf("/jobs/%s/oidc/tokens", testData.OidcTokenRequest.JobId) + path := fmt.Sprintf("/jobs/%s/oidc/tokens", testData.OIDCTokenRequest.JobId) - server := newOidcTokenServer( + server := newOIDCTokenServer( t, testData.AccessToken, - testData.OidcToken.Token, + testData.OIDCToken.Token, path, testData.ExpectedBody, ) @@ -124,19 +124,19 @@ func TestOidcToken(t *testing.T) { DebugHTTP: true, }) - if token, resp, err := client.OidcToken(testData.OidcTokenRequest); err != nil { + if token, resp, err := client.OIDCToken(testData.OIDCTokenRequest); err != nil { if !errors.Is(err, testData.Error) { t.Fatalf( - "OidcToken(%v) got error = %v, want error = %v", - testData.OidcTokenRequest, + "OIDCToken(%v) got error = %v, want error = %v", + testData.OIDCTokenRequest, err, testData.Error, ) } - } else if !cmp.Equal(token, testData.OidcToken) { - t.Fatalf("OidcToken(%v) got token = %v, want %v", testData.OidcTokenRequest, token, testData.OidcToken) + } else if !cmp.Equal(token, testData.OIDCToken) { + t.Fatalf("OIDCToken(%v) got token = %v, want %v", testData.OIDCTokenRequest, token, testData.OIDCToken) } else if resp.StatusCode != http.StatusOK { - t.Fatalf("OidcToken(%v) got StatusCode = %v, want %v", testData.OidcTokenRequest, resp.StatusCode, http.StatusOK) + t.Fatalf("OIDCToken(%v) got StatusCode = %v, want %v", testData.OIDCTokenRequest, resp.StatusCode, http.StatusOK) } }() } diff --git a/clicommand/odic_request_token.go b/clicommand/odic_request_token.go index 39faa25ae5..7c24119a4f 100644 --- a/clicommand/odic_request_token.go +++ b/clicommand/odic_request_token.go @@ -12,7 +12,7 @@ import ( "github.com/urfave/cli" ) -type OidcTokenConfig struct { +type OIDCTokenConfig struct { Audience string `cli:"audience"` Job string `cli:"job" validate:"required"` @@ -50,7 +50,7 @@ Example: maxAttempts = 5 ) -var OidcRequestTokenCommand = cli.Command{ +var OIDCRequestTokenCommand = cli.Command{ Name: "request-token", Usage: "Requests and prints an OIDC token from Buildkite with the specified audience,", Description: oidcTokenDescription, @@ -82,7 +82,7 @@ var OidcRequestTokenCommand = cli.Command{ }, Action: func(c *cli.Context) error { // The configuration will be loaded into this struct - cfg := OidcTokenConfig{} + cfg := OIDCTokenConfig{} loader := cliconfig.Loader{CLI: c, Config: &cfg} warnings, err := loader.Load() @@ -106,19 +106,19 @@ var OidcRequestTokenCommand = cli.Command{ client := api.NewClient(l, loadAPIClientConfig(cfg, "AgentAccessToken")) // Find the meta data value - var token *api.OidcToken + var token *api.OIDCToken var resp *api.Response if err := roko.NewRetrier( roko.WithMaxAttempts(maxAttempts), roko.WithStrategy(roko.Exponential(backoffSeconds*time.Second, 0)), ).Do(func(r *roko.Retrier) error { - req := &api.OidcTokenRequest{ + req := &api.OIDCTokenRequest{ JobId: cfg.Job, Audience: cfg.Audience, } - token, resp, err = client.OidcToken(req) + token, resp, err = client.OIDCToken(req) if resp != nil { switch resp.StatusCode { // Don't bother retrying if the response was one of these statuses diff --git a/main.go b/main.go index 8afb2bcd75..f93ecf3442 100644 --- a/main.go +++ b/main.go @@ -94,7 +94,7 @@ func main() { Name: "oidc", Usage: "Interact with Buildkite OpenID Connect (OIDC)", Subcommands: []cli.Command{ - clicommand.OidcRequestTokenCommand, + clicommand.OIDCRequestTokenCommand, }, }, { From 5ddef488663eb7557a0a4e1a2c5374cbd7a2ef0c Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Mon, 14 Nov 2022 13:39:23 +1100 Subject: [PATCH 28/37] Change OIDCToken method to not return a nil pointer in case of error --- api/oidc.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/api/oidc.go b/api/oidc.go index 055eaf294a..d3c74fbb65 100644 --- a/api/oidc.go +++ b/api/oidc.go @@ -28,5 +28,9 @@ func (c *Client) OIDCToken(methodReq *OIDCTokenRequest) (*OIDCToken, *Response, t := &OIDCToken{} resp, err := c.doRequest(httpReq, t) - return t, resp, err + if err != nil { + return nil, resp, err + } + + return t, resp, nil } From fb2dca55e1ae13d631bf6369ead26f1d6546b385 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Mon, 14 Nov 2022 13:46:49 +1100 Subject: [PATCH 29/37] Fix name of a file --- clicommand/{odic_request_token.go => oidc_request_token.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename clicommand/{odic_request_token.go => oidc_request_token.go} (100%) diff --git a/clicommand/odic_request_token.go b/clicommand/oidc_request_token.go similarity index 100% rename from clicommand/odic_request_token.go rename to clicommand/oidc_request_token.go From 82dc5fe25d2b15f6bc853d5d9f9e58503ba1935f Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Mon, 14 Nov 2022 13:56:15 +1100 Subject: [PATCH 30/37] Change test to declare table separately from iteration --- api/oidc_test.go | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/api/oidc_test.go b/api/oidc_test.go index 6c0ce3d8f2..c4a4e6fe72 100644 --- a/api/oidc_test.go +++ b/api/oidc_test.go @@ -78,7 +78,7 @@ func TestOIDCToken(t *testing.T) { const oidcToken = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWUsImlhdCI6MTUxNjIzOTAyMn0.NHVaYe26MbtOYhSKkoKYdFVomg4i8ZJd8_-RU8VNbftc4TSMb4bXP3l3YlNWACwyXPGffz5aXHc6lty1Y2t4SWRqGteragsVdZufDn5BlnJl9pdR_kdVFUsra2rWKEofkZeIC4yWytE58sMIihvo9H1ScmmVwBcQP6XETqYd0aSHp1gOa9RdUPDvoXQ5oqygTqVtxaDr6wUFKrKItgBMzWIdNZ6y7O9E0DhEPTbE9rfBo6KTFsHAZnMg4k68CDp2woYIaXbmYTWcvbzIuHO7_37GT79XdIwkm95QJ7hYC9RiwrV7mesbY4PAahERJawntho0my942XheVLmGwLMBkQ" const accessToken = "llamas" - for _, testData := range []struct { + tests := []struct { OIDCTokenRequest *api.OIDCTokenRequest AccessToken string ExpectedBody []byte @@ -103,16 +103,18 @@ func TestOIDCToken(t *testing.T) { `), OIDCToken: &api.OIDCToken{Token: oidcToken}, }, - } { + } + + for _, test := range tests { func() { // this exists to allow closing the server on each iteration - path := fmt.Sprintf("/jobs/%s/oidc/tokens", testData.OIDCTokenRequest.JobId) + path := fmt.Sprintf("/jobs/%s/oidc/tokens", test.OIDCTokenRequest.JobId) server := newOIDCTokenServer( t, - testData.AccessToken, - testData.OIDCToken.Token, + test.AccessToken, + test.OIDCToken.Token, path, - testData.ExpectedBody, + test.ExpectedBody, ) defer server.Close() @@ -124,19 +126,19 @@ func TestOIDCToken(t *testing.T) { DebugHTTP: true, }) - if token, resp, err := client.OIDCToken(testData.OIDCTokenRequest); err != nil { - if !errors.Is(err, testData.Error) { + if token, resp, err := client.OIDCToken(test.OIDCTokenRequest); err != nil { + if !errors.Is(err, test.Error) { t.Fatalf( "OIDCToken(%v) got error = %v, want error = %v", - testData.OIDCTokenRequest, + test.OIDCTokenRequest, err, - testData.Error, + test.Error, ) } - } else if !cmp.Equal(token, testData.OIDCToken) { - t.Fatalf("OIDCToken(%v) got token = %v, want %v", testData.OIDCTokenRequest, token, testData.OIDCToken) + } else if !cmp.Equal(token, test.OIDCToken) { + t.Fatalf("OIDCToken(%v) got token = %v, want %v", test.OIDCTokenRequest, token, test.OIDCToken) } else if resp.StatusCode != http.StatusOK { - t.Fatalf("OIDCToken(%v) got StatusCode = %v, want %v", testData.OIDCTokenRequest, resp.StatusCode, http.StatusOK) + t.Fatalf("OIDCToken(%v) got StatusCode = %v, want %v", test.OIDCTokenRequest, resp.StatusCode, http.StatusOK) } }() } From e22417c631b3ee0b38d5c11b57a3db5b386165e4 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Mon, 14 Nov 2022 13:58:58 +1100 Subject: [PATCH 31/37] Change test to have single source of truth for expected audience --- api/oidc_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/api/oidc_test.go b/api/oidc_test.go index c4a4e6fe72..c950763383 100644 --- a/api/oidc_test.go +++ b/api/oidc_test.go @@ -77,6 +77,7 @@ func TestOIDCToken(t *testing.T) { const jobId = "b078e2d2-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 @@ -97,11 +98,10 @@ func TestOIDCToken(t *testing.T) { AccessToken: accessToken, OIDCTokenRequest: &api.OIDCTokenRequest{ JobId: jobId, - Audience: "sts.amazonaws.com", + Audience: audience, }, - ExpectedBody: []byte(`{"audience":"sts.amazonaws.com"} -`), - OIDCToken: &api.OIDCToken{Token: oidcToken}, + ExpectedBody: []byte(fmt.Sprintf(`{"audience":%q}`+"\n", audience)), + OIDCToken: &api.OIDCToken{Token: oidcToken}, }, } From a3418bbf1e70280d94ba6d208eb3da846cdc71fa Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Mon, 14 Nov 2022 14:25:31 +1100 Subject: [PATCH 32/37] Fix an overzealous use of else if and e.Fatal It's better to ensure that all conditions are checked by using t.Error instead of t.Fatal. Similarly, else if would exclude certain checks --- api/oidc_test.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/api/oidc_test.go b/api/oidc_test.go index c950763383..e579f2ab9c 100644 --- a/api/oidc_test.go +++ b/api/oidc_test.go @@ -126,19 +126,22 @@ func TestOIDCToken(t *testing.T) { DebugHTTP: true, }) - if token, resp, err := client.OIDCToken(test.OIDCTokenRequest); err != nil { + token, resp, err := client.OIDCToken(test.OIDCTokenRequest) + if err != nil { if !errors.Is(err, test.Error) { - t.Fatalf( + t.Errorf( "OIDCToken(%v) got error = %v, want error = %v", test.OIDCTokenRequest, err, test.Error, ) } - } else if !cmp.Equal(token, test.OIDCToken) { - t.Fatalf("OIDCToken(%v) got token = %v, want %v", test.OIDCTokenRequest, token, test.OIDCToken) - } else if resp.StatusCode != http.StatusOK { - t.Fatalf("OIDCToken(%v) got StatusCode = %v, want %v", test.OIDCTokenRequest, resp.StatusCode, http.StatusOK) + } + 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) } }() } From 0a03238ca4b54a926243847313e3c5edd1bca4f2 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Mon, 14 Nov 2022 14:39:31 +1100 Subject: [PATCH 33/37] Fix tests were failing too fast --- api/oidc_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/api/oidc_test.go b/api/oidc_test.go index e579f2ab9c..ba563a6339 100644 --- a/api/oidc_test.go +++ b/api/oidc_test.go @@ -128,18 +128,18 @@ func TestOIDCToken(t *testing.T) { token, resp, err := client.OIDCToken(test.OIDCTokenRequest) if err != nil { - if !errors.Is(err, test.Error) { - t.Errorf( - "OIDCToken(%v) got error = %v, want error = %v", - test.OIDCTokenRequest, - err, - test.Error, - ) - } + 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) } From 0e4e15ad600e1ebf28bbeaa27307ec30d945105c Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Mon, 14 Nov 2022 16:30:54 +1100 Subject: [PATCH 34/37] Rewrite tests to have separate test tables for success and error --- api/oidc_test.go | 169 ++++++++++++++++++++++++++++++++++++----------- 1 file changed, 129 insertions(+), 40 deletions(-) diff --git a/api/oidc_test.go b/api/oidc_test.go index ba563a6339..7b5aff15fd 100644 --- a/api/oidc_test.go +++ b/api/oidc_test.go @@ -2,7 +2,6 @@ package api_test import ( "bytes" - "errors" "fmt" "io" "net/http" @@ -14,50 +13,58 @@ import ( "github.com/google/go-cmp/cmp" ) -func newOIDCTokenServer( - t *testing.T, - accessToken, oidcToken, path string, - expectedBody []byte, -) *httptest.Server { - t.Helper() +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: - if got, want := authToken(req), accessToken; got != want { - http.Error( - rw, - fmt.Sprintf("authToken(req) = %q, want %q", got, want), - http.StatusUnauthorized, - ) - return - } - body, err := io.ReadAll(req.Body) if err != nil { http.Error( rw, - fmt.Sprintf(`{"message:"Internal Server Error: %q"}`, err), + fmt.Sprintf(`{"message:"Internal Server Error: %s"}`, err), http.StatusInternalServerError, ) return } - if !bytes.Equal(body, expectedBody) { - t.Errorf("wanted = %q, got = %q", expectedBody, body) + 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"}`, - expectedBody, - body, - ), + fmt.Sprintf(`{"message:"Bad Request: wanted = %q, got = %q"}`, s.expectedBody, body), http.StatusBadRequest, ) return } - io.WriteString(rw, fmt.Sprintf(`{"token":"%s"}`, oidcToken)) + 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( @@ -74,7 +81,8 @@ func newOIDCTokenServer( } func TestOIDCToken(t *testing.T) { - const jobId = "b078e2d2-86e9-4c12-bf3b-612a8058d0a4" + 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" @@ -84,12 +92,11 @@ func TestOIDCToken(t *testing.T) { AccessToken string ExpectedBody []byte OIDCToken *api.OIDCToken - Error error }{ { AccessToken: accessToken, OIDCTokenRequest: &api.OIDCTokenRequest{ - JobId: jobId, + JobId: jobID, }, ExpectedBody: []byte("{}\n"), OIDCToken: &api.OIDCToken{Token: oidcToken}, @@ -97,7 +104,7 @@ func TestOIDCToken(t *testing.T) { { AccessToken: accessToken, OIDCTokenRequest: &api.OIDCTokenRequest{ - JobId: jobId, + JobId: jobID, Audience: audience, }, ExpectedBody: []byte(fmt.Sprintf(`{"audience":%q}`+"\n", audience)), @@ -107,15 +114,13 @@ func TestOIDCToken(t *testing.T) { for _, test := range tests { func() { // this exists to allow closing the server on each iteration - path := fmt.Sprintf("/jobs/%s/oidc/tokens", test.OIDCTokenRequest.JobId) - - server := newOIDCTokenServer( - t, - test.AccessToken, - test.OIDCToken.Token, - path, - test.ExpectedBody, - ) + 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 @@ -137,11 +142,95 @@ func TestOIDCToken(t *testing.T) { } if !cmp.Equal(token, test.OIDCToken) { - t.Errorf("OIDCToken(%v) got token = %v, want %v", test.OIDCTokenRequest, 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) + 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{ + JobId: jobID, + Audience: audience, + }, + ExpectedStatus: http.StatusUnauthorized, + }, + { + AccessToken: accessToken, + OIDCTokenRequest: &api.OIDCTokenRequest{ + JobId: unauthorizedJobID, + Audience: audience, + }, + ExpectedStatus: http.StatusForbidden, + }, + { + AccessToken: accessToken, + OIDCTokenRequest: &api.OIDCTokenRequest{ + JobId: "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, + ) } }() } From bc55667d885d317095b1bc53e17fcca16f0df6ae Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Mon, 14 Nov 2022 16:50:24 +1100 Subject: [PATCH 35/37] Use interface to stderr instead of it directly --- api/oidc.go | 4 ++-- api/oidc_test.go | 10 +++++----- clicommand/oidc_request_token.go | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/api/oidc.go b/api/oidc.go index d3c74fbb65..2ef897c448 100644 --- a/api/oidc.go +++ b/api/oidc.go @@ -9,7 +9,7 @@ type OIDCToken struct { } type OIDCTokenRequest struct { - JobId string + Job string Audience string } @@ -20,7 +20,7 @@ func (c *Client) OIDCToken(methodReq *OIDCTokenRequest) (*OIDCToken, *Response, Audience: methodReq.Audience, } - u := fmt.Sprintf("jobs/%s/oidc/tokens", methodReq.JobId) + u := fmt.Sprintf("jobs/%s/oidc/tokens", methodReq.Job) httpReq, err := c.newRequest("POST", u, m) if err != nil { return nil, nil, err diff --git a/api/oidc_test.go b/api/oidc_test.go index 7b5aff15fd..cad3072ba1 100644 --- a/api/oidc_test.go +++ b/api/oidc_test.go @@ -96,7 +96,7 @@ func TestOIDCToken(t *testing.T) { { AccessToken: accessToken, OIDCTokenRequest: &api.OIDCTokenRequest{ - JobId: jobID, + Job: jobID, }, ExpectedBody: []byte("{}\n"), OIDCToken: &api.OIDCToken{Token: oidcToken}, @@ -104,7 +104,7 @@ func TestOIDCToken(t *testing.T) { { AccessToken: accessToken, OIDCTokenRequest: &api.OIDCTokenRequest{ - JobId: jobID, + Job: jobID, Audience: audience, }, ExpectedBody: []byte(fmt.Sprintf(`{"audience":%q}`+"\n", audience)), @@ -178,7 +178,7 @@ func TestOIDCTokenError(t *testing.T) { { AccessToken: "camels", OIDCTokenRequest: &api.OIDCTokenRequest{ - JobId: jobID, + Job: jobID, Audience: audience, }, ExpectedStatus: http.StatusUnauthorized, @@ -186,7 +186,7 @@ func TestOIDCTokenError(t *testing.T) { { AccessToken: accessToken, OIDCTokenRequest: &api.OIDCTokenRequest{ - JobId: unauthorizedJobID, + Job: unauthorizedJobID, Audience: audience, }, ExpectedStatus: http.StatusForbidden, @@ -194,7 +194,7 @@ func TestOIDCTokenError(t *testing.T) { { AccessToken: accessToken, OIDCTokenRequest: &api.OIDCTokenRequest{ - JobId: "2", + Job: "2", Audience: audience, }, ExpectedStatus: http.StatusNotFound, diff --git a/clicommand/oidc_request_token.go b/clicommand/oidc_request_token.go index 7c24119a4f..7adee572eb 100644 --- a/clicommand/oidc_request_token.go +++ b/clicommand/oidc_request_token.go @@ -87,7 +87,7 @@ var OIDCRequestTokenCommand = cli.Command{ loader := cliconfig.Loader{CLI: c, Config: &cfg} warnings, err := loader.Load() if err != nil { - fmt.Fprintf(os.Stderr, "%s\n", err) + fmt.Fprintf(c.App.ErrWriter, "%s\n", err) os.Exit(1) } @@ -114,7 +114,7 @@ var OIDCRequestTokenCommand = cli.Command{ roko.WithStrategy(roko.Exponential(backoffSeconds*time.Second, 0)), ).Do(func(r *roko.Retrier) error { req := &api.OIDCTokenRequest{ - JobId: cfg.Job, + Job: cfg.Job, Audience: cfg.Audience, } From 5b860fb91dad92494feaf1fa02b9e0eae411df63 Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Mon, 14 Nov 2022 17:33:52 +1100 Subject: [PATCH 36/37] Fix a comment --- clicommand/oidc_request_token.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clicommand/oidc_request_token.go b/clicommand/oidc_request_token.go index 7adee572eb..e78e3a3359 100644 --- a/clicommand/oidc_request_token.go +++ b/clicommand/oidc_request_token.go @@ -105,7 +105,7 @@ var OIDCRequestTokenCommand = cli.Command{ // Create the API client client := api.NewClient(l, loadAPIClientConfig(cfg, "AgentAccessToken")) - // Find the meta data value + // Request the token var token *api.OIDCToken var resp *api.Response From 15bc08d2be27f8850449a9b17cc933252e1961bd Mon Sep 17 00:00:00 2001 From: Narthana Epa Date: Mon, 14 Nov 2022 17:34:02 +1100 Subject: [PATCH 37/37] Scope the response to be within the retry loop --- clicommand/oidc_request_token.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clicommand/oidc_request_token.go b/clicommand/oidc_request_token.go index e78e3a3359..713e81c7a5 100644 --- a/clicommand/oidc_request_token.go +++ b/clicommand/oidc_request_token.go @@ -107,7 +107,6 @@ var OIDCRequestTokenCommand = cli.Command{ // Request the token var token *api.OIDCToken - var resp *api.Response if err := roko.NewRetrier( roko.WithMaxAttempts(maxAttempts), @@ -118,6 +117,7 @@ var OIDCRequestTokenCommand = cli.Command{ Audience: cfg.Audience, } + var resp *api.Response token, resp, err = client.OIDCToken(req) if resp != nil { switch resp.StatusCode {