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

Constant time password comparison #81152

Merged
merged 1 commit into from Aug 8, 2019

Conversation

@tedyu
Copy link
Contributor

commented Aug 8, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR uses constant time password comparison to fix issue #81126

Which issue(s) this PR fixes:
fixes #81126

kube-apiserver: the `--basic-auth-file` flag and authentication mode is deprecated and will be removed in a future release. It is not recommended for production environments.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

needs bazel update. also should mark the --basic-auth-file parameter deprecated before the referenced issue is completely closed

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

add something like this to where the flag is registered:

fs.MarkDeprecated("basic-auth-file", "Basic authentication mode is deprecated and will be removed in a future release. It is not recommended for production environments.")

@tedyu tedyu force-pushed the tedyu:const-pass-cmp branch from 4886585 to 3d2bc6f Aug 8, 2019

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

@liggitt
Deprecation has been added.

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

/test pull-kubernetes-integration

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

/test pull-kubernetes-e2e-gce

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

/test pull-kubernetes-e2e-gce-100-performance

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

cc @kubernetes/sig-api-machinery-api-reviews @kubernetes/sig-auth-api-reviews for marking the --basic-auth-file flag as deprecated

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, tedyu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fejta-bot

This comment has been minimized.

Copy link

commented Aug 8, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@@ -85,7 +86,7 @@ func (a *PasswordAuthenticator) AuthenticatePassword(ctx context.Context, userna
if !ok {
return nil, false, nil
}
if user.password != password {
if subtle.ConstantTimeCompare([]byte(user.password), []byte(password)) == 0 {

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 8, 2019

Member

arghghghgh

This comment has been minimized.

Copy link
@liggitt

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 8, 2019

Member

translation: incoherent despair that we could possibly have had such a bug in 2019

We also don't salt+hash the plain text passwords in the csv file, sigh...

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

/hold

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

incoherent despair that we could possibly have had such a bug in 2019

great :)
/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit f4e39af into kubernetes:master Aug 8, 2019

23 checks passed

cla/linuxfoundation tedyu authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 8, 2019

@tallclair

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

We should be more specific about the deprecation timeline. I think the standard for flags is 2 releases, which puts removal at 1.18?

@jberkus

This comment has been minimized.

Copy link

commented Aug 13, 2019

@tallclair @liggitt the deprecation notice doesn't make it clear what we're replacing --basic-auth with for development/troubleshooting purposes, nor when we're replacing it. Shouldn't actual removal be dependent on having a replacement first?

@mikedanese

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

We have the static token authenticator to replace this with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.