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

make configure awsoidc-idp transparent #46747

Merged
merged 3 commits into from
Oct 3, 2024

Conversation

GavinFrazar
Copy link
Contributor

@GavinFrazar GavinFrazar commented Sep 19, 2024

Related issues:

Applies to the integration command that the web UI discover flows tell users to run in AWS CloudShell to setup the AWS OIDC identity provider:

$ teleport integration configure awsoidc-idp ...flags...

The command describes itself, its actions, and the desired state after it runs.
It then prompts the user (by default) to confirm the action plan before proceeding.
The confirmation prompt can be overridden with cli flag --confirm if desired.

The IAM role it configures is no longer required to have the "ownership" tags that teleport applies if it's created by teleport, since the user is now prompted for confirmation before making changes.
This allows a user to configure an existing IAM role without tagging the role for configuration by teleport.
The command will still attempt to ensure the IAM role it configures has teleport tags, but failing to do so is only a warning.

An example of the output is available as a golden test, but I'll provide a real one here as well:

Updated:

"awsoidc-idp" will perform the following actions:

1. Create an OpenID Connect identity provider in AWS IAM for your Teleport cluster.
CreateOpenIDConnectProvider: {
    "Url": "https://gavin-leaf.cloud.gravitational.io",
    "ClientIDList": [
        "discover.teleport"
    ],
    "Tags": [
        {
            "Key": "teleport.dev/cluster",
            "Value": "gavin-leaf.cloud.gravitational.io"
        },
        {
            "Key": "teleport.dev/integration",
            "Value": "teleport-dev-2"
        },
        {
            "Key": "teleport.dev/origin",
            "Value": "integration_awsoidc"
        }
    ],
    "ThumbprintList": [
        "696db3af0dffc17e65c6a20d925c5a7bd24dec7e"
    ]
}

2. Create IAM role "gavin-leaf-cloud-gravitational-io" with a custom trust policy.
CreateRole: {
    "AssumeRolePolicyDocument": {
        "Version": "2012-10-17",
        "Statement": [
            {
                "Effect": "Allow",
                "Action": "sts:AssumeRoleWithWebIdentity",
                "Principal": {
                    "Federated": "arn:aws:iam::651149123960:oidc-provider/gavin-leaf.cloud.gravitational.io"
                },
                "Condition": {
                    "StringEquals": {
                        "gavin-leaf.cloud.gravitational.io:aud": "discover.teleport"
                    }
                }
            }
        ]
    },
    "RoleName": "gavin-leaf-cloud-gravitational-io",
    "Description": "Used by Teleport to provide access to AWS resources.",
    "MaxSessionDuration": null,
    "Path": null,
    "PermissionsBoundary": null,
    "Tags": [
        {
            "Key": "teleport.dev/cluster",
            "Value": "gavin-leaf.cloud.gravitational.io"
        },
        {
            "Key": "teleport.dev/integration",
            "Value": "teleport-dev-2"
        },
        {
            "Key": "teleport.dev/origin",
            "Value": "integration_awsoidc"
        }
    ]
}

Do you want "awsoidc-idp" to perform these actions? [y/N]: y
2024-10-01T19:01:13-07:00 INFO  Running step:1 action:CreateOpenIDConnectProvider provisioning/operations.go:185
2024-10-01T19:01:13-07:00 INFO  Creating OpenID Connect identity provider awsactions/create_oidc_idp.go:65
2024-10-01T19:01:14-07:00 INFO  OpenID Connect identity provider already exists awsactions/create_oidc_idp.go:70
2024-10-01T19:01:14-07:00 INFO  Running step:2 action:CreateRole provisioning/operations.go:185
2024-10-01T19:01:14-07:00 INFO  Checking for existing IAM role role:gavin-leaf-cloud-gravitational-io awsactions/create_role.go:109
2024-10-01T19:01:14-07:00 INFO  IAM role already exists role:gavin-leaf-cloud-gravitational-io awsactions/create_role.go:128
2024-10-01T19:01:14-07:00 INFO  Checking IAM role trust policy role:gavin-leaf-cloud-gravitational-io awsactions/create_role.go:164
2024-10-01T19:01:14-07:00 INFO  IAM role trust policy does not require update role:gavin-leaf-cloud-gravitational-io awsactions/create_role.go:169
2024-10-01T19:01:14-07:00 INFO  Checking for tags on IAM role role:gavin-leaf-cloud-gravitational-io awsactions/create_role.go:197
2024-10-01T19:01:14-07:00 INFO  IAM role is already tagged role:gavin-leaf-cloud-gravitational-io awsactions/create_role.go:201
2024-10-01T19:01:14-07:00 INFO  Success! operation:awsoidc-idp provisioning/operations.go:197

