Add support for "-p" option, to copy permissions, modification time, access time

[dxjones]

The command line version of scp includes a -p option to preserve file permissions, modification time, access time.

go-scp is incomplete because it does not support this option. For copying local files to a remote server, it is possible use os.Stat() and pass the file permissions, but that doesn't preserve mtime/atime, and nothing can be preserved copying in the other direction (remote to local).

Clearly the SCP protocol supports -p, so it this functionality can be added to go-scp.

[bramvdbogaerde]

Thanks for your issue.

Your proposal makes a lot of sense and seems like it is something that has plenty of use cases. In the short term, I do not have the time to work on it myself, but I would welcome any contributions.

[dxjones]

Can you recommend any good references for the SCP protocol? What URLs did you use when developing go-scp ? I am willing to take a look at how the protocol supports the "-p" option, and take a crack at writing a new function scp.CopyFromRemotePreserveMode() Thanks, --David Jones

…

On Sun, Jul 10, 2022 at 2:10 PM Bram Vandenbogaerde < ***@***.***> wrote: Thanks for your issue. Your proposal makes a lot of sense and seems like it is something that has plenty of use cases. In the short term, I do not have the time to work on it myself, but I would welcome any contributions. — Reply to this email directly, view it on GitHub <#63 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACAWUD5EPGIHIOJ2BGUQVDVTMG2XANCNFSM53FLTU5A> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[bramvdbogaerde]

Dear David,

Thanks for your help. Adding "-v" (verbose mode) to any SCP command already reveals quite a lot about how the protocol works.

Here I send a file using the following command:

scp -v -p ask-pattern.rkt bram@HOST:~

Trace:

debug1: Sending command: scp -v -p -t ~/
File mtime 1657283010 atime 1657283011
Sending file timestamps: T1657283010 0 1657283011 0
Sink: T1657283010 0 1657283011 0
Sending file modes: C0644 375 ask-pattern.rkt
Sink: C0644 375 ask-pattern.rkt
ask-pattern.rkt                                                        100%  375     8.6KB/s   00:00

The trace above reveals that before sending the information of the file (C message: permissions, size and name) a timestamp message (starting with T) is sent that has the following format:

T<mtime> 0 <atime> 0

where mtime = modification time and atime = access time (as defined by POSIX), there seems to be no way to add ctime(creation time).

More information about these messages can be found here [1].

Diving into the code the go-scp library, it seems these messages will need to be sent before line 168 in client.go.

Hope this helps.

Regards,

[1] https://web.archive.org/web/20090806212235/http://blogs.sun.com/janp/entry/how_the_scp_protocol_works

[dxjones]

Thanks for the info. By the way, is there a known issue with the program hanging when trying copy a non-existent remote file? *err = client.CopyFromRemote(context.Background(), f, path)* The above line works as expected when the remote file exists, but it *never returns* when the file does not exist. I could possibly force the function to return with a timeout, but that shouldn't be necessary. Amu I the only one who has encountered this bug? Am I calling the function wrong? Thanks --David Jones

…

On Sun, Jul 10, 2022 at 5:31 PM Bram Vandenbogaerde < ***@***.***> wrote: Dear David, Thanks for your help. Adding "-v" (verbose mode) to any SCP command already reveals quite a lot about how the protocol works. Here I send a file using the following command: scp -v -p ask-pattern.rkt ***@***.***:~ Trace: debug1: Sending command: scp -v -p -t ~/ File mtime 1657283010 atime 1657283011 Sending file timestamps: T1657283010 0 1657283011 0 Sink: T1657283010 0 1657283011 0 Sending file modes: C0644 375 ask-pattern.rkt Sink: C0644 375 ask-pattern.rkt ask-pattern.rkt 100% 375 8.6KB/s 00:00 The trace above reveals that before sending the information of the file (C message: permissions, size and name) a timestamp message (starting with T) is sent that has the following format: T<mtime> 0 <atime> 0 where mtime = modification time and atime = access time (as defined by POSIX), there seems to be no way to add ctime(creation time). More information about these messages can be found here <https://web.archive.org/web/20090806212235/http://blogs.sun.com/janp/entry/how_the_scp_protocol_works> [1]. Diving into the code the go-scp library, it seems these messages will need to be sent before li [1] https://web.archive.org/web/20090806212235/http://blogs.sun.com/janp/entry/how_the_scp_protocol_works — Reply to this email directly, view it on GitHub <#63 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACAWUHRLTGVTTG52MAN5SDVTM6MPANCNFSM53FLTU5A> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[bramvdbogaerde]

Regarding the scenario of a non-existing file that you just described: there is a test in our test suite that checks just that, so it should work fine and return an error in case the file does not exist.

Could you compare your test code with the code here in the test named TestFileNotFound, maybe there is something that has been overlooked.

Thanks.

[datadius]

I encountered an issue where a server would send by default all the information linked to a file.

The following is the code that it would use to write the file to the scp-client.

This would cause an EFO error, as the size of the int will come as 0.

func (e *FileEntry) Write(w io.Writer) error {
	if e.Mtime > 0 && e.Atime > 0 {
		if _, err := fmt.Fprintf(w, "T%d 0 %d 0\n", e.Mtime, e.Atime); err != nil {
			return fmt.Errorf("failed to write file: %q: %w", e.Filepath, err)
		}
	}
	if _, err := fmt.Fprintf(w, "C%s %d %s\n", octalPerms(e.Mode), e.Size, e.Name); err != nil {
		return fmt.Errorf("failed to write file: %q: %w", e.Filepath, err)
	}

	if _, err := io.Copy(w, e.Reader); err != nil {
		return fmt.Errorf("failed to read file: %q: %w", e.Filepath, err)
	}

	if _, err := w.Write(NULL); err != nil {
		return fmt.Errorf("failed to write file: %q: %w", e.Filepath, err)
	}
	return nil
}

In order to test this, I ran ParseResponse one more time in CopyFromRemotePassThru

res, err := ParseResponse(r)
		if err != nil {
			errCh <- err
			return
		}
		if res.IsFailure() {
			errCh <- errors.New(res.GetMessage())
			return
		}
		// Parse a second time
		res, err = ParseResponse(r)
		if err != nil {
			errCh <- err
			return
		}
		if res.IsFailure() {
			errCh <- errors.New(res.GetMessage())
			return
		}
		//

		infos, err := res.ParseFileInfos()
		if err != nil {
			errCh <- err
			return
		}

This would make the file copy successfully.

I believe the solution is handling it in the ParseResponse function and I plan to contribute with a pull request once I figure out how to do it best after I tested it.

@bramvdbogaerde what is your take on this?

[bramvdbogaerde]

Hi @datadius, thanks for your comment.

First of all, I believe that we should clearly distinguish between source and sink modes in go-scp, both require some changes to the library in different locations for the -p option to work. You seem to describe a case where the library operates in sink mode (i.e., receives files from a remote), and does not expect to receive a T message (since we don't support -p currently).

I am not sure what the default behavior is in scp itself whenever a server sends this information unprompted, I suppose it either results in an error or is simply ignored, can you confirm this? Nonetheless, I think that parsing T messages in protocol.go (similar to what we do for parsing C messages), might be useful. Additional code for handling these types of messages should then also be added to CopyFromRemotePassThru.

Note that the information in a T message might not always make sense for someone that uses the CopyFromRemotePassThru function, since the remote file might not even end up in a file system at all. However, this information should still be returned in some way to the caller of the function.

So in short, I believe the following tasks (for implementing -p when operating in sink mode) need to be completed:

• Adapt protocol.go to parse Tmessages in addition to C messages, there are also a D and E messages which are currently not handled, but this is related to issue Allow entire directories to be copied (recursively) #61 .
• Update CopyFromRemotePassThru to handle T messages, potentially informed by a flag set in the ClientConfigurer. To know what the default behavior should be, we should check what scp is doing when it receives T messages unprompted.
• Change the information returned from CopyFromRemotePassThru to include information from T messages and return it to the user of that function.

I welcome contributions for any of these tasks, so you are welcome to propose your changes in a PR.

Thanks!

Regards,

Bram

[datadius]

Thanks for the response @bramvdbogaerde. Currently I'm investigating the following implementation https://github.com/openssh/openssh-portable/blob/master/scp.c and I'm already testing some changes to the protocol.

I agree with your points.

My only question is about the third point. Wouldn't adding a different output break the current applications using the library? It is new functionality, so it could be a new method, my only problem with that is that you get some code duplicates.

Where do you see this going?

Let me know if I'm asking too many questions and you would just prefer to see the code first in a pull request😄

[bramvdbogaerde]

You are not asking too many questions at all, I appreciate your input.

I agree with your remark about breaking current applications if we decide to change the signature of existing methods. If I recall correctly, we had a similar problem before when we wanted to add the passThru parameter to Copy. For that situation we decided to express all other methods in terms of a new method called CopyPassThru. We accomplished this by setting the PassThru parameter to nil for all other methods. In my opinion, this approach reduces code duplication to an acceptable level. Thus, a similar approach could be taken here.

Looking forward to your PR.

Kind regards,

Bram

[bramvdbogaerde]

This was added for 'sink' mode in #81, source mode already supported passing permissions explicitly but currently does not allows modification and access time to be sent. A separate issue is created to track its progress.