Skip to content

feat(join-tokens): [44794] Add Join Token form for GitHub #54308

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

Merged
merged 37 commits into from
May 1, 2025

Conversation

nicholasmarais1158
Copy link
Contributor

@nicholasmarais1158 nicholasmarais1158 commented Apr 25, 2025

Overview

It's currently not possible to create a new Join Token for GitHub via the Join Tokens page (/web/tokens) - it can however, be done by creating a Bot linked to GitHub. This change adds a form for creating and editing GitHub join tokens.

Issue: #44794

Depends on: #54374

changelog: Create and edit GitHub join tokens from the Join Tokens page

Changes

  • Add form for create/edit
  • Handle unsupported fields during edit - this prevents any custom/unsupported config being silently overwritten when editing a token.
  • Fix spacing on side panel - remove double spacing on right of forms
  • Fix/silence a few lint issues

Demo

Screen.Recording.2025-04-28.at.17.49.23.mov

export const checkIAMYAMLData = (data: unknown) => {
const keys = collectKeys(data);
return (
!keys || new Set(keys).isSubsetOf(new Set(['.aws_account', '.aws_arn']))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think here and line 267 should match how you're doing it elsewhere, i.e. the static set is assigned to a variable - makes it more readable

return !keys || new Set(keys).isSubsetOf(supportedFields)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in c2d0292.

Comment on lines 199 to 254
<Text fontWeight={700} mt={5}>
GHES configuration
</Text>

<Text fontWeight="regular" mb={2}>
Additional settings for configuring GitHub Enterprise Server.
</Text>

<FieldInput
label="Server host"
placeholder="github.example.com"
value={github.server_host}
onChange={e =>
onUpdateState({
...tokenState,
github: {
...tokenState.github,
server_host: e.target.value,
},
})
}
readonly={readonly}
/>

<FieldInput
label="Slug"
placeholder="octo-enterprise"
value={github.enterprise_slug}
onChange={e =>
onUpdateState({
...tokenState,
github: {
...tokenState.github,
enterprise_slug: e.target.value,
},
})
}
readonly={readonly}
/>

<FieldInput
label="Static JWKS"
placeholder='{"keys":[--snip--]}'
toolTipContent="JSON Web Key Set used to verify the token issued by GitHub Actions"
value={github.static_jwks}
onChange={e =>
onUpdateState({
...tokenState,
github: {
...tokenState.github,
static_jwks: e.target.value,
},
})
}
readonly={readonly}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on making this a collapsable section, collapsed by default? I imagine for most cases these won't be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in e1f827b.

Reused a component from Roles. Not very pretty, but I didn't want to include extracting a reusable component in this PR.

@zmb3
Copy link
Collaborator

zmb3 commented Apr 25, 2025

Ideally the GHES Configuration would not show up in OSS builds because that's an enterprise only feature.

@nicholasmarais1158 nicholasmarais1158 marked this pull request as ready for review April 29, 2025 08:19
readonly={readonly}
/>

<FieldInput
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we asking for this information twice by requiring a "fully qualified" repository in the previous step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, potentially. Either a repo (inc owner) or just the owner is required. The owner is redundant if a repo is provided.

We could consolidate both into a single field and infer which to populate based on the presence of a slash. On the other hand, matching the two fields from the yaml representation seems suited to this non-day-one use case.

Which approach would you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I lean towards reflecting the underlying YAML here, otherwise we'll have to find a way to reflect cases where both the owner and repo are set in the underlying YAML as a single field.

@strideynet strideynet requested a review from avatus April 29, 2025 12:11
Comment on lines 28 to 32
export const collectKeys = (value: unknown, prefix: string = '') => {
if (typeof value !== 'object' || value === null) {
return prefix ? [prefix] : null;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to be somewhat "generic" of a utility. if its only really used from the JoinTokens directory, then id just export it from JoinTokens tbh. if you wanna use it everywhere, you could add it somewhere in the shared package. either way, i wont die on the hill just giving some info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to shared/utils in 521b629.

Comment on lines 137 to 141
export const checkIamYamlData = (data: unknown) => {
const keys = collectKeys(data);
return !keys || new Set(keys).isSubsetOf(supportedFieldsIam);
};

Copy link
Contributor

Choose a reason for hiding this comment

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

similar to my comment about collectKeys, this is exported but only used in UpsertJoinTokenDialog so lets just throw it in there and not export it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to UpsertJoinTokenDialog in 521b629.

/>
<FieldInput
label="ARN"
toolTipContent={`The joining nodes must match this ARN. Supports wildcards "*" and "?"`}
placeholder="arn:aws:iam::account-id:role/*"
value={rule.aws_arn}
onChange={e => setTokenRulesField(index, 'aws_arn', e.target.value)}
readonly={readonly}
Copy link
Contributor

Choose a reason for hiding this comment

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

when would readonly be true for IAM forms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When it's data includes configuration other than .aws_account and .aws_arn. In which case editing the fields is prevented for all join methods and the submit button is disabled.

Does that behaviour feel ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah sounds good!

Comment on lines +120 to +121
placeholder="gravitational/teleport"
toolTipContent="Fully qualified name of a GitHub repository (i.e. including the owner)"
Copy link
Contributor

Choose a reason for hiding this comment

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

does this placeholder match the suggestion from the tooltip? if so, it seems a bit off. when i think of "fully qualified name" to me it includes some sort of host/domain type stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I grabbed the copy from the resource reference; https://goteleport.com/docs/reference/machine-id/github-actions/

Would something this fit better?

The full name of a GitHub repository, prefixed by the owning user or organisation (e.g. gravitational/teleport)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think keeping it in line with the resource reference is fine. thanks for the context

Base automatically changed from nickmarais/feat/44794-github-join-tokens-api to master May 1, 2025 13:49
@nicholasmarais1158 nicholasmarais1158 added this pull request to the merge queue May 1, 2025
Merged via the queue into master with commit 0eabb00 May 1, 2025
40 checks passed
@nicholasmarais1158 nicholasmarais1158 deleted the nickmarais/feat/44794-github-join-tokens branch May 1, 2025 15:15
@backport-bot-workflows
Copy link
Contributor

@nicholasmarais1158 See the table below for backport results.

Branch Result
branch/v16 Failed
branch/v17 Failed

nicholasmarais1158 added a commit that referenced this pull request May 2, 2025
* Fix/silence lint issues

* Fix spacing on side panel

* Return github-specific config from `GET /webapi/tokens`

* Add github form for create/edit

* Handle unsupported fields during edit

* Add js docs for check functions

* Add js docs for github check function

* Fix casing in test mocks

* Fix missing `readonly` prop

* Remove unused import

* Retain metadata (inc expiry) on token edit

* Remove mutual-exclusivity on repo/owner fields

* Improve supported fields readability

* Hide GHES config

* Ignore token not found errors when creating

* Lock GHES fields when using OSS

* Lint fix

* Return github-specific config from `GET /webapi/tokens`

* Support "token" in `/webapi/yaml/parse/:kind`

* Use empty time.Time for token expiry (`POST /webapi/tokens`)

* Revert createTokenForDiscoveryHandle changes

* Fix acronym casing

* Cover enterprise token types in tests

* Cover github tokens in existing tests

* Tweak handling of `tokenId`

* Check expiry is not overwritten

* Revert removing tokenId check

* Refactor supported fields

* Remove use of `X-Teleport-TokenName`
# Conflicts:
#	web/packages/teleport/src/JoinTokens/JoinTokenForms.tsx
#	web/packages/teleport/src/JoinTokens/UpsertJoinTokenDialog.tsx
#	web/packages/teleport/src/Roles/RoleEditor/StandardEditor/sections.tsx
#	web/packages/teleport/src/services/joinToken/types.ts
#	web/packages/teleport/src/services/yaml/types.ts
#	web/packages/teleport/src/teleportContext.tsx
nicholasmarais1158 added a commit that referenced this pull request May 2, 2025
* Fix/silence lint issues

* Fix spacing on side panel

* Return github-specific config from `GET /webapi/tokens`

* Add github form for create/edit

* Handle unsupported fields during edit

* Add js docs for check functions

* Add js docs for github check function

* Fix casing in test mocks

* Fix missing `readonly` prop

* Remove unused import

* Retain metadata (inc expiry) on token edit

* Remove mutual-exclusivity on repo/owner fields

* Improve supported fields readability

* Hide GHES config

* Ignore token not found errors when creating

* Lock GHES fields when using OSS

* Lint fix

* Return github-specific config from `GET /webapi/tokens`

* Support "token" in `/webapi/yaml/parse/:kind`

* Use empty time.Time for token expiry (`POST /webapi/tokens`)

* Revert createTokenForDiscoveryHandle changes

* Fix acronym casing

* Cover enterprise token types in tests

* Cover github tokens in existing tests

* Tweak handling of `tokenId`

* Check expiry is not overwritten

* Revert removing tokenId check

* Refactor supported fields

* Remove use of `X-Teleport-TokenName`
github-merge-queue bot pushed a commit that referenced this pull request May 12, 2025
…54477)

* Fix/silence lint issues

* Fix spacing on side panel

* Return github-specific config from `GET /webapi/tokens`

* Add github form for create/edit

* Handle unsupported fields during edit

* Add js docs for check functions

* Add js docs for github check function

* Fix casing in test mocks

* Fix missing `readonly` prop

* Remove unused import

* Retain metadata (inc expiry) on token edit

* Remove mutual-exclusivity on repo/owner fields

* Improve supported fields readability

* Hide GHES config

* Ignore token not found errors when creating

* Lock GHES fields when using OSS

* Lint fix

* Return github-specific config from `GET /webapi/tokens`

* Support "token" in `/webapi/yaml/parse/:kind`

* Use empty time.Time for token expiry (`POST /webapi/tokens`)

* Revert createTokenForDiscoveryHandle changes

* Fix acronym casing

* Cover enterprise token types in tests

* Cover github tokens in existing tests

* Tweak handling of `tokenId`

* Check expiry is not overwritten

* Revert removing tokenId check

* Refactor supported fields

* Remove use of `X-Teleport-TokenName`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants