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

Security fix (CVE-2018-18264) #3400

Merged
merged 11 commits into from Dec 14, 2018

Conversation

Projects
None yet
5 participants
@floreks
Copy link
Member

floreks commented Dec 7, 2018

Fixes #2672

Skip option has been disabled by default and access to the Dashboard SA has been restricted to our backend only. In case user tries to send a request without auth info 401 error will be returned.

If user tries to "manually" skip login page by adding cookie in the frontend, every page that tries to access API server will look like that:
zrzut ekranu z 2018-12-07 11-13-13

Thanks to #3077 most of the work has been already done.

@floreks

This comment has been minimized.

Copy link
Member

floreks commented Dec 7, 2018

@floreks floreks changed the title Security fix Security fix (CVE-2018-18264) Dec 7, 2018

@maciaszczykm

This comment has been minimized.

Copy link
Member

maciaszczykm commented Dec 7, 2018

Looks good to me. I will do some more testing tomorrow and if it is also okay with others let's make a release to solve this critical issue.

@@ -66,7 +66,7 @@ var (
argMetricClientCheckPeriod = pflag.Int("metric-client-check-period", 30, "Time in seconds that defines how often configured metric client health check should be run. Default: 30 seconds.")
argAutoGenerateCertificates = pflag.Bool("auto-generate-certificates", false, "When set to true, Dashboard will automatically generate certificates used to serve HTTPS. Default: false.")
argEnableInsecureLogin = pflag.Bool("enable-insecure-login", false, "When enabled, Dashboard login view will also be shown when Dashboard is not served over HTTPS. Default: false.")
argDisableSkip = pflag.Bool("disable-skip", false, "When enabled, the skip button on the login page will not be shown. Default: false.")
argDisableSkip = pflag.Bool("disable-skip", true, "When enabled, the skip button on the login page will not be shown. Default: false.")

This comment has been minimized.

@liggitt

liggitt Dec 7, 2018

Member

change Default: false

This comment has been minimized.

@liggitt

liggitt Dec 7, 2018

Member

the double negative makes this hard to follow... can the flag be changed to --enable-skip-login and default to false?

This comment has been minimized.

@floreks

floreks Dec 7, 2018

Member

Sure we can but it also requires frontend and build pipeline refactoring. As skip option should be disabled by default then it is argDisableSkip=true.

This comment has been minimized.

@maciaszczykm

maciaszczykm Dec 7, 2018

Member

I think we should do this anyways.

@@ -279,6 +283,10 @@ func (self *clientManager) extractAuthInfo(req *restful.Request) (*api.AuthInfo,
return self.tokenManager.Decrypt(jweToken)
}

This comment has been minimized.

@liggitt

liggitt Dec 7, 2018

Member

not knowing how this method is used, does returning nil,nil mean the service account credentials are used instead?

if so, shouldn't the if req == nil { case above return an error?

This comment has been minimized.

@liggitt

liggitt Dec 7, 2018

Member

the "If request is nil then authentication will be skipped." comment on Client(req) sounds fragile (a call that doesn't pass in the request would escalate)

This comment has been minimized.

@floreks

floreks Dec 7, 2018

Member

I actually investigated our code to check if it would be possible that external request comes as nil, and it is not. restful-go will always initialize request object for us. The first if req == nil is used to initialize insecureClient (by insecure I mean client that uses dashboard SA privileges) that is used internally by our backend only and return at the end is there, not to break dashboard in case someone chooses to actually set argDisableSkip to false.

This comment has been minimized.

@liggitt

liggitt Dec 10, 2018

Member

I actually investigated our code to check if it would be possible that external request comes as nil, and it is not.

That may be true at this point in time, but a code path could be introduced that could call this with a nil internally as part of building a response to satisfy an external request.

The first if req == nil is used to initialize insecureClient (by insecure I mean client that uses dashboard SA privileges) that is used internally by our backend only

I'd like to see the internal client building made more separate from the function that is used to return clients used to handle external requests, for safety

@@ -79,6 +79,10 @@ function getBackendArgs(mode) {
args.push(`--apiserver-host=${conf.backend.envApiServerHost || conf.backend.apiServerHost}`);
}

if (!conf.backend.disableSkipButton) {

This comment has been minimized.

@liggitt

liggitt Dec 7, 2018

Member

since undefined is falsy, would a missing config value re-enable the skip button? that seems fragile

This comment has been minimized.

@floreks

floreks Dec 7, 2018

Member

This is only used by our development pipeline and can't actually affect anyone.

@jessfraz

This comment has been minimized.

Copy link

jessfraz commented Dec 7, 2018

awesome thanks, agree with @liggitt above but ovverall really great thanks!

floreks added some commits Dec 8, 2018

@floreks

This comment has been minimized.

Copy link
Member

floreks commented Dec 8, 2018

Applied @liggitt 's suggestions and simplified our dev pipeline, not to break anything. To test it on serve/serve:prod use:
gulp serve:prod --serveHttps --autoGenerateCerts --enableSkipButton=false

@liggitt
Copy link
Member

liggitt left a comment

thanks for the updates. a couple more comments, then this looks good to me. note that I haven't swept all callers, or verified there are no code paths that use the internal client to look up data to serve external requests

/**
* Allows to override disable skip button option on the backend.
*/
enableSkipButton: gulpUtil.env.enableSkipButton !== undefined ? gulpUtil.env.enableSkipButton :

This comment has been minimized.

@liggitt

liggitt Dec 10, 2018

Member

simplify to gulpUtil.env.enableSkipButton === true?

This comment has been minimized.

@floreks

floreks Dec 10, 2018

Member

The current one is actually correct as we need skip button to be enabled in our dev environments and tests by default. Most of our UI functionality tests rely on dashboard SA privileges.

It only affects our gulp serve, gulp serve:prod and tasks that rely directly on them, so tests mostly.

So in case someone simply uses gulp serve skip option will be visible. To disable it user will have to provide a enableSkipButton parameter. gulp serve --enableSkipButton=false.

This comment has been minimized.

@liggitt

liggitt Dec 11, 2018

Member

would gulp serve or gulp serve:prod ever be used to actually run the dashboard?

This comment has been minimized.

@floreks

floreks Dec 11, 2018

Member

@liggitt I don't think so. They are used only for manual tests before creating a PR or to check the changes. Also, they do not deploy container in the cluster but rather start a dashboard process outside of the cluster.

Show resolved Hide resolved src/app/backend/args/builder.go Outdated
@@ -279,6 +283,10 @@ func (self *clientManager) extractAuthInfo(req *restful.Request) (*api.AuthInfo,
return self.tokenManager.Decrypt(jweToken)
}

This comment has been minimized.

@liggitt

liggitt Dec 10, 2018

Member

I actually investigated our code to check if it would be possible that external request comes as nil, and it is not.

That may be true at this point in time, but a code path could be introduced that could call this with a nil internally as part of building a response to satisfy an external request.

The first if req == nil is used to initialize insecureClient (by insecure I mean client that uses dashboard SA privileges) that is used internally by our backend only

I'd like to see the internal client building made more separate from the function that is used to return clients used to handle external requests, for safety

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Dec 10, 2018

@floreks

This comment has been minimized.

Copy link
Member

floreks commented Dec 10, 2018

PTAL

floreks added some commits Dec 11, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 11, 2018

Codecov Report

Merging #3400 into master will increase coverage by 0.06%.
The diff coverage is 70.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3400      +/-   ##
==========================================
+ Coverage    54.6%   54.66%   +0.06%     
==========================================
  Files         565      565              
  Lines       12434    12466      +32     
==========================================
+ Hits         6790     6815      +25     
- Misses       5382     5389       +7     
  Partials      262      262
Impacted Files Coverage Δ
src/app/backend/client/manager.go 64.15% <70.49%> (+3.85%) ⬆️
src/app/backend/sync/secret.go 66.94% <0%> (-2.55%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4e81bf...b444f47. Read the comment docs.

@floreks floreks force-pushed the floreks:security-fix branch from 1a377a8 to 2e0f4f3 Dec 11, 2018

@floreks

This comment has been minimized.

Copy link
Member

floreks commented Dec 11, 2018

@liggitt @maciaszczykm PTAL. Everything is green now, so if there are no more issues we could proceed with the release.


// Secure mode means that every request to Dashboard has to be authenticated and privileges
// of Dashboard SA can not be used.
func (self *clientManager) isSecureModeEnabled(req *restful.Request) bool {

This comment has been minimized.

@liggitt

liggitt Dec 12, 2018

Member

this logic is extremely difficult to follow. can we do something like this as a first step to make sure this always returns true when skip login is not enabled?

if !args.Holder.GetEnableSkipLogin() {
  return true
}

This comment has been minimized.

@floreks

floreks Dec 13, 2018

Member

It would have to be a bit different actually.

if self.isLoginEnabled(req) && !args.Holder.GetEnableSkipLogin() {
  return true
}

In case login option is completely disabled we can not block user out of Dashboard. First we have to make sure that it is enabled and then skip option has to be disabled. Bascially if we would split that then whole method would become:

func (self *clientManager) isSecureModeEnabled(req *restful.Request) bool {
	if self.isLoginEnabled(req) && !args.Holder.GetEnableSkipLogin() {
		return true
	}

	authInfo, _ := self.extractAuthInfo(req)
	return self.isLoginEnabled(req) && args.Holder.GetEnableSkipLogin() && authInfo != nil
}

It might be slightly easier to follow. @liggitt WDYT?

This comment has been minimized.

@liggitt

liggitt Dec 14, 2018

Member

that is simpler, but if I've configured the server to use TLS and not skip login, the incoming request shouldn't have any ability to make isSecureModeEnabled return false. Can we make the initial check only look at how the server was configured, not look at req?

This comment has been minimized.

@floreks

floreks Dec 14, 2018

Member

Can we make the initial check only look at how the server was configured, not look at req?

It would make this method even more complicated as we have several arguments responsible for TLS mode. The easiest way is to check if req.Request.TLS != nil. If it is then we are sure that the request itself is secure.

There is no way to alter that from the outside. Only by corrupting restful-go used by dashboard someone would be able to somehow influence that.

This comment has been minimized.

@liggitt

liggitt Dec 14, 2018

Member

Letting a characteristic of an incoming request control bypassing security seems like a bad idea when the intent to serve securely can be determined from the config options alone. I'd like to see that adjusted in the future.

@maciaszczykm

This comment has been minimized.

Copy link
Member

maciaszczykm commented Dec 13, 2018

Tested it locally on prod and it works as expected 👍

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Dec 14, 2018

/lgtm

This improves the current state, though the comments at https://github.com/kubernetes/dashboard/pull/3400/files#discussion_r241135916 are still relevant

@k8s-ci-robot

This comment has been minimized.

Copy link

k8s-ci-robot commented Dec 14, 2018

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@k8s-ci-robot k8s-ci-robot merged commit 5abb13a into kubernetes:master Dec 14, 2018

5 checks passed

cla/linuxfoundation floreks authorized
Details
codecov/patch 70.49% of diff hit (target 54.6%)
Details
codecov/project 54.66% (+0.06%) compared to 4ada1fb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
tide In merge pool.
Details

@maciaszczykm maciaszczykm deleted the floreks:security-fix branch Dec 14, 2018

@floreks floreks referenced this pull request Dec 28, 2018

Closed

Remove skip login option #2672

@liggitt liggitt referenced this pull request Jan 2, 2019

Merged

Bump dashboard to v1.10.1 #72495

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment