Windows file permissions #104660
Windows file permissions #104660
Conversation
k8s-ci-robot
added
release-note
claudiubelu
force-pushed
the
windows-file-permissions
branch
from
697b146
to
2d56a50
Compare
pkg/volume/util/util_windows.go
Outdated
| ) | ||
|
|
||
| func Chmod(name string, mode os.FileMode) error { | ||
| return acl.Chmod(name, mode) |
There was a problem hiding this comment.
thanks for the PR @claudiubelu
i had a look at hectane/go-acl and here is a short summary.
- it looks well written
- license = MIT
- its only dependency is golang.org/x/sys/windows
https://github.com/hectane/go-acl/blob/master/go.mod - has no release tags, which is mostly problematic for k/k (but not a precedent)
- there are open issues / PRs from 2018 with no response from the maintainers. this is a problematic sign, but at least these seem unrelated to the Chmod usage.
- it requires advapi32.dll to be present on the host for API calls (not new, similarly to Go's stdlib).
- the mapping between the UNIX permissions and Windows is interesting and i haven't seen this before:
https://github.com/hectane/go-acl/blob/master/chmod.go#L14
our SIG Windows experts should have a good look to see if they agree with them.
one question: depending on who / what runs this code on a Windows host and outside of tests, can Administrators be the only group that can change the permissions? i'm assuming this volume code is used in the kubelet and trying to understand if we are somehow changing the requirements of how the kubelet is run on Windows.
/sig windows storage
There was a problem hiding this comment.
@aravindhp the PR / issue you've linked is actually this PR. What is the actually link you meant?
There was a problem hiding this comment.
Sorry about that. I have fixed the link.
There was a problem hiding this comment.
@aravindhp are you sure it was me who had an issue introducing a new library?
As @neolit123 pointed out the way they map UNIX-like permissions to Windows permissions is interesting and I want to take a deeper look at the library.
I am used to using the built in tool icacls to modify permissions. It is not the easiest tool to use but it is very powerful.
can Administrators be the only group that can change the permissions?
On Windows file "owners" can set permissions on files they own even if they aren't administrators. Administrators can assign ownership of files to non-admin users with icacls /setowner.
Admins can almost always change file permissions too (unless the files are owned by TrustedInstalled which is reserved for modifying/updating Windows system components).
There was a problem hiding this comment.
@marosset, sorry my bad. It was @jsturtevant. Slack reference
There was a problem hiding this comment.
On Windows file "owners" can set permissions on files they own even if they aren't administrators. Administrators can assign ownership of files to non-admin users with icacls /setowner.
Admins can almost always change file permissions too (unless the files are owned by TrustedInstalled which is reserved for modifying/updating Windows system components).
ah, yes i know that non-admin user can also change permissions; didn't know about TrustedInstalled though.
i guess my question was more about whether we are going to somehow surprise our users with an unexpected change to the current set of Windows k8s distributions, such as the one that resides in https://github.com/kubernetes-sigs/sig-windows-tools.
possibly/hopefully not.
claudiubelu
force-pushed
the
windows-file-permissions
branch
from
2d56a50
to
58514c2
Compare
There was a problem hiding this comment.
Thanks for doing this, @claudiubelu.
We could translate the Linux user / group / other to the Windows SIDs:
Creator Owner ID / Creator Group ID / World, or more precisely,
S-1-3-0 / S-1-3-1 / S-1-1-0.
I suspect you will still run into issues if the runAsUsername is set in the Pod SecurityContext until microsoft/Windows-Containers#136 is resolved.
pkg/volume/util/util_windows.go
Outdated
| ) | ||
|
|
||
| func Chmod(name string, mode os.FileMode) error { | ||
| return acl.Chmod(name, mode) |
pkg/volume/emptydir/empty_dir.go
Outdated
| @@ -450,7 +450,7 @@ func (ed *emptyDir) setupDir(dir string) error { | |||
| // the thread, clearing the umask, creating the dir, restoring | |||
| // the umask, and unlocking the thread, we do a chmod to set | |||
| // the specific bits we need. | |||
| err := os.Chmod(dir, perm) | |||
| err := volumeutil.Chmod(dir, perm) | |||
There was a problem hiding this comment.
Wouldn't this need to be done in every spot Chmod is being called? Example: https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/util/atomic_writer.go#L398
There was a problem hiding this comment.
Sure, we can do it, although I'm not sure what additional effect it will have.
There was a problem hiding this comment.
True, I am just worried that there are other places where we are not setting the Windows file permissions correctly
There was a problem hiding this comment.
Thanks for doing this, @claudiubelu.
We could translate the Linux user / group / other to the Windows SIDs:
Creator Owner ID / Creator Group ID / World, or more precisely,
S-1-3-0 / S-1-3-1 / S-1-1-0.I suspect you will still run into issues if the
runAsUsernameis set in the PodSecurityContextuntil microsoft/Windows-Containers#136 is resolved.
Will have to double-check, but from what I've seen, from the Container's perspective, all the files / folders belong to the user the Container is running as (ContainerAdministrator, or ContainerUser). I don't remember seeing both names (although, ContainerAdministrator and ContainerUser are so long, they get truncated when running ls). I wonder if this is not the case on Linux Containers as well.
pkg/volume/emptydir/empty_dir.go
Outdated
| @@ -450,7 +450,7 @@ func (ed *emptyDir) setupDir(dir string) error { | |||
| // the thread, clearing the umask, creating the dir, restoring | |||
| // the umask, and unlocking the thread, we do a chmod to set | |||
| // the specific bits we need. | |||
| err := os.Chmod(dir, perm) | |||
| err := volumeutil.Chmod(dir, perm) | |||
There was a problem hiding this comment.
Sure, we can do it, although I'm not sure what additional effect it will have.
pkg/volume/util/util_windows.go
Outdated
| ) | ||
|
|
||
| func Chmod(name string, mode os.FileMode) error { | ||
| return acl.Chmod(name, mode) |
There was a problem hiding this comment.
@aravindhp the PR / issue you've linked is actually this PR. What is the actually link you meant?
|
This was discussed at a sig-windows community meeting and the next steps here are for someone from Microsoft to perform a security review of https://github.com/hectane/go-acl. /assign @immuzz |
claudiubelu
force-pushed
the
windows-file-permissions
branch
from
58514c2
to
ad7f93b
Compare
|
FWIW, here is a bit of information about what this PR enables. It enables the following E2E Conformance tests to run and pass on Windows [1]: The following permissions-related E2E tests are not passing on Windows [2]: As for the reasons, they are as follows:
If we are to proceed with this PR, we'd have to split it into 3 PRs, corresponding to the 3 commits. If we agree on it, I'll go ahead and do that. [1] https://paste.ubuntu.com/p/h73jMr87z9/ |
|
/retest |
|
/milestone v1.24 |
|
New changes are detected. LGTM label has been removed. |
claudiubelu
force-pushed
the
windows-file-permissions
branch
2 times, most recently
from
1452362
to
4c8073e
Compare
The Windows file permissions / ACLs are a bit more complex and fine-grained than the Linux file permissions. However, the Linux file permissions could be translated to some extent to Windows ACLs. We could translate the Linux user / group / other to the Windows SIDs: Creator Owner ID / Creator Group ID / World, or more precisely, S-1-3-0 / S-1-3-1 / S-1-1-0. As for the permissions, we can use the Generic Access Rights: GENERIC_READ / GENERIC_WRITE / GENERIC_EXECUTE. Adds a Windows implementation of Chmod which takes into consideration the details mentioned above. Adds a unit test which verifies that the permissions are set and behave as expected.
claudiubelu
force-pushed
the
windows-file-permissions
branch
from
4c8073e
to
1b6ed5a
Compare
|
@claudiubelu @ddebroy what is the status of this pr? Was there something blocking it? |
|
My concern about this is that it is a change of behavior since previously the field was ignored. Could somebody be depending on the ignored behavior and then break if we make this change? |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
k8s-ci-robot
removed
the
lifecycle/stale
@claudiubelu - can you comment on this? |
|
@claudiubelu ping on this again. Any updates? |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
I think also |
k8s-ci-robot
added
the
sig/security
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: claudiubelu The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
claudiubelu
force-pushed
the
windows-file-permissions
branch
from
805ca07
to
dfa9591
Compare
|
Simplified the code a bit. Reused more of the functions that exists in |
| // Assert that we cannot read the file, as we don't have read permissions. | ||
| // There shouldn't be any ACEs on the file, so os.Open should end up with a "Access is denied." error. | ||
| // However, that doesn't happen. Interestingly, it does happen in other languages, or even Powershell. | ||
| // We test using Powershell instead. |
There was a problem hiding this comment.
Note: I've done some additional debugging here. I've used delve to debug this code, and stopped before Chmod, at line 44:
(dlv) call f.Name()
> k8s.io/kubernetes/pkg/volume/util.TestWindowsFileMode() C:/Users/Administrator/workdir/kubernetes/pkg/volume/util/util_windows_test.go:44 (PC: 0x297c085)
Values returned:
~r0: "C:\\Users\\Administrator\\AppData\\Local\\Temp\\permissions_test3589977734"
39: // Write a sample string into the file, and then expect to be able to read it later.
40: _, err = f.WriteString("hello!")
41: require.NoError(t, err)
42:
43: f.Close()
=> 44: defer os.Remove(f.Name())
45:
46: err = Chmod(f.Name(), 0000)
47: require.NoError(t, err)
48:
49: // Assert that the File Mode changed.
(dlv)
The filename was C:\\Users\\Administrator\\AppData\\Local\\Temp\\permissions_test3589977734, as seen above. As expected, I can read the file without any issues in other languages:
PS C:\Users\Administrator\workdir\kubernetes> python
Python 3.11.2 (tags/v3.11.2:878ead1, Feb 7 2023, 16:38:35) [MSC v.1934 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> f = open("C:\\Users\\Administrator\\AppData\\Local\\Temp\\permissions_test3589977734", "r")
>>> f.read()
'hello!'
>>>
Next, stepping over Chmod where we remove all permissions.:
(dlv) n
> k8s.io/kubernetes/pkg/volume/util.TestWindowsFileMode() C:/Users/Administrator/workdir/kubernetes/pkg/volume/util/util_windows_test.go:50 (PC: 0x297c191)
45:
46: err = Chmod(f.Name(), 0000)
47: require.NoError(t, err)
48:
49: // Assert that the File Mode changed.
=> 50: mode, err := GetFileMode(f.Name())
51: require.NoError(t, err)
52: assert.Equal(t, 0000, int(mode))
53:
54: // Assert that we cannot read the file, as we don't have read permissions.
55: // There shouldn't be any ACEs on the file, so os.Open should end up with a "Access is denied." error.
(dlv)
The expectation is that the file cannot be read anywhere:
PS C:\Users\Administrator\workdir\kubernetes> get-acl C:\Users\Administrator\AppData\Local\Temp\permissions_test3589977734 | fl *
PSPath : Microsoft.PowerShell.Core\FileSystem::C:\Users\Administrator\AppData\Local\Temp\permissions_test3589977734
PSParentPath : Microsoft.PowerShell.Core\FileSystem::C:\Users\Administrator\AppData\Local\Temp
PSChildName : permissions_test3589977734
PSDrive : C
PSProvider : Microsoft.PowerShell.Core\FileSystem
CentralAccessPolicyId :
CentralAccessPolicyName :
Path : Microsoft.PowerShell.Core\FileSystem::C:\Users\Administrator\AppData\Local\Temp\permissions_test3589977734
Owner : BUILTIN\Administrators
Group : WIN-AE7D7USH1E2\None
Access : {}
Sddl : O:BAG:S-1-5-21-2370214383-5423859-4228389985-513D:PAI
AccessToString :
AuditToString :
AccessRightType : System.Security.AccessControl.FileSystemRights
AccessRuleType : System.Security.AccessControl.FileSystemAccessRule
AuditRuleType : System.Security.AccessControl.FileSystemAuditRule
AreAccessRulesProtected : True
AreAuditRulesProtected : False
AreAccessRulesCanonical : True
AreAuditRulesCanonical : True
PS C:\Users\Administrator\workdir\kubernetes> cat C:\Users\Administrator\AppData\Local\Temp\permissions_test3589977734
cat : Access to the path 'C:\Users\Administrator\AppData\Local\Temp\permissions_test3589977734' is denied.
At line:1 char:1
+ cat C:\Users\Administrator\AppData\Local\Temp\permissions_test3589977 ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : PermissionDenied: (C:\Users\Admini..._test3589977734:String) [Get-Content], UnauthorizedAccessException
+ FullyQualifiedErrorId : GetContentReaderUnauthorizedAccessError,Microsoft.PowerShell.Commands.GetContentCommand
PS C:\Users\Administrator\workdir\kubernetes> python
Python 3.11.2 (tags/v3.11.2:878ead1, Feb 7 2023, 16:38:35) [MSC v.1934 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> f = open("C:\\Users\\Administrator\\AppData\\Local\\Temp\\permissions_test3589977734", "r")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
PermissionError: [Errno 13] Permission denied: 'C:\\Users\\Administrator\\AppData\\Local\\Temp\\permissions_test3589977734'
>>>
However, the file can be read in golang, somehow. This issue doesn't occur for the write permissions, in that case we encounter Access is denied, error in golang as well. I suspect that the flags given by golang's Windows' os.Open for reading may have something to do with this, but I didn't dig into that yet.
There was a problem hiding this comment.
PS: Forgot to add the golang bits.
Let's say we add this to the test:
git diff .\pkg\
diff --git a/pkg/volume/util/util_windows_test.go b/pkg/volume/util/util_windows_test.go
index e640e058d76..d3c8edfb259 100644
--- a/pkg/volume/util/util_windows_test.go
+++ b/pkg/volume/util/util_windows_test.go
@@ -64,6 +64,12 @@ func TestWindowsFileMode(t *testing.T) {
t.Fatalf("Unexpected error message while opening the file for reading. Got: %v, expected: %v", err, expectedErrMsg)
}
+ _, err = os.Open(f.Name())
+ expectedErrMsg = "Access is denied."
+ if err == nil || !strings.Contains(err.Error(), expectedErrMsg) {
+ t.Fatalf("Unexpected error message while opening the file for reading. Got: %v, expected: %v", err, expectedErrMsg)
+ }
+
// Assert that we cannot write in the file, as we do not have write permissions.
_, err = os.Create(f.Name())
expectedErrMsg = "Access is denied."
And when we run the tests, we see this:
PS C:\Users\Administrator\workdir\kubernetes> go test -v .\pkg\volume\util\
...
=== RUN TestWindowsFileMode
util_windows_test.go:70: Unexpected error message while opening the file for reading. Got: <nil>, expected: Access is denied.
--- FAIL: TestWindowsFileMode (0.44s)
This occurs after we already checked that we encounter an Access Denied error when cat-ing the file in Powershell. We're expecting to see an "Access Denied" error when opening the file, but we don't see any. If we try to read from the file, we see the following:
diff --git a/pkg/volume/util/util_windows_test.go b/pkg/volume/util/util_windows_test.go
index e640e058d76..6042509b397 100644
--- a/pkg/volume/util/util_windows_test.go
+++ b/pkg/volume/util/util_windows_test.go
@@ -64,6 +64,17 @@ func TestWindowsFileMode(t *testing.T) {
t.Fatalf("Unexpected error message while opening the file for reading. Got: %v, expected: %v", err, expectedErrMsg)
}
+ f, err = os.Open(f.Name())
+ require.NoError(t, err)
+
+ bytes := make([]byte, 64)
+ n, err := f.Read(bytes)
+ require.NoError(t, err)
+ assert.Equal(t, "hello!", string(bytes[:n]))
+
+ f.Close()
+
// Assert that we cannot write in the file, as we do not have write permissions.
_, err = os.Create(f.Name())
expectedErrMsg = "Access is denied."
Output:
=== RUN TestWindowsFileMode
--- PASS: TestWindowsFileMode (0.44s)
Interestingly, we could read from the file and the assertions passed.
claudiubelu
force-pushed
the
windows-file-permissions
branch
from
dfa9591
to
ee104b3
Compare
|
@claudiubelu: The following tests failed, say
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. |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
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-sigs/prow repository. |
What type of PR is this?
/sig windows
What this PR does / why we need it:
The Windows file permissions / ACLs are a bit more complex and fine-grained than the Linux file permissions. However, the Linux file permissions could be translated to some extent to Windows ACLs.
According to [1], we could translate the Linux
user / group / otherto the Windows SIDsCreator Owner ID / Creator Group ID / World, or more precisely,S-1-3-0/S-1-3-1/S-1-1-0.As for the permissions, we can use the Generic Access Rights [3][2]:
GENERIC_READ / GENERIC_WRITE / GENERIC_EXECUTE.[1] https://docs.microsoft.com/en-US/windows/security/identity-protection/access-control/security-identifiers#well-known-sids
[2] https://docs.microsoft.com/en-us/windows/win32/secauthz/access-mask
[3] https://docs.microsoft.com/en-us/windows/win32/secauthz/generic-access-rights
Which issue(s) this PR fixes:
Related: #51540
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: