Add warning for directory permission to in_tail.rb #4865

moatom · 2025-03-14T21:23:20Z

Which issue(s) this PR fixes:
~~Fixes #~~

What this PR does / why we need it:
This PR provides a warning for typical configuration mistakes that prevent log transfer from starting.

When using the tail type as a source, it is common for directories with inappropriate partitions to be mistakenly included in the source path.

In my opinion, it would be preferable to issue a warning in such cases to help users avoid these mistakes.

Steps to Reproduce:

$ sudo mkdir /var/log/noaccess
$ sudo mkdir /var/log/noaccess/noaccess2
$ sudo touch /var/log/noaccess/noaccess2/fluentd.log
$ sudo chmod 0000 /var/log/noaccess
$ sed -Ei 's|^([[:space:]]*)path .*|\1path /var/log/noaccess/noaccess2/fluentd.log|' example/in_tail.conf
$ bundle exec fluentd -c example/in_tail.conf
{no warning...}

Docs Changes:
No documentation changes required.

Release Note:
in_tail: add warning for directory permission.

moatom · 2025-03-16T11:47:53Z

lib/fluent/plugin/in_tail.rb

@@ -385,6 +385,13 @@ def expand_paths
          $log.warn "expand_paths: stat() for #{path} failed with #{e.class.name}. Skip file."
        end
      }
+      if check_permission
+        (paths - excluded).map { |path| File.dirname(path) }.uniq.each { |dir|


It might be better to add select { |path| !FileTest.exist?(path) } before map.

moatom · 2025-03-17T05:52:42Z

Test / Ruby 3.4 on macos-latest (pull_request)Failing after 23m

https://github.com/fluent/fluentd/actions/runs/13865592763/job/38858881719?pr=4865#step:6:13416

4) Failure: test_unwatched_files_should_be_removed(TailInputTest::path)
/Users/runner/work/fluentd/fluentd/test/plugin/test_in_tail.rb:1757:in 'test_unwatched_files_should_be_removed'
     1754:       waiting(20) { sleep 0.1 until Dir.glob("#{@tmp_dir}/*.txt").size == 0 } # Ensure file is deleted on Windows
     1755:       waiting(5) { sleep 0.1 until d.instance.instance_variable_get(:@tails).keys.size <= 0 }
     1756: 
  => 1757:       assert_equal(
     1758:         {
     1759:           files: [],
     1760:           tails: []
<{:files=>[], :tails=>[]}> expected but was
<{:files=>[],
 :tails=>
  ["/Users/runner/work/fluentd/fluentd/test/plugin/../tmp/tail/6085abb32771a48075e1/tail.txt"]}>

diff:
? {:files=>[], :tails=>[]}
+  :tails=>
+   ["/Users/runner/work/fluentd/fluentd/test/plugin/../tmp/tail/6085abb32771a48075e1/tail.txt"]}
Error: <{:files=>[], :tails=>[]}> expected but was
<{:files=>[],
 :tails=>
  ["/Users/runner/work/fluentd/fluentd/test/plugin/../tmp/tail/6085abb32771a48075e1/tail.txt"]}>.

diff:
- {:files=>[], :tails=>[]}
+ {:files=>[],
+  :tails=>
+   ["/Users/runner/work/fluentd/fluentd/test/plugin/../tmp/tail/6085abb32771a48075e1/tail.txt"]}

👀

daipom · 2025-03-17T07:02:22Z

Test / Ruby 3.4 on macos-latest (pull_request)Failing after 23m

https://github.com/fluent/fluentd/actions/runs/13865592763/job/38858881719?pr=4865#step:6:13416

👀

@moatom Thanks for this PR!
I will review this soon!

This test failure is a known test issue, so we don't need to care about it in this PR.

daipom · 2025-03-17T09:32:31Z

This PR provides a warning for typical configuration mistakes that prevent log transfer from starting.
When using the tail type as a source, it is common for directories with inappropriate partitions to be mistakenly included in the source path.

I see.

In some cases, Permission denied warning logs appear.
In other cases, they do not.

If the directory does not have the x permission, then warning logs do not appear.
This PR will solve that problem.

daipom · 2025-03-17T10:22:09Z

@moatom
Thanks for this PR!
The direction of checking the readability of the parent directory seems to have some effect.
However, I have some concerns.

A: It does not seem to be able to support the case of using glob (such as *).
B: We do not want to warn for paths that do not yet exist.

About A, it would be possible that this feature supports only static paths that do not use wildcards, etc.

About B, I feel the essential difficulty of this issue.
There could be cases where the mid-level parent directory, rather than the direct parent directory, does not have x permissions.
As long as there is no x permission, it would be difficult to check whether the path does not exist yet or whether the permission is inappropriate.
If we do this strictly, it will be necessary to find the directories whose existence can be verified and check their permissions.

I don't think it necessarily needs to be strictly confirmed.
However, it would be better to take these considerations into account and be clear about which cases are to be supported.

For example, I want to avoid false positives, such as generating warning logs for paths that just do not yet exist.

daipom · 2025-03-18T05:15:28Z

However, it would be better to take these considerations into account and be clear about which cases are to be supported.

For example, I want to avoid false positives, such as generating warning logs for paths that just do not yet exist.

To avoid false positives, we can consider the following direction for example.

Instead of checking directory permissions, generate a warning log in the next process when the path does not exist.

fluentd/lib/fluent/plugin/in_tail.rb

Lines 372 to 374 in e3dcebc

    
           (paths - excluded).select { |path| 
        
             FileTest.exist?(path) 
        
           }.each { |path|

Log message:

... [warn] ... Unable to confirm the existence of #{path}.
You can ignore this warning log if the file is created later.
If the file already exists, please check the configuration and directory permissions.

(This supports only static paths that do not use wildcards, etc.)

moatom · 2025-03-21T13:45:57Z

@daipom Thanks for the review and I'm sorry for my late response🙇

If the directory does not have the x permission, then warning logs do not appear.

Oops. I thought the required permission was r. FileTest.readable?(dir) in the commit might not be appropriate...

(This supports only static paths that do not use wildcards, etc.)

The following part seems to expand the glob, so I believe this issue shouldn't occur (if my understanding is correct):

https://github.com/fluent/fluentd/pull/4865/files#diff-456fdeb51bc472beb48891caac0d063e0073655dba7ac2b72e6fdc67dc6ac802L333-R346

I don't think it necessarily needs to be strictly confirmed.
However, it would be better to take these considerations into account and be clear about which cases are to be supported.

For example, I want to avoid false positives, such as generating warning logs for paths that just do not yet exist.

I am polishing my idea as follows:

Experienced users are the ones who primarily use complex paths, and they are likely to manage to notice issues on their own. Therefore, in this PR, I would like to focus on adding a warning for the following simple case (if glob expansion does not occur, I want to ignore such cases):

/p_1/p_2/.../p_t/.../p_n

where

p_i is a directory for 0<=i<=n-1
- p_0 := / (the root directory)
p_n is the log file
p_t is the shallowest directory (resp. file, if t = n) that lacks the x (resp. r) permission in the path.

For this specific case, the ideal conditions for generating warnings can be summarized in the following table:

↓ All Permissions in the Path \ → File (`p_n`) Actually Exist	Exist	Not Exist
Granted (`p_t` doesn't exist)	○ (No Warning)	○ (No Warning)
Not Granted (`p_t` exists)	☓ (Warning about `p_t`)	☓ (Warning about `p_t`)

We are concerned about false positives for Granted×Not Exist (We need this test case).
Can we avoid it by not emitting warnings when we cannot detect p_t from p_0 .. p_{n-1}?

To avoid false positives, we can consider the following direction for example.

Log message:
... [warn] ... Unable to confirm the existence of #{path}.
You can ignore this warning log if the file is created later.
If the file already exists, please check the configuration and directory permissions.

I agree with this direction if the above approach is not effective!

moatom · 2025-03-21T14:15:10Z

(This supports only static paths that do not use wildcards, etc.)

The following part seems to expand the glob, so I believe this issue shouldn't occur (if my understanding is correct):

https://github.com/fluent/fluentd/pull/4865/files#diff-456fdeb51bc472beb48891caac0d063e0073655dba7ac2b72e6fdc67dc6ac802L333-R346

I might have missed cases where logs are emitted only at the debug level:
https://github.com/fluent/fluentd/pull/4865/files#diff-456fdeb51bc472beb48891caac0d063e0073655dba7ac2b72e6fdc67dc6ac802R352-R355

Signed-off-by: Tomoaki Kobayashi <[email protected]>

moatom · 2025-03-21T17:41:28Z

lib/fluent/plugin/in_tail.rb

      (paths - excluded).select { |path|
+        if check_permission && !FileTest.readable?(path)
+          is_bad_permission = begin
+            current_path = File.expand_path(path)
+            loop do
+              current_path = File.dirname(current_path)
+              break true unless FileTest.executable?(current_path)
+              break false if current_path == "/"
+            end
+          end
+          if is_bad_permission
+            $log.warn "Skip #{path} because a directory in the path lacks execute permission."
+          end
+        end
        FileTest.exist?(path)
      }.each { |path|


This should work fine unless I've overlooked something.

daipom · 2025-03-25T02:53:27Z

@moatom
Thanks for the update! I will see it soon.

(This supports only static paths that do not use wildcards, etc.)

The following part seems to expand the glob, so I believe this issue shouldn't occur (if my understanding is correct):

https://github.com/fluent/fluentd/pull/4865/files#diff-456fdeb51bc472beb48891caac0d063e0073655dba7ac2b72e6fdc67dc6ac802L333-R346

If there are not enough permissions for the directory, Dir.glob() does not return the files under that directory.
So, it would be very difficult to tell if the files do not exist or if there are not enough permissions.

daipom · 2025-03-28T05:15:06Z

Experienced users are the ones who primarily use complex paths, and they are likely to manage to notice issues on their own. Therefore, in this PR, I would like to focus on adding a warning for the following simple case (if glob expansion does not occur, I want to ignore such cases):

This would be no problem!

We are concerned about false positives for Granted×Not Exist (We need this test case).
Can we avoid it by not emitting warnings when we cannot detect p_t from p_0 .. p_{n-1}?

Yes. False positives are my concern.
break true unless FileTest.executable?(current_path) breaks when the current_path does not exist.
So, it causes false positives.

To avoid false positives, we can consider the following direction for example.

Log message:
... [warn] ... Unable to confirm the existence of #{path}.
You can ignore this warning log if the file is created later.
If the file already exists, please check the configuration and directory permissions.
I agree with this direction if the above approach is not effective!

If it is difficult to resolve this false positive, let's go in this direction.

I explain specific examples of the false positive below.

Case

For example, consider the following case.

$ sudo ls -lR /tmp/2
/tmp/2:
合計 12
drwxrwx--- 2 root root 4096  3月 28 13:47 d---
drwxrwx--x 2 root root 4096  3月 28 13:47 d--x
drwxrwxr-x 2 root root 4096  3月 28 13:46 dr-x

/tmp/2/d---:
合計 8
-rw-r----- 1 root root 6  3月 28 13:47 file_---.log
-rw-r--r-- 1 root root 5  3月 28 13:47 file_r--.log

/tmp/2/d--x:
合計 8
-rw-r----- 1 root root 6  3月 28 13:47 file_---.log
-rw-r--r-- 1 root root 5  3月 28 13:47 file_r--.log

/tmp/2/dr-x:
合計 8
-rw-rw---- 1 root root 6  3月 28 13:46 file_---.log
-rw-rw-r-- 1 root root 5  3月 28 13:37 file_r--.log

Let in_tail collect these 6 patterns and non-existent path (totally 7 paths).

<source>
  @type tail
  tag test
  path /tmp/2/dr-x/file_r--.log, /tmp/2/dr-x/file_---.log, /tmp/2/d--x/file_r--.log, /tmp/2/d--x/file_---.log, /tmp/2/d---/file_r--.log, /tmp/2/d---/file_---.log, /tmp/2/not_exist/test.log
  read_from_head true
  refresh_interval 5s
  <parse>
    @type none
  </parse>
</source>

<match test.**>
  @type stdout
</match>

Result: master

path	read	warning
`dr-x/file_r--.log`	y	-
`dr-x/file_---.log`	n	y
`d--x/file_r--.log`	y	-
`d--x/file_---.log`	n	y
`d---/file_r--.log`	n	n
`d---/file_---.log`	n	n
`not_exist/test.log`	-	n

[info]: #0 following tail of /tmp/2/dr-x/file_r--.log
{datetime} test: {"message":"test"}
[info]: #0 following tail of /tmp/2/dr-x/file_---.log
[warn]: #0 Permission denied @ rb_sysopen - /tmp/2/dr-x/file_---.log
[info]: #0 following tail of /tmp/2/d--x/file_r--.log
{datetime} test: {"message":"test"}
[info]: #0 following tail of /tmp/2/d--x/file_---.log
[warn]: #0 Permission denied @ rb_sysopen - /tmp/2/d--x/file_---.log

Result: PR

path	read	warning
`dr-x/file_r--.log`	y	-
`dr-x/file_---.log`	n	y
`d--x/file_r--.log`	y	-
`d--x/file_---.log`	n	y
`d---/file_r--.log`	n	y
`d---/file_---.log`	n	y
`not_exist/test.log`	-	y (FP)

[warn]: #0 Skip /tmp/2/d---/file_r--.log because a directory in the path lacks execute permission.
[warn]: #0 Skip /tmp/2/d---/file_---.log because a directory in the path lacks execute permission.
[warn]: #0 Skip /tmp/2/not_exist/test.log because a directory in the path lacks execute permission.
[info]: #0 following tail of /tmp/2/dr-x/file_r--.log
{datetime} test: {"message":"test"}
[info]: #0 following tail of /tmp/2/dr-x/file_---.log
[warn]: #0 Permission denied @ rb_sysopen - /tmp/2/dr-x/file_---.log
[info]: #0 following tail of /tmp/2/d--x/file_r--.log
{datetime} test: {"message":"test"}
[info]: #0 following tail of /tmp/2/d--x/file_---.log
[warn]: #0 Permission denied @ rb_sysopen - /tmp/2/d--x/file_---.log

moatom · 2025-04-06T23:53:59Z

If it is difficult to resolve this false positive, let's go in this direction.

Thanks for explaining the case!
I'll try to investigate this when I have time.

I'm thinking it might work if I switch the traversal direction in the code above to top-down instead.
#4865 (comment)

Signed-off-by: Tomoaki Kobayashi <[email protected]>

moatom · 2025-04-11T16:04:33Z

@daipom
I have fixed this PR to work for all cases!🙏
If it looks good, should I squash the commits and add all 7 test cases to test/plugin/test_in_tail.rb?

P.S. I'm not sure, but it might be a false negative when even / lacks the required permissions in this approach. (Should I mention this in a comment?)

Result: Latest PR

<source>
  @type tail
  tag test
  path /tmp/2/dr-x/file_r--.log, /tmp/2/dr-x/file_---.log, /tmp/2/d--x/file_r--.log, /tmp/2/d--x/file_---.log, /tmp/2/d---/file_r--.log, /tmp/2/d---/file_---.log, /tmp/2/not_exist/test.log
  pos_file /tmp/fluentd-test.pos
  read_from_head true
  refresh_interval 5s
  <parse>
    @type none
  </parse>
</source>

<match test.**>
  @type stdout
</match>

path	read	warning
`dr-x/file_r--.log`	y	-
`dr-x/file_---.log`	n	y
`d--x/file_r--.log`	y	-
`d--x/file_---.log`	n	y
`d---/file_r--.log`	n	y
`d---/file_---.log`	n	y
`not_exist/test.log`	-	n

$ sudo ls -lR /tmp/2
/tmp/2:
total 12
drwxrwx--- 2 root root 4096 Apr 11 14:09 d---
drwxrwx--x 2 root root 4096 Apr 11 14:09 d--x
drwxrwxr-x 2 root root 4096 Apr 11 14:09 dr-x

/tmp/2/d---:
total 8
-rw-r----- 1 root root 5 Apr 11 14:11 file_---.log
-rw-r--r-- 1 root root 5 Apr 11 14:11 file_r--.log

/tmp/2/d--x:
total 8
-rw-r----- 1 root root 5 Apr 11 14:11 file_---.log
-rw-r--r-- 1 root root 5 Apr 11 14:11 file_r--.log

/tmp/2/dr-x:
total 8
-rw-rw---- 1 root root 5 Apr 11 14:11 file_---.log
-rw-rw-r-- 1 root root 5 Apr 11 14:11 file_r--.log

$ bundle exec fluentd -c in_tail.conf
025-04-11 15:44:22 +0000 [warn]: #0 Skip /tmp/2/d---/file_r--.log because '/tmp/2/d---' lacks execute permission.
2025-04-11 15:44:22 +0000 [warn]: #0 Skip /tmp/2/d---/file_---.log because '/tmp/2/d---' lacks execute permission.
2025-04-11 15:44:22 +0000 [info]: #0 following tail of /tmp/2/dr-x/file_r--.log
2025-04-11 15:44:22.441945749 +0000 test: {"message":"test"}
2025-04-11 15:44:22 +0000 [info]: #0 following tail of /tmp/2/dr-x/file_---.log
2025-04-11 15:44:22 +0000 [warn]: #0 Permission denied @ rb_sysopen - /tmp/2/dr-x/file_---.log
2025-04-11 15:44:22 +0000 [info]: #0 following tail of /tmp/2/d--x/file_r--.log
2025-04-11 15:44:22.442540302 +0000 test: {"message":"test"}
2025-04-11 15:44:22 +0000 [info]: #0 following tail of /tmp/2/d--x/file_---.log
2025-04-11 15:44:22 +0000 [warn]: #0 Permission denied @ rb_sysopen - /tmp/2/d--x/file_---.log

moatom commented Mar 16, 2025

View reviewed changes

daipom self-assigned this Mar 17, 2025

moatom force-pushed the master branch 2 times, most recently from 9b1c108 to 5de5a2e Compare March 21, 2025 15:10

Add warning for directory permission to in_tail.rb

009d270

Signed-off-by: Tomoaki Kobayashi <[email protected]>

moatom force-pushed the master branch from 5de5a2e to 009d270 Compare March 21, 2025 17:37

moatom commented Mar 21, 2025

View reviewed changes

Fix the path traversal to go top-down

031ee8e

Signed-off-by: Tomoaki Kobayashi <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning for directory permission to in_tail.rb #4865

Add warning for directory permission to in_tail.rb #4865

moatom commented Mar 14, 2025

moatom Mar 16, 2025

moatom commented Mar 17, 2025

daipom commented Mar 17, 2025

daipom commented Mar 17, 2025

daipom commented Mar 17, 2025

daipom commented Mar 18, 2025

moatom commented Mar 21, 2025 •

edited

Loading

moatom commented Mar 21, 2025

moatom Mar 21, 2025

daipom commented Mar 25, 2025

daipom commented Mar 28, 2025

moatom commented Apr 6, 2025

moatom commented Apr 11, 2025 •

edited

Loading

Add warning for directory permission to in_tail.rb #4865

Are you sure you want to change the base?

Add warning for directory permission to in_tail.rb #4865

Conversation

moatom commented Mar 14, 2025

moatom Mar 16, 2025

Choose a reason for hiding this comment

moatom commented Mar 17, 2025

daipom commented Mar 17, 2025

daipom commented Mar 17, 2025

daipom commented Mar 17, 2025

daipom commented Mar 18, 2025

moatom commented Mar 21, 2025 • edited Loading

moatom commented Mar 21, 2025

moatom Mar 21, 2025

Choose a reason for hiding this comment

daipom commented Mar 25, 2025

daipom commented Mar 28, 2025

Case

Result: master

Result: PR

moatom commented Apr 6, 2025

moatom commented Apr 11, 2025 • edited Loading

Result: Latest PR

moatom commented Mar 21, 2025 •

edited

Loading

moatom commented Apr 11, 2025 •

edited

Loading