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

Access request compatibility for servers without v2 api #13373

Merged
merged 6 commits into from
Jun 13, 2022

Conversation

xacrimon
Copy link
Contributor

Fixes a compatability issue introduced by security fixes where the client would call a grpc method that doesn't exist.

@xacrimon xacrimon requested review from r0mant and Joerger and removed request for Joerger June 10, 2022 10:54
@xacrimon xacrimon marked this pull request as ready for review June 10, 2022 10:54
Copy link
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

Could we add a test for this?

api/client/client.go Show resolved Hide resolved
api/client/client.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

lgtm once @codingllama's comments are addressed

@xacrimon xacrimon requested a review from codingllama June 10, 2022 21:37
@xacrimon
Copy link
Contributor Author

@codingllama Please take another look now

@codingllama
Copy link
Contributor

Hey @xacrimon, the main ask here was if we could add a test for this. WDYT?

@xacrimon
Copy link
Contributor Author

@codingllama Ah, the emails I got from GitHub had that first comment collapsed. If what you're asking is if we can precisely test the compat to make sure it hits the fallback. I'm not sure on how to do that to be honest, you'd have to somehow mock gRPC into thinking a method is unimplemented (otherwise we'd end up with a lot of wacky interfaces and indirections everywhere), I'm not aware of any other similar tests in Teleport but if you have any ideas I'm all ears.

@codingllama codingllama changed the title Access request compatability for servers without v2 api Access request compatibility for servers without v2 api Jun 10, 2022
@codingllama
Copy link
Contributor

Approving, as it seems unfair to hold you on something we don't seem to do. We can do a follow up if you like.

if you have any ideas I'm all ears.

I think creating a new gprc.Server and binding proto.UnimplementedAuthServiceServer to it would do it.

@xacrimon xacrimon enabled auto-merge (squash) June 13, 2022 16:40
@xacrimon xacrimon disabled auto-merge June 13, 2022 17:43
@xacrimon xacrimon enabled auto-merge (squash) June 13, 2022 17:44
@xacrimon xacrimon merged commit c083518 into master Jun 13, 2022
@github-actions
Copy link

@xacrimon See the table below for backport results.

Branch Result
branch/v8 Create PR
branch/v9 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants