Enforce file permissions by liamsi · Pull Request #2286 · tendermint/tendermint

liamsi · 2018-08-27T13:54:12Z

Resolves #2285

codecov-io · 2018-08-27T14:01:59Z

Codecov Report

Merging #2286 into develop will increase coverage by 1.4%.
The diff coverage is 47.05%.

@@            Coverage Diff             @@
##           develop    #2286     +/-   ##
==========================================
+ Coverage    61.26%   62.67%   +1.4%     
==========================================
  Files          197      215     +18     
  Lines        16260    17608   +1348     
==========================================
+ Hits          9962    11035   +1073     
- Misses        5431     5659    +228     
- Partials       867      914     +47

Impacted Files	Coverage Δ
libs/db/fsdb.go	`66.66% <12.5%> (-3.12%)`	⬇️
libs/autofile/autofile.go	`75.34% <77.77%> (+5.64%)`	⬆️
consensus/replay.go	`54.85% <0%> (-11.66%)`	⬇️
libs/db/go_level_db.go	`65% <0%> (-1.67%)`	⬇️
crypto/encoding/amino/amino.go	`84.21% <0%> (-1.51%)`	⬇️
consensus/state.go	`76.58% <0%> (-1.07%)`	⬇️
libs/clist/clist.go	`65.44% <0%> (-1.06%)`	⬇️
node/node.go	`64.26% <0%> (-0.76%)`	⬇️
state/txindex/kv/kv.go	`74.77% <0%> (-0.44%)`	⬇️
privval/priv_validator.go	`69.44% <0%> (ø)`	⬆️
... and 58 more

melekes

🥇 🏎 🍰

melekes · 2018-08-27T14:12:37Z

Do you think it make sense to test new behaviour?

liamsi · 2018-08-27T15:06:32Z

Do you think it make sense to test new behaviour?

I think it basically always makes sense to test any new behaviour. I'm not sure in this case though: The methods in question are private and it would be like testing the correctness of the standard go-library (and if we can properly write to the filesystem where the test is running etc).

But let me add a few lines to the existing autofile_test.go. I would not introduce a new test-file that only has one test (for write) for fsdb.go.

melekes · 2018-08-28T05:15:01Z

libs/autofile/autofile.go

+	if err != nil {
+		return err
+	}
+	fInf, err := file.Stat()


May I suggest a better name: fileInfo? It's a bit hard to decipher fInf.

I will change myself if that's ok

melekes · 2018-08-28T05:16:46Z

libs/db/fsdb.go

 		return err
 	}
 	defer f.Close()
+	fInf, err := f.Stat()


melekes · 2018-08-28T05:18:34Z

libs/autofile/autofile_test.go

+	// Manually modify file permissions, close, and reopen using autofile:
+	// We expect the file permissions to be changed back to the intended perms.
+	file, err := ioutil.TempFile("", "permission_test")
+	if err != nil {


Would be nice to use https://github.com/stretchr/testify here to simplify code

require.NoError(t, err)

ValarDragon

This change looks fine to me, but I'd prefer that we don't change the permission mid-session. Most other tools I use don't do this, they just error on invalid perms. (e.g. things that involve ssh keys) This is going to be an edge case that may frustrate users if they want these files to have certain perms, and they get silently changed. If we prefer to auto-upgrade the perms, we should at least log that we are doing so.

This is kind of minor, so I'm fine with the overall change being merged. (Hence the approve)

melekes · 2018-08-28T07:36:21Z

On the second thought, I don't think we should try to fix errors one level above Tendermint. If someone hacked the OS and changed file's permissions, then owners are screwed in any case. If it's due to some glitch, again it's not the Tendermint task to log that permissions got changed (although it may be nice).

liamsi · 2018-08-28T09:03:18Z

An alternative would be to error if the file permissions changed (as suggested in the related issue #2285). Could be a sentinel error so the caller can handle it and give the user a proper error message on the command-line / the UI. I do not think we should just ignore it if the permissions changed (either due some glitch or a hack).

liamsi · 2018-08-28T09:08:47Z

This is kind of minor, so I'm fine with the overall change being merged. (Hence the approve)

@ValarDragon did you request changes by accident then?

melekes · 2018-08-28T09:10:53Z

if the permissions changed

keep in mind that if Tendermint loses permission to write to that file, it will panic. so we're only talking about group rw/other rw permissions.

xla

Also in favour of returning a meaningful error instead of changing the perms.

melekes · 2018-09-04T08:18:12Z

keep in mind that if Tendermint loses permission to write to that file, it will panic. so we're only talking about group rw/other rw permissions.

That said, the reasonable thing to do is to log a warning (e.g. "%s permissions were modified (was: %v, become: %v)"). Don't think we should panic if Tendermint still has a right to write to the file.

- if we can't read the file, we'll still panic

liamsi · 2018-09-04T14:58:43Z

libs/autofile/autofile_test.go

-	err = af.Close()
-	require.NoError(t, err)
+	require.Error(t, err)
+	t.Log(err)


Will print:
expected file permissions: -rw-------, got: -rwxr-xr-x: file permissions changed

melekes · 2018-09-04T19:11:10Z

libs/autofile/autofile.go

-			return err
-		}
+		return errors.WithMessage(PermissionsChangedErr,
+			fmt.Sprintf("expected file permissions: %v, got: %v", autoFilePerms, fileInfo.Mode()))


does it make sense to print the filename too?

Yes, now it prints:

file: [/var/folders/qf/7nq2274n50v5x9s2y4jcmfgm0000gn/T/permission_test343097720] expected file permissions: -rw-------, got: -rwxr-xr-x

melekes · 2018-09-04T19:11:50Z

libs/autofile/autofile.go

 	autoFilePerms        = os.FileMode(0600)
 )

+var PermissionsChangedErr = errors.New("file permissions changed")


godoc is missing

melekes · 2018-09-04T19:14:59Z

libs/autofile/autofile.go

 	"sync"
 	"time"

+	"github.com/pkg/errors"


we should use https://github.com/tendermint/tendermint/blob/master/libs/common/errors.go

I started with that but it was annoyingly difficult to sth simple as just annotating an error with additional info (the formatted string). Maybe, I was just using it wrong.

Is there a simple example somewhere?

I want a sentinel error as above PermissionsChangedErr (such that the caller can check against it) and simply annotate it with more info. I can use ErrorWrap but I do not see a way to get the annotated content without the whole stack trace a la:

=== RUN TestOpenAutoFilePerms --- PASS: TestOpenAutoFilePerms (0.00s) autofile_test.go:85: --= Error =-- Data: "file permissions changed" Msg Traces: 0 /Users/ismail/go/src/github.com/tendermint/tendermint/libs/autofile/autofile.go:133 - [/var/folders/qf/7nq2274n50v5x9s2y4jcmfgm0000gn/T/permission_test853068086]: expected file permissions : -rw-------, got: -rwxr-xr-x Stack Trace: 0 /Users/ismail/go/src/github.com/tendermint/tendermint/libs/common/errors.go:14 1 /Users/ismail/go/src/github.com/tendermint/tendermint/libs/autofile/autofile.go:133 2 /Users/ismail/go/src/github.com/tendermint/tendermint/libs/autofile/autofile.go:60 3 /Users/ismail/go/src/github.com/tendermint/tendermint/libs/autofile/autofile_test.go:82 4 /usr/local/Cellar/go/1.10.3/libexec/src/testing/testing.go:777 5 /usr/local/Cellar/go/1.10.3/libexec/src/runtime/asm_amd64.s:2362 --= /Error =-- PASS

That's way to verbose for this simple case. Basically, we just want the first msg trace (as possible with github.com/pkg/errors via errors.WithMessage). cmn.NewErrorWithData doesn't provide sth. similar either. NewError can not keep the orig. error/Error.

