Allow non-admin user processes to make changes to the log file #513
Conversation
monitor.c
Outdated
| @@ -301,7 +301,7 @@ monitor_child_preauth(struct ssh *ssh, struct monitor *pmonitor) | |||
| auth2_authctxt_reset_info(authctxt); | |||
|
|
|||
| authenticated = (monitor_read(ssh, pmonitor, | |||
| mon_dispatch, &ent) == 1); | |||
| mon_dispatch, &ent, 1) == 1); | |||
There was a problem hiding this comment.
Instead of doing like this, take dependency on the "privsep_auth_child".
the logic should go inside monitor_read_log().
Inside #ifdef windows block, you can do an "extern int privsep_auth_child", if privsep_auth_child is not true then add the "[pre-auth]" tag
monitor.c
Outdated
| @@ -1888,8 +1893,10 @@ monitor_openfds(struct monitor *mon, int do_logfds) | |||
| FD_CLOSEONEXEC(pair[1]); | |||
| mon->m_log_recvfd = pair[0]; | |||
| mon->m_log_sendfd = pair[1]; | |||
| } else | |||
| } | |||
| else { | |||
There was a problem hiding this comment.
minimize the differences. Let our code be in sync with upstream code to avoid future merge conflicts.
sftp-server.c
Outdated
| @@ -1743,6 +1778,8 @@ sftp_server_main(int argc, char **argv, struct passwd *user_pw) | |||
| } | |||
|
|
|||
| log_init(__progname, log_level, log_facility, log_stderr); | |||
|
|
|||
… go through monitor
contrib/win32/win32compat/w32log.c
Outdated
| errno_t err; | ||
| if (strcmp(identity, "sftp-server") == 0) { | ||
| err = _wsopen_s(&sftp_server_logfd, log_file, O_WRONLY | O_CREAT | O_APPEND, SH_DENYNO, S_IREAD | S_IWRITE); | ||
| if (sftp_server_logfd != -1) |
There was a problem hiding this comment.
This is not required. There is no child process using this handle.
contrib/win32/win32compat/w32log.c
Outdated
| SetHandleInformation((HANDLE)_get_osfhandle(sftp_server_logfd), HANDLE_FLAG_INHERIT, 0); | ||
| } else { | ||
| err = _wsopen_s(&logfd, log_file, O_WRONLY | O_CREAT | O_APPEND, SH_DENYNO, S_IREAD | S_IWRITE); | ||
| if (logfd != -1) |
There was a problem hiding this comment.
I doubt if this is required. Child sshd process is not doing logging anymore.
remove this logic and compare the sshd.log file with and without this change to make sure we are not missing any child process.
| if (Test-Path $sshdconfig_custom) { | ||
| Remove-Item $sshdconfig_custom -Force | ||
| } | ||
| Copy-Item $sshdconfig_ori $sshdconfig_custom |
There was a problem hiding this comment.
You can directly modify the sshd_config file in regress/pestertests/data/ folder.
There was a problem hiding this comment.
I decided not to do that just because there are other tests that rely on that config file, so I just made a copy of it before I made my changes.
| { | ||
| Stop-SSHDTestDaemon -Port $port | ||
| } | ||
| if(($platform -eq [PlatformType]::Windows) -and ([Environment]::OSVersion.Version.Major -le 6)) |
There was a problem hiding this comment.
Did you run automated tests / manually test your changes on windows 7?
There was a problem hiding this comment.
Not yet. I'll do that next.
|
|
||
| $sshdConfigPath = $sshdconfig_custom | ||
|
|
||
| function Setup-KeyBasedAuth |
There was a problem hiding this comment.
What's the need for doing this? Why can't it be same as sshd tests where we read the password from ask_passutility
There was a problem hiding this comment.
I wanted to use that -b flag to include a batch file with a couple of commands in the sftp tests, but that only works if we use it with non-interactive authentication, so that's why I had to use key based authentication for the sftp tests.
| Clear-Content "$env:ProgramData\ssh\logs\sftp-server.log" -Force -ErrorAction SilentlyContinue | ||
|
|
||
| $tI = 1 | ||
| } |
| Set-Content $batchFilePath -Encoding UTF8 -value $commands | ||
|
|
||
| # clear logs so that next testcase will get fresh logs. | ||
| Clear-Content "$env:ProgramData\ssh\logs\sftp-server.log" -Force -ErrorAction SilentlyContinue |
| $tC++ | ||
| } | ||
|
|
||
| It "$tC.$tI-Nonadmin SFTP Connection" -skip:$skip { |
There was a problem hiding this comment.
remove the new lines after each statement
| Copy-Item "$env:ProgramData\ssh\logs\sftp-server.log" $sftplog -Force -ErrorAction SilentlyContinue | ||
|
|
||
| # clear logs so that next testcase will get fresh logs. | ||
| Clear-Content "$env:ProgramData\ssh\logs\sftp-server.log" -Force -ErrorAction SilentlyContinue |
- pull latest changes from GitHub - increment version to 8.6.0.1 in version.rc Related work items: PowerShell#513
Non-admin users can't write to log files, and because of that post-authenticated sshd processes and sftp-server processes that are running on a non-admin user context can't write logs to either etw or log files. In order to solve that problem, this PR forwards log messages coming from sftp-server and sshd child processes to the sshd system process for writing.
Logs on ETW for a SSH connection to a non-admin user:
Before the changes in this PR, no logs coming from the post-authenticated sshd child process would be written. Now, as you can see in the screenshots below, the logs coming from both the pre-authenticated and post-authenticated child are being recorded to ETW.
Debug
Operational
Logs on SSHD.log file for a SSH connection to a non-admin user:
You can see in the screenshot below that both log messages coming from the pre-authenticated and post-authenticated child are being written to SSHD.log file.
Logs on ETW for a SFTP connection to a non-admin user.
Debug
Operational
Logs on sftp-server.log for a SFTP connection to a non-admin user.
