pyjuju

Merge lp://staging/~hazmat/pyjuju/security-policy-rules-redux into lp://staging/pyjuju

security-policy-rules-redux
Merge into trunk

Proposed by Kapil Thangavelu on 2011-08-04

Status:

Work in progress

Proposed branch:

lp://staging/~hazmat/pyjuju/security-policy-rules-redux

Merge into:

lp://staging/pyjuju

Prerequisite:

lp://staging/~hazmat/pyjuju/security-policy-with-topology

Diff against target:

1217 lines (+586/-208) (has conflicts)

14 files modified

juju/providers/local/__init__.py (+0/-1)
juju/state/initialize.py (+2/-2)
juju/state/security.py (+17/-27)
juju/state/securityrules.py (+192/-0)
juju/state/tests/common.py (+29/-39)
juju/state/tests/test_agent.py (+0/-11)
juju/state/tests/test_auth.py (+3/-3)
juju/state/tests/test_base.py (+7/-7)
juju/state/tests/test_environment.py (+0/-4)
juju/state/tests/test_firewall.py (+1/-1)
juju/state/tests/test_initialize.py (+14/-3)
juju/state/tests/test_security.py (+38/-108)
juju/state/tests/test_securityrules.py (+223/-0)
juju/state/tests/test_service.py (+60/-2)

Text conflict in juju/state/service.py
Text conflict in juju/state/tests/common.py
Text conflict in juju/state/tests/test_initialize.py
Text conflict in juju/state/tests/test_service.py

To merge this branch:

bzr merge lp://staging/~hazmat/pyjuju/security-policy-rules-redux

High

In Progress

Link a bug report

Reviewer	Review Type	Date Requested	Status
Gustavo Niemeyer		2011-08-04	Needs Fixing on 2011-08-10
Review via email: mp+70502@code.staging.launchpad.net

This proposal supersedes a proposal from 2011-08-04.

Description of the change

Redone with updated api and some simplifications.

Security ACL generator rules to match nodes based on path. The meat of the security ACL policy is embodied here, and this will likely some tweaking in the future.

Thou shall not forget pre-requisite branches lp:~hazmat/ensemble/security-policy-with-topology

lp://staging/~hazmat/pyjuju/security-policy-rules-redux updated on 2011-08-10

323. By Kapil Thangavelu on 2011-08-05: Merged security-policy-with-topology into security-policy-rules-redux.
324. By Kapil Thangavelu on 2011-08-05: Merged security-policy-with-topology into security-policy-rules-redux.
325. By Kapil Thangavelu on 2011-08-05: resolve conflict from security-group merge
326. By Kapil Thangavelu on 2011-08-05: resolve conflict with security-groups merge
327. By Kapil Thangavelu on 2011-08-05: Merged security-policy-with-topology into security-policy-rules-redux.
328. By Kapil Thangavelu on 2011-08-08: Merged security-policy-with-topology into security-policy-rules-redux.
329. By Kapil Thangavelu on 2011-08-08: Merged security-policy-with-topology into security-policy-rules-redux.
330. By Kapil Thangavelu on 2011-08-08: resolve conflict from security-policy-with-topology merge
331. By Kapil Thangavelu on 2011-08-09: yank security group accessor from service state
332. By Kapil Thangavelu on 2011-08-10: resolve conflict from states-with-principals
333. By Kapil Thangavelu on 2011-08-10: update tests in aftermath of removing service state security group accessor.

Revision history for this message

Gustavo Niemeyer (niemeyer) wrote on 2011-08-10:

This is looking very good overall. Thanks!

A few comments:

[1]

+ machine_token = yield policy.get_token(machine_id)
+ provider_token = yield policy.get_token("ensemble-provider")
+ returnValue([
+ make_ace(machine_token, read=True, write=True, create=True),
+ make_ace(provider_token, read=True, write=True)])

This logic looks very nice.

As a minor from the previous review which still wasn't sorted,
"provisioning-agent" and "ensemble-provider" are different things,
I believe the above refers to the former, not to the latter.

[2]

+ for service_id, service_data in services.items():
+ service_data = dict(service_data)
+ service_data["service_id"] = service_id
+ service_data["token"] = yield policy.get_token(service_id)
+ results.append(service_data)

Why is this using an arbitrary blob of data rather than a typed
value with proper fields? I think we've learned our lesson with the
machine_data.

[3]

+ matched_service = [data for data in service_data \
+ if data["role"] == container].pop()
+ related_service = [data for data in service_data \
+ if data != matched_service].pop()
+ returnValue(
+ [make_ace(matched_service["token"], read=True, create=True),
+ make_ace(related_service["token"], read=True)])