Am I missing sth. obvious here?

I just got rid of github.com/pkg/errors and type aliased a string. I think that is the simplest way to handle this.

xla

Some changes to make the code more idiomatic.

xla · 2018-09-14T10:06:34Z

libs/autofile/autofile.go

+)
+
+// PermissionsChangedErr occurs if the file permission have changed since the file was created.
+type PermissionsChangedErr string


Prefixing of Err is the de facto standard, compared to suffxing: PermissionsChangedErr -> ErrPermissionsChanged.

xla · 2018-09-14T10:10:00Z

libs/autofile/autofile.go

+		return err
+	}
+	if fileInfo.Mode() != autoFilePerms {
+		return PermissionsChangedErr(fmt.Sprintf("file: [%v]\nexpected file permissions: %v, got: %v",


This error string construction might be better placed in the Error method of the error type and carry the relevant info in a struct:

type ErrPermissionsChanged struct { name string expected, got os.FileMode } func (e ErrPermissionsChanged) Error() string { return fmt.Sprintf( "file: [%v]\nexpected file permissions: %v, got: %v", e.name, e.expected, e.got, ) }

xla · 2018-09-14T10:12:45Z

libs/autofile/autofile.go

+		return err
+	}
+	if fileInfo.Mode() != autoFilePerms {
+		return PermissionsChangedErr(fmt.Sprintf("file: [%v]\nexpected file permissions: %v, got: %v",


Additionally this way of formatting, I know brought it up before, is confusing. There is no clear rule to when to break a line and recognising where one construct starts and another requires a lot of effort. More consistently it should look like this:

return PermissionsChangedErr( fmt.Sprintf( "file: [%v]\nexpected file permissions: %v, got: %v", file.Name(), autoFilePerms, fileInfo.Mode(), ), )

This is allows for faster reading and faster editing of the code.

xla · 2018-09-14T10:13:30Z

libs/db/fsdb.go

 )

+// PermissionsChangedErr occurs if the file permission have changed since the file was created.
+type PermissionsChangedErr string


Prefix over suffix as above.

Is there a reason that both packages need to export their own version of the same error, can those be consolidated?

Of course they could. What package would be a good place? libs/common/errror_types.go ?

libs/common does have errors, but comes with a lot of other things. I'd say a central errors package would be good, maybe for now under libs.

good point. done.

- prefix instead of suffix - add state to err and construct formatting in Error() method

- move error to libs/errors

xla

👍 🍡

LGTM

The current behavior is panic if autofile#openFile returns an error. But for ErrPermissionsChanged, we should warn a user that permissions are different (given we still retain the ability to write to the file). This also modifies fsdb. Issue from our Riot: ``` S: `Panicked on a Sanity Check: Error opening Mempool wal file: file: [/tendermint/mytestnet/node0/data/mempool.wal/wal] expected file permissions: -rw-------, got: -rwxrwxrwx` M: somebody/something changed wal file permissions S: I'm running a docker-compose setup with 4 nodes, It used to work with tendermint 22.0 and started to break when upgraded to tendermint 25.0 ``` Refs #2286

current code results in panic and we certainly don't want that. #2286 (comment)

current code results in panic and we certainly don't want that. tendermint#2286 (comment)

current code results in panic and we certainly don't want that. #2286 (comment)

current code results in panic and we certainly don't want that. tendermint/tendermint#2286 (comment)

Enforce file permissions in case they've changed

2cad5ab

liamsi requested review from ebuchman, melekes, xla and zramsay as code owners August 27, 2018 13:54

liamsi changed the base branch from master to develop August 27, 2018 13:54

melekes approved these changes Aug 27, 2018

View reviewed changes

test behaviour for autofile

37d4b5f

liamsi force-pushed the file_perms branch from a92d0ef to 37d4b5f Compare August 27, 2018 15:43

melekes reviewed Aug 28, 2018

View reviewed changes

use testify in tests and rename fInf to fileInfo

8de6bff

melekes force-pushed the file_perms branch from bc13668 to 8de6bff Compare August 28, 2018 05:33

ValarDragon previously requested changes Aug 28, 2018

View reviewed changes

xla suggested changes Aug 30, 2018

View reviewed changes

melekes assigned liamsi Sep 4, 2018

melekes changed the title ~~Enforce file permissions~~ [WIP] Enforce file permissions Sep 4, 2018

melekes mentioned this pull request Sep 4, 2018

Handling changed file permissions #2285

Closed

return an error if file permissions have changed

4535e52

- if we can't read the file, we'll still panic

liamsi commented Sep 4, 2018

View reviewed changes

melekes reviewed Sep 4, 2018

View reviewed changes

liamsi changed the title ~~[WIP] Enforce file permissions~~ Enforce file permissions Sep 7, 2018

get rid of "github.com/pkg/errors" dependency

28ee747

liamsi force-pushed the file_perms branch from 33b5964 to 28ee747 Compare September 7, 2018 15:23

melekes approved these changes Sep 8, 2018

View reviewed changes

xla suggested changes Sep 14, 2018

View reviewed changes

liamsi added 2 commits September 14, 2018 12:42

address review comments:

cf8bc79

- prefix instead of suffix - add state to err and construct formatting in Error() method

address review comments:

df93ecc

- move error to libs/errors

xla approved these changes Sep 14, 2018

View reviewed changes

melekes merged commit 8ae3334 into develop Sep 17, 2018

melekes deleted the file_perms branch September 17, 2018 10:38

melekes mentioned this pull request Oct 9, 2018

[libs/autofile] log error instead of panicking #2579

Closed

4 tasks

melekes added a commit that referenced this pull request Nov 5, 2018

comment out until someone decides to tackle #2285

861b5f8

current code results in panic and we certainly don't want that. #2286 (comment)

melekes added a commit that referenced this pull request Nov 5, 2018

comment out until someone decides to tackle #2285

b53a1e3

current code results in panic and we certainly don't want that. #2286 (comment)

melekes mentioned this pull request Nov 5, 2018

autofile/fsdb: comment out permissions check until someone decides to tackle #2285 #2760

Merged

enlight pushed a commit to enlight/tendermint that referenced this pull request Nov 6, 2018

comment out until someone decides to tackle tendermint#2285

c4c0f70

current code results in panic and we certainly don't want that. tendermint#2286 (comment)

wu-ziching mentioned this pull request Nov 16, 2018

Facing Error while try to run tendermint using docker #2861

Closed

ebuchman pushed a commit that referenced this pull request Nov 16, 2018

comment out until someone decides to tackle #2285 (#2760)

60018d6

current code results in panic and we certainly don't want that. #2286 (comment)

kfangw pushed a commit to kfangw/blockchain that referenced this pull request Jun 10, 2019

comment out until someone decides to tackle #2285 (#2760)

b55f6de

current code results in panic and we certainly don't want that. tendermint/tendermint#2286 (comment)

Conversation

liamsi commented Aug 27, 2018

Uh oh!

codecov-io commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

melekes commented Aug 27, 2018

Uh oh!

liamsi commented Aug 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ValarDragon left a comment

Choose a reason for hiding this comment

Uh oh!

melekes commented Aug 28, 2018

Uh oh!

liamsi commented Aug 28, 2018

Uh oh!

liamsi commented Aug 28, 2018

Uh oh!

melekes commented Aug 28, 2018

Uh oh!

xla left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liamsi Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamsi Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xla left a comment

codecov-io commented Aug 27, 2018 •

edited

Loading

xla left a comment •

edited

Loading

melekes commented Sep 4, 2018 •

edited

Loading

liamsi Sep 4, 2018 •

edited

Loading

liamsi Sep 14, 2018 •

edited

Loading