Add `buildkite-agent oidc request-token` command #1827

triarius · 2022-11-10T06:15:04Z

I decided to have just 1 method abstracting the API call for when there is and is not an audience. ~~The way I chose to implement the method gives room to extend it if we decided Buildkite's API should allow multiple audiences.~~

Per the spec, aud can be EITHER an array or a string: https://openid.net/specs/openid-connect-core-1_0.html#IDToken. So it is certainly possible for us to make that choice in the future, even if we do not want to do that now. Indeed, there may be services that would require multiple audience claims in the single JWT that we may want to allow integration with. Moreover, I have seen aud be an array in the wild: https://community.auth0.com/t/audience-value-in-returned-bearer-token-jwt-is-a-list/6418.

However, as a note for if we do enable such support, we should ensure that the API issues a JWT with a string valued aud if there is only a single audience as some JWT libraries don't easily support validating JWTs with an array valued aud: dgrijalva/jwt-go#445 and so many implementations may break when using them.

triarius · 2022-11-10T06:34:56Z

api/client_test.go

@@ -1,4 +1,4 @@
-package api
+package api_test


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.

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

pda · 2022-11-11T00:19:45Z

api/oidc.go

+	switch len(audience) {
+	case 0:
+	case 1:
+		m.Audience = audience[0]


I initially read this as C-style switch fall-through, where cases 0 and 1 are handled the same, and that left me wondering why it didn't panic on audience[0] for an empty slice.

Maybe it's worth making it clearer that case 0: is a no-op?

(Alternatively, maybe the complexity of handling multiple audiences is YAGNI since we may never support that?)

I didn't consider it to be YAGNI as it's in the spec, and we don't control whether 3rd parties would read the spec and think it would be a good idea to require multiple audiences on a OIDC token they consume. Then our implementation would not work with such a service.

But this it not really the right place to have a debate about it, bk/bk needs to support it too. So I'll implement it with at most one audience in the agent for now.

clicommand/oidc_token.go

main.go

Co-authored-by: Paul Annesley <paul@annesley.cc>

This is more extensible.

As per https://github.com/golang/go/wiki/TestComments#equality-comparison-and-diffs

…ent into triarius/oidc-token-command

DrJosh9000

It's looking pretty good! Still, have some comments.

api/oidc.go

DrJosh9000 · 2022-11-13T23:08:55Z

api/oidc.go

+	}
+
+	t := &OidcToken{}
+	resp, err := c.doRequest(httpReq, t)


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.

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

agent/clicommand/meta_data_get.go

Lines 107 to 119 in 1a9d87e

).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.

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

DrJosh9000 · 2022-11-13T23:11:35Z

api/oidc_test.go

