Atomic writing of files on Windows with a specific security descriptor #3577
Conversation
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
amartinezfayo
requested review from
evan2645,
azdagron,
MarcosDY and
rturner3
as code owners
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
There was a problem hiding this comment.
Thanks, @amartinezfayo! The new function names are very clear in intent and increase the readability, IMO. Nice work. I have only one material comment around the closing of a handle in the unit tests. The rest are nitpicks :)
| // then swaps the file in. os.Rename is an atomic operation, so this sequence avoids having | ||
| // a partially written file at the final location. Finally, fsync is called on the directory | ||
| // to ensure the rename is persisted. | ||
| func AtomicWritePubliclyReadableFile(path string, data []byte) error { |
There was a problem hiding this comment.
nit: seems like the two new functions in here have an identical implementation with exception of the mode; maybe that implementation can be shared?
| // It writes to a temp file first, fsyncs that file, then swaps the file in. | ||
| // Rename file using a custom MoveFileEx that uses 'MOVEFILE_WRITE_THROUGH' | ||
| // witch waits until file is synced to disk. | ||
| func AtomicWritePrivateFile(path string, data []byte) error { |
There was a problem hiding this comment.
nit: the two new functions in here also share an implementation other than the SDDL; maybe that implementation can be shared?
pkg/common/diskutil/file_windows.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func getFileWithSecurityAttr(path, sddl string) (*fileWithSecurityAttr, error) { | ||
| sa := windows.SecurityAttributes{Length: 0} |
There was a problem hiding this comment.
nit: seems like sa is only used for the return value, perhaps it could be created directly in the return value on line 202 using a composite literal...
|
|
||
| require.NoError(t, err) | ||
| sd, err := windows.GetSecurityInfo(handle, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Handle won't be cleaned up if this assertion fails. Perhaps we can defer closing the handle? If it needs to be closed before the rest of the test runs then we can still close on line 84, or, alternatively, this code that grabs the SD could be moved to its own function that handles the cleanup before returning the SD.
There was a problem hiding this comment.
If it needs to be closed before the rest of the test runs then we can still close on line 84
This is indeed the case. I think you meant closing on this line 83 before this assertion? That would be simple and would work well.
pkg/common/diskutil/file_windows.go
Outdated
| // then swaps the file in. Rename file using a custom MoveFileEx that uses 'MOVEFILE_WRITE_THROUGH' witch waits until | ||
| // file is synced to disk. | ||
| func AtomicWriteFile(path string, data []byte, mode os.FileMode) error { | ||
| type fileWithSecurityAttr struct { |
There was a problem hiding this comment.
nit: the naming of this struct, and of the vars at the usage sites, seemed to imply there might need to be something freed or cleaned up... maybe this can be renamed? maybe fileAttribs?
Alternatively, maybe getFileWithSecurityAttribs could be renamed to processPathAndSDDL and return a (pathPtr *uint16, sa *windows.SecurityAttributes, err error)?
There was a problem hiding this comment.
Good call. Naming it fileAttribs would be more clear.
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
spiffe#3577) * Atomic writing of files on Windows with a specific security descriptor Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Current
AtomicWriteFilefunction in thediskutilpackage does not allow to define a specific security descriptor to be used in the files written on Windows.In SPIRE, we call this function to either atomically write files with permissions granted to read/write to the owner only, or with permissions granted to read/write to the owner and to read by everyone.
This PR makes the changes to have two separate functions that have os-specific implementations:
AtomicWritePrivateFileandAtomicWritePubliclyReadableFilethat respect the intended permissions in each platform.Fixes part of #3189.