Check Kubelet is running with correct Windows Permissions #96616

perithompson · 2020-11-16T18:18:14Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR adds a Windows-specific permissions check on the kubelet startup. Currently, the log file outputs an error on windows stating that the UID is -1 as UID does not exist in a windows context.
Which issue(s) this PR fixes:

Fixes #96512

Special notes for your reviewer:
As part of this PR I have moved the existing checkPermissions function to server_others.go to replicate similar behaviour in kube-proxy as the checkPermissions is not linux specific it should be build whenever not running on windows.

Does this PR introduce a user-facing change?:

NONE

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

The token.IsMember(sid) returns TRUE if the caller's process is a member of the Administrators local group. Caller is NOT
expected to be impersonating anyone and is expected to be able to open its own process and process token. In order for the process to "Run as administrator" the excuting user must be in the BUILTIN\Administrators Group so I have added a check for that too.

More information about this can be found in in the c++ implementation routine and this go issue

Release Notes

Kubelet.exe on Windows now checks that the process running as administrator and the executing user account is listed in the built-in administrators group.  This is the equivalent to checking the process is running as uid 0.

k8s-ci-robot · 2020-11-16T18:18:22Z

Welcome @perithompson!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-11-16T18:18:23Z

Hi @perithompson. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

jayunit100 · 2020-11-16T18:20:21Z

@perithompson ok, next steps get your CLAs sorted internally (i.e. at vmware and sign the cla contributor stuff)

perithompson · 2020-11-17T11:25:49Z

/sig windows

pacoxu · 2021-01-07T06:28:02Z

cmd/kubelet/app/BUILD

@@ -135,7 +137,7 @@ go_library(
        ],
        "@io_bazel_rules_go//go/platform:windows": [
            "//pkg/windows/service:go_default_library",
-            "//vendor/golang.org/x/sys/windows:go_default_library",
+            "//vendor/golang.org/x/sys/windows:go_default_library",    


minor comments here: Extra space.

marosset · 2021-03-09T18:40:50Z

/test pull-kubernetes-e2e-aks-engine-azure-windows
/test pull-kubernetes-e2e-aks-engine-windows-containerd

perithompson · 2021-03-09T18:57:50Z

@perithompson can you rebase the PR?

Not sure how you did it @marosset but it suddenly realised it was ok 🙂

marosset · 2021-03-09T19:16:29Z

/retest

perithompson · 2021-03-09T19:39:29Z

/retest

marosset · 2021-03-10T02:24:05Z

/retest

cmd/kubelet/app/server_windows.go

adisky · 2021-03-10T09:26:26Z

@perithompson The e2e test failures looks legit failures https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/96616/pull-kubernetes-e2e-aks-engine-azure-windows/1369474116045770752

perithompson · 2021-03-10T09:51:28Z

@perithompson The e2e test failures looks legit failures https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/96616/pull-kubernetes-e2e-aks-engine-azure-windows/1369474116045770752

From talking with some of the MSFT guys this was test infra related, but agree it does always seem like the same problem and I was thinking it was something to do with CSI that was causing it. I wouldn't have expected the kubelet to even start with where the checkPermissions function is called, if there was an error in that code then kubelet would fail to start and all the e2es would go red

perithompson · 2021-03-10T13:28:14Z

/retest

perithompson · 2021-03-10T14:24:28Z

/test pull-kubernetes-e2e-kind

perithompson · 2021-03-10T15:36:33Z

@jsturtevant guessing/hoping these are still the same failures?

Mar 10 14:32:59.730: INFO: 
Latency metrics for node k8s-master-36380154-0
Mar 10 14:32:59.730: INFO: Waiting up to 3m0s for all (but 0) nodes to be ready
STEP: Destroying namespace "azuredisk-9336" for this suite.
• Failure [413.600 seconds]
Dynamic Provisioning
/home/prow/go/src/sigs.k8s.io/azuredisk-csi-driver/test/e2e/dynamic_provisioning_test.go:38
  [single-az]
  /home/prow/go/src/sigs.k8s.io/azuredisk-csi-driver/test/e2e/dynamic_provisioning_test.go:41
    should create a statefulset object, write and read to it, delete the pod and write and read to it again [kubernetes.io/azure-disk] [disk.csi.azure.com] [Windows] [It]
    /home/prow/go/src/sigs.k8s.io/azuredisk-csi-driver/test/e2e/dynamic_provisioning_test.go:702
    Unexpected error:
      
        <*errors.errorString | 0xc0003a22b0>: {
            s: "timed out waiting for the condition",
        }
        timed out waiting for the condition
    occurred
    /home/prow/go/src/sigs.k8s.io/azuredisk-csi-driver/test/e2e/testsuites/testsuites.go:634
------------------------------
SSSSSSSSSSSSSSSSSSS
[Fail] Dynamic Provisioning [single-az] [It] should create a statefulset object, write and read to it, delete the pod and write and read to it again [kubernetes.io/azure-disk] [disk.csi.azure.com] [Windows] 
/home/prow/go/src/sigs.k8s.io/azuredisk-csi-driver/test/e2e/testsuites/testsuites.go:634
Ran 9 of 44 Specs in 1796.376 seconds
FAIL! -- 8 Passed | 1 Failed | 0 Pending | 35 Skipped
--- FAIL: TestE2E (1796.38s)
FAIL
FAIL	sigs.k8s.io/azuredisk-csi-driver/test/e2e	1796.452s
FAIL
make: *** [Makefile:112: e2e-test] Error 1
2021/03/10 14:33:06 process.go:155: Step 'make e2e-test' finished in 31m43.364986236s
2021/03/10 14:33:06 aksengine_helpers.go:424: downloading /root/tmp863472555/log-dump.sh from https://raw.githubusercontent.com/kubernetes-sigs/cloud-provider-azure/master/hack/log-dump/log-dump.sh
2021/03/10 14:33:06 util.go:68: curl https://raw.githubusercontent.com/kubernetes-sigs/cloud-provider-azure/master/hack/log-dump/log-dump.sh
2021/03/10 14:33:06 process.go:153: Running: chmod +x /root/tmp863472555/log-dump.sh

 Mar 10 14:16:26.559: INFO: 
Latency metrics for node k8s-master-23042803-0
Mar 10 14:16:26.559: INFO: Waiting up to 3m0s for all (but 0) nodes to be ready
STEP: Destroying namespace "azurefile-5802" for this suite.
• Failure [305.802 seconds]
Dynamic Provisioning
/home/prow/go/src/sigs.k8s.io/azurefile-csi-driver/test/e2e/dynamic_provisioning_test.go:40
  should create a volume on demand with useDataPlaneAPI [kubernetes.io/azure-file] [file.csi.azure.com] [Windows] [It]
  /home/prow/go/src/sigs.k8s.io/azurefile-csi-driver/test/e2e/dynamic_provisioning_test.go:646
[Fail] Dynamic Provisioning [It] should create a volume on demand with useDataPlaneAPI [kubernetes.io/azure-file] [file.csi.azure.com] [Windows] 
/home/prow/go/src/sigs.k8s.io/azurefile-csi-driver/test/e2e/testsuites/testsuites.go:219
Ran 7 of 28 Specs in 709.103 seconds
FAIL! -- 6 Passed | 1 Failed | 0 Pending | 21 Skipped
--- FAIL: TestE2E (709.10s)
FAIL
FAIL	sigs.k8s.io/azurefile-csi-driver/test/e2e	709.155s
FAIL
make: *** [Makefile:84: e2e-test] Error 1
2021/03/10 14:16:38 process.go:155: Step 'make e2e-test' finished in 13m7.41104087s
2021/03/10 14:16:38 aksengine_helpers.go:424: downloading /root/tmp863250335/log-dump.sh from https://raw.githubusercontent.com/kubernetes-sigs/cloud-provider-azure/master/hack/log-dump/log-dump.sh
2021/03/10 14:16:38 util.go:68: curl https://raw.githubusercontent.com/kubernetes-sigs/cloud-provider-azure/master/hack/log-dump/log-dump.sh
2021/03/10 14:16:39 process.go:153: Running: chmod +x /root/tmp863250335/log-dump.sh

jayunit100 · 2021-03-10T15:47:04Z

/lgtm

marosset · 2021-03-10T17:44:07Z

/retest

k8s-ci-robot · 2021-03-10T19:09:01Z

@perithompson: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-aks-engine-azure-windows	ad8de3818efc9253e8aac5ba89fd3dfa33302588	link	`/test pull-kubernetes-e2e-aks-engine-azure-windows`
pull-kubernetes-e2e-azure-file-windows	`46738b7`	link	`/test pull-kubernetes-e2e-azure-file-windows`
pull-kubernetes-e2e-azure-disk-windows	`46738b7`	link	`/test pull-kubernetes-e2e-azure-disk-windows`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

mrunalp · 2021-03-11T23:25:28Z

/approve

k8s-ci-robot · 2021-03-11T23:25:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jayunit100, mrunalp, perithompson

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:

~~cmd/kubelet/OWNERS~~ [mrunalp]

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 added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 16, 2020

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Nov 16, 2020

k8s-ci-robot requested review from vishh and yujuhong November 16, 2020 18:19

perithompson force-pushed the windows-check-permissions branch from 5d4a15f to af6a2ad Compare November 16, 2020 20:11

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 17, 2020

perithompson force-pushed the windows-check-permissions branch from bf096c6 to 0c73106 Compare November 17, 2020 11:06

k8s-ci-robot added the sig/windows Categorizes an issue or PR as relevant to SIG Windows. label Nov 17, 2020

perithompson marked this pull request as ready for review November 17, 2020 11:26

perithompson changed the title ~~(WIP) Check Kubelet is running with correct Windows Permissions~~ Check Kubelet is running with correct Windows Permissions Nov 17, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 17, 2020

ehashman added this to Triage in SIG Node PR Triage Jan 6, 2021

pacoxu reviewed Jan 7, 2021

View reviewed changes

k8s-ci-robot assigned sjenning Mar 9, 2021

ehashman moved this from Needs Reviewer to Needs Approver in SIG Node PR Triage Mar 9, 2021

adisky reviewed Mar 10, 2021

View reviewed changes

cmd/kubelet/app/server_windows.go Outdated Show resolved Hide resolved

cmd/kubelet/app/server_windows.go Outdated Show resolved Hide resolved

perithompson force-pushed the windows-check-permissions branch from ad8de38 to 23eac3c Compare March 10, 2021 11:00

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2021

perithompson force-pushed the windows-check-permissions branch from 23eac3c to f1e2041 Compare March 10, 2021 11:02

Added check for Windows Permissions

46738b7

perithompson force-pushed the windows-check-permissions branch from f1e2041 to 46738b7 Compare March 10, 2021 11:16

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2021

mrunalp moved this from Needs Approver to Done in SIG Node PR Triage Mar 11, 2021

k8s-ci-robot merged commit a8ac0c9 into kubernetes:master Mar 12, 2021

SIG-Windows automation moved this from In Review (v1.21) to Done (v1.21) Mar 12, 2021

This was referenced Feb 15, 2022

Kubelet permissions check on Windows does not work if running as a LocalSystem #108140

Closed

Fixing logic for kubelet permissions check on windows #108146

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check Kubelet is running with correct Windows Permissions #96616

Check Kubelet is running with correct Windows Permissions #96616

perithompson commented Nov 16, 2020 •

edited

k8s-ci-robot commented Nov 16, 2020

k8s-ci-robot commented Nov 16, 2020

jayunit100 commented Nov 16, 2020

perithompson commented Nov 17, 2020

pacoxu Jan 7, 2021

marosset commented Mar 9, 2021

perithompson commented Mar 9, 2021 •

edited

marosset commented Mar 9, 2021

perithompson commented Mar 9, 2021

marosset commented Mar 10, 2021

adisky commented Mar 10, 2021

perithompson commented Mar 10, 2021

perithompson commented Mar 10, 2021

perithompson commented Mar 10, 2021

perithompson commented Mar 10, 2021

jayunit100 commented Mar 10, 2021

marosset commented Mar 10, 2021

k8s-ci-robot commented Mar 10, 2021

mrunalp commented Mar 11, 2021

k8s-ci-robot commented Mar 11, 2021

Check Kubelet is running with correct Windows Permissions #96616

Check Kubelet is running with correct Windows Permissions #96616

Conversation

perithompson commented Nov 16, 2020 • edited

k8s-ci-robot commented Nov 16, 2020

k8s-ci-robot commented Nov 16, 2020

jayunit100 commented Nov 16, 2020

perithompson commented Nov 17, 2020

pacoxu Jan 7, 2021

Choose a reason for hiding this comment

marosset commented Mar 9, 2021

perithompson commented Mar 9, 2021 • edited

marosset commented Mar 9, 2021

perithompson commented Mar 9, 2021

marosset commented Mar 10, 2021

adisky commented Mar 10, 2021

perithompson commented Mar 10, 2021

perithompson commented Mar 10, 2021

perithompson commented Mar 10, 2021

perithompson commented Mar 10, 2021

jayunit100 commented Mar 10, 2021

marosset commented Mar 10, 2021

k8s-ci-robot commented Mar 10, 2021

mrunalp commented Mar 11, 2021

k8s-ci-robot commented Mar 11, 2021

perithompson commented Nov 16, 2020 •

edited

perithompson commented Mar 9, 2021 •

edited