+	accessToken, oidcToken, path string,
+	expectedBody []byte,
+) *httptest.Server {
+	t.Helper()


Breaking function declarations across multiple lines feels weird to me. It's the sort of thing that leads to indentation-level confusion.
And many other Go folks think there's no problem with long lines anyway:
https://github.com/golang/go/wiki/CodeReviewComments#line-length

I think this line would be “uncomfortably long” if the function declaration were not wrapped. It's a lot easier to read a function declaration that has been intentionally wrapped for each argument than one that has been arbitrarily wrapped by the code editor. So I prefer to do it this way.

Okay - I won't hold you up with a debate around wrapping. It's not unreadable. I'll go away and think about it.

DrJosh9000 · 2022-11-13T23:12:11Z

api/oidc_test.go

+		switch req.URL.Path {
+		case path:
+			if got, want := authToken(req), accessToken; got != want {
+				http.Error(


Similarly here, and below - is it so uncomfortably long to put it on one line?

Given that it is already quite indented, I do think that this line would be uncomfortably long if it were not wrapped.

api/oidc_test.go

DrJosh9000 · 2022-11-13T23:29:04Z

clicommand/odic_request_token.go

+		ExperimentsFlag,
+		ProfileFlag,
+	},
+	Action: func(c *cli.Context) error {


Something that all the other commands do, that I would like to undo in most of them, is to change these Action closures into named top-level functions. A few of them "feel" too big and complicated to be an inline func, so it would pay to reduce the indentation level even by one tab.

I agree. However, I prefer to consistently follow a bad practice until we decide to explicitly adopt a better one.

clicommand/odic_request_token.go

DrJosh9000 · 2022-11-13T23:34:04Z

clicommand/odic_request_token.go

+		// Create the API client
+		client := api.NewClient(l, loadAPIClientConfig(cfg, "AgentAccessToken"))
+
+		// Find the meta data value


It would be a bit shorter to print the token within the retry func as soon as the request succeeded. (You could declare token, resp, err := client.OIDCToken(req) on 121.)

I think it's nicer to think of token as the value that the retrier produces. I removed resp as there is no reason for it to escape the retrier.

As per https://github.com/golang/go/wiki/CodeReviewComments#initialisms

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

DrJosh9000

LGTM 🥳

This is good. I think anything else I could comment would be nitpicking.

triarius commented Nov 10, 2022

View reviewed changes

triarius force-pushed the triarius/oidc-token-command branch from 9a8b6b8 to 3c55028 Compare November 10, 2022 06:45

triarius added 10 commits November 10, 2022 17:45

Create method to call API method :jobid/oidc/tokens

7c9e4dd

Create command for oidc test

2946a0f

Add support for 0 or 1 length audience

1c1b516

Fix http client does not require explict unmarshal

3766723

Convert api tests to be blackbox

53b4098

Add test for oidc token

892baee

Add table test for 0, 1 and 2 audiences

f947ad1

Factor out oidc token into test table

7f7cf5d

Make OidcTokenRequest a local struct

b11ea30

Add a comment about audience

d9f9fb1

triarius force-pushed the triarius/oidc-token-command branch from 3c55028 to d9f9fb1 Compare November 10, 2022 06:45

Use exp backoff

3a7e5dd

triarius force-pushed the triarius/oidc-token-command branch from c54ee95 to 23f0451 Compare November 10, 2022 07:06

Adjust help text and comments

52995c2

triarius force-pushed the triarius/oidc-token-command branch from 23f0451 to 52995c2 Compare November 10, 2022 07:08

triarius added 6 commits November 10, 2022 18:27

Fix handle error from roko

e6d7ff4

Fix retry on errors that should not trigger retry

fcbff6f

Test the request body created for Oidc Token API calls

8753f8a

Add skipping of retry for 400 errors

2c5170b

Remove a debug print

778010d

triarius requested a review from a team November 10, 2022 21:21

pda reviewed Nov 11, 2022

View reviewed changes

triarius and others added 5 commits November 11, 2022 12:22

Update main.go

f2ebe2b

Co-authored-by: Paul Annesley <paul@annesley.cc>

Fix up help text

918ca9b

Use a struct as the only argument to the OidcTokenMethod

5a10ef1

This is more extensible.

Fix test does not close server until after all iteration

b265e4a

Use cmp.Equals

49065c0

As per https://github.com/golang/go/wiki/TestComments#equality-comparison-and-diffs

triarius added 3 commits November 11, 2022 14:47

Change subcommand name to request-token

705e40f

Change sub-command to request-token

863e572

Merge branch 'triarius/oidc-token-command' of github.com:buildkite/ag…

3ec96cc

…ent into triarius/oidc-token-command

triarius requested a review from pda November 11, 2022 07:16

Refactor oidc to take advantage of omitempty

1a9d87e

DrJosh9000 reviewed Nov 13, 2022

View reviewed changes

pda changed the title ~~Add oidc token command to agent~~ Add buildkite-agent oidc request-token command Nov 14, 2022

triarius added 11 commits November 14, 2022 13:16

Capitalise OIDC initialism

9395a9e

As per https://github.com/golang/go/wiki/CodeReviewComments#initialisms

Change OIDCToken method to not return a nil pointer in case of error

5ddef48

Fix name of a file

fb2dca5

Change test to declare table separately from iteration

82dc5fe

Change test to have single source of truth for expected audience

e22417c

Fix an overzealous use of else if and e.Fatal

a3418bb

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

Fix tests were failing too fast

0a03238

Rewrite tests to have separate test tables for success and error

0e4e15a

Use interface to stderr instead of it directly

bc55667

Fix a comment

5b860fb

Scope the response to be within the retry loop

15bc08d

triarius force-pushed the triarius/oidc-token-command branch from 1191bd5 to 15bc08d Compare November 14, 2022 07:13

triarius requested a review from DrJosh9000 November 14, 2022 07:17

DrJosh9000 approved these changes Nov 14, 2022

View reviewed changes

triarius merged commit 7022d6f into main Nov 15, 2022

triarius deleted the triarius/oidc-token-command branch November 15, 2022 02:58

sj26 mentioned this pull request Nov 16, 2022

Add Buildkite to federation sigstore/fulcio#889

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `buildkite-agent oidc request-token` command #1827

Add `buildkite-agent oidc request-token` command #1827

triarius commented Nov 10, 2022 •

edited

Loading

triarius Nov 10, 2022

pda Nov 11, 2022

triarius Nov 11, 2022 •

edited

Loading

DrJosh9000 left a comment

DrJosh9000 Nov 13, 2022

triarius Nov 14, 2022

DrJosh9000 Nov 14, 2022

DrJosh9000 Nov 13, 2022

triarius Nov 14, 2022

DrJosh9000 Nov 14, 2022

DrJosh9000 Nov 13, 2022

triarius Nov 14, 2022

DrJosh9000 Nov 13, 2022

triarius Nov 14, 2022

DrJosh9000 Nov 13, 2022

triarius Nov 14, 2022

DrJosh9000 left a comment

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

	resp, err := c.doRequest(req, e)
	if err != nil {
	return nil, resp, err
	}

	return e, resp, err

	).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
	})

Add buildkite-agent oidc request-token command #1827

Add buildkite-agent oidc request-token command #1827

Conversation

triarius commented Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triarius Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

DrJosh9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrJosh9000 left a comment

Choose a reason for hiding this comment

Add `buildkite-agent oidc request-token` command #1827

Add `buildkite-agent oidc request-token` command #1827

triarius commented Nov 10, 2022 •

edited

Loading

triarius Nov 11, 2022 •

edited

Loading