operation `files.line` does not correctly handle files that do not end with a newline

yunzheng commented 2 years ago

Describe the bug

When adding a line to a file that does NOT end with a newline in the file can cause the file to get corrupted or get unexpected contents. This can be easily tested when when adding public_keys to an account with an authorized_keys file that does not end with a newline.

To Reproduce

Steps to reproduce the behavior (include code & usage example):

Create a test authorized_keys file, note the -n to echo to suppress the newline:

echo -n "ssh-rsa key_without_newline_at_eof" > /root/.ssh/authorized_keys

contents of deploy.py:

from pyinfra import host
from pyinfra.operations import server

server.user("root", home="/root", public_keys=["ssh-rsa root"])

Execute with: pyinfra @local ~/deploy.py -vv --debug. After executing the result is:

root@a35a0dbcd78d:~# cat /root/.ssh/authorized_keys
ssh-rsa key_without_newline_at_eofssh-rsa root

Expected behavior

I expect public keys to be correctly added (per line), taking into account that the file can end without a newline and will take this into account.

Current result now:

root@a35a0dbcd78d:~# cat /root/.ssh/authorized_keys
ssh-rsa key_without_newline_at_eofssh-rsa root

Expected result:

root@a35a0dbcd78d:~# cat /root/.ssh/authorized_keys
ssh-rsa key_without_newline_at_eof
ssh-rsa root

Meta

Include output of pyinfra --support. --> Support information:

If you are having issues with pyinfra or wish to make feature requests, please check out the GitHub issues at https://github.com/Fizzadar/pyinfra/issues . When adding an issue, be sure to include the following:

System: Linux Platform: Linux-5.10.0-10-amd64-x86_64-with-glibc2.35 Release: 5.10.0-10-amd64 Machine: x86_64 pyinfra: v2.1 Executable: /usr/local/bin/pyinfra Python: 3.10.4 (CPython, GCC 11.2.0)
How was pyinfra installed (source/pip)? pip
Include pyinfra-debug.log (if one was created) none

Consider including output with -vv and --debug.


root@a35a0dbcd78d:~# pyinfra @local ~/deploy.py -vv --debug
--> Loading config...
--> Loading inventory...
[pyinfra_cli.inventory] Creating fake inventory...
[pyinfra_cli.inventory] Checking possible group_data directory: /root

--> Connecting to hosts... [@local] Connected [pyinfra.api.state] Activating host: @local

--> Preparing operations... Loading: /root/deploy.py [pyinfra.api.operation] Adding operation, {'Server/User'}, opOrder=(0, 4), opHash=18996c2767e7142774328ef0b335b99342919a9e [pyinfra.api.facts] Getting fact: server.Users () (ensure_hosts: None) [pyinfra.connectors.local] --> Running command on localhost: sh -c 'for i in cat /etc/passwd | cut -d: -f1; do ENTRY=grep ^$i: /etc/passwd; LASTLOG=lastlog -u $i | grep ^$i | tr -s '"'"' '"'"'; echo "$ENTRY|id -gn $i|id -Gn $i|$LASTLOG"; done' [@local] >>> sh -c 'for i in cat /etc/passwd | cut -d: -f1; do ENTRY=grep ^$i: /etc/passwd; LASTLOG=lastlog -u $i | grep ^$i | tr -s '"'"' '"'"'; echo "$ENTRY|id -gn $i|id -Gn $i|$LASTLOG"; done' [pyinfra.connectors.util] --> Waiting for exit status... [pyinfra.connectors.util] --> Command exit status: 0 [@local] Loaded fact server.Users [pyinfra.api.facts] Getting fact: files.Directory (path=/root) (ensure_hosts: None) [pyinfra.connectors.local] --> Running command on localhost: sh -c '! (test -e /root || test -L /root ) || ( stat -c '"'"'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N'"'"' /root 2> /dev/null || stat -f '"'"'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY'"'"' /root )' [@local] >>> sh -c '! (test -e /root || test -L /root ) || ( stat -c '"'"'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N'"'"' /root 2> /dev/null || stat -f '"'"'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY'"'"' /root )' [pyinfra.connectors.util] --> Waiting for exit status... [pyinfra.connectors.util] --> Command exit status: 0 [@local] Loaded fact files.Directory (path=/root) [@local] noop: directory /root already exists [pyinfra.api.facts] Getting fact: files.Directory (path=/root/.ssh) (ensure_hosts: None) [pyinfra.connectors.local] --> Running command on localhost: sh -c '! (test -e /root/.ssh || test -L /root/.ssh ) || ( stat -c '"'"'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N'"'"' /root/.ssh 2> /dev/null || stat -f '"'"'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY'"'"' /root/.ssh )' [@local] >>> sh -c '! (test -e /root/.ssh || test -L /root/.ssh ) || ( stat -c '"'"'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N'"'"' /root/.ssh 2> /dev/null || stat -f '"'"'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY'"'"' /root/.ssh )' [pyinfra.connectors.util] --> Waiting for exit status... [pyinfra.connectors.util] --> Command exit status: 0 [@local] Loaded fact files.Directory (path=/root/.ssh) [@local] noop: directory /root/.ssh already exists [pyinfra.api.facts] Getting fact: files.File (path=/root/.ssh/authorized_keys) (ensure_hosts: None) [pyinfra.connectors.local] --> Running command on localhost: sh -c '! (test -e /root/.ssh/authorized_keys || test -L /root/.ssh/authorized_keys ) || ( stat -c '"'"'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N'"'"' /root/.ssh/authorized_keys 2> /dev/null || stat -f '"'"'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY'"'"' /root/.ssh/authorized_keys )' [@local] >>> sh -c '! (test -e /root/.ssh/authorized_keys || test -L /root/.ssh/authorized_keys ) || ( stat -c '"'"'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N'"'"' /root/.ssh/authorized_keys 2> /dev/null || stat -f '"'"'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY'"'"' /root/.ssh/authorized_keys )' [pyinfra.connectors.util] --> Waiting for exit status... [pyinfra.connectors.util] --> Command exit status: 0 [@local] Loaded fact files.File (path=/root/.ssh/authorized_keys) [@local] noop: file /root/.ssh/authorized_keys already exists [pyinfra.api.facts] Getting fact: files.FindInFile (interpolate_variables=False, path=/root/.ssh/authorized_keys, pattern=^.ssh-rsa root.$) (ensure_hosts: None) [pyinfra.connectors.local] --> Running command on localhost: sh -c 'grep -e '"'"'^.ssh-rsa root.$'"'"' /root/.ssh/authorized_keys 2> /dev/null || ( find /root/.ssh/authorized_keys -type f > /dev/null && echo __pyinfraexists/root/.ssh/authorized_keys || true )' [@local] >>> sh -c 'grep -e '"'"'^.ssh-rsa root.$'"'"' /root/.ssh/authorized_keys 2> /dev/null || ( find /root/.ssh/authorized_keys -type f > /dev/null && echo __pyinfraexists/root/.ssh/authorized_keys || true )' [pyinfra.connectors.util] --> Waiting for exit status... [pyinfra.connectors.util] --> Command exit status: 0 [@local] Loaded fact files.FindInFile (interpolate_variables=False, path=/root/.ssh/authorized_keys, pattern=^.ssh-rsa root.$) [@local] Ready: /root/deploy.py

--> Proposed changes: Groups: @local [@local] Operations: 1 Commands: 1

--> Beginning operation run... --> Starting operation: Server/User (root, home=/root, public_keys=['ssh-rsa root']) [pyinfra.api.operations] Starting operation Server/User on @local [pyinfra.connectors.local] --> Running command on localhost: sh -c 'echo '"'"'ssh-rsa root'"'"' >> /root/.ssh/authorized_keys' [@local] >>> sh -c 'echo '"'"'ssh-rsa root'"'"' >> /root/.ssh/authorized_keys' [pyinfra.connectors.util] --> Waiting for exit status... [pyinfra.connectors.util] --> Command exit status: 0 [@local] Success

--> Results: Groups: @local [@local] Successful: 1 Errors: 0 Commands: 1/1

Fizzadar commented 2 years ago

Thanks for reporting this @yunzheng I'll look into this and fix ASAP.

yunzheng commented 1 year ago

Maybe something can be done using awk as it looks like when you print it will always be at a new line, example:

$ echo -n "ssh-rsa key_without_newline_at_eof" > /tmp/a.txt
$ awk '{ print } END { print "ssh-rsa root" }' /tmp/a.txt
ssh-rsa key_without_newline_at_eof
ssh-rsa root

same applies if there is already a newline:

$ echo "ssh-rsa key_without_newline_at_eof" > /tmp/b.txt
$ awk '{ print } END { print "ssh-rsa root" }' /tmp/b.txt
ssh-rsa key_without_newline_at_eof
ssh-rsa root

Above tested on OpenBSD, and Debian Linux so I assume it's POSIX compliant.

yunzheng commented 1 year ago

See PR #950, instead of awk and needing to write to a temporary file first, i opted for an inline check using tail -c1 and wc -l.

pyinfra-dev / pyinfra