Skip to content

Added note tls version below 1.2 should set compatible cipher suites #2502

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
Merged
istio-testing merged 3 commits into from
Oct 20, 2022
Merged

Added note tls version below 1.2 should set compatible cipher suites #2502

istio-testing merged 3 commits into from
Oct 20, 2022

Conversation

@hzxuzhonghu
Copy link
Member

@hzxuzhonghu hzxuzhonghu commented Oct 11, 2022 •
edited
Loading

@hzxuzhonghu hzxuzhonghu requested a review from a team as a code owner October 11, 2022 03:13
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 11, 2022
@hzxuzhonghu hzxuzhonghu changed the title Added note tls version below 1.2 should set valida cipher suites Added note tls version below 1.2 should set compatible cipher suites Oct 11, 2022
@hzxuzhonghu hzxuzhonghu added the release-notes-none Indicates a PR that does not require release notes. label Oct 11, 2022
@hzxuzhonghu
Copy link
Member Author

hzxuzhonghu commented Oct 11, 2022

/test release-notes

ericvn
ericvn reviewed Oct 11, 2022
View reviewed changes
networking/v1alpha3/gateway.proto Outdated
// Optional: Minimum TLS protocol version.
// Optional: Minimum TLS protocol version. By default, it is `TLSV1_2`.
// TLS protocol versions below TLSV1_2 require setting compatible ciphers with the
// `cipherSuites` setting as the default ciphers no longer include compatible ciphers.
Copy link

@ericvn ericvn Oct 11, 2022

Choose a reason for hiding this comment

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

Reading this the default is 1.2. Versions below 1.2 require setting compatible ciphers. Then the last part says that 1.2, the default, no longer includes compatible ciphers. I think we either need to say 1.2 and below don't include compatible ciphers or more likely its change

Suggested change
// `cipherSuites` setting as the default ciphers no longer include compatible ciphers.
// `cipherSuites` setting as they no longer include compatible ciphers.

Copy link
Member Author

@hzxuzhonghu hzxuzhonghu Oct 14, 2022

Choose a reason for hiding this comment

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

done

@howardjohn
Copy link
Member

howardjohn commented Oct 11, 2022

Can we add a warning to the validation webhook for this as well? Or alterntively, just automatically set the ciphers?

@hzxuzhonghu
Copy link
Member Author

hzxuzhonghu commented Oct 13, 2022

Can we add a warning to the validation webhook for this as well? Or alterntively, just automatically set the ciphers?

SG, this will bring better UX

@hzxuzhonghu hzxuzhonghu requested a review from ericvn October 18, 2022 01:58
@hzxuzhonghu
Copy link
Member Author

hzxuzhonghu commented Oct 19, 2022

@howardjohn

@istio-testing istio-testing merged commit 902325b into istio:master Oct 20, 2022
@hzxuzhonghu hzxuzhonghu deleted the added-tls-desc branch October 20, 2022 08:06
@hzxuzhonghu
Copy link
Member Author

hzxuzhonghu commented Oct 20, 2022

/cherry-pick release-1.16

@hzxuzhonghu hzxuzhonghu added the cherrypick/release-1.16 Set this label on a PR to auto-merge it to the release-1.16 branch label Oct 20, 2022
@istio-testing istio-testing mentioned this pull request Oct 20, 2022
Merged
@istio-testing
Copy link
Collaborator

istio-testing commented Oct 20, 2022

@hzxuzhonghu: new pull request created: #2513

Details

In response to this:

/cherry-pick release-1.16

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@istio-testing
Copy link
Collaborator

istio-testing commented Oct 20, 2022

In response to a cherrypick label: new pull request could not be created: failed to create pull request against istio/api#release-1.16 from head istio-testing:cherry-pick-2502-to-release-1.16: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for istio-testing:cherry-pick-2502-to-release-1.16."}],"documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-pull-request"}

louiscryan
louiscryan reviewed Oct 20, 2022
View reviewed changes
networking/v1beta1/gateway.proto
}

// Optional: Minimum TLS protocol version.
// Optional: Minimum TLS protocol version. By default, it is `TLSV1_2`.
Copy link
Contributor

@louiscryan louiscryan Oct 20, 2022

Choose a reason for hiding this comment

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

@lei-tang please review

Copy link
Contributor

@lei-tang lei-tang Oct 20, 2022

Choose a reason for hiding this comment

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

LGTM.
@hzxuzhonghu
Nit: Consider replacing "Using TLS protocol versions below TLSV1_2 has serious security considerations and risks." with "Using TLS protocol versions below TLSV1_2 has serious security risks."

Copy link
Member Author

@hzxuzhonghu hzxuzhonghu Oct 21, 2022

Choose a reason for hiding this comment

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

SG, i will update

lei-tang
lei-tang reviewed Oct 20, 2022
View reviewed changes
Copy link
Contributor

@lei-tang lei-tang left a comment

Choose a reason for hiding this comment

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

LGTM.
Nit: Consider replacing "Using TLS protocol versions below TLSV1_2 has serious security considerations and risks." with "Using TLS protocol versions below TLSV1_2 has serious security risks."

@kfaseela kfaseela mentioned this pull request Oct 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@louiscryan louiscryan louiscryan left review comments

@lei-tang lei-tang lei-tang left review comments

@howardjohn howardjohn howardjohn approved these changes

@ericvn ericvn ericvn approved these changes

Assignees

No one assigned

Labels

cherrypick/release-1.16 Set this label on a PR to auto-merge it to the release-1.16 branch release-notes-none Indicates a PR that does not require release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@hzxuzhonghu @howardjohn @istio-testing @louiscryan @ericvn @lei-tang