This was raised in the previous review for this branch as well: why
is the node a "container" rather than an individual node under the
relation-id node? Can't we have multiple nodes within those
containers?

[4]

+ # Unit agent can modify tweak any unitother unit settings.

Comment is a bit corrupted.

[5]

+ # Created by the unit agent, read by the provisioning agent.

Might be good to comment here that in the future it will be moved to
the machine agent.

[6]

+ elif path == "/topology":
+ # Only the admin cli can write to this.
+ returnValue([make_ace("anyone", "world", read=True)])

Also part of the original review, IIRC. It feels bad to be exposing
this to _anyone_. We don't have to sort that out right now, but at
least a comment with a recommended future solution would be nice.

[7]

+ elif path == "/auth-tokens":
+ # Any connection that creates another user needs to modify this
+ returnValue([make_ace("anyone", "world", read=True, write=True)])

_Anyone_ can write to the auth tokens!?

This is looking very good overall.  Thanks!

A few comments:

[1]

+    machine_token = yield policy.get_token(machine_id)
+    provider_token = yield policy.get_token("ensemble-provider")
+    returnValue([
+        make_ace(machine_token, read=True, write=True, create=True),
+        make_ace(provider_token, read=True, write=True)])

This logic looks very nice.

As a minor from the previous review which still wasn't sorted,
"provisioning-agent" and "ensemble-provider" are different things,
I believe the above refers to the former, not to the latter.

[2]

+    for service_id, service_data in services.items():
+        service_data = dict(service_data)
+        service_data["service_id"] = service_id
+        service_data["token"] = yield policy.get_token(service_id)
+        results.append(service_data)

Why is this using an arbitrary blob of data rather than a typed
value with proper fields?  I think we've learned our lesson with the
machine_data.

[3]

+            matched_service = [data for data in service_data \
+                               if data["role"] == container].pop()
+            related_service = [data for data in service_data \
+                               if data != matched_service].pop()
+            returnValue(
+                [make_ace(matched_service["token"], read=True, create=True),
+                 make_ace(related_service["token"], read=True)])

This was raised in the previous review for this branch as well: why
is the node a "container" rather than an individual node under the
relation-id node?  Can't we have multiple nodes within those
containers?

[4]

+        # Unit agent can modify tweak any unitother unit settings.

Comment is a bit corrupted.

[5]

+        # Created by the unit agent, read by the provisioning agent.

Might be good to comment here that in the future it will be moved to
the machine agent.

[6]

+    elif path == "/topology":
+        # Only the admin cli can write to this.
+        returnValue([make_ace("anyone", "world", read=True)])

Also part of the original review, IIRC.  It feels bad to be exposing
this to _anyone_.  We don't have to sort that out right now, but at
least a comment with a recommended future solution would be nice.

[7]

+    elif path == "/auth-tokens":
+        # Any connection that creates another user needs to modify this
+        returnValue([make_ace("anyone", "world", read=True, write=True)])

_Anyone_ can write to the auth tokens!?

review: Needs Fixing

lp://staging/~hazmat/pyjuju/security-policy-rules-redux updated on 2012-08-10

334. By Kapil Thangavelu on 2011-08-17: Merged security-policy-with-topology into security-policy-rules-redux.
335. By Kapil Thangavelu on 2012-03-31: merge trunk
336. By Kapil Thangavelu on 2012-03-31: merge trunk
337. By Kapil Thangavelu on 2012-04-02: Merged security-policy-with-topology into security-policy-rules-redux.
338. By Kapil Thangavelu on 2012-08-10: merge pipeline states-with-principles

Unmerged revisions

338. By Kapil Thangavelu on 2012-08-10: merge pipeline states-with-principles
337. By Kapil Thangavelu on 2012-04-02: Merged security-policy-with-topology into security-policy-rules-redux.
336. By Kapil Thangavelu on 2012-03-31: merge trunk
335. By Kapil Thangavelu on 2012-03-31: merge trunk
334. By Kapil Thangavelu on 2011-08-17: Merged security-policy-with-topology into security-policy-rules-redux.
333. By Kapil Thangavelu on 2011-08-10: update tests in aftermath of removing service state security group accessor.
332. By Kapil Thangavelu on 2011-08-10: resolve conflict from states-with-principals
331. By Kapil Thangavelu on 2011-08-09: yank security group accessor from service state
330. By Kapil Thangavelu on 2011-08-08: resolve conflict from security-policy-with-topology merge
329. By Kapil Thangavelu on 2011-08-08: Merged security-policy-with-topology into security-policy-rules-redux.