Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Implement a crate like opencontainers/selinux #2718

Open
Gekko0114 opened this issue Mar 3, 2024 · 17 comments
Open

Implement a crate like opencontainers/selinux #2718

Gekko0114 opened this issue Mar 3, 2024 · 17 comments
Assignees

Comments

@Gekko0114
Copy link
Contributor

Gekko0114 commented Mar 3, 2024

background

In this PR #2688, it was found that the implementation of linux mount label is different between runc and youki.

I have some question regarding the MountLabel implementation itself : As per the oci spec, the MountLabel field marks the selinux label to be used for that mount. Thus in runc https://github.com/opencontainers/runc/blob/d5e4c33001d74176222fe8f48a323f3e8ad89999/libcontainer/rootfs_linux.go#L536 , they use the selinux package and set the label using that. However in youki, we are passing that from https://github.com/containers/youki/blob/main/crates/libcontainer/src/rootfs/rootfs.rs#L90 to the syscall implementation, which finally reaches and then goes https://github.com/containers/youki/blob/main/crates/libcontainer/src/syscall/linux.rs#L471 . Where it finally calls nix::mount. I'm not sure how that is supposed to correspond to selinux. Can you check if there is some issue with out implementation , or am I doing some wrong code follow?

What we will do

Youki should follow runc's implementation.
Therefore, we will implement the crate like opencontainers/selinux in this issue.

@Gekko0114
Copy link
Contributor Author

@utam0k
I created a issue here, please correct me if I misunderstand.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Mar 20, 2024

Hey @Gekko0114 , do you need any help with this?

@Gekko0114
Copy link
Contributor Author

Thanks, but I don't have enough time to work on these days..
I plan to work on it when I have time. Sorry for inconvenience.
I thought it is not so urgent, but if it is urgent, please let me know.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Mar 20, 2024

Thanks, but I don't have enough time to work on these days..

I completely understand, no worries!

I plan to work on it when I have time. Sorry for inconvenience.
I thought it is not so urgent, but if it is urgent, please let me know.

This is not exactly urgent, but given that this is an incorrect implementation , I would prefer to have it fixed sooner than later. I was just wondering if you are still planning to work on this or not, so pinged you. Take your time 💜

@Gekko0114
Copy link
Contributor Author

Hi @YJDoc2, @utam0k

Sorry for not doing this task for a while. I will resume it.
Seems that https://github.com/opencontainers/selinux has several files. So I would like to take this task into several PRs, like these. Do you have any suggestions about this?

  • go-selinux/label/
  • go-selinux/selinux.go
  • selinux_stub.go
  • pkg/pwalk/
    and so on...

@YJDoc2
Copy link
Collaborator

YJDoc2 commented May 19, 2024

Sorry for not doing this task for a while. I will resume it.

No worries!

Hey @Gekko0114 , I don't think I'll be able to help much in near future, but one thing I can suggest is we can implement this under the experimental/ , similar to how utam0k is implementing the rust libseccomp. That way you can make PRs for individual files, or features, along with their tests when applicable. Once we feel that it is complete, we can move it to the youki workspace and use it in youki where needed. wdyt?

Also checking the files, pkg/pwalk seems to be deprecated and pkg/pwalkdir should be checked once if we need it at all or not, it can be directly added as part of the new library we are making instead of a separate package.

@Gekko0114
Copy link
Contributor Author

Sure, thanks for your suggestion! SGTM.
Then, I will create PRs under the experimental/.
After understanding the library selinux, I will start working it.

@utam0k
Copy link
Member

utam0k commented May 19, 2024

I agree with @YJDoc2 . It would be an good idea.

@Gekko0114
Copy link
Contributor Author

Gekko0114 commented May 24, 2024

Hi @utam0k, @YJDoc2

I have a question.

go-selinux handles xattr https://github.com/opencontainers/selinux/blob/main/go-selinux/selinux_linux.go#L346.

However, https://github.com/nix-rust/nix doesn't seem to have functions handling xattr.

Should I create a crate handling xattr as well?
Or should I import the library outside of this repo, like this https://github.com/Stebalien/xattr ?

@YJDoc2
Copy link
Collaborator

YJDoc2 commented May 25, 2024

Hey regarding this, if the implementation of related xattr functions is not too complex, and there are not too many edge-cases to be considered, I'd prefer not to add a dependency to deal with it. For implementing it , I'd prefer to have it as a module in the same selinux crate instead of separate crate, so that we don't have to publish and manage two crates for this. wdyt?

@Gekko0114
Copy link
Contributor Author

SGTM, thanks! Then I will add xattr function in one crate.

@Gekko0114
Copy link
Contributor Author

Hi @utam0k @YJDoc2

Since the initial motivation for the SELinux project is that Youki doesn't follow runc's implementation regarding SELinux, can I implement the SELinux part in Youki?
#2688 (comment)

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Sep 23, 2024

Hey, I think we should be able to start with this change now that we have finished the experimental crate. I have a couple of minor comments for that crate I feel we should discuss before we move that crate from experimental to "stable" . Will add them here in some time. Also need to discuss if we are going to publish this crate as a standalone crate (like libcontainer,libcgroups etc) as well.

I also want to thank you @Gekko0114 for all your work and follow up on this. Thanks for making Youki better 💜

@Gekko0114
Copy link
Contributor Author

I think we should be able to start with this change now that we have finished the experimental crate

Sure, then I will implement the part utilizing SELinux in runc.

I also want to thank you @Gekko0114 for all your work and follow up on this. Thanks for making Youki better 💜

You're welcome, thank you so much for your kind review ❤️

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Sep 23, 2024

So mu questions were -

  1. selinux: create Vagrantfile for SELinux #2900 (comment)
  2. As we will be using this as a dependency in libcontainer, can we remove the anyhow dependency, and use thiserror for all errors like we are doing in all lib* crates.

Apart from that I think we can bring the create from exp to normal crates and start integrating with youki. Also we need to make sure that we make this releasable, so in next release the dependencies do not give issues.

@Gekko0114
Copy link
Contributor Author

Thanks @YJDoc2

Q1:
When I ran main.rs with CREATE, I encountered an OS error or similar error. That's why I changed it to REPLACE. However, if the xattr doesn't exist on the file, it might be better to use CREATE. I will investigate and fix this.
Q2:
Sure, I will remove anyhow and use thiserror.

After creating a PR fixing these two questions, I will start implementing SELinux part in Youki.

@Gekko0114
Copy link
Contributor Author

Since I've merged the PR based on feedback from @YJDoc2, I will start implementing the SELinux part in Youki.

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

No branches or pull requests

3 participants