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

Add support for setting RPMTAG_FILECAPS header #166

Merged
merged 9 commits into from
Aug 16, 2023

Conversation

dsteeley
Copy link
Contributor

@dsteeley dsteeley commented Aug 15, 2023

This change adds support for setting the RPMTAG_FILECAPS header to add capabilities to the files packaged in the rpm.

📜 Checklist

  • Commits are cleanly separated and have useful messages
  • A changelog entry or entries has been added to CHANGELOG.md
  • Documentation is thorough
  • Test coverage is excellent and passes
  • Works when tests are run --all-features enabled

@dralley
Copy link
Collaborator

dralley commented Aug 15, 2023

It would be great if we could fill out support for reading filecaps also, so that we can write functional tests for this.

https://github.com/rpm-rs/rpm/blob/master/src/rpm/package.rs#L836

@dralley
Copy link
Collaborator

dralley commented Aug 15, 2023

I haven't checked the behavior of RPM, but if it doesn't set FILECAPS if none were used in the specfile, we should probably avoid setting that tag if the user didn't provide any as well.

If it always sets the FILECAPS tag, then this is fine.

@dsteeley
Copy link
Contributor Author

dsteeley commented Aug 15, 2023

I've got these changes on a separate branch at the moment.
Would you like them here with a functional test or kept separate?

My understanding of the behaviour here is that if not present then the header isn't added.
I've updated the behaviour to not add the header unless any file has a capability.

@dralley
Copy link
Collaborator

dralley commented Aug 15, 2023

Feel free to put it all in one PR, IMO it would be a bit cleaner particularly w/r/t testing. You can keep commits separate if you want.

README.md Outdated
@@ -50,6 +50,7 @@ let pkg = rpm::PackageBuilder::new("test", "1.0.0", "MIT", "x86_64", "some aweso
// you can set a custom mode and custom user too
rpm::FileOptions::new("/etc/awesome/second.toml")
.mode(rpm::FileMode::regular(0o644))
.caps("cap_sys_admin,cap_net_admin=pe")
Copy link
Collaborator

@dralley dralley Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any validation that can or should be done here? (Genuinely a question)

Copy link
Contributor Author

@dsteeley dsteeley Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked into the options, RPM verifies by calling libcap's cap_from_text().
There are several crates providing this mechanism I've gone with capctl as it provides a FileCaps struct that will verify the string provided by a user. Erroring if the capability is invalid.

I then convert back to String for the writing of the header, hope that's an acceptable solution?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsteeley FYI, I think I will make this into a feature (probably a default one but I'm not certain yet) to restore the ability to compile on Windows

@dralley
Copy link
Collaborator

dralley commented Aug 16, 2023

There are some test failures but apart from that I like the way this looks.

Once that's addressed go ahead and squash it all into one commit (or I can do that when merging it, I suppose)

@dsteeley
Copy link
Contributor Author

Fixed up the test, my switch to use FileCaps causes the default capability (in the case where the header is added) to be =. Which the following doc indicates as correct behaviour for no capability, https://www.man7.org/linux/man-pages/man3/cap_from_text.3.html.
In the case that no files have a capability set the default is still to not add the header.

@dsteeley
Copy link
Contributor Author

@dralley The failing job is an issue compiling time-macros on rust 1.65.
Any preference or suggestion on how I resolve?

@dralley
Copy link
Collaborator

dralley commented Aug 16, 2023

The failing job is an issue compiling time-macros on rust 1.65. Any preference or suggestion on how I resolve?

You can just pin time to an older (compatible) version for now.

@dralley
Copy link
Collaborator

dralley commented Aug 16, 2023

@dsteeley Are you looking for a release soon or is waiting a few weeks OK?

And just out of curiosity, are you actively using (or considering) this for building RPMs at MS?

@dsteeley
Copy link
Contributor Author

@dsteeley Are you looking for a release soon or is waiting a few weeks OK?

And just out of curiosity, are you actively using (or considering) this for building RPMs at MS?

Not in a rush, although I'd like to get this plumbed through cargo-generate-rpm at some point.

We are already using this (via cargo-generate-rpm) to package rust binaries as rpms and then containerise using rpmoci.
I'd like to have to option to set capabilities on those binaries, currently doing it via post install script which isn't the neatest solution.

@dsteeley
Copy link
Contributor Author

@dralley Are you planning to squash this or do you want me to combine into one commit?

@dralley dralley merged commit f6d2c8e into rpm-rs:master Aug 16, 2023
@dralley
Copy link
Collaborator

dralley commented Aug 16, 2023

Nah I got it.

@dralley
Copy link
Collaborator

dralley commented Aug 16, 2023

@dsteeley Thanks!

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

Successfully merging this pull request may close these issues.

2 participants