Skip to content

always generate request IDs server-side#31760

Merged
fspmarshall merged 2 commits intomasterfrom
fspmarshall/server-side-request-ids
Sep 13, 2023
Merged

always generate request IDs server-side#31760
fspmarshall merged 2 commits intomasterfrom
fspmarshall/server-side-request-ids

Conversation

@fspmarshall
Copy link
Copy Markdown
Contributor

@fspmarshall fspmarshall commented Sep 12, 2023

Improve best-practices around Access Request ID generation. This change is relatively minor, but needs to land in a major version since it deprecates an API.

Related: https://github.com/gravitational/teleport.e/pull/2194

Note: this has been manually tested using both UI and CLI request flows.

Changelog: Security improvements for access request API (users may be prompted to upgrade tsh).

@github-actions github-actions Bot added size/sm tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Sep 12, 2023
@fspmarshall fspmarshall requested a review from jentfoo September 12, 2023 16:40
Copy link
Copy Markdown
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

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

Is the v14 backport only going to contain client changes?

Comment thread e Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's not forget to circle back and update this to point at the commit on master when the accompanying PR in e lands

Comment thread lib/auth/grpcserver.go Outdated
Comment thread lib/auth/auth_with_roles_test.go Outdated
Copy link
Copy Markdown
Contributor

@jentfoo jentfoo left a comment

Choose a reason for hiding this comment

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

Overall changes look good, but it looks like integration tests are still trying to use the old CreateAccessRequest

@fspmarshall fspmarshall force-pushed the fspmarshall/server-side-request-ids branch 2 times, most recently from 0bc19db to 636dcdf Compare September 13, 2023 14:44
@fspmarshall fspmarshall force-pushed the fspmarshall/server-side-request-ids branch from 636dcdf to 5623215 Compare September 13, 2023 15:09
@fspmarshall fspmarshall added this pull request to the merge queue Sep 13, 2023
Merged via the queue into master with commit 2d8e6d3 Sep 13, 2023
@fspmarshall fspmarshall deleted the fspmarshall/server-side-request-ids branch September 13, 2023 16:26
@public-teleport-github-review-bot
Copy link
Copy Markdown

@fspmarshall See the table below for backport results.

Branch Result
branch/v14 Failed

fspmarshall added a commit that referenced this pull request Sep 14, 2023
github-merge-queue Bot pushed a commit that referenced this pull request Sep 14, 2023
* always generate request IDs server-side (#31760)

* server-side request ids

* update e-ref
camscale added a commit to gravitational/teleport-plugins that referenced this pull request Sep 21, 2023
Update Teleport version to 14.0.0 from 13.8.8. The former has now been
released and is available.

Update the Go version to 1.21.1, as used by Teleport 14.

Fix a couple of tests that used `lib/client.Client.CreateAccessRequest()`
to use `CreateAccessRequestV2()` as that supersedes the former. The fix
is as per gravitational/teleport#31760.

Co-authored-by: Cam Hutchison <camh@goteleport.com>
camscale added a commit to gravitational/teleport-plugins that referenced this pull request Sep 21, 2023
Update Teleport version to 14.0.0 from 13.8.8. The former has now been
released and is available.

Update the Go version to 1.21.1, as used by Teleport 14.

Fix a couple of tests that used `lib/client.Client.CreateAccessRequest()`
to use `CreateAccessRequestV2()` as that supersedes the former. The fix
is as per gravitational/teleport#31760.

Co-authored-by: Cam Hutchison <camh@goteleport.com>
marcoandredinis added a commit to gravitational/teleport-plugins that referenced this pull request Sep 21, 2023
Update Teleport version to 14.0.0 from 13.8.8. The former has now been
released and is available.

Update the Go version to 1.21.1, as used by Teleport 14.

Fix a couple of tests that used `lib/client.Client.CreateAccessRequest()`
to use `CreateAccessRequestV2()` as that supersedes the former. The fix
is as per gravitational/teleport#31760.

Co-authored-by: Cam Hutchison <camh@goteleport.com>
marcoandredinis added a commit to gravitational/teleport-plugins that referenced this pull request Sep 21, 2023
* Prepare for Teleport V14

Update Teleport version to 14.0.0 from 13.8.8. The former has now been
released and is available.

Update the Go version to 1.21.1, as used by Teleport 14.

Fix a couple of tests that used `lib/client.Client.CreateAccessRequest()`
to use `CreateAccessRequestV2()` as that supersedes the former. The fix
is as per gravitational/teleport#31760.

Co-authored-by: Cam Hutchison <camh@goteleport.com>

* Update go.mod to Teleport 14 tag

Update the go.mod file to use Teleport 14 from the tag not the branch by
running:

   make update-teleport-dep-version VERSION=14.0.0

The codeql workflow still seems to use an older version of Go which
fails to parse the new `toolchain` directive in `go.mod`, so add a "Set
up Go" step the same as the Teleport repo to ensure the latest version
is installed for the workflow.

* Update protoc-gen-terraform version to 2.1.0

Use 2.1.0 as this is the current latest of
gravitational/protoc-gen-terraform, and the instructions say to use the
latest.

* Specify RoleV6...KuberbetesResources.Verbs as computed

Specify `RoleV6.Spec.Allow.KubernetesResources.Verbs` as a computed
field and re-generate the terraform provider. This is needed as the
verbs field has a default value of `[ "*" ]`, which causes the error:

    role_test.go:244: Step 7/8 error: Error running apply: exit status 1

        Error: Provider produced inconsistent result after apply

        When applying changes to teleport_role.upgrade, provider
        "provider[\"registry.terraform.io/hashicorp/teleport\"]" produced an
        unexpected new value: .spec.allow.kubernetes_resources[0].verbs: was null,
        but now cty.ListVal([]cty.Value{cty.StringVal("*")}).

        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.

Ideally this would not be computed but would have a default specified
instead, but it does not look like we can do that with the current
generator.

---------

Co-authored-by: Cam Hutchison <camh@goteleport.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-for-v14 size/sm tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants