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.

Sign up for GitHub

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

Jump to bottom

advancedtls: populate verified chains when using custom buildVerifyFunc #7181

Merged
merged 8 commits into from
May 22, 2024
Merged

advancedtls: populate verified chains when using custom buildVerifyFunc #7181

merged 8 commits into from
May 22, 2024

Conversation

mudhireddy
Copy link
Contributor

@mudhireddy mudhireddy commented May 2, 2024 •
edited
Loading

This request is to address the issue opened #7176

Basically this patch will set the verifiedChains correctly when advancedtls package is used with buildVerifyFunc() and applications need to access TLSInfo.State.VerifiedChains in their logic.

RELEASE NOTES: none

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented May 2, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@mudhireddy mudhireddy mentioned this pull request May 2, 2024
Closed
@mudhireddy
Copy link
Contributor Author

My local testing shows it ok though

grpc-go/test/xds @ramesh-937457-0.sjc% go test
PASS
ok google.golang.org/grpc/test/xds 11.131s

Is there a way to trigger the re-test of it?

@mudhireddy
Copy link
Contributor Author

@purnesh42H any documentation on who and how to add LABEL, MILESTONE so that all checks on this pull request will pass ? right now Mergeable is failing for missing LABEL, MILESTONE

@purnesh42H purnesh42H self-assigned this May 7, 2024
@purnesh42H
Copy link
Contributor

@mudhireddy I have assigned it to myself. I will discuss with other maintainers and update you back. Thanks!

@dfawley dfawley requested a review from gtcooke94 May 7, 2024 16:35
@dfawley dfawley assigned gtcooke94 and unassigned purnesh42H May 7, 2024
@gtcooke94 gtcooke94 added the Type: Security A bug or other problem affecting security label May 7, 2024
@gtcooke94 gtcooke94 added this to the 1.64 Release milestone May 7, 2024
gtcooke94
gtcooke94 reviewed May 7, 2024
View reviewed changes
Copy link
Contributor

@gtcooke94 gtcooke94 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! In general looks good, one big comment about where we set the value.
I'm reasoning through a few things on the overall code flow to make sure this wouldn't have any risk of leaking cert chains around if someone used the same credential with multiple connections - I'll update here when I'm done looking through that.

In the meanwhile, could you add tests that validate the expected behaviors here?
Also just curious, what is your use case for this?

security/advancedtls/advancedtls.go Outdated Show resolved Hide resolved
@arvindbr8 arvindbr8 modified the milestones: 1.64 Release, 1.65 Release May 9, 2024
@dfawley dfawley changed the title populate verified chains when using custom buildVerifyFunc advancedtls: populate verified chains when using custom buildVerifyFunc May 14, 2024
@mudhireddy
Copy link
Contributor Author

Thanks for the PR! In general looks good, one big comment about where we set the value. I'm reasoning through a few things on the overall code flow to make sure this wouldn't have any risk of leaking cert chains around if someone used the same credential with multiple connections - I'll update here when I'm done looking through that.

In the meanwhile, could you add tests that validate the expected behaviors here? Also just curious, what is your use case for this?

Added tests. Reg use case we use the CommonName details from VerifiedChains

@mudhireddy mudhireddy removed their assignment May 14, 2024
gtcooke94
gtcooke94 previously approved these changes May 15, 2024
View reviewed changes
Copy link
Contributor

@gtcooke94 gtcooke94 left a comment

Choose a reason for hiding this comment

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

This looks good, thanks again for the PR!
We'll get one more reviewer to check it out

@gtcooke94 gtcooke94 requested a review from dfawley May 15, 2024 17:00
@gtcooke94
Copy link
Contributor

@dfawley can you review this or assign to another go team member?

dfawley
dfawley reviewed May 15, 2024
View reviewed changes
security/advancedtls/advancedtls.go Outdated Show resolved Hide resolved
security/advancedtls/advancedtls.go Outdated Show resolved Hide resolved
security/advancedtls/advancedtls.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned mudhireddy and unassigned dfawley May 15, 2024
@mudhireddy mudhireddy removed their assignment May 15, 2024
@mudhireddy mudhireddy requested a review from dfawley May 15, 2024 20:39
@mudhireddy
Copy link
Contributor Author

mudhireddy commented May 20, 2024 •
edited
Loading

@dfawley @gtcooke94, can I please get approval for this PR to merge ?

@dfawley dfawley self-assigned this May 21, 2024
@dfawley dfawley removed their assignment May 21, 2024
@dfawley dfawley requested a review from gtcooke94 May 21, 2024 23:26
gtcooke94
gtcooke94 previously approved these changes May 22, 2024
View reviewed changes
Copy link
Contributor

@gtcooke94 gtcooke94 left a comment

Choose a reason for hiding this comment

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

Giving the type a name makes the code a little more readable, I think it's a good change

security/advancedtls/advancedtls.go Outdated Show resolved Hide resolved
gtcooke94
gtcooke94 reviewed May 22, 2024
View reviewed changes
Copy link
Contributor

@gtcooke94 gtcooke94 left a comment

Choose a reason for hiding this comment

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

Sorry meant to not approve yet because I don't want the shortened name

@gtcooke94 gtcooke94 self-requested a review May 22, 2024 15:43
@gtcooke94 gtcooke94 dismissed their stale review May 22, 2024 15:45

Didn't mean to approve

gtcooke94
gtcooke94 approved these changes May 22, 2024
View reviewed changes
Copy link
Contributor

@gtcooke94 gtcooke94 left a comment

Choose a reason for hiding this comment

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

I think this is looking good - it would be nice to be able to replace and use CertificateChains everywhere, but we could potentially just do that in a separate PR

@mudhireddy
Copy link
Contributor Author

Thank you guys for the review and approval.

@gtcooke94, I see that it is still open, is it going to auto merge or do I need to do anything for it to merge ?

@gtcooke94 gtcooke94 merged commit 5ffe0ef into grpc:master May 22, 2024
12 checks passed
arjan-bal
arjan-bal reviewed Jun 4, 2024
View reviewed changes
security/advancedtls/advancedtls.go
@@ -35,6 +35,8 @@ import (
credinternal "google.golang.org/grpc/internal/credentials"
)

type CertificateChains [][]*x509.Certificate
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is an exported type, it should have a doc string.

@tbg tbg mentioned this pull request Nov 27, 2024
Draft
16 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@arjan-bal arjan-bal arjan-bal left review comments

@gtcooke94 gtcooke94 gtcooke94 approved these changes

@dfawley dfawley dfawley approved these changes

Assignees

@gtcooke94 gtcooke94

Labels
Type: Security A bug or other problem affecting security
Projects
None yet
Milestone
1.65 Release
Development

Successfully merging this pull request may close these issues.
6 participants
@mudhireddy @purnesh42H @gtcooke94 @dfawley @arjan-bal @arvindbr8