-
Notifications
You must be signed in to change notification settings - Fork 57
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
Ebuild backend #274
base: main
Are you sure you want to change the base?
Ebuild backend #274
Conversation
8a345e5
to
a13fe6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some initial comments. I'd consider rebasing and cleanly splitting the refactoring before adding the ebuild backend as well, like the changes to the Debian side & md5 parts
AS_HELP_STRING([--with-ebuild],[Use the ebuild database as a trust source]), | ||
use_ebuild=$withval,use_ebuild=no) | ||
|
||
if test x$use_ebuild = xyes ; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use AS_IF
. Bare if
has issues with quoting and is no longer recommended in the autoconf manual.
a13fe6b
to
dc272c2
Compare
ff73fd5
to
1796a0f
Compare
Here's the other work I'm hoping to land before the next release, but if we don't make it I can snapshot the code for Gentoo after it's merged. #277 has been raised to separate out the deb-backend changes; I'll rebase once that's merged so that there's less to review here. Would you like me to split out this commit too, or should we squash all of the ebuild-related mess in the middle down into a few logical commits? Edit: I have my TODO up the top, but aside from getting derailed by some 'not caused by my changes' ASAN stuff that I'm debugging in the background, I need to:
On my todo, but not part of this PR, is adding a Gentoo CI/CD stage - there's some work that I'll need to do to come up with something suitable for downstream use. I suppose a short-term alternative, as the ebuild backend adds no new dependencies, would be to tarball that up as test data and just extract it in the test container? I'll ping you again once I think the code is ready for review, but if you have any early feedback I'm willing to address it. :) Edit the second: It won't make this PR / release, but would you be receptive to a Meson build system as an alternative to (or replacement for) autotools? It's starting to get to me a bit, so I'm willing to put in the porting work. |
bffb243
to
e4eabce
Compare
PR #277 appears ready to merge. Do we want to do that first and then rework this patch (if needed)? I have not yet looked this patch over. |
7c256e3
to
2f3e330
Compare
Yes, please - that's a prereq for this PR. I've had some people better at C than I provide some feedback offline and I'll be addressing it over the next few days. |
b239ec1
to
08b0dfa
Compare
I've adapted the deb test to provide some basic assurances that the ebuild backend can read a vdb. I spent a bit of time today working on generating a psuedo-vdb that would enable us to test this without relying on a real Portage installation / database, but I didn't get there in the end. I'll keep plugging away at it in the background, but it may not make it in here. I'm reasonably happy with the current state, reading the VDB is fast enough, though I could stand to work out if there's anything else on a Gentoo system can be safely filtered to reduce the amount of md5ing that happens when populating the trust db. Happy to address any feedback! |
OK, the other patch was merged which appears to create a couple conflicts that need fixing. I'll see if I can look this PR over soon. |
3149256
to
65041a5
Compare
Was it really necessary to rename init.d to extra? That added a whole lot of churn. There is a lot of code in /usr/share. It's in the HFS docs as the place for purely interpreted libraries. Have you compiled and run this with address/undefined behavior sanitizers? |
Apologies for the delayed response, I got deployed to help out with the response to the local severe weather event - today is my first day back at the keyboard.
It seemed like the best way to separate the systemd init scripts, new openrc init stuff, and the application data / configuration that was all stored together under the 'init' folder. If you have a better way of organising it (or just one that you prefer) I'm happy to amend that commit.
In Gentoo it's 'architecture independent application data which is not modified at runtime.', but I do see some python, JS, and... C header files in amongst all the other binary files installed there on my development box. I'll have to come up with a better filter. I'll force push and drop that for the short term.
Yes. I actually have a branch where I'm tracking down some weirdness that I thought I caused but apparently existed in the code before I started touching it. I have a minimal broken example so if I can't work that out I'll log a bug:
I am currently leaking 13 bytes allocated here that I haven't been able to track down, I'll try and look into that soon but I'm still recovering from working a week and a half of my vacation! |
Hello, sounds like you've been busy. I think it might be best to not change the name of the directory as part of this patch set. That can/should be done separately. The idea is to minimize the changes in case we need to bisect back to this. The original idea was that everything related to config and initialization would live in the init directory. Let's address this after this patch is merged. Also, I think we should schedule this patch for after the 1.3.3 release so that we can get other bug fixes out to the community. Maybe since this is a new platform being on-boarded, we call the next release 1.4. Regarding /usr/share, you might look at how we do this for rpm. We use a config file, fapolicyd-filter.conf. There is a library/filter.h file that shows the API that uses it. We had to resort to this because we found that we may be getting rid of files that the user really wants to trust. So, we gave them a way to take control. As for the address sanitizer issue, I think that should be filed separately and treated as a 1.3.3 release blocker. I can see an issue it my testing too. Thanks for spotting it. Issue #281 was opened for this. @radosroka , have you seen the above address sanitizer issue? |
Would you be happy for a patch/PR that moves the existing files into the new structure so that the 'gentoo PR' can just add the openrc files, or is there a strong preference for sticking with the existing approach?
No worries, less time pressure to look at filtering. 😄 Gentoo has a binary package host now, so it will be sane to add a containerised CI/CD option where it still wasn't late last year - I'll look into that too if I have time!
Will do. |
Is this from the daemon itself or CLI? |
It's from the daemon itself. |
And was it on this branch or main? |
Not forgotten, just had a few other projects eat up my time. I'll aim to get this finished off for late April / Early may; I have some leave coming up :) |
- add ebuild backend to backend manager - add ebuilddb to database - remove `MD5` call from `USE_DEB` gating in `get_hash_from_fd2`
Based on examples from debian backend (md5) and RPM backend.
- ./init -> ./extra (there's more than just init stuff here) - systemd unit -> extra/systemd - new ./extra/openrc/{init,conf}.d/fapolicy - exclude operc files from RPM build
Also: - properly nest FAQ content - fix indentation of code block under notes - fix table formatting (at least, as good as GFM tables get)
65041a5
to
f513fbf
Compare
f513fbf
to
396803f
Compare
OK I put fvwm3 to bed and finally have some time for this. Thanks for the pointer on the filter - I'm almost there with it's implementation - I seem to be having some difficulty getting anything to match the filters though, which is annoying. I'll try and track that down over the next little bit. All that I can tell for now is that the filter file is loaded (just a slightly modified fedora config because our kernel path is
My "sophisticated debugging" techniques here seem to reveal that
I also note that
Otherwise we seem to be looking pretty good; after 15 minutes or so in permissive mode; it's even happy with the package manager (at least in terms of it removing things!):
|
library/md5-backend.c \ | ||
library/md5-backend.h | ||
library/md5-backend.h \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since WITH_EBUILD
pulls in NEED_MD5
now, this should be removable. I would similarly suggest that WITH_DEB
should be checked to ensure it does, and remove that from the WITH_DEB
chunk as well.
typedef struct contents { | ||
char *md5; | ||
char *path; | ||
} ebuildfiles; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, but this describes a single file, so it should properly be ebuildfile
- otherwise it is confusing if the object is an array of file objects or just a single file.
@@ -462,11 +462,7 @@ char *get_hash_from_fd2(int fd, size_t size, const int is_sha) | |||
if (is_sha) { | |||
SHA256(mapped, size, (unsigned char *)&hptr); | |||
} else { | |||
#ifdef USE_DEB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should likely use an MD5 preprocessor definition instead of becoming unconditional.
#include "fapolicyd-backend.h" // for SRC_EBUILD, backend | ||
#include "filter.h" // for filter_destroy, filter_init, filter_l... | ||
#include "llist.h" // for list_empty, list_init | ||
#include "md5-backend.h" // for add_file_to_backend_by_md5 | ||
#include "message.h" // for msg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From other code in this repository, 8 column tabs are used; these comments don't align with the others when using 8 column tabs.
I stumbled across fapolicyd and thought "that sounds pretty cool" and "I bet I could teach it to read the portage database".
After a month or so of "Fun" this PR is the result.
It's currently a WIP but I figured that good enough to request feedback and see if (once I write some tests and validate that I didn't break anything else...) we can sneak it in before the next release.
I am a C novice, so please let me know what can be improved!
Highlights:
init
->extra
(and categorised some files)TODOs:
libmd
unescape_shell
during the refactoring (I didn't, just noticed something extant with asan enabled, will investigate further)