Fix context.principal #7335
Closed
Fix context.principal #7335
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abbra
added
needs review
abbra
force-pushed
the
fix-context.principal
branch
from
c346d23
to
effe8af
Compare
|
@t-woerner please review. |
abbra
force-pushed
the
fix-context.principal
branch
from
bac54ff
to
b482ef1
Compare
|
There are more places where getattr(context, 'principal') is used in plugins. Would it be good to fix them too? |
|
Yes, I am in the process of fixing those too. The problem I face is how to handle cases where there is no principal to map to and yet that principal is expected. I can do a hard fail (like in cert plugin) but that would be bad. |
|
I would suggest a helper function for the plugin principal retrieval. There is a matching pattern and by my count there are 15 or 16 times we use it. |
abbra
force-pushed
the
fix-context.principal
branch
from
b482ef1
to
5742d7e
Compare
Yep. Moved the check to |
|
The remaining place to investigate is a |
|
@rcritten so I think we can leave out vault's plugin use of |
|
I tried a batch of user_add in server_context and got an AttributeError but there is no visible traceback so I need to dig into this further to see what is going on. |
abbra
force-pushed
the
fix-context.principal
branch
from
5742d7e
to
39a127c
Compare
|
@rcritten I added an experimental commit that performs auditing to the journal for every single command run in the server context. This will have a side-effect that all commands used during installation will also be recorded. Let me know if anything there can be improved. |
|
Fyi, when I say 'all commands', it also means we get auditing of IPA API JSON-RPC endpoint as well: |
abbra
force-pushed
the
fix-context.principal
branch
from
39a127c
to
2436fb2
Compare
|
Here is an example, in TestSimpleReplication::install test: Here is the full output, without dates and hostname prefix: |
|
I just realized I switched the labels. The idea was to put IPA.API as the program name so that |
|
I like the journal logger. I've been testing with -g for now. It is working fine. I was playing with things though and maybe it's just a console error so out-of-band for this change, but try something like showing an unknown user:
It results in a rather gigantic traceback with the raw ldap.NO_SUCH_OBJECT not caught. The journal logged: |
|
Thanks. I see the same in a Fedora's 4.11.1 on F39, so this is unrelated. I think it would be nice if I added an experimental commit -- let me know if that one is OK for you. |
|
@t-woerner could you please check whether this PR works for ansible-freeipa? |
abbra
force-pushed
the
fix-context.principal
branch
from
ec93955
to
6b77e11
Compare
|
The override does make it work a lot nicer in console. |
|
For the enablement of batch command in the IpaAnsibleModule backend for client context we would need to have a way to detect the availability of this PR on the server side. |
|
I tried to bump This is unfortunate and does not allow us to use the versioning of the commands for anything useful. Alternatives that might be used:
Both approaches are easy if ansible-freeipa needs to detect newer API. Relying on API version is cumbersome, especially if these calls need to be backported. |
|
@abbra I think it would be good to add an option to the existing batch command. Something that might help for ansible-freeipa would be to have some sort of a |
|
The only field that ansible-freeipa modules are currently using from the batch results is |
In server-like context we use LDAPI connection with auto-binding to LDAP object based on the UID of the process connecting to LDAPI UNIX domain socket. This means context.principal is not set and we cannot use it. In principal_has_privilege() we can take None principal object as a sign that currently bound LDAP DN has to be checked for the privilege. This allows to match any type of account to the privilege, with exception of the cn=Directory Manager which is never added to privileges explicitly. cn=Directory Manager will be allowed any privilege because it already can write to any LDAP entry. Fixes: https://pagure.io/freeipa/issue/9583 Signed-off-by: Alexander Bokovoy <[email protected]>
|
@t-woerner I added This is what you'd get: |
t-woerner
reviewed
May 17, 2024
abbra
force-pushed
the
fix-context.principal
branch
2 times, most recently
from
1599946
to
f6e5d22
Compare
|
The tests are working, you can see results here: http://freeipa-org-pr-ci.s3-website.eu-central-1.amazonaws.com/jobs/301658bc-144a-11ef-a5bd-fa163e792862/report.html?sort=result |
abbra
force-pushed
the
fix-context.principal
branch
from
f6e5d22
to
5138b28
Compare
|
Removed temp commit and added |
abbra
force-pushed
the
fix-context.principal
branch
from
5138b28
to
74cef3c
Compare
|
Moved instead to gating.yaml because otherwise we'd have to add it to all nightly variants and that is not needed, for sure. |
abbra
force-pushed
the
fix-context.principal
branch
from
74cef3c
to
0822582
Compare
abbra
force-pushed
the
fix-context.principal
branch
2 times, most recently
from
5a41ea7
to
8238dab
Compare
|
I've updated |
abbra
force-pushed
the
fix-context.principal
branch
from
8238dab
to
7f90a93
Compare
Follow pylint recommendations (turned errors in recent pylint updates) and use PEP-380 syntax for subgenerators. This is supported by all Python 3 versions since ~2011. Signed-off-by: Alexander Bokovoy <[email protected]>
batch(methods=Dict(), keeponly=list) will allow to execute batch of commands and remove from the output everything but the attributes which names were passed in the keeponly list. This can be useful if you are only interested in getting names and assigned random passwords, for example. Fix batch API test in test_integration/test_idm_api.py and use it to validate keeponly option. Fixes: https://pagure.io/freeipa/issue/9583 Signed-off-by: Alexander Bokovoy <[email protected]>
abbra
force-pushed
the
fix-context.principal
branch
from
7f90a93
to
95827a4
Compare
|
Moved API audit commit into a separate PR: #7348 |
rjeffman
reviewed
May 20, 2024
|
LGTM keeponly works like a charm. |
t-woerner
approved these changes
May 21, 2024
|
master:
|
|
@abbra we need a manual backport of this PR to ipa-4-11. |
This was referenced May 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.