Back up and restore windows metadata like created ts, file attribs like hidden, readonly, encrypted with a common extensible mechanism #4611
Conversation
aneesh-n
changed the title
aneesh-n
changed the title
There was a problem hiding this comment.
(This is just a preliminary question, not a full review in any way)
Is there a reason why you've copied(?) a large amount of code to internal/fs/ instead of using github.com/Microsoft/go-winio as library? I didn't check how similar / different the fs code is compared to the library.
Using a separate set of attributes instead of trying to hack things into the extended attributes looks like a good idea. That's also reasonably forward-compatible such that older restic version can still restore a backup (minus the new attributes obviously).
I'd prefer to split this PR into smaller parts. At first glance extended attributes support would nicely fit into a PR and everything else into another one. What do you think?
|
The reason I clubbed ExtendedAttributes with this one is because to implement that for windows, it required some refactoring in node.go where we now use restoreExtendedAttributes and fillExtendedAttributes. And that change fits nicely with the addition of restoreGenericAttributes and fillGenericAttributes in all node_xx, making it easier to review together. If that seems acceptable that would be great, otherwise I could go ahead and split them. |
|
W.r.t go-winio, its ea.go does not expose some of the functions we need like SetFileEA, GetFileEA, which we would have to create separately anyways. Similarly, sd.go does not expose SetFileSecurityDescriptor and GetFileSecurityDescriptor for a file path. |
Hmm, the PR in its current state is far too large to review it properly (I usually start complaining around 500+ lines of new code). Most changes for extended and generic attributes appear to be independent of each other, such that we don't gain much from keeping the in one PR. So please split the PRs. We could do the following: first split extended attributes into a separate PR and then once we're done rebase this PR on top of it.
In that case please update the copyright notices to give a rough idea which code is copied from go-winio and which isn't. As the notice was used as a file header, I've assumed that it covers the complete file and ended up confused after noticing that the corresponding files in go-winio are far shorter. |
|
Or we could start with the GenericAttributes CreationTime and FileAttributes, then add security descriptors in a follow up PR and finally add extended attributes. |
aneesh-n
changed the title
Updated this PR. Will create the next ones, once this is done. |
|
I am working on a similar PR, but restricted to the security descriptor (owner, group, dacl and sacl) and extended the node with one member, a optional string named "sddl", which stores the SDDL of the file or folder. My experience is, that the Therefore I decided to not check for those Privileges, but let restic fail on backup ore restore, if privileges, ownership and /or or access rights are not sufficient. Additionally, I planned to have a cli option which can be used to specify which parts of the security descriptor shall be backuped or restored, defaulting to owner, group and DACL, so that restic can be used without hassle by ordinary users for their own files. Users do not care about SACLs anyway. For backup and restore of files of many different users one has to be member of the groups "Administrators" or "Backup Operators". Those usually have the needed Privileges and can access the SACL, too. For accessing the security descriptor I use the functions |
Thanks for the input. In Microsoft's go-winio package (check https://github.com/microsoft/go-winio/blob/bc421d9108ade8ae2f76c8ef3e90cd4ee690e2c0/backuptar/tar.go#L130), they have deprecated sddl-based usage of SecurityDescriptors and instead use base64 encoded raw descriptors. I had updated my implementation from sddl to raw accordingly. I considered the cli options as well but thought it might be adding unnecessary complications in the early implementation. But I do feel there may be valid scenarios where users may want to customize which parts of the SDs should be backed up or restored and if they should be skipped altogether during restore. Could you elaborate on why "Users do not care about SACLs anyway."? It seems important from a security, auditing and incident investigation perspective. I think there would be use cases where it is important. Let me review and test out your remaining points regarding privileges and get back. I have already removed the SecurityDescriptor change from this PR, so this shouldn't affect the current PR. |
I would prefer the SDDL aynway, because it some readable an better for testing.
It is not important, if a normal user want's to backup his own personal data and the SACL is the only part of the sd, which really needs special privileges.
Yes, it's different for the admins, they want backup the SACL, too. But those usually have the privileges to backup/restore the SACL.
I think it's a good idea to make my PR at least halfway ready, so we can compare the solutions. |
|
Agreed. While I agree SDDL is better for testing, the functionality is more important and you do not need to read SDs in normal usage anyways. |
Behind this there is issue winio-go#19 from 2016:
So the switch to base64 encoded sd looks to me like a work around for a bug in windows or winio-go, they have not solved by this time. Since then winio-go switched from it's own sddl-funtions to those of "golang.org/x/sys/windows". So it's in question, whether the bug still exist and there is not enough information about this bug, to check this.
I still prefer SDDL, but restic can do it's work with a base64 encoded sd, too. |
|
Thanks for the suggestions. I did make the change to use GetNamedSecurityInfo and SetNameSecurityInfo which avoid the need for the file handle. |
aneesh-n
changed the title
There was a problem hiding this comment.
I'm partially through with the first review round. Given the size of the PR, I'm pretty sure that I've missed some important things. For restorer_windows_test.go I've stopped reading the code after the first half; it's just too much for one go. It also looks like quite a lot of code from restorer_test.go was copied and partially adapted, see the comment for details.
The commits will also have to be rebased and (partially) squashed (now or later on).
I've also started to wonder whether GenericAttributes should refer to a struct with fixed attributes or whether the current generic list of attributes is better. What's your take on that?
I've finished resolving the review comments. Let me know how we should rebase/squash. Should we squash all commits in this PR? |
|
Sorry for being slow to respond, we had to make a bugfix release of restic, so your PR had to wait. I'll try to find time for it in the next few days. |
There was a problem hiding this comment.
Thanks for all the changes, I'm mostly happy with the code, there are just a few nitpicks in the comments below. The main point that still has to be discussed is the JSON format of the GenericAttributes, see below.
I think the current approach with generic list of arguments could be more flexible to accommodate future use cases for other OSs as well. Also, after using a common Attribute struct the code seems cleaner now.
While thinking about the pros and cons of either variant, I've arrived at the following requirements:
The list of possible attribute names is predetermined by restic. The user cannot influence the name of the attributes. However, later restic versions may add new attribute names. An older restic version should be able to recognize that there are some attributes which it does not support. This should also allow the rewrite command process such Trees, at least as long as no changes to the generic attributes are necessary. The order of attributes is actually irrelevant, it should be up to the implementation to select a suitable order of processing the attributes.
I've noticed that there are actually quite a few possible designs here:
GenericAttributes []Attributesatisfies most requirements, although the attribute order is not necessary and attributes have to be decoded manually (which introduces the risk of missing format checks, and results in leaking encoding details into the tests like the usage ofUInt32ToBytesin the restorer test.)GenericAttributes map[string][]byteis very similar, but gets rid of the unnecessary attribute ordering. The JSON library sorts map keys prior to serialization, such that the resulting JSON encoding is deterministic.GenericAttributes map[string]json.RawMessagewould allow using the JSON decoder to decode values instead of having to do it manually. It still requires code that accesses the generic attribute to know how to deserialize it. This could be combined with a functiongenericAttributesToWindowsAttrs(map[string]json.RawMessage) (WindowsAttributes, []string)that decodes the Windows attributes and returns a list of unknown attributes.GenericAttributes struct { Field1 Type1, Field2 Type2}. Is a much better fit for the property that all attribute values are determined by restic. However, due to limitations of the Go JSON decoder, it would drop unknown fields.GenericAttributesRaw json.RawMessage+genericAttributes struct { Field1 Type1, Field2 Type2} json:"-". The Go JSON decoder would fill inGenericAttributesRaw, which is then further decoded into genericAttributes byUnmarshalJSON.GenericAttributesRawwould only be overwritten if the generic attributes are modified. Detecting unknown attributes would be possible, but based on a quick test it would be extremely messy to implement.
Interestingly, the last three approaches can generate identical JSON representations, variant 2 is also quite similar. Variant 1 has a rather distinct format.
Based on these variants, I'm currently slightly preferring variant 3 (map[string]json.RawMessage). Although that is still open for debate.
Let me know how we should rebase/squash. Should we squash all commits in this PR?
The variant with the least effort would be to split the PR into about three parts:
- helper function extensions (errors package and the test helper; one separate commit for each of them)
- the GenericAttributes code (one large commit is probably OK; adding the GenericAttributes mechanism first and adding the Windows code later would also be an option. Although I don't think it's really worth the effort)
- the restorer changes & test
I've attempted to refactor the Conventions - All I've also fixed all other review comments. |
There was a problem hiding this comment.
I'm somewhat undecided regarding whether it's worth the added complexity to use reflection for handling the generic attributes. as I don't have a strong opinion in either direction, I'd be fine with keeping it. However, what I'd like to change is the naming of the GenericAttributeTypes. These are currently written in CamelCase, which looks very out of place, when taking a look at the JSON representation. All other fields of a Node are written in snake_case, so I'd like to stay consistent in that regard. That would convert WindowsAttributes.CreationTime to windows.creation_time (the attributes part is redundant). So for GenericAttributesToOSAttrs let's just pass in windows as key prefix and use field.Tag.Get("generic") to retrieve the snake_case name (see internal/options/options.go, the struct fields then have to be annotated using `generic:"creation_time" ).
Alternatively, we could also just drop the reflection code and manually implement the conversion code for the two attributes. Just go with the variant, that you think is less work to implement, if it becomes necessary we can still change the implementation in the future.
Besides that I only have a few nitpicks left, see below. Thanks for keeping up with the flood of review comments so far!
This looks good. I've made the required updates. |
internal/restic/node_windows.go
Outdated
| HandleUnknownGenericAttributesFound(unknownAttribs, warn) | ||
| return errors.CombineErrors(errs...) | ||
| } | ||
|
|
||
| // genericAttributesToWindowsAttrs converts the generic attributes map to a WindowsAttributes and also returns a string of unkown attributes that it could not convert. | ||
| func genericAttributesToWindowsAttrs(attrs map[GenericAttributeType]json.RawMessage) (windowsAttributes WindowsAttributes, unknownAttribs []GenericAttributeType, err error) { | ||
| waValue := reflect.ValueOf(&windowsAttributes).Elem() | ||
| unknownAttribs, err = GenericAttributesToOSAttrs(attrs, reflect.TypeOf(windowsAttributes), &waValue) | ||
| unknownAttribs, err = genericAttributesToOSAttrs(attrs, reflect.TypeOf(windowsAttributes), &waValue, runtime.GOOS) |
There was a problem hiding this comment.
I'd actually prefer to just hardcode windows here. Then it's much more obvious what's happening.
Add new generic_attributes attribute in Node. Use the generic attributes to add support for creation time and file attributes like hidden, readonly, encrypted in windows. Handle permission errors for readonly files in windows. Handle backup and restore of encrypted attributes using windows system calls.
Report warnings to terminal when unrecognized generic attributes are found in the repository.
aneesh-n
force-pushed
the
windows-metadata-support
branch
from
7305673
to
e8211cb
Compare
I'm done with the changes and rebasing and squashing the commits. I'll create the Security Descriptor PR next, followed by Extended Attribs for Windows and finally ADS. Appreciate the detailed review and all the suggestions to improve the code. Thank you! |
There was a problem hiding this comment.
LGTM. Thanks a lot for all the work!
One last thing: please try to keep the follow up PRs smaller in size (aim for about 500 lines). I usually try to review the whole PR at once, which at the current size requires a few hours of uninterrupted time which can be rather hard to find.
What does this PR change? What problem does it solve?
Back up and restore windows-specific metadata like file Created time and File Attributes like hidden, readonly, encrypted with a common extensible mechanism.
Restic did not back up multiple windows-specific meta-data.
Restic now backs up file Created time and File Attributes like hidden flag when backing up files and folders on windows.
Note: Separate PRs will be created for SecurityDescriptors, Windows Extended File Attributes, ADS.
Was the change previously discussed in an issue or on the forum?
Add support for CreationTime and File Attributes in Windows -
#3622
Directory and file attribute "hidden" not restored -
Closes #2075
Pull request 3622 attempts to use "CreationTime" and "FileAttributes" as keys in restic's extended attributes.
However, windows can have its own extended attributes - https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_ea_information
Using these 2 values as keys would then cause conflicts while implementing Windows extended attributes, as there could be extended attributes themselves with those names.
The objective of this pull request is to handle numerous pending Windows specific features in a generic way.
This update adds a new "generic_attributes" map to the repository similar to the extended_attributes but is used exclusively for implementing custom OS specific features in restic.
Originally, I had attempted to reuse the extended_attributes array, but that could cause conflicts with real extended_attributes on linux and windows as the custom attributes would be attempted to be restored as real extended_attributes. I also thought of keeping a new IsSpecial flag on extended_attributes which would have value "1" for custom attributes and "0" for real extended_attributes, in an attempt to keep changes to the repository to a minimum, however, that change would break if a new restic repository (with custom attributes added) would be attempted to be restored using an older restic executable.
Hence keeping a separate "generic_attributes" seemed to be the cleanest way to go and this update to the repository is fully backward compatible, i.e. existing repositories will continue to work with the new restic versions and the new features will start working on existing repositories with the updated restic version.
Updated repositories will also work with older restic executables (in which case the new attributes will simply be ignored).
Also, this update of having a "generic_attributes" key-value array could potentially be reused for other OS specific features in future.
I understand the concern with making any update to the repository format and that this requires a thorough review. Please let me know if there are any suggestions to improve this mechanism or a better way to implement this.
Checklist
changelog/unreleased/that describes the changes for our users (see template).gofmton the code in all commits.