Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

seccomp listener: define the max size of Container Process State messages #1134

Open
AkihiroSuda opened this issue Feb 2, 2022 · 4 comments

Comments

@AkihiroSuda
Copy link
Member

https://github.com/opencontainers/runtime-spec/blob/8958f93039ab90be53d803cd7e231a775f644451/config-linux.md#the-container-process-state

Currently we don't have any definition of the message size.
I suggest to limit the size to be 4096 bytes corresponding to https://github.com/opencontainers/runc/blob/v1.1.0/contrib/cmd/seccompagent/seccompagent.go#L78 , but that may result in losing some annotations.

We should have prepended uint32le len field to the messages, but probably we are too late to change the spec.

@AkihiroSuda
Copy link
Member Author

cc @rata @alban @cyphar PTAL

@rata
Copy link
Member

rata commented Feb 2, 2022

@AkihiroSuda can you provide some context on why to limit the size?

That struct includes the OCI state, that AFAIK does not have a size limit, right? (annotations are arbitrary long, for example). How would we limit the size and why would we do it (note that the example is just an example as stated in the README)?

Please note that the listenerPath description says:

This socket MUST use AF_UNIX domain and SOCK_STREAM type. The runtime MUST send exactly one container process state per connection. The connection MUST NOT be reused and it MUST be closed after sending a seccomp state

This basically makes it super easy to handle big messages (it is SOCK_STREAM, no fixed limit), there will be only one message sent per connection, so it is trivial to reassemble all the pieces in one buffer and, of course, the agent can deny if the message if bigger than X.

Can you please elaborate?

@AkihiroSuda
Copy link
Member Author

Defining hard limit is not needed, but implementations have to raise an error when the message exceeds some implementation-specific threshold value anyway (to avoid malloccing too much), so it might be still helpful to say The message size SHOULD (not "MUST") be 4096 (or 65536?) bytes at maximum.

@rata
Copy link
Member

rata commented Feb 2, 2022

Oh, I'm fine with a SHOULD addition.

I'm not sure if the limit should be 4096 or what, though. In an ideal world, I'd like to see stats of containers running in the wild and see how long the OCI state is. But if someone can provide something that seems meaningful, or some other limit that was already used in the past, seems good to :)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants