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

Rbac integration #5086

Merged
merged 17 commits into from
Nov 27, 2020
Merged

Rbac integration #5086

merged 17 commits into from
Nov 27, 2020

Conversation

hnanchahal
Copy link
Contributor

@hnanchahal hnanchahal commented Nov 19, 2020

Integrate RBAC backend https://github.com/StackStorm/st2-rbac-backend into st2 core natively.

@pull-request-size pull-request-size bot added the size/XS PR that changes 0-9 lines. Quick fix/merge. label Nov 19, 2020
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

I left a comment below that'll help to integrate the rbac py package into st2.

fixed-requirements.txt Outdated Show resolved Hide resolved
@arm4b arm4b added the RBAC label Nov 20, 2020
@arm4b arm4b added this to the 3.4.0 milestone Nov 20, 2020
fixed-requirements.txt Outdated Show resolved Hide resolved
@arm4b
Copy link
Member

arm4b commented Nov 20, 2020

As we're still waiting for TravisCI, wondering that there is also a need to test for RBAC presence via st2-packaging tests.
Additionally, looks like RBAC ships the st2-apply-rbac-defintions binary required to apply the rbac rules.

@hnanchahal Can you please open a PR against the st2-packages to include something along the lines there:
StackStorm/st2-packages@0bf2076

This will ensure that st2-apply-rbac-defintions binary is shipped with the st2 deb/rpm package and test it.

@arm4b
Copy link
Member

arm4b commented Nov 20, 2020

CI reports some real issues. Please check the https://travis-ci.org/github/StackStorm/st2/jobs/744939156#L1775-L1787 with the instructions. Similar to previous PR, this needs another make requirements to update it all properly.

@pull-request-size pull-request-size bot added size/S PR that changes 10-29 lines. Very easy to review. and removed size/XS PR that changes 0-9 lines. Quick fix/merge. labels Nov 23, 2020
@arm4b
Copy link
Member

arm4b commented Nov 23, 2020

st2-packages still needs a change for enabling apply-rbac-defintions, described here in details: #5086 (comment)
@hnanchahal Can you please compose that PR to ensure we're shipping all the RBAC bits with the packages?

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

To recap the blockers for this PR:

@hnanchahal
Copy link
Contributor Author

@armab I think the Unit test is failing because its not getting "default" from available_rbac_backends. Any pointers?

def validate_rbac_is_correctly_configured():
"""
Function which verifies that RBAC is correctly set up and configured.
"""
if not cfg.CONF.rbac.enable:
return True

from st2common.rbac.backends import get_available_backends
available_rbac_backends = get_available_backends()

# 1. Verify auth is enabled
if not cfg.CONF.auth.enable:
    msg = ('Authentication is not enabled. RBAC only works when authentication is enabled. '
           'You can either enable authentication or disable RBAC.')
    raise ValueError(msg)

# 2. Verify default backend is set
if cfg.CONF.rbac.backend != 'default':
    msg = ('You have enabled RBAC, but RBAC backend is not set to "default". '
           'For RBAC to work, you need to install "st2-rbac-backend" package, set '
           '"rbac.backend" config option to "default" and restart st2api service.')
    raise ValueError(msg)

# 2. Verify default RBAC backend is available
if 'default' not in available_rbac_backends:
    msg = ('"default" RBAC backend is not available. Make sure '
           '"st2-rbac-backend" system packages are installed.')
    raise ValueError(msg)

@arm4b
Copy link
Member

arm4b commented Nov 25, 2020

@hnanchahal Right, the new state is that RBAC is now part of the st2 core and those tests need an adjustment to reflect the integration.

For instance, instructions like:

For RBAC to work, you need to install "st2-rbac-backend" package, set rbac.backend" config option to "default" and restart st2api service

recommends installing the st2-rbac-backend, while it's not needed anymore.

@hnanchahal
Copy link
Contributor Author

@hnanchahal Right, the new state is that RBAC is now part of the st2 core and those tests need an adjustment to reflect the integration.

For instance, instructions like:

Understood that part. But since the rbac-backend package is being included in the requirements for st2common, why the driver_loader is failing to pick up rbac backend.
I assume
from st2common.rbac.backends import get_available_backends
available_rbac_backends = get_available_backends()

is not returning the default rbac backend.

@arm4b
Copy link
Member

arm4b commented Nov 25, 2020

Are you referring to this https://travis-ci.org/github/StackStorm/st2/jobs/745513542#L2230-L2236 unit test failure case?

   AssertionError: ValueError not raised by validate_rbac_is_correctly_configured

From what I understand, the test enables rbac, auth and expects that it'll raise an error because previously RBAC wasn't available in the default installation.

def test_validate_rbac_is_correctly_configured_default_backend_not_available(self):
cfg.CONF.set_override(group='rbac', name='enable', override=True)
cfg.CONF.set_override(group='rbac', name='backend', override='default')
cfg.CONF.set_override(group='auth', name='enable', override=True)
expected_msg = ('"default" RBAC backend is not available. ')
self.assertRaisesRegexp(ValueError, expected_msg,
validate_rbac_is_correctly_configured)

Because RBAC package is now part of the core, I think the unit test case there should not expect error message (ValueError) anymore.

Let's start from the point you mentioned, like more explicitly check that default is now in get_available_backends() for RBAC in the Unit tests?

st2api/st2api/validation.py Outdated Show resolved Hide resolved
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

The Unit tests are passing now, - that's some good progress 👍

However linting is unhappy due to minor formatting issues. Check the instructions here: https://travis-ci.org/github/StackStorm/st2/jobs/745963664#L1642-L1646 about how to fix it.

I also provided other minor comments we'll need to fix before merging this PR.

st2api/tests/unit/test_validation_utils.py Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Looks good now.

I've tested the produced packages myself and can confirm that RBAC worked.

@arm4b
Copy link
Member

arm4b commented Nov 27, 2020

@hnanchahal Thanks for working on this integration!

We'll need st2docs update for the RBAC feature as a next step, see more details here: StackStorm/st2docs#1038

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RBAC size/S PR that changes 10-29 lines. Very easy to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants