Conversation
- Access Director Agent, for distributing access information to Agents on the OCS, and for granting exclusive access grants. - Under-the-hood support for credential checking, in ocs_agent.py and OCSClient. - Tests. - Documentation.
BrianJKoopman
left a comment
BrianJKoopman
left a comment
There was a problem hiding this comment.
A long overdue review. I have some questions about the configuration and meaning of some things, as well as a bunch of syntax/typo suggestions below. I've tested running this code with a system that doesn't have an AD running yet, to confirm everything works still without setting up AD (to confirm upgrading would be safe, even if one doesn't use the new control rules), but I haven't yet tried to configure the AD/AC policies myself.
docs/agents/access_director.rst
Outdated
| Example 3 -- like Example 2 except that the agent with `instance_id` | ||
| of "danger4" will have the level 2 and level 3 access totally | ||
| disabled (even if "danger4" is a FakeDataAgent.):: | ||
|
|
||
| passwords: | ||
| - default: true | ||
| password_1: '' | ||
| password_2: 'special2' | ||
| password_3: 'special3' | ||
| password_4: 'superuser' | ||
| - agent_class: FakeDataAgent | ||
| instance_id: '!danger4' | ||
| password_2: 'fake2' |
There was a problem hiding this comment.
Not sure I follow this example. My first impression (I started reviewing this a while ago...), was that this should maybe read "...will have the level 3 and level 4 access totally disabled...", since we're only setting the level 2 password. But coming back to it now I'm still confused -- I would think that if the block:
- agent_class: FakeDataAgent
instance_id: '!danger4'
password_2: 'fake2'
matches all FakeDataAgent instances and uses !danger4, then it wouldn't match to the agent with instance id 'danger4', so that agent instance would still follow the:
- default: true
password_1: ''
password_2: 'special2'
password_3: 'special3'
password_4: 'superuser'
block.
There was a problem hiding this comment.
You are correct... I've made a new example 3, which I believe is technically accurate. Not sure what the point of example 3 was supposed to be, I think it probably made more sense in an earlier implementation!
docs/agents/access_director.rst
Outdated
| distributing passwords to agents. (Does not affect passwords that | ||
| were provided already hashed.) |
There was a problem hiding this comment.
Sorry, so what happens if you provide an already hashed password?
There was a problem hiding this comment.
Added the answer, which is "those are distributed in hashed form, correctly annotated with the hashfunc declared for them."
| """ | ||
|
|
||
| def __init__(self, instance_id, **kwargs): | ||
| def __init__(self, instance_id, privs=None, **kwargs): |
There was a problem hiding this comment.
Add new privs arg to docstring.
There was a problem hiding this comment.
Can you also add a test for when the client wants to pass a password but the agent doesn't support it? That's the only thing not covered.
There was a problem hiding this comment.
I believe I got this now. Had to refactor some test_ocs_client stuff a bit.
| couple of seconds for them to synchronize. | ||
|
|
||
| """ | ||
| time.sleep(2) |
There was a problem hiding this comment.
Is there a more reliable way to test (and possibly wait) if they're synchronized? Instances of 'wait for this to be true' tend to be flaky depending on the computing environment. And waiting longer to compensate just adds to overall test run time.
There was a problem hiding this comment.
Yes... I have implemented this via the notion of the AD's "step".
| that the programmed passwords work. | ||
|
|
||
| """ | ||
| # This should work tho... |
There was a problem hiding this comment.
Why does 'bad_privs' work? Because access_policy=None?
There was a problem hiding this comment.
Changed to "This should work because acq requires only level 1, which is not password protected."
Co-authored-by: Brian Koopman <BrianJKoopman@users.noreply.github.com>
Co-authored-by: Brian Koopman <BrianJKoopman@users.noreply.github.com>
Co-authored-by: Brian Koopman <BrianJKoopman@users.noreply.github.com>
Co-authored-by: Brian Koopman <BrianJKoopman@users.noreply.github.com>
Co-authored-by: Brian Koopman <BrianJKoopman@users.noreply.github.com>
Co-authored-by: Brian Koopman <BrianJKoopman@users.noreply.github.com>
Co-authored-by: Brian Koopman <BrianJKoopman@users.noreply.github.com>
- add manager session.data to docstring - validate agent_poll args - add grantee to grant rejection message
|
Ok, I think I got everything. Thanks for the detailed review. |
2 participants
Description
Includes:
This does not include an OAuth -> OCS privileges system, but it is heading in that direction.
Motivation and Context
Resolves #124
How Has This Been Tested?
This PR includes integration tests; I also tested extensively with a dummy ocs including FakeDataAgent, HostManager, Agg. Used OCSClient to command, but also ocs-web (which was coded for this a long time ago). The unit tests in this PR include "what if an OCSClient that from earlier ocs is used?"
Types of changes
Checklist: