search

trusteddomainproject / OpenARC

Open source ARC implementation
BSD 2-Clause "Simplified" License
135 stars 45 forks source link

Spec file cleanup #103

Open xavierba opened 6 years ago

xavierba commented 6 years ago

PR against master, although the real OpenArc works happens in the develop branch, but the 2 previous specfile cleanup commits were not synch'ed to the develop branch.

mdomsch commented 6 years ago

https://bugzilla.redhat.com/show_bug.cgi?id=1615162 filed. xavierba, please note that the Source0 URL pattern is incorrect in your PR, compared to the release filename in github.

xavierba commented 6 years ago

I may be missing something, but why do you think the Source0 is not correct ? It will expand to: https://github.com/trusteddomainproject/OpenARC/archive/v1.0.0.Beta0/openarc-1.0.0.Beta0.tar.gz

mdomsch commented 6 years ago

The file name is https://github.com/trusteddomainproject/OpenARC/archive/v1.0.0.Beta0.tar.gz https://github.com/trusteddomainproject/OpenARC/archive/v1.0.0.Beta0/openarc-1.0.0.Beta0.tar.gz

It is poorly named in the release.

On Sun, Aug 12, 2018, 6:03 PM Xavier Bachelot notifications@github.com wrote:

I may be missing something, but why do you think the Source0 is not correct ? It will expand to:

https://github.com/trusteddomainproject/OpenARC/archive/v1.0.0.Beta0/openarc-1.0.0.Beta0.tar.gz

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/trusteddomainproject/OpenARC/pull/103#issuecomment-412377883, or mute the thread https://github.com/notifications/unsubscribe-auth/AAqDqrK8_NAb9gGY_XdPsV6IILeNSHlIks5uQLQDgaJpZM4VsYj4 .

xavierba commented 6 years ago

Ok, I understand now. Yes, indeed, it doesn't match, but one can somewhat "fix it" by using a better constructed url and get a properly named tarball. That's the reason I did it this way.

mdomsch commented 6 years ago

You are correct, I retract my above comment. However, rpmlint does not like the %{URL} in the Source0 line, it will not expand it and will throw an error. For the Fedora package review I manually replaced that, and would suggest you do likewise in this PR for consistency. Thank you for your cleanups!

xavierba commented 6 years ago

@mskucherawy, I did the tweak you requested.

@mdomsch, I took the opportunity to also add a commit that sync with Fedora specfile where relevant. As stated in the commit message, I sync everything but the release bump and changelog entry and also not the patches. The first patch is/was PR #104, which hes been fixed differently, the second, which is a configure.ac fix, should imho have a PR filed if not done already.

matt-domsch-sp commented 6 years ago

I've merged these changes into the Fedora package rpm spec file and build for epel7, f30, f29, f28. Updates pending.

sshambar commented 4 years ago

An additional fix: use "%{_rundir}" in place of "%{_localstatedir}/run" -- as at least on Fedora, it resolves to "/run" rather than "/var/run" (really the same directory), and systemd complains whenever I perform a systemctl daemon-reload with:

systemd[1]: /usr/lib/systemd/system/openarc.service:8: PIDFile= references a path below legacy directory /var/run/, updating /var/run/openarc/openarc.pid → /run/openarc/openarc.pid; please update the unit file accordingly.

-- Also, to support socket access:

All this keeps the openarc user (hacked server) from modifying anything, but makes it easy to "chmod o-rwx" to go extra paranoid mode too :)

-- One other item, openarc.conf doesn't support Umask (best to remove the commented option)... and in line with openarc group being able to read socket files, the openarc.service file needs to have:

[Service] UMask=0007

Added to it or else the default is 0022, which prevents group writes (prob best in openarc source, but quick patch could be added to the .spec file :)

mdomsch commented 4 years ago

Thanks much for the suggestions. I finally got around to incorporating them into the Fedora packaging.
https://src.fedoraproject.org/rpms/openarc/blob/master/f/openarc.spec is building now.

This takes all your suggestions, and also adds systemd system unit file options for ProtectSystem and ProtectHome, which further prevents writes to protected directories.

sshambar commented 4 years ago

Finally had a chance to try the build, but there are a couple problems that make it a little broken:

Typo in the config file template: (missing the trailing r) Socket local:%{_rundi}/%{name}/%{name}.sock

Protect system too strict: ProtectSystem=strict ... should be =full (strict makes /run/openarc read-only too, and openarc can't start)

Not a bug, but you could also remove the tmpfiles.d/openarc.conf file completely, and add the following to the [Service] section of the .service file: RuntimeDirectory=memcached RuntimeDirectoryMode=0750

Just some more ideas.. might be worth a new build too so people don't have to create a drop-in service file to override the ProtectSystem :)

Bodhi wouldn't load the page with the output, but there appeared to be a bunch of rpmlint issues too; I might try running that later and see if I can find out what it didn't like...

mdomsch commented 4 years ago

Thanks for catching the typo and for the other suggestions. I implemented RuntimeDirectory, but am getting an selinux denial now when sendmail goes to open the socket, which didn't happen with the tmpfiles.d solution. This will need more investigation before using this method. Any ideas? This is on a CentOS 8 install. opendkim and opendmarc both use dkim_milter_data_t instead of var_run_t here. I'll probably need an semanage fcontext somewhere here.

type=AVC msg=audit(1588397271.937:223834): avc: denied { getattr } for pid=9348 comm="sendmail" path="/run/openarc/openarc.sock" dev="tmpfs" ino=17712527 scontext=system_u:system_r:sendmail_t:s0 tcontext=system_u:object_r:var_run_t:s0 tclass=sock_file permissive=0

sshambar commented 4 years ago

That's very interesting... you had it working with SELinux using tmpfiles.d?? Openarc isn't in the SELinux config on my Fedora 31 system (and I'm guessing not in CentOS, but I don't have an install to test) -- I had to add my own rules to get it to work:

semanage fcontext -a -t dkim_milter_data_t '/var/run/openarc(/.*)?'
semanage fcontext -a -t dkim_milter_data_t '/var/spool/postfix/var/run/openarc(/.*)?'

Without those, it always came up as var_run_t regardless of RuntimeDirectory= or tmpfiles.d...

I'm not sure if a package can add it's own "missing" SELinux policies or not, but until the selinux package is fixed, you might need to add a README-SELinux or similar to the docs.

sshambar commented 4 years ago

In fact... since openarc is unconfined, I also had to add the following custom policy to get postfix to work (didn't try sendmail, probably needs something similar)...

policy_module(myopenarc,1.0.0)

require {
        type postfix_cleanup_t, postfix_smtpd_t;
        type unconfined_service_t;
        class unix_stream_socket { connectto };
};

# also allow connectto openarc (unconfined)
allow postfix_cleanup_t unconfined_service_t:unix_stream_socket connectto;
allow postfix_smtpd_t unconfined_service_t:unix_stream_socket connectto;

I considered trying to get openarc to use the opendkim context, but I wasn't sure what would happen if an upgrade actually added an openarc policy, so I went with the less conservative approach :)

sshambar commented 4 years ago

OK, did a little research, and it appears you can attach a policy to an rpm, see:

https://selinuxproject.org/page/RPM#Adding_Policy_to_an_RPM

I also successfully ran openarc as dkim_milter_t (used by opendkim and opendmarc), which means the policy becomes as simple as 3 fcontext lines in two files:

--- openarc.te ---

policy_module(openarc,0.9.0)

--- openarc.fc ---

/usr/sbin/openarc      --  gen_context(system_u:object_r:dkim_milter_exec_t,s0)
/var/run/openarc(/.*)?         gen_context(system_u:object_r:dkim_milter_data_t,s0)
/var/spool/postfix/var/run/openarc(/.*)?       gen_context(system_u:object_r:dkim_milter_data_t,s0)

I also tried installing the module when the file context already existed (from another policy, to mock openarc actually getting added to the default policy later), and it installed without error, so it should be "future proof" (at least as long as the opendkim policy doesn't conflict with openarc's behavior :D)

mdomsch commented 4 years ago

On Centos8 with sendmail, reverting to using tmpfiles.d to create /run/openarc, and setting the fcontext is sufficient for sendmail to be able to open the socket.

I cannot get systemd service RuntimeDirectory labels correct at service startup without manually running a restorecon on them using an ExecStartPost=/sbin/restorecon -r -F /run/openarc. I suppose that's better than tmpfiles.d but still odd. I found one reference to a similar problem back in 2015 which was fixed back then. https://bugzilla.redhat.com/show_bug.cgi?id=1192726

I will see about adding the policy as described above.

sshambar commented 4 years ago

Ah, a static fcontext would work for an old redhat build, but newer systemd's put /run on tmpfs (so fresh every boot, RuntimeDirectory just re-creates it on every service start!), and you'd need the ExecStartPost=restorecon anyway without the fcontext in policy...

sshambar commented 4 years ago

Oh, and I ran rpmlint (still can't see the bodhi report), but here's the output:

openarc.src: W: spelling-error Summary(en_US) milter -> molter, miler, miter openarc.src: W: spelling-error %description -l en_US milter -> molter, miler, miter openarc.src:82: W: macro-in-comment %{_docdir} openarc.src:82: W: macro-in-comment %{name} openarc.src:82: W: macro-in-comment %{version} openarc.src:93: W: macro-in-comment %{_sysconfdir} openarc.src:94: W: macro-in-comment %{_sysconfdir} openarc.src:99: W: macro-in-comment %{_sysconfdir} openarc.src: W: invalid-url Source0: https://github.com/trusteddomainproject/OpenARC/archive/v1.0.0.Beta3/openarc-1.0.0.Beta3.tar.gz HTTP Error 404: Not Found

Clearly, ignore the spelling ones, but the comments can be fixed by either removing the extra %{xx} refs (as they're expanded before parsing, so could break things), or using a double %, eg:

# opendkim-genkey -D %%{_sysconfdir}...

The source error or just because you used a different label for the release, could use the real name, (rel-openarc...) ie:

https://github.com/trusteddomainproject/OpenARC/archive/rel-openarc-1-0-0-Beta3.tar.gz

or just label the same as the source archive next release :)

Just an FYI...

sshambar commented 4 years ago

BTW, there's a bug in the current selinux-policy on Fedora wrt opendkim and postfix in chroot (which probably means on other distributions)... I've logged it as:

https://bugzilla.redhat.com/show_bug.cgi?id=1830587

but you could include my fix in your openarc.te file (as it will affect openarc too as it'll use the same executable context):

require {
        type dkim_milter_t;
};

postfix_search_spool(dkim_milter_t);
mdomsch commented 4 years ago

As the Fedora Packaging Guidelines suggest, I've started a thread on the selinux@lists.fedoraproject.org mailing list for advice on how best to proceed - if we need a new module all together or if leaning on the existing dkim module is sufficient, and then how to package the result. https://lists.fedoraproject.org/archives/list/selinux@lists.fedoraproject.org/thread/RORNZO33NETOTFXBAHAADUOU3WAFWOCH/

All the rpmlint warnings/errors are benign. The comment lines are all about content going into configuration files and should be there. The tarball is misnamed because of the way things are tagged. I left a comment about it and will leave it at that.

sshambar commented 4 years ago

Looks good, although you did always refer to /etc/openarc/openarc.sock when I think you meant /run/openarc/openarc.sock, but I'm guessing they'll figure that out :)