protocols/grpc: implement function to copy files by devimc · Pull Request #433 · kata-containers/agent

devimc · 2018-12-10T22:35:29Z

CopyFileRequest can be used to copy files from the host to the workload
rootfs (guest), this request is intended to copy "static" and small
files like resolv.conf and hosts, but any other file can be copied.

fixes #432

Signed-off-by: Julio Montes julio.montes@intel.com

devimc · 2018-12-10T22:35:45Z

cc @egernst

devimc · 2018-12-10T23:04:16Z

/test

egernst

lgtm, any tests we should be adding here?

sboeuf · 2018-12-11T06:27:09Z

+
+message UploadFileRequest {
+	string path = 1;
+	string content = 2;


I think it'd make more sense to have this as an array of bytes instead of a string since it represents the content of the file.
And I'd use an extra field holding the size of this byte array.

sboeuf · 2018-12-11T06:29:30Z

+		return emptyResp, err
+	}
+
+	if err := ioutil.WriteFile(req.Path, []byte(req.Content), os.FileMode(req.FileMode)); err != nil {


Having the content as an array of bytes would prevent from the cast here, and would be more appropriate.

devimc · 2018-12-11T21:29:40Z

changes applied

devimc · 2018-12-11T21:29:46Z

/test

devimc · 2018-12-11T21:43:18Z

/test

sboeuf

Thanks @devimc !

bergwolf · 2018-12-12T08:53:12Z

 }
+
+message CopyFileRequest {
+	string path = 1;


Please add offset and length here so that it is possible to send just part of a file. Also please put content at last of the struct.

What happens when the file size exceeds the grpc payload limit? If we support offset and length, client can send a file by parts.

@bergwolf
I think length is not needed, since we can get it with len(content), is that correct?

@devimc good point!

Is this the absolute guest root context path? In other words, is this API allowing changes to, say, /sbin/init? Or is it designed purely to copy files into the workloads rootfs directory?

If it's possible to overwrite the init binary, we'd better disallow that for now until the agent is able to serialise it's state and re-exec itself.

What about adding a DoNotOverwrite bool so the client can change the default behaviour of overwriting an existing file? It could be called NoClobber bool I guess, but "clobber" is not a common word even if English is your native tongue ;)

But maybe the default should be to not overwrite? What is the intent of this change?

🤓 according with g, clobber means hit (someone) hard. "if he does that I'll clobber him!"

Yeah, probably best not to use that term! No NoClobber I say! 😄

I'm even okay with the overwrite just happening, if it keeps it simple. You can do similar things today with docker CLI, afaiu, right? Either way - adding overwrite option seems reasonable to me.

Are we still waiting for more changes to this proto? Or can we resolve this item?

devimc · 2018-12-12T15:05:24Z

/test

devimc · 2018-12-12T15:29:57Z

/test

bergwolf · 2018-12-13T01:25:23Z

+		return emptyResp, err
+	}
+
+	if _, err := file.Write(req.Content); err != nil {


You can simplify the two with file.WriteAt().

It would be safer to write the copied file to a temporary one, then atomically move it over the top of the existing one, overwriting the original. The downside being we'll temporarily need more disk space to retain two copies. We'll also have to take care to ensure the temp copy is written in the same directory as the original to avoid any FS-crossing performance issues.

sounds good, but then we'll need two commands CopyFile and MoveFile because CopyFile doesn't know when the temporarily file should be renamed/moved

I just mean that the function could do something like the following (untested):

dir := filepath.Dir(req.Path) file := filepath.Base(req.Path) // create hidden temp file in same directory as original file with a random suffix tmpfile, err := ioutil.TempFile(dir, "."+file+"*") name := tmpfile.Name() tmpfile.WriteAt(...) tmpfile.Close() // move the temp file over the top of the destination path atomically err := os.Rename(name, req.Path)

@jodh-intel ohh I see a problem with this design, if offset is != 0 and dest exist, it must be copied to the temporary file, the problem is when dest is a humongous file, copying dest to the temporarily file will be expensive in both time and disk space.

yep - slow, but safe. We need to capture these sorts of scenarios in a doc and decide on the best approach in each case I think.

@sboeuf writes:
well you need to think about this CopyFile() command as a workaround for 9p, so we're not planning on having huge files being passed through here. Thus, don't know if we should worry about performance here. I value more the security check personally.

I have changed the algorithm to copy files, please take a look

Is this comment properly addressed? Can we resolve it?

jodh-intel

I've got lots of questions about this. Ultimately, I think we need a design doc for this change to understand the intention, scope, potential security impact and so on as we're essentially adding a one-way ftp server here.

jodh-intel · 2018-12-13T12:20:06Z

 }
+
+message CopyFileRequest {
+	string path = 1;


Is this the absolute guest root context path? In other words, is this API allowing changes to, say, /sbin/init? Or is it designed purely to copy files into the workloads rootfs directory?

If it's possible to overwrite the init binary, we'd better disallow that for now until the agent is able to serialise it's state and re-exec itself.

jodh-intel · 2018-12-13T12:22:51Z

+		return emptyResp, err
+	}
+
+	file, err := os.OpenFile(req.Path, os.O_WRONLY|os.O_CREATE, os.FileMode(req.FileMode))


What happens if the file exists as, say, a socket? Won't this overwrite it as a regular file? Maybe we should check to ensure the types are the same?

Maybe we should enhance the getAgentDetails()gRPC API to include the architecture "just in case" to handle the scenario where the caller wants to send a binary file?

I don't think we should just blindly create a file with the mode specified. What if this is a long-running container and the CopyFile() ends up opening ports somehow or overwriting critical files, or making critical file world-writable...

jodh-intel · 2018-12-13T12:23:43Z

 }
+
+message CopyFileRequest {
+	string path = 1;


What about adding a DoNotOverwrite bool so the client can change the default behaviour of overwriting an existing file? It could be called NoClobber bool I guess, but "clobber" is not a common word even if English is your native tongue ;)

But maybe the default should be to not overwrite? What is the intent of this change?

jodh-intel · 2018-12-13T12:30:04Z

 	return &gpb.Empty{}, nil
 }
+
+func (a *agentGRPC) CopyFile(ctx context.Context, req *pb.CopyFileRequest) (*gpb.Empty, error) {


We need unit tests for this - think security :)

devimc · 2018-12-13T15:46:47Z

@jodh-intel

Is this the absolute guest root context path? In other words, is this API allowing changes to, say, /sbin/init? Or is it designed purely to copy files into the workloads rootfs directory?

it's designed to copy files into the workloads rootfs directory, I think we can validate that Path always starts with /tmp

caoruidong · 2018-12-14T01:53:01Z

How to evaluate a file is small enough? Will this used for only /etc stuffs?

devimc · 2018-12-14T13:06:22Z

@caoruidong , actually I need to update the commit message, since the algorithm to copy file has changed and now there is not limit in the size

bergwolf · 2018-12-17T02:23:19Z

Along with the copyfile approach, did we evaluate the option of reading metadata from the FC guest-facing metadata service?

bergwolf · 2018-12-17T02:25:39Z

+	}).Debugf("Checking temporary file size")
+	// if file size is not equal to the expected size means that copy file operation has not finished.
+	// CopyFile should be called again with new content and a different offset.
+	if st.Size() != req.FileSize {


Why is this needed? If WriteAt is not buggy, I don't see why we need to re-check the file size.

this is not to check WriteAt, client sends file's size (expected size), temporary file is renamed to destination file once its size is equal to expected size

I don't see why we need stat(). In each request, we have offset and len(content), and the sum of them should just equal to file size in the request otherwise the request is illegal on its own. If we really want to validate that, we should do it upon handling the request and before really writing to the file. The current implementation of write->stat->check size, is validating the golang fs interfaces instead.

Oh, I see your point. Sorry for the noise.

egernst · 2018-12-17T05:30:18Z

@bergwolf - we haven't tested with FC's MDS, just vsock so far. While this may be interesting down the road, I want to enable this no-9p use case for more hypervisors than just FC (NEMU/QEMU, and simplify requirements for additional hypervisors).

bergwolf · 2018-12-17T06:54:02Z

@egernst I see, let's move forward with copyfile then.

devimc · 2018-12-17T13:38:42Z

/test

sboeuf · 2018-12-17T16:14:15Z

@jodh-intel @bergwolf
I've read your comments and concerns, but I think we should really keep this method as simple as possible here for now.
First, the need for this function comes from the need to support hypervisor without a 9p or simple filesystem support. So the container rootfs is exposed as a block device, but simple files such as resolv.conf still needs to be passed inside the VM through a different way since they're not part of the rootfs.

The call needs to be kept simple as it will be the runtime's responsibility to update the files by monitoring any change. But for now that's a simple way to pass those files.

From a security perspective, if we keep it simple, and one-way, we're not opening any security holes, since we won't provide any mechanism to write back to the host.
By keeping it simple, I mean that we should not try to support copying the same file in several parts, at least for now.

I agree we should make sure we don't allow this call to override some files of our VM rootfs, but I think @devimc took care of this already by writing to /tmp.

@devimc
You should make sure to clearly document in the code the purpose of this function, explaining the need for it and the very limited scope that is targeted here.

jodh-intel · 2018-12-17T17:08:39Z

+
+	// create a temporary file and write the content.
+	tmpPath := req.Path + ".tmp"
+	tmpFile, err := os.OpenFile(tmpPath, os.O_WRONLY|os.O_CREATE, 0755)


Could we make this 0600 as we set the perms to the requested values later?

jodh-intel · 2018-12-17T17:10:09Z

+		return emptyResp, err
+	}
+
+	return emptyResp, os.Chmod(req.Path, os.FileMode(req.FileMode))


It's safer to do the os.Chmod() before the os.Rename() as that way there is no "window" for the destination file to have higher privs (from the temp file) than the requested privs.

devimc · 2018-12-17T18:09:18Z

@sboeuf @jodh-intel changes applied

sboeuf · 2018-12-17T18:10:20Z

/test

devimc · 2018-12-17T18:16:26Z

/test

sboeuf

@devimc oh okay I thought we would go with simpler implementation, but sending the file through multiple calls is handled properly here, so I'm actually fine with it!
Thanks for adding more comments, this helps a lot understanding the purpose here.

Nit: Adding some comments to protocols/grpc/agent.proto, particularly each field of CopyFileRequest would be a good way to provide information about how CopyFileRequest is supposed to be used.

devimc · 2018-12-17T20:26:19Z

@sboeuf changes applied

devimc · 2018-12-17T20:26:26Z

/test

bergwolf · 2018-12-18T03:07:44Z

LGTM! Thanks @devimc !

jodh-intel · 2018-12-18T11:43:43Z

+func (m *mockServer) CopyFile(ctx context.Context, req *pb.CopyFileRequest) (*types.Empty, error) {
+	mockLock.RLock()
+	defer mockLock.RUnlock()
+	if err := m.podExist(); err != nil {


You could simplify to just:

return nil, m.podExist()

jodh-intel · 2018-12-18T12:02:31Z

+	// to the destination path.
+	int64 file_size = 2;
+	// FileMode is the file mode.
+	uint32 file_mode = 3;


Don't we also need to send uid+gid for both file and directory as currently they will only ever be created as the user the agent is running as (root)? Maybe less important but also what about atime, mtime, ctime?

yes files are created as root, I'll add uid and gid, the other options can be added later on, what do you think?

sounds good.

jodh-intel · 2018-12-18T12:04:09Z

 }
+
+message CopyFileRequest {
+	// Path is the destination path in the guest.


Thanks for adding the comment. I think it could be made clearer though that the path must be:

absolute and canonical.

below /run.

jodh-intel · 2018-12-18T12:10:25Z

+func (a *agentGRPC) CopyFile(ctx context.Context, req *pb.CopyFileRequest) (*gpb.Empty, error) {
+	// container's rootfs is mounted at /run, in order to avoid overwrite guest's rootfs files, only
+	// is possible to copy files to /run
+	if !strings.HasPrefix(req.Path, containersRootfsPath) {


This isn't strict enough - consider the following, which will be accepted:

req.Path = "/run/../sbin/init"

But since we can't guarantee the file will already exist, it can't be expanded trivially. How about just adding an extra check and denying the request if it contains "../" ?

good catch!, I think realpath can help us here

thanks - wfm.

CopyFileRequest can be used to copy files from the host to the workload rootfs (guest), this request is intended to copy "static" and small files like resolv.conf and hosts, but any other file can be copied. fixes kata-containers#432 Signed-off-by: Julio Montes <julio.montes@intel.com>

devimc · 2018-12-18T13:39:02Z

@jodh-intel changes applied, thanks

devimc · 2018-12-18T13:39:07Z

/test

jodh-intel · 2018-12-18T14:02:05Z

+	// DirMode is the mode for the parent directories of destination path.
+	uint32 dir_mode = 4;
+	// Uid is the numeric user id.
+	int32 uid = 5;


There is the question of how the client is going to know the uid and gid values of course. They can prolly safely assume the standard users exist, but if they want to get elaborate, we may need to consider expanding GetGuestDetails() to return a list of uids+gids (optionally).... I know! This appears to be such a simple feature, but... 😄

yes, I think so 😃

jodh-intel · 2018-12-18T15:05:51Z

Given the recent discussion and changes, @sboeuf and @egernst may want to re-ack this PR?

egernst

LGTM! Thanks for the great work, @devimc and @jodh-intel !

sboeuf

Thanks guys! looks good!

devimc force-pushed the topic/uploadFile branch from ca1cc2c to 9ac03c0 Compare December 10, 2018 22:58

egernst reviewed Dec 11, 2018

View reviewed changes

sboeuf suggested changes Dec 11, 2018

View reviewed changes

devimc force-pushed the topic/uploadFile branch 2 times, most recently from 1eb59e3 to 18e5dd2 Compare December 11, 2018 19:08

devimc changed the title ~~protocols/grpc: implement function to upload files~~ protocols/grpc: implement function to copy files Dec 11, 2018

devimc force-pushed the topic/uploadFile branch 2 times, most recently from 9f0704d to 73e1956 Compare December 11, 2018 21:35

sboeuf approved these changes Dec 12, 2018

View reviewed changes

egernst approved these changes Dec 12, 2018

View reviewed changes

bergwolf reviewed Dec 12, 2018

View reviewed changes

devimc force-pushed the topic/uploadFile branch from 73e1956 to a5fff83 Compare December 12, 2018 15:04

bergwolf reviewed Dec 13, 2018

View reviewed changes

jodh-intel suggested changes Dec 13, 2018

View reviewed changes

devimc force-pushed the topic/uploadFile branch 2 times, most recently from b31d58f to a0ed28e Compare December 13, 2018 22:06

devimc force-pushed the topic/uploadFile branch from a0ed28e to de48ce2 Compare December 14, 2018 16:37

mcastelino mentioned this pull request Dec 15, 2018

Add support for firecracker in addition to QEMU as a supported hypervisor in Kata. kata-containers/runtime#1031

Closed

11 tasks

bergwolf reviewed Dec 17, 2018

View reviewed changes

devimc force-pushed the topic/uploadFile branch from de48ce2 to 7f1bab0 Compare December 17, 2018 13:32

jodh-intel reviewed Dec 17, 2018

View reviewed changes

devimc force-pushed the topic/uploadFile branch 2 times, most recently from 84cb5bd to ef7f3f8 Compare December 17, 2018 18:04

devimc force-pushed the topic/uploadFile branch from ef7f3f8 to edcd564 Compare December 17, 2018 18:15

sboeuf approved these changes Dec 17, 2018

View reviewed changes

devimc mentioned this pull request Dec 17, 2018

Copy files from host to guest kata-containers/runtime#1041

Merged

devimc force-pushed the topic/uploadFile branch from edcd564 to a66b3f5 Compare December 17, 2018 20:26

jodh-intel reviewed Dec 18, 2018

View reviewed changes

devimc force-pushed the topic/uploadFile branch from a66b3f5 to 795d9f9 Compare December 18, 2018 13:37

devimc force-pushed the topic/uploadFile branch from 795d9f9 to 169d755 Compare December 18, 2018 13:38

jodh-intel reviewed Dec 18, 2018

View reviewed changes

jodh-intel approved these changes Dec 18, 2018

View reviewed changes

egernst approved these changes Dec 18, 2018

View reviewed changes

sboeuf approved these changes Dec 18, 2018

View reviewed changes

sboeuf merged commit 8e48125 into kata-containers:master Dec 18, 2018

Conversation

devimc commented Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devimc commented Dec 10, 2018

Uh oh!

devimc commented Dec 10, 2018

Uh oh!

egernst 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!

Uh oh!

devimc commented Dec 11, 2018

Uh oh!

devimc commented Dec 11, 2018

Uh oh!

devimc commented Dec 11, 2018

Uh oh!

sboeuf 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!

Choose a reason for hiding this comment

Uh oh!

devimc Dec 13, 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!

devimc commented Dec 12, 2018

Uh oh!

devimc commented Dec 12, 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!

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!

jodh-intel 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!

Choose a reason for hiding this comment

Uh oh!

devimc commented Dec 10, 2018 •

edited

Loading

devimc Dec 13, 2018 •

edited

Loading