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

security/advancedtls: add TlsVersionOption to select desired min/max TLS versions #6007

Merged
merged 1 commit into from Apr 10, 2023
Merged

security/advancedtls: add TlsVersionOption to select desired min/max TLS versions #6007

merged 1 commit into from Apr 10, 2023

Conversation

joeljeske
Copy link
Contributor

@joeljeske joeljeske commented Feb 4, 2023 •
edited by dfawley

(Continuation of #5797 cc @ZhenLian)

Adding an "TlsVersionOption" for users to select their desired min/max TLS versions, if advanced TLS is used, per request by #5667

RELEASE NOTES:

  • security/advancedtls: add min/max TLS version selection options

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 4, 2023 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

@ZhenLian @joeljeske
advancedtls: add min/max TLS version option
23c3143 
Adding an "TlsVersionOption" for users to select their desired min/max TLS versions, if advanced TLS is used, per request by grpc#5667

RELEASE NOTES:

security/advancedtls: add min/max TLS version selection options
@joeljeske joeljeske mentioned this pull request Feb 4, 2023
Open
@dfawley dfawley requested a review from ZhenLian February 7, 2023 18:54
@ZhenLian
Copy link
Contributor

I am OOO right now... Adding @erm-g to take a look, thanks!

@ZhenLian ZhenLian requested review from erm-g and removed request for ZhenLian February 11, 2023 00:55
@zasweq
Copy link
Contributor

zasweq commented Feb 21, 2023

@erm-g can you please take a look at this?

@erm-g
Copy link
Contributor

erm-g commented Feb 21, 2023

Hi @dfawley ,
Could you take a look at comment by Zhen #5667 (comment) ?
Actually, from my perspective it's a good idea to expose tls.Config for kind of 'power users'. Does this pattern make sense for you?

@arvindbr8 arvindbr8 assigned dfawley and unassigned ZhenLian and erm-g Feb 28, 2023
@dfawley dfawley changed the title Advancedtls cipher suites security/advancedtls: add TlsVersionOption to select desired min/max TLS versions Feb 28, 2023
@dfawley
Copy link
Member

dfawley commented Mar 7, 2023 •
edited

Could you take a look at comment by Zhen #5667 (comment) ?
Actually, from my perspective it's a good idea to expose tls.Config for kind of 'power users'. Does this pattern make sense for you?

I have no strong feelings here one way or another. I'll leave it up to the security team to define the API here. If you think users will need direct interaction with tls.Config then that's fine, just keep in mind that we provide base-level access to that via our base TLS credentials.

@dfawley dfawley assigned erm-g and unassigned dfawley Mar 7, 2023
@easwars
Copy link
Contributor

easwars commented Mar 14, 2023

Another gentle ping @erm-g

@easwars easwars modified the milestones: 1.54 Release, 1.55 Release Mar 17, 2023
erm-g
erm-g requested changes Mar 28, 2023
View reviewed changes
Copy link
Contributor

@erm-g erm-g left a comment

Choose a reason for hiding this comment

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

@joeljeske We had an internal discussion and can merge this PR. Please keep in mind that, in the long term, we will be exposing 'tls.Config' and signatures may change. This API is still in an experimental stage.
I see there are some failing tests - once it's fix I can merge the PR.

@dfawley dfawley assigned joeljeske and unassigned erm-g Apr 4, 2023
@dfawley dfawley closed this Apr 4, 2023
@dfawley dfawley reopened this Apr 4, 2023
@dfawley dfawley added the Type: Feature New features or improvements in behavior label Apr 4, 2023
@easwars
Copy link
Contributor

easwars commented Apr 10, 2023

@erm-g : Looks like you requested changes, but never approved the PR. Could you please approve it, and I can merge it. Thanks.

@rockspore rockspore requested a review from erm-g April 10, 2023 18:10
erm-g
erm-g approved these changes Apr 10, 2023
View reviewed changes
Copy link
Contributor

@erm-g erm-g left a comment

Choose a reason for hiding this comment

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

Approved

@easwars easwars merged commit 81b3092 into grpc:master Apr 10, 2023
11 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@erm-g erm-g erm-g approved these changes

@dfawley dfawley Awaiting requested review from dfawley

Assignees

@joeljeske joeljeske

@erm-g erm-g

Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Milestone
1.55 Release
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@joeljeske @ZhenLian @zasweq @erm-g @dfawley @easwars