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

credentials/alts: ClientAuthorizationCheck to case-fold compare of peer SA #3792

Merged
merged 2 commits into from
Sep 1, 2020

Conversation

AntonNep
Copy link
Contributor

@AntonNep AntonNep commented Aug 5, 2020

expectedServiceAccounts are commonly mixed-case, while the PeerServiceAccount is lowel-case.
The discrepancy can be error-prone, recommendation is to use Unicode case-folding comparison.
Nit: change status.Newf(...).Err() to the semantically equal status.Errorf(...).

`expectedServiceAccounts` are commonly mixed-case, while the `PeerServiceAccount` is lowel-case.
The discrepancy can be error-prone, recommendation is to use Unicode case-folding comparison.
Nit: change `status.Newf(...).Err()` to the semantically equal `status.Errorf(...)`.
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 5, 2020

CLA Check
The committers are authorized under a signed CLA.

@AntonNep AntonNep changed the title ClientAuthorizationCheck ignore-case on SA compare ClientAuthorizationCheck to case-fold compare of peer SA Aug 5, 2020
@menghanl
Copy link
Contributor

menghanl commented Aug 5, 2020

Can you sign the CLA, please? Thanks!

@easwars easwars changed the title ClientAuthorizationCheck to case-fold compare of peer SA credentials/alts: ClientAuthorizationCheck to case-fold compare of peer SA Aug 6, 2020
@AntonNep
Copy link
Contributor Author

@menghanl can you please explain what clarification is required from my side?

@menghanl
Copy link
Contributor

@AntonNep We were waiting for the CLA. It looks good now.
@cesarghali Can you take a look at the code? Thanks

@AntonNep
Copy link
Contributor Author

AntonNep commented Sep 1, 2020

Hi @cesarghali and @menghanl , is there anything blocking this PR from being approved?

@AntonNep AntonNep closed this Sep 1, 2020
@AntonNep AntonNep reopened this Sep 1, 2020
@cesarghali
Copy link
Contributor

cesarghali commented Sep 1, 2020

Sorry for the late response, this fell through the cracks for some reason.

@cesarghali cesarghali merged commit 48bf772 into grpc:master Sep 1, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants