clientv3: make saved snapshot readable by user only #9977
Conversation
|
Note for reviewer: Previously, the file was opened with |
dlipovetsky
force-pushed
the
etcdctl-snapshot-permissions
branch
from
8ac1e51
to
c5c63ab
Compare
Codecov Report
@@ Coverage Diff @@
## master #9977 +/- ##
==========================================
- Coverage 69.34% 69.24% -0.11%
==========================================
Files 386 386
Lines 35914 35914
==========================================
- Hits 24905 24868 -37
- Misses 9212 9257 +45
+ Partials 1797 1789 -8
Continue to review full report at Codecov.
|
| @@ -24,6 +24,8 @@ import ( | |||
| "testing" | |||
| "time" | |||
|
|
|||
| "github.com/coreos/etcd/pkg/fileutil" | |||
|
|
|||
| "github.com/coreos/etcd/clientv3" | |||
There was a problem hiding this comment.
Will fix, thanks!
| @@ -102,7 +102,7 @@ func (s *v3Manager) Save(ctx context.Context, cfg clientv3.Config, dbPath string | |||
| defer os.RemoveAll(partpath) | |||
|
|
|||
| var f *os.File | |||
| f, err = os.Create(partpath) | |||
| f, err = os.OpenFile(partpath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fileutil.PrivateFileMode) | |||
| if err != nil { | |||
| return fmt.Errorf("could not open %s (%v)", partpath, err) | |||
There was a problem hiding this comment.
If possible could you verify this works on Windows?
There was a problem hiding this comment.
Good point. I'll look into this.
The os.Chmod docs indicates that on Windows, this change will not affect file permissions:
On Windows, the mode must be non-zero but otherwise only the 0200 bit (owner writable) of mode is used; it controls whether the file's read-only attribute is set or cleared. attribute. The other bits are currently unused. Use mode 0400 for a read-only file and 0600 for a readable+writable file.
I'll run an experiment on Windows and update with the results.
|
@dlipovetsky, thank you for the PR a few nits here but otherwise, this works as expected for me locally. As the previous permissions of the snapshot are something that could be assumed in various workflows we will need to document this possible breaking change well. Also as I noted in the review if we could verify the OpenFile usage work fine in Windows I think that would be smart as I am not sure our tests cover this. I guess the final question is should the client be in charge of this? While it should be up to the admin to provide a secure env I think that giving the user a little help and being proactive about something as important as a snapshot is a good thing and a step forward so I vote +1. |
|
I ran
For completeness, here's the top of the etcd log (with my commit in the version): |
|
@hexfusion I think my experiment demonstrates that this PR does not break |
|
@dlipovetsky as you may have seen a lot of effort is being put into the CNCF TOC meeting which is tomorrow. Please allow a few days for this PR to get squared away as I want to get additional feedback from the community. Thank you for the detailed Windows tests I am very happy we had no regression there. |
|
@hexfusion Thanks for your feedback! I'll keep an eye for an update. |
|
Can you fix https://travis-ci.org/coreos/etcd/jobs/411980747#L607? This should be safe, since we also use private file mode for WAL. And when snapshot file is fetched from remote storage, the user can always updates its file permission for their own use case. |
|
@gyuho Thanks for your feedback. The test failure should be be resolved when I squash the commits. I'll do that now. |
dlipovetsky
force-pushed
the
etcdctl-snapshot-permissions
branch
from
418b7ac
to
ddde272
Compare
|
@dlipovetsky thank you for your contribution, I hope this is the first of many. |
|
@dlipovetsky Could you also add change log here https://github.com/coreos/etcd/blob/master/CHANGELOG-3.4.md#client-v3, so people know who worked on it? |
|
@hexfusion Thanks for guiding me along! |
Fixes #9976