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

Use TLS 1.2 to access healthz API #28895

Closed
wants to merge 1 commit into from
Closed

Conversation

dims
Copy link
Member

@dims dims commented Jul 13, 2016

In the following PR, TLS was set to 1.2 as the minimum
because TLS1.0 and TLS1.1 are vulnerable:
#26169

However the scripts that used curl were not updated to match
the TLS version.

Fixes #28888

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Jul 13, 2016
@dims dims force-pushed the fix-issue-26169 branch 5 times, most recently from 2da2ab1 to 0d34e6c Compare July 13, 2016 21:10
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 13, 2016
@@ -1211,7 +1211,13 @@ function wait-master() {
echo " up."
echo

until $(curl --insecure --user ${KUBE_USER}:${KUBE_PASSWORD} --max-time 5 \
if [[ $(curl -V | awk '{print $2}' | head -1) > 7.34.0 ]]; then
TLS_OPTS="--insecure --tlsv1.2"
Copy link
Member

Choose a reason for hiding this comment

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

It seems like none of these are useful because we are always using --insecure. What extra security does this provide?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mike,

--insecure tells libcurl to not verify the peer (https://curl.haxx.se/docs/sslcerts.html). If you see the man page, it has additional details

(SSL) This option explicitly allows curl to perform "insecure" SSL connections and transfers. All SSL connections are attempted to be made secure by using the CA certificate bundle installed by default. This makes all connections considered "insecure" fail unless -k, --insecure is used.

fyi. bug reporter has confirmed that adding this option makes the scenario work.

the bug reporter has confirmed that adding --tlsv1.2 works as well.

Thanks,
Dims

@k8s-github-robot k8s-github-robot added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. retest-not-required-docs-only and removed retest-not-required-docs-only labels Jul 28, 2016
In the following PR, TLS was set to 1.2 as the minimum
because TLS1.0 and TLS1.1 are vulnerable:
kubernetes#26169

However the scripts that used curl were not updated to match
the TLS version.

Since --tlsv1.2 was introduced in curl 7.34.0, we should check
the version before using the option.

Fixes kubernetes#28888
@dims dims force-pushed the fix-issue-26169 branch from 0d34e6c to 1eb2348 Compare August 5, 2016 12:49
@k8s-bot
Copy link

k8s-bot commented Aug 5, 2016

GCE e2e build/test passed for commit 1eb2348.

@dims
Copy link
Member Author

dims commented Aug 5, 2016

@mikedanese Does not look like there's much interest in this PR. Closing for now

@dims dims closed this Aug 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants