Skip to content
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

Add tctl plugins install awsic #51239

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

smallinsky
Copy link
Contributor

What

AWS CLI to install AWS IC plugin with local creds.

usage:

tctl plugins install awsic --token $(op read "op://Employee/AWS IC Teleport Identity Provider/token") --url https://scim.eu-north-1.amazonaws.com/.../scim/v2 --default-owner=marek --region=eu-north-1 --arn=arn:aws:sso:::instance/ssoins...

Adds a comand-line installation tool for the AWS Identity Center integration.

Co-authored-by: Marek Smoliński <[email protected]>
@tcsc tcsc force-pushed the smallinsky/aws-ic-instal-plugin branch from 5ca783d to 367ac0a Compare January 31, 2025 12:27
@tcsc tcsc marked this pull request as ready for review January 31, 2025 12:29
@github-actions github-actions bot added size/md tctl tctl - Teleport admin tool labels Jan 31, 2025
@github-actions github-actions bot requested review from ryanclark and Tener January 31, 2025 12:30
@tcsc tcsc self-assigned this Jan 31, 2025
@tcsc tcsc requested review from flyinghermit and kopiczko January 31, 2025 12:53
@tcsc tcsc added no-changelog Indicates that a PR does not require a changelog entry aws-iam-identity-center backport/branch/v17 labels Jan 31, 2025
Comment on lines 143 to 145
CredentialsSource: types.AWSICCredentialsSource_AWSIC_CREDENTIALS_SOURCE_SYSTEM,
GroupSyncFilters: groupFilters,
AwsAccountsFilters: accountFilters,
Copy link
Contributor Author

@smallinsky smallinsky Jan 31, 2025

Choose a reason for hiding this comment

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

The user_sync_filter is missing.

When using Okta as the Identity Source, we want to sync only Okta-originated users to AWS IC using the mechanism implemented in this PR.

Otherwise, a Teleport local user will be pushed to the AWS IC catalog and then fetched by Okta. - I’m not sure what side effects this behavior might cause.

Copy link
Contributor

Choose a reason for hiding this comment

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

Untested hypothesis:

  • user will be provisioned into AWS IC
  • user AWS permissions will be managed as per any other
  • User will not be able to log in as Okta cannot verify them

},
}

_, err = args.plugins.CreatePlugin(ctx, req)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we expose an API like we do with Okta? It would be nice to stop creating those plugins directly and start using the same API that the UI does

Copy link
Contributor Author

@smallinsky smallinsky left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@flyinghermit flyinghermit left a comment

Choose a reason for hiding this comment

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

Looks good, just a few comments to ensure cli installation handles default dependencies and validation that is on par with the plugin enrollment supported in the UI.

Comment on lines +114 to +118
cmd.Flag("default-owner", "List of Teleport users that are default owners for the imported access lists. Multiple flags allowed.").Required().StringsVar(&p.install.awsIC.defaultOwners)
cmd.Flag("url", "AWS Identity Center SCIM provisioning endpoint").Required().StringVar(&p.install.awsIC.scimURL)
cmd.Flag("token", "AWS Identify Center SCIM provisioning token.").Required().StringVar(&p.install.awsIC.scimToken)
cmd.Flag("region", "AWS Identity center instance region").Required().StringVar(&p.install.awsIC.region)
cmd.Flag("arn", "AWS Identify center instance ARN").Required().StringVar(&p.install.awsIC.arn)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: since these flags are available under root awsic subcommand, i think spelling out flag names provides better context on the expected values. So the changes would become:

  • default-owner -> access-list-default-owner
  • url -> scim-url
  • token -> scim-token
  • region -> instance-region
  • arn -> instance-arn

Comment on lines +60 to +62
// We are using a manual validator here rather than the canonical one defined
// in the AWS IC integration itself, because those filter tools are not
// available to OSS builds of tctl.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it be a non-trivial migration to move that to oss? I think we should reuse validation logics at minimum rather than having two different definition that risk going out of sync.

Copy link
Contributor

Choose a reason for hiding this comment

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

I definitely want to move it, but I think it should be done in a separate PR.

accountIDFilters []string
}

func (a *awsICArgs) validate() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

add validation for region, scimURL field?

},
},
}

Copy link
Contributor

Choose a reason for hiding this comment

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

before installing plugin, I think we should check if saml service provider for the identity center is installed and guide user to do so if it isn't.

func (p *PluginsCommand) initInstallAWSIC(parent *kingpin.CmdClause) {
p.install.awsIC.cmd = parent.Command("awsic", "Install an AWS Identity Center integration.")
cmd := p.install.awsIC.cmd
cmd.Flag("default-owner", "List of Teleport users that are default owners for the imported access lists. Multiple flags allowed.").Required().StringsVar(&p.install.awsIC.defaultOwners)
Copy link
Contributor

Choose a reason for hiding this comment

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

will it be better ux by supporting comma separate values over multiple flag? not sure what we do for other existing commands.

@@ -330,6 +332,8 @@ func (p *PluginsCommand) TryRun(ctx context.Context, cmd string, clientFunc comm
commandFunc = p.InstallEntra
case p.install.netIQ.cmd.FullCommand():
commandFunc = p.InstallNetIQ
case p.install.awsIC.cmd.FullCommand():
commandFunc = p.InstallAWSIC
Copy link
Contributor

Choose a reason for hiding this comment

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

Does plugin deletion works by default? If yes, i think we should support resource cleanup or guide users to do it manually before letting them delete the plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws-iam-identity-center backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/md tctl tctl - Teleport admin tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants