-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat(join-tokens): [44794] Add Join Token form for GitHub #54308
Conversation
export const checkIAMYAMLData = (data: unknown) => { | ||
const keys = collectKeys(data); | ||
return ( | ||
!keys || new Set(keys).isSubsetOf(new Set(['.aws_account', '.aws_arn'])) |
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 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)
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.
Done in c2d0292.
<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} | ||
/> |
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.
Thoughts on making this a collapsable section, collapsed by default? I imagine for most cases these won't be used
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.
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.
Ideally the GHES Configuration would not show up in OSS builds because that's an enterprise only feature. |
readonly={readonly} | ||
/> | ||
|
||
<FieldInput |
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.
Are we asking for this information twice by requiring a "fully qualified" repository in the previous step?
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.
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?
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.
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.
export const collectKeys = (value: unknown, prefix: string = '') => { | ||
if (typeof value !== 'object' || value === null) { | ||
return prefix ? [prefix] : null; | ||
} | ||
|
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 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.
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.
Moved to shared/utils in 521b629.
export const checkIamYamlData = (data: unknown) => { | ||
const keys = collectKeys(data); | ||
return !keys || new Set(keys).isSubsetOf(supportedFieldsIam); | ||
}; | ||
|
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.
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
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.
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} |
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.
when would readonly
be true for IAM forms?
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.
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?
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 sounds good!
placeholder="gravitational/teleport" | ||
toolTipContent="Fully qualified name of a GitHub repository (i.e. including the owner)" |
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.
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.
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 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)
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 keeping it in line with the resource reference is fine. thanks for the context
…marais/feat/44794-github-join-tokens
# Conflicts: # lib/web/join_tokens.go # lib/web/join_tokens_test.go
@nicholasmarais1158 See the table below for backport results.
|
* 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
* 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`
…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`
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
Demo
Screen.Recording.2025-04-28.at.17.49.23.mov