TODO

I'll update the other integration commands that users run as part of the flow in followup PRs.
I decided not to include those changes in this PR because the change diff was quite large.

@GavinFrazar GavinFrazar added kubernetes-access aws Used for AWS Related Issues. application-access database-access Database access related issues and PRs server-access discover Issues related to Teleport Discover no-changelog Indicates that a PR does not require a changelog entry backport/branch/v16 labels Sep 19, 2024
@GavinFrazar GavinFrazar force-pushed the gavinfrazar/aws-integration-script-transparency branch 2 times, most recently from 2d7cf2b to da5197f Compare September 24, 2024 06:14
@GavinFrazar GavinFrazar changed the title make aws discover integration commands transparent make configure awsoidc-idp transparent Sep 24, 2024
@GavinFrazar GavinFrazar force-pushed the gavinfrazar/aws-integration-script-transparency branch 2 times, most recently from 96ae986 to 40f9a01 Compare September 24, 2024 06:39
@GavinFrazar GavinFrazar marked this pull request as ready for review September 24, 2024 06:39
@GavinFrazar GavinFrazar force-pushed the gavinfrazar/aws-integration-script-transparency branch from 40f9a01 to dd4ca5e Compare September 24, 2024 07:02
@GavinFrazar GavinFrazar requested review from greedy52 and kimlisa and removed request for timothyb89 and bl-nero September 24, 2024 07:04
@greedy52
Copy link
Contributor

@webvictim @stevenGravy let us know what you think about the updated output. thanks!

@marcoandredinis
Copy link
Contributor

Adding links to the actual resource might be useful as well.
The logs are not very readable when skimming, however if we add links on their own line, that might be more readable.
What do you think?

@GavinFrazar
Copy link
Contributor Author

Adding links to the actual resource might be useful as well. The logs are not very readable when skimming, however if we add links on their own line, that might be more readable. What do you think?

you mean the issuer_url links? I could just remove those too, since they aren't meant to be clicked on, it's just a way to refer to the IdP

@marcoandredinis
Copy link
Contributor

you mean the issuer_url links? I could just remove those too, since they aren't meant to be clicked on, it's just a way to refer to the IdP

Not only the OIDC IdP issuer's url but all the resources.
Basically adding something that gets the user attention to the resources.
Aadding an empty line, an emoji (🟢 or 🔗) or something else that calls for attention.

...
2024-09-23T23:34:57-07:00 INFO  Running step:1 action:CreateOpenIDConnectIdentityProvider provisioning/operations.go:185
2024-09-23T23:34:57-07:00 INFO  Creating OpenID Connect identity provider issuer_url:https://gavin-leaf.cloud.gravitational.io awsactions/create_oidc_idp.go:64
2024-09-23T23:34:57-07:00 INFO  OpenID Connect identity provider already exists issuer_url:https://gavin-leaf.cloud.gravitational.io awsactions/create_oidc_idp.go:76
2024-09-23T23:34:57-07:00 INFO  🔗 https://us-east-1.console.aws.amazon.com/iam/home?region=eu-west-2#/identity_providers/details/OPENID/arn%3Aaws%3Aiam%3A%3A123456789012%3Aoidc-provider%2Fmarco.cloud.gravitational.io
2024-09-23T23:34:57-07:00 INFO  🟢 https://us-east-1.console.aws.amazon.com/iam/home?region=eu-west-2#/identity_providers/details/OPENID/arn%3Aaws%3Aiam%3A%3A123456789012%3Aoidc-provider%2Fmarco.cloud.gravitational.io
2024-09-23T23:34:57-07:00 INFO  https://us-east-1.console.aws.amazon.com/iam/home?region=eu-west-2#/identity_providers/details/OPENID/arn%3Aaws%3Aiam%3A%3A123456789012%3Aoidc-provider%2Fmarco.cloud.gravitational.io
2024-09-23T23:34:57-07:00 INFO  ----
2024-09-23T23:34:57-07:00 INFO  Running step:2 action:CreateRole provisioning/operations.go:185
....

