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

Fixes #10443 - added nova/glance/neutron rules #45

Closed
wants to merge 1 commit into from

Conversation

lzap
Copy link
Member

@lzap lzap commented Feb 5, 2015

Ok I've missed several other ports. The discussion is in the linked BZ.

There is one snag - the commented macro is missing in RHEL6 and we need to,
once again, define our own port. It will likely cause issues because I expect
this one to be introduced in RHEL6. I need to find a way how to deal with this
first. Then let's get back to this.

@domcleal
Copy link
Contributor

domcleal commented Feb 5, 2015

Administrative note, please don't reference a closed + shipped ticket - create a new one and describe the issue.

@lzap
Copy link
Member Author

lzap commented Feb 5, 2015

Administrative note, please don't reference a closed + shipped ticket - create a new one and describe the issue.

Not so sure about this one. The original patch did not actually fix the
issue. The linked BZ failed verification and returned on me. I don't
want to introduce confusion between RM-BZ, thus I've decided to simply
ref the original one.

Later,
Lukas #lzap Zapletal

@domcleal
Copy link
Contributor

domcleal commented Feb 5, 2015

The Redmine issue has shipped, please file a new issue for it.

@lzap
Copy link
Member Author

lzap commented May 18, 2015

This patch introduces new foreman_allowed_port_t type for all
foreman-defined ports. Also it introduces helper function in the enable
script which is now the most sane way of adding ports - if a port number is
already defined, it uses -m option to redefine it.

Fixed issue reference.

@lzap lzap changed the title WIP Refs #7346 - added nova/glance/neutron rules Fixes #10443 - added nova/glance/neutron rules May 18, 2015
}

is_redhat_6() {
uname -r | grep -o el6 &>/dev/null
Copy link
Member Author

Choose a reason for hiding this comment

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

Are you aware of better way identifying RHEL version?

Copy link
Contributor

Choose a reason for hiding this comment

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

test x$(rpm -q --whatprovides redhat-release --qf '%{version}') = x6 would be more standard I believe

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@domcleal
Copy link
Contributor

domcleal commented Jun 1, 2015

Have you checked this builds on F19?

@lzap
Copy link
Member Author

lzap commented Jun 3, 2015

Not checked the build, checking. Amended the rhel6 test function.

@lzap
Copy link
Member Author

lzap commented Jun 3, 2015

Ok it looks like I tested against newer versions of RHEL6, in koji it fails. The same for F19.

I am going to change the strategy. Instead of quering for RHEL6 version in the enable script, I will query per port. If a port is not assigned (e.g. neutron) I will assign to foreman_allowed_port_t. Opinions?

@lzap
Copy link
Member Author

lzap commented Jun 3, 2015

In the policy, I'd do something like:

# port is assigned via foreman_allowed_port_t if this fails
optional_policy(`
    tunable_policy(`passenger_can_connect_openstack',`
        # keystone (identity service)
        corenet_tcp_connect_commplex_port(passenger_t)
    ')
')

for all ports.

@lzap
Copy link
Member Author

lzap commented Jun 3, 2015

Pushed the idea, untested. What you think?

@domcleal
Copy link
Contributor

domcleal commented Jun 3, 2015

Not really sure where you're going with this - so there's now a boolean, but the port might also be in foreman_allowed_port_t? So to disable it you have to edit the port list and disable the boolean, as doing only one might just cause it to be allowed through the second..

@lzap
Copy link
Member Author

lzap commented Jun 3, 2015

Ok I get your point - boolean must work on all platform. For this reason, I will introduce foreman_openstack_port_t and put this in the tunable_policy block.

Now, the solution with the optional blocks does not work. I haven't realized that syntax errors are not caught by this macro. Scratch that.

We have a problem here - we build on RHEL 6.3 but some OpenStack bits were added in later versions (6.6). For this reason, I think I need to do a fallback solution - everything that is not defined in 6.3 we will re-assign into our own port type.

@lzap lzap force-pushed the improv-openstack-7346 branch 2 times, most recently from 178ca72 to e3967ce Compare June 3, 2015 13:25
@lzap
Copy link
Member Author

lzap commented Jun 3, 2015

Something like this (untested, does not yet build).

@lzap
Copy link
Member Author

lzap commented Jun 3, 2015

FYI The code is just an idea, still building and finding what is defined and what not.

@lzap
Copy link
Member Author

lzap commented Jul 13, 2015

Ok I have amended and simplified everything. It turns out we only need two ports to let Foreman to work properly: 5000 and 8774 (http://docs.openstack.org/kilo/config-reference/content/firewalls-default-ports.html).

Therefore I simplified everything and doublechecked what is defined in RHEL 6.3+ and in RHEL 7.0+. This is the result.

I am going to test builds from Koji tomorrow.

@lzap
Copy link
Member Author

lzap commented Jul 14, 2015

@ohadlevy
Copy link
Member

@lzap ping ?

@lzap
Copy link
Member Author

lzap commented Jul 23, 2015

Ok I have renamed the port type name to foreman_osapi_compute_port_t as per our IRC discussion. This confirms to naming convention.

@lzap lzap force-pushed the improv-openstack-7346 branch 2 times, most recently from aa04598 to e52c71b Compare July 23, 2015 12:25
assign_or_change_existing() {
if ! /usr/sbin/semanage port -E | grep -qEe "${1}.*-p (tcp|udp) ${2}"; then
if /usr/sbin/semanage port -E | grep -qEe "-p (tcp|udp) $2"; then
echo "port -m -t $1 -p tcp $2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this ever needed? I don't see how it'd be called - might be worth losing it if we can, since semanage isn't particularly fast and we can save a few seconds of installation time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Resurrecting this one after some time, sorry for the delay. What you mean by that? Semanage is indeed slow as hell, but what can we do about it? We need to assign or change existing port, otherwise the policy won't be effective.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I was asking, why have this code to run semanage and reassign ports? The two existing ports, elasticsearch and docker are both added every time around line 40, which works fine. It looks like there's no need to check if the port exists already.

Copy link
Member Author

Choose a reason for hiding this comment

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

So the reasoning behind this is I expect this port to be introduced in SELinux core policy for RHEL6. Then this will cause a problem as you can't add an existing port number or range - you need explicitly to provide -m flag. This was coded as improvement for the future, I could perhaps extend this to the other two types. You have right point this makes things slower - typical semanage port call is about 1-2 seconds. I can perhaps store the list of ports once in a temp file and then grep it multiple times.

On the other hand, we are getting rid both of docker and elasticsearch quite soon. I can perhaps just file a ticket to make this faster. Or I can completely drop this helper and we can solve this as we hit it (if we hit it).

Copy link
Member Author

Choose a reason for hiding this comment

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

To be fair, I need to say that the function is not ideal, it has problems with port ranges - wont work for overlapping ranges.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the reasoning behind this is I expect this port to be introduced in SELinux core policy for RHEL6. Then this will cause a problem as you can't add an existing port number or range - you need explicitly to provide -m flag

If the port's added to the base policy then I don't think this will work, because semanage -E is only going to list ports that customisable (i.e. the ones we add, not base policy ones). It's always going to be empty, and always go to the port -a case.

I do vaguely remember there being the possibility of a conflict with base policy, but I can't reproduce it on EL7 at the moment - it seems to let me redefine a port in base policy. Possibly the reverse isn't true, that if there's a port and base policy adds the same, it fails - but I don't have evidence for it.

Or I can completely drop this helper and we can solve this as we hit it (if we hit it).

I'd suggest this for now.

@lzap
Copy link
Member Author

lzap commented Mar 24, 2016

I have rebased this patch, please elaborate on that comment I don't understand what your concerns are.

@lzap
Copy link
Member Author

lzap commented Apr 25, 2016

Removed the helper, now it should be fast.

@@ -25,6 +29,10 @@ do
/usr/sbin/semanage port -E | grep -q docker_port_t || \
echo "port -a -t docker_port_t -p tcp 2375-2376" >> $TMP

if is_redhat_6; then
echo "port -a -t foreman_osapi_compute_port_t -p tcp 8774" >> $TMP
Copy link
Contributor

Choose a reason for hiding this comment

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

The same semanage port -E check is needed here to skip the step if it's already defined (on EL6), else running this script multiple times will result in errors:

# /usr/sbin/foreman-selinux-enable
/usr/sbin/semanage: Port tcp/8774 already defined
# echo $?
1

The code I wanted removed was only the modification stuff.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok added, note I am not adding TMP_PORTS improvement in this patch, I will rebase the other one and add it here once we merge this one.

This patch introduces new type for missing OpenStack port Compute (Nova).
Also introduces helper function in the enable script which is now the most
sane way of adding ports - if a port number is already defined, it uses `-m`
option to redefine it.
@domcleal
Copy link
Contributor

domcleal commented May 3, 2016

Merged as e54934d, thanks @lzap.

@domcleal domcleal closed this May 3, 2016
@lzap lzap deleted the improv-openstack-7346 branch May 3, 2016 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants