Fix for unauthenticated secret access

[floreks]

Fix for a security issue reported by k8s security team. We will reveal all the information about the issue later on.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floreks

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:

• OWNERS [floreks]

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

[codecov]

Codecov Report

Merging #3289 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #3289      +/-   ##
=========================================
- Coverage   54.61%   54.6%   -0.02%     
=========================================
  Files         565     565              
  Lines       12430   12433       +3     
=========================================
  Hits         6789    6789              
- Misses       5379    5382       +3     
  Partials      262     262

Impacted Files	Coverage Δ
src/app/backend/auth/api/types.go	30.76% <ø> (ø)	⬆️
src/app/backend/auth/api/common.go	66.66% <0%> (-13.34%)	⬇️

---

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 cfc62d8...4016a2c. Read the comment docs.

[jessfraz]

Can you just return unauthenticated instead?

[maciaszczykm]

@floreks Tested current solution and it works as it was expected.

We should investigate @jessfraz suggestion though.

[bryk]

That's a good quick fix solution. Although I suggest two follow ups:

1. @jessfraz suggestion on unauthenticated when user sends no auth headers
2. Changing default config to that secrets are not visible at all. I believe that'd better approach than the current one which has exclude list. I believe we can do this easily and prevent similar bugs in the future.
   WDYT?

[floreks]

@jessfraz suggestion on unauthenticated when user sends no auth headers

IMHO even if user is authenticated he should not have access to those dashboard exclusive resources. They are critical and all users should be blocked from viewing them. There is no point for users to actually view or modify them. They should be only accessible using kubectl. If we would follow this path then I guess "Unauthorized" is a correct error.

Changing default config to that secrets are not visible at all. I believe that'd better approach than the current one which has exclude list. I believe we can do this easily and prevent similar bugs in the future.
WDYT?

In case user tries to use direct link to access the resource should we just show 404 resource not found error? This could be a bit misleading as kubectl get secret xxx would still return it correctly.

[maciaszczykm]

@floreks I agree that we can block access in all scenarios, but what about blocking access to whole Dashboard when there is no auth token provided?

[floreks]

@maciaszczykm when there is no auth token it does not mean that no one is authenticated. It means that user is authenticated as the Dashboard user. Only in case where Skip option is disabled we should always redirect to the login page if user has not logged in.

[maciaszczykm]

@floreks Yes, but perhaps we should disable Skip by default to improve security. There are plenty of users that try to use Dashboard without getting into details and using setup that "just works".

[liggitt]

It means that user is authenticated as the Dashboard user

I would never want this behavior in a dashboard deployment I was responsible for. What can we do to enable requiring user login and prevent any possibility of use of the dashboard credentials via web requests to the dashboard?

[liggitt]

this method is basically "allow by default", with a list of specific exclusions. it is vulnerable to drifting from the actual permissions granted to the dashboard service account. I'd strongly recommend supporting a mode that requires user login and never allows dashboard users to make use of dashboard service account API permissions.

[floreks]

We have to think how to achieve that as we have to support an alternative option also where our login mechanism is disabled and user relies on authentication header or dashboard privileges to use it.

[maciaszczykm]

@liggitt I agree that this is something that we need to investigate and fix.

[floreks]

I would never want this behavior in a dashboard deployment I was responsible for. What can we do to enable requiring user login and prevent any possibility of use of the dashboard credentials via web requests to the dashboard?

We should first disable Skip option by default and not allow access to any views except login in case user has not logged in. This should be easy to do if login mechanism is enabled.

[jessfraz]

can we move this forward... thanks!

[maciaszczykm]

It's merged as a quick fix. We have to investigate how to disable skipping auth, so no one will be able to enter any other page than login if he is not authenticated already.

[jessfraz]

I’d really love to see this fixed well the first time, even if it takes more time. Can you explain the problems with changing this as per @liggitt’s comments

[bryk]

@jessfraz do you perhaps know someone who could contribute the change? Seems like either we do with showing no secrets at all or disabling the "skip" option. Both of them sound like deleting code.

[philips]

@maciaszczykm @bryk was a longterm fix implemented?

[maciaszczykm]

@philips not yet, it is still on our roadmap as we transition to the new version of the frontend.

[philips]

Can you file a new issue so I can watch that one?

…

On Wed, Nov 28, 2018 at 11:19 PM Marcin Maciaszczyk < ***@***.***> wrote: @philips <https://github.com/philips> not yet, it is still on our roadmap as we transition to the new version of the frontend. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3289 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACDCPmDuLODjv01RkjbtwgkFPSoP9Ylks5uz4qFgaJpZM4XYOX5> .

[maciaszczykm]

@philips #2672