Extracting the important parts on the log lines is not easy if we don't add an extra visual artifacts.

@webvictim
Copy link
Contributor

I like the new output!

@marcoandredinis I don't think adding full URLs and/or emojis to lines is necessary unless we require the user to click on them. It will just confuse people otherwise.

lib/cloud/provisioning/awsactions/create_oidc_idp.go Outdated Show resolved Hide resolved
lib/cloud/provisioning/operations.go Show resolved Hide resolved
lib/cloud/provisioning/awsactions/create_oidc_idp.go Outdated Show resolved Hide resolved
lib/cloud/provisioning/awsactions/create_role.go Outdated Show resolved Hide resolved
lib/cloud/provisioning/operations_test.go Show resolved Hide resolved
@GavinFrazar
Copy link
Contributor Author

Not only the OIDC IdP issuer's url but all the resources. Basically adding something that gets the user attention to the resources. Aadding an empty line, an emoji (🟢 or 🔗) or something else that calls for attention.

Oh, I see you mean linking to every created resource.
I could see how it might be confusing to a user as Gus pointed out, but also how it could be useful to just click a link.
What I'm more concerned with is how tedious it will be to properly link everything.

Since it's all tagged and we show the tags I think the resources are at least easy enough to track down by tag anyway, so let's at least limit scope for now and not link all resources.

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.

LGTM.
Though I personally would vote for the detailed confirmation as an "opt-in" behavior rather than default.

@GavinFrazar
Copy link
Contributor Author

LGTM. Though I personally would vote for the detailed confirmation as an "opt-in" behavior rather than default.

keep in mind that people will be running this command with bash -c $(curl ...), so if the detailed behavior is opt-in then realistically no one will ever see it

Roman suggested an interactive menu to show/hide details, but I'd like to defer on that for now.

Applies to the integration command that the web UI discover flows tell
users to run in AWS CloudShell to setup the AWS OIDC identity provider:
    teleport integration configure awsoidc-idp

The command describes itself, its actions, and the desired state after
it runs. It then prompts the user (by default) to confirm the action
plan before proceeding.
The confirmation prompt can be overridden with cli flag --confirm
if desired.

The IAM role it configures is no longer required to have the "ownership"
tags that teleport applies if it's created by teleport, since the user is
now prompted for confirmation before making changes.
This allows a user to configure an existing IAM role without tagging the
role for configuration by teleport.
The command will still attempt to ensure the IAM role it configures has
teleport tags, but failing to do so is only a warning.
@GavinFrazar GavinFrazar force-pushed the gavinfrazar/aws-integration-script-transparency branch from 0067358 to 7b8be35 Compare October 3, 2024 16:30
@GavinFrazar GavinFrazar enabled auto-merge October 3, 2024 16:30
@GavinFrazar GavinFrazar added this pull request to the merge queue Oct 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 3, 2024
@GavinFrazar GavinFrazar enabled auto-merge October 3, 2024 19:50
@GavinFrazar GavinFrazar added this pull request to the merge queue Oct 3, 2024
Merged via the queue into master with commit a3e1316 Oct 3, 2024
41 checks passed
@GavinFrazar GavinFrazar deleted the gavinfrazar/aws-integration-script-transparency branch October 3, 2024 21:06
@public-teleport-github-review-bot

@GavinFrazar See the table below for backport results.

Branch Result
branch/v16 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws Used for AWS Related Issues. backport/branch/v16 discover Issues related to Teleport Discover no-changelog Indicates that a PR does not require a changelog entry size/lg ux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants