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

Spec file cleanup #103

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

xavierba
Copy link

@xavierba xavierba commented Aug 2, 2018

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
Copy link
Contributor

mdomsch commented Aug 12, 2018

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
Copy link
Author

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
Copy link
Contributor

mdomsch commented Aug 12, 2018 via email

@xavierba
Copy link
Author

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
Copy link
Contributor

mdomsch commented Aug 14, 2018

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!

Copy link
Member

@mskucherawy mskucherawy left a comment

Choose a reason for hiding this comment

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

Minor tweak to version generation, then this is good to go.

Summary: An open source library and milter for providing ARC service
Name: openarc
Version: @VERSION@
Release: 1%{?dist}
#Version: @VERSION@
Copy link
Member

Choose a reason for hiding this comment

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

configure.ac now generates 1.0.0, so this isn't needed anymore.

All changes but the release bump and additional changelog entry, as well
as the non-upstream patches.
All of Matt Domsch' changes to the specfile are there though.
@xavierba
Copy link
Author

@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
Copy link

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

@sshambar
Copy link

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:

  • tmpfiles.d: /run/openarc should be {0750 openarc:openarc} (not 0700) so only group members can get to the socket.
    And it'd be best to follow good security practices for the rest of the files:
  • /etc/openarc.conf should be {0644 root:openarc} (no secrets here, but don't want rogue openarc changing things)
  • /etc/openarc should be {0755 root:openarc} (only root can create files)
  • key.txt should be {0444 openarc:openarc} - in DNS after all, so all can read
  • key.private should be {0400 openarc:openarc}
  • (NOTE: might be nice to use a keys directory here too, but it'd need to be {0755 root:openarc} too so DNS setup can still get to the .txt records...)
  • other files in /etc/openarc should be {444 openarc:openarc} (could be 0440...)

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
Copy link
Contributor

mdomsch commented Apr 22, 2020

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
Copy link

sshambar commented May 2, 2020

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
Copy link
Contributor

mdomsch commented May 2, 2020

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
Copy link

sshambar commented May 2, 2020

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
Copy link

sshambar commented May 2, 2020

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
Copy link

sshambar commented May 2, 2020

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
Copy link
Contributor

mdomsch commented May 2, 2020

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
Copy link

sshambar commented May 2, 2020

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
Copy link

sshambar commented May 2, 2020

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
Copy link

sshambar commented May 2, 2020

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
Copy link
Contributor

mdomsch commented May 3, 2020

As the Fedora Packaging Guidelines suggest, I've started a thread on the [email protected] 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/[email protected]/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
Copy link

sshambar commented May 3, 2020

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 :)

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.

5 participants