Skip to content

Correctly clone RoleV6 when downgrading#35230

Merged
nklaassen merged 4 commits intomasterfrom
tigrato/fix-downgrade-clone
Nov 30, 2023
Merged

Correctly clone RoleV6 when downgrading#35230
nklaassen merged 4 commits intomasterfrom
tigrato/fix-downgrade-clone

Conversation

@tigrato
Copy link
Copy Markdown
Contributor

@tigrato tigrato commented Nov 30, 2023 •
edited
Loading

Fix panic caused by improper clone of RoleV6 when downgrading. This PR forces a clone before downgrading it.

Fixes #35229

Changelog: Fixed a possible panic when downgrading Teleport Roles to older versions.

@github-actions github-actions Bot requested review from Joerger and zmb3 November 30, 2023 20:48
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 30, 2023

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 30, 2023

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@tigrato tigrato force-pushed the tigrato/fix-downgrade-clone branch from 3ab19c1 to 3351840 Compare November 30, 2023 20:52
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 30, 2023

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

zmb3
zmb3 reviewed Nov 30, 2023
View reviewed changes
Comment thread lib/anomaly_detection/maxminddb/db.mmdb Outdated
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 Nov 30, 2023

Choose a reason for hiding this comment

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

oops!

Copy link
Copy Markdown
Contributor Author

@tigrato tigrato Nov 30, 2023

Choose a reason for hiding this comment

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

It looks like I am still working on anomaly detection 😆

@tigrato tigrato requested a review from nklaassen November 30, 2023 20:54
@tigrato tigrato force-pushed the tigrato/fix-downgrade-clone branch from 3351840 to ff515b7 Compare November 30, 2023 20:56
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 30, 2023

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

1 similar comment
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 30, 2023

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@tigrato
Correctly clone RoleV6 when downgrading
840dbbd 
Fix panic caused by improper clone of `RoleV6` when downgrading.
This PR forces a clone before downgrading it.

Fixes #35229

Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
@tigrato tigrato force-pushed the tigrato/fix-downgrade-clone branch from ff515b7 to 840dbbd Compare November 30, 2023 21:08
jakule
jakule reviewed Nov 30, 2023
View reviewed changes
Comment thread lib/auth/grpcserver.go
)
downgraded = *r
var restricted bool
downgraded := apiutils.CloneProtoMsg(r)
Copy link
Copy Markdown
Contributor

@jakule jakule Nov 30, 2023

Choose a reason for hiding this comment

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

This looks like quite an important change. We "probably" should add a test or at least a comment.

Copy link
Copy Markdown
Contributor Author

@tigrato tigrato Nov 30, 2023

Choose a reason for hiding this comment

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

Added in a496294

@tigrato tigrato enabled auto-merge November 30, 2023 21:47
nklaassen
nklaassen reviewed Nov 30, 2023
View reviewed changes
Comment thread lib/auth/grpcserver_test.go Outdated
Comment thread lib/auth/grpcserver.go Outdated
Comment thread lib/auth/grpcserver.go Outdated
@tigrato tigrato added this pull request to the merge queue Nov 30, 2023
@hugoShaka hugoShaka removed this pull request from the merge queue due to a manual request Nov 30, 2023
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from Joerger November 30, 2023 22:08
@nklaassen nklaassen enabled auto-merge November 30, 2023 22:55
@nklaassen nklaassen mentioned this pull request Nov 30, 2023
Merged
@nklaassen nklaassen added this pull request to the merge queue Nov 30, 2023
Merged via the queue into master with commit 56939dd Nov 30, 2023
@nklaassen nklaassen deleted the tigrato/fix-downgrade-clone branch November 30, 2023 23:25
@public-teleport-github-review-bot
Copy link
Copy Markdown

public-teleport-github-review-bot Bot commented Nov 30, 2023

@tigrato See the table below for backport results.

Branch Result
branch/v13 Failed

nklaassen added a commit that referenced this pull request Nov 30, 2023
@nklaassen
[v13] Correctly clone RoleV6 when downgrading
d0e5d9b 
Backport #35230 to branch/v13

Changelog: Fixed a possible panic when downgrading Teleport Roles to
older versions.
@nklaassen nklaassen mentioned this pull request Nov 30, 2023
Merged
github-merge-queue Bot pushed a commit that referenced this pull request Dec 1, 2023
@nklaassen
[v13] Correctly clone RoleV6 when downgrading (#35237)
bdbc76f 
* [v13] Correctly clone RoleV6 when downgrading

Backport #35230 to branch/v13

Changelog: Fixed a possible panic when downgrading Teleport Roles to
older versions.

* use golang.org/x/exp/maps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zmb3 zmb3 zmb3 left review comments

@r0mant r0mant r0mant approved these changes

@hugoShaka hugoShaka hugoShaka approved these changes

@nklaassen nklaassen nklaassen approved these changes

+1 more reviewer

@jakule jakule jakule left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size/sm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Auth server panics on gRPC request

6 participants

@tigrato @r0mant @zmb3 @jakule @hugoShaka @nklaassen