-
Notifications
You must be signed in to change notification settings - Fork 118
action.yml,src: add support for workload identity federation #221
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
Conversation
|
TODO: setup tests using workload identity |
c64c251 to
c75f428
Compare
oxtoacart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM, just one question.
| authArgs.push(`--client-id=${config.oauthClientId}`); | ||
| authArgs.push(`--id-token=${token}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the behaviour if a user provides an audience + clientID but their Tailscale version is <1.90.x? Do we fail gracefully or they get a generic "arguments do not exist" message?
Is it worth being more defensive and checking the CLI version to surface a more obvious error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added a check here
action.yml
Outdated
| deprecationMessage: 'An OAuth API client https://tailscale.com/s/oauth-clients is recommended instead of an authkey' | ||
| oauth-client-id: | ||
| description: 'Your Tailscale OAuth Client ID.' | ||
| description: 'Your Tailscale OAuth or Workload Identity Federation Client ID.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I heard the new "type" of client referred to as Federated client instead of Workload Identity Federation client. i'm not sure if we have an official term yet but I have a slight preference for the shorter variation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the UI now refers to this concept as Trusted Credentials, so should that be Your OIDC trusted Credentials Client ID? Are we switching "client" to "trusted credentials" everywhere so people do not confuse them for separate concepts and resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're generally swapping "client" for "credentials" except for the ClientID itself given that this client_id in the API / for the spec, so potentially something like ...Federated Identity clientID or `...OIDC Federated Identity clientID would work here.
README.md
Outdated
| id-token: write # This is required for the tailscale action to request a JWT from GitHub | ||
| ``` | ||
|
|
||
| Federated identity credentials used for this purpose must have the [`auth_keys` scope.][kb-trust-credentials-scopes] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is implied because our scopes do not have :write variants, but we should find a way to be more explicit this is a write scope. There is an issue already that indicates not calling out this is a write scope is confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like this?
| Federated identity credentials used for this purpose must have the [`auth_keys` scope.][kb-trust-credentials-scopes] | |
| Federated identity credentials used for this purpose must have the writable [`auth_keys` scope.][kb-trust-credentials-scopes] |
README.md
Outdated
| ### requested tags [tag:mytag] are invalid or not permitted | ||
|
|
||
| You may encounter this error when using an OAuth client. OAuth clients must have the [`auth_keys` scope](https://tailscale.com/kb/1215/oauth-clients#scopes) with one or more [tags](https://tailscale.com/kb/1068/acl-tags/), and the tags specified with `tags` must match all tags on the OAuth client. | ||
| You may encounter this error when using an OAuth client. OAuth clients must have the [`auth_keys` scope][kb-trust-credentials-scopes] with one or more [tags][kb-tags], and the tags specified with `tags` must match all tags on the OAuth client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General thought unrelated to the PR:
If the tags passed on the Tailscale CLI command MUST match all tags configured on the client, why is it a configurable input? Is there a reason we cannot take care of this server-side and make the --tags argument optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah didn't catch this when adjusting / when the PR for the original change flew by. This can also be tags owned by the tags associated with the client.
E.g., assuming a policy file with:
"tagOwners": {
"tag:main": ["autogroup:admin", "autogroup:owner"],
"tag:test-tag": ["autogroup:admin", "autogroup:owner", "tag:main"],
"tag:test-tag-2": ["autogroup:admin", "autogroup:owner", "tag:main"],
}
and a trust credential that has tag:main applied, then --tags could be any of tag:main, tag:test-tag, or tag:test-tag-2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL tags can own tags
| on tailnets which use [Device Approval][kb-device-approval] | ||
|
|
||
|
|
||
| ### Workload identity federation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mark this as the preferred method? Potentially even use it for the default example starting on l.6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! Had been thinking that we might want to leave this as-is for beta, but I feel like there's no harm in swapping this now given the improved UX / security posture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine either way, I see your point that for the Beta phase there could be bugs or rough edges that make authkeys the preferable default for a little longer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will leave it as-is for now, can adjust later
| 1. At least one configured [tag][kb-tags]. | ||
| 1. An [OAuth client][kb-oauth-clients] ID and secret OR an [auth key][kb-auth-keys]. | ||
| 1. At least one configured [tag][kb-tags] if using OAuth or workload identity federation. | ||
| 1. An [OAuth client][kb-oauth-clients] ID and secret, [federated identity][kb-workload-identity-federation] client ID and audience, OR an [auth key][kb-auth-keys]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the KB have a section tailored to setup a federated client compatible with GitHub? If not how will people know what issuer URL, subject and audience they should use?
c75f428 to
e129d0b
Compare
src/main.ts
Outdated
|
|
||
| // Prepare auth and tags | ||
| let finalAuthKey = config.authKey; | ||
| const authArgs: string[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a test that verifies the priority order when arguments for multiple auth methods are provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even better might be to check that you only specify one, and if it's ambiguous, bail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Careful though because at the moment if someone passes both a client secret and an authkey the authkey has precedence, we bailing if both are provided is breaking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think we're at the point where we should look at adding proper unit testing of some of these functions / flows. Going to punt on that for now but have filed https://github.com/tailscale/corp/issues/33870 to track
962d174 to
45d945b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read through the PR again and that looks good to me. I trust you for any doc adjustments we may want to do based on the comments, and we can always adjust defensive checks and READMEs later.
45d945b to
653d5ed
Compare
| "@actions/github": "^6.0.1", | ||
| "@actions/tool-cache": "^2.0.1" | ||
| "@actions/tool-cache": "^2.0.1", | ||
| "semver": "^6.3.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were already transitively depending on this, explicitly added here to surface the dependency better
oxtoacart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I have just one comment about the priority order of authentication arguments that probably worth resolving.
src/main.ts
Outdated
|
|
||
| if (config.oauthSecret) { | ||
| finalAuthKey = `${config.oauthSecret}?preauthorized=true&ephemeral=true`; | ||
| if (config.authKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the priority order relative to the current v4, in which config.oauthSecret took priority over config.authKey. If we're going to do that, we might as well just add validation that ensures you've only specified one or the other, not both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops! Will flip back to oauthSecret having priority over the authKey for now.
Add support for workload identity federation based authentication. Updates tailscale/corp#31264 Signed-off-by: Mario Minardi <[email protected]>
653d5ed to
ef403f0
Compare
Add support for workload identity federation based authentication.
Updates https://github.com/tailscale/corp/issues/31264