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

ca-add: fix permission issue #837

Conversation

@frasertweedale
Copy link
Contributor

frasertweedale commented Jun 1, 2017

The ca-add command pre_callback uses ldap.can_add() to check whether
the user has permission to add CAs. Alas, the GetEffectiveRights
control used by ldap.can_add() doesn't correctly interpret ACIs with
'targetfilter' constraints, and returns a false-negative for
non-admin users, even when they have the 'System: Add CA'
permission.

To work around this, add the CA object to FreeIPA before attempting
to create the CA in Dogtag. If the CA creation in Dogtag succeds,
the user then updates the FreeIPA object with the Authority ID and
other authoritative data returned by Dogtag. If the CA creation in
Dogtag fails, the user cleans up by deleting the newly-created CA
object from FreeIPA.

This modified procedure ensures that the user certainly has the
'System: Add CA' permission before the CA creation in Dogtag is
attempted. But it also means that the user must have 'write' and
'delete' permission on 'ipaca' objects in FreeIPA, so that it can
complete the object after CA creation in Dogtag, or clean up if that
step fails. Therefore, update the 'System: Add CA' permission to
confer 'write' and 'delete' access on 'ipaca' objects, as well as
'add' access.

Fixes: https://pagure.io/freeipa/issue/6609

@frasertweedale frasertweedale force-pushed the frasertweedale:fix/6609-ca-add-permissions branch from 6c54e99 to fcd9cb1 Jun 1, 2017
@rcritten

This comment has been minimized.

Copy link
Contributor

rcritten commented Jun 1, 2017

Is there a bug filed on the GER issue?

@frasertweedale

This comment has been minimized.

Copy link
Contributor Author

frasertweedale commented Jun 1, 2017

@rcritten

This comment has been minimized.

Copy link
Contributor

rcritten commented Jun 1, 2017

Ok cool. I shouldn't have been so terse in my previous comment, what I should have added was "does it make sense to include a pointer to the bug as a hint so workaround can be removed some time in the future?"

This PR is sort of a brute-force solution but given the infrequency it will be executed it seems perfectly reasonable.

@frasertweedale

This comment has been minimized.

Copy link
Contributor Author

frasertweedale commented Jun 2, 2017

@frasertweedale frasertweedale force-pushed the frasertweedale:fix/6609-ca-add-permissions branch from fcd9cb1 to 3e13670 Jun 2, 2017
@frasertweedale frasertweedale force-pushed the frasertweedale:fix/6609-ca-add-permissions branch from 3e13670 to a521c7a Jul 25, 2017
@frasertweedale frasertweedale force-pushed the frasertweedale:fix/6609-ca-add-permissions branch from a521c7a to 8a369d2 Aug 3, 2017
@frasertweedale frasertweedale force-pushed the frasertweedale:fix/6609-ca-add-permissions branch from 8a369d2 to 908c93f Sep 13, 2017
@rcritten

This comment has been minimized.

Copy link
Contributor

rcritten commented Sep 14, 2017

Reading this again it makes me wonder. Which is worse?

  • Allowing entries to be written and deleted in an Add ACI?
  • Dropping the targetfilter to relax why kind of object is writeable in the container?

The first would purposely combine the Add, Delete and Write ACI's into one which could confuse users who really want atomic rights.

@frasertweedale

This comment has been minimized.

Copy link
Contributor Author

frasertweedale commented Sep 15, 2017

@rcritten neither is desirable; this is just a workaround until the issue gets fixed in DS,
after all ;)

@rcritten rcritten self-assigned this Oct 2, 2017
@rcritten

This comment has been minimized.

Copy link
Contributor

rcritten commented Oct 2, 2017

Elsewhere in the framework we just let the add fail rather than using GER to check in advance. What is the benefit of using GER in this case rather than just letting the add fail?

@frasertweedale

This comment has been minimized.

Copy link
Contributor Author

frasertweedale commented Oct 3, 2017

@rcritten the benefit of GER (if it worked the way we want) is that it would allow us to:

  1. check rights
  2. create LWCA in Dogtag
  3. add CA object in IPA (and the Add CA permission would not need to also imply write and delete)

instead of the procedure implemented in this commit:

  1. add CA object in IPA
  2. create LWCA in Dogtag
  3. modify CA object in IPA (add authority ID) or delete it if LWCA creation failed

The latter procedure (which this commit implements) is messier and requires the Add CA permission to grant more privileges than the name suggests.

@rcritten

This comment has been minimized.

Copy link
Contributor

rcritten commented Oct 4, 2017

I guess what I don't understand are the possible reasons creating the LWCA in dogtag would fail.

@frasertweedale

This comment has been minimized.

Copy link
Contributor Author

frasertweedale commented Oct 4, 2017

There aren't too many - database error, direct database manipulation, or direct action against dogtag to e.g. delete or disable the issuing CA would be the main reasons.

But ignoring deletion, there is still the need to modify the IPA entry to add the authority ID, as part of the normal procedure when creation is successful.

@frasertweedale frasertweedale force-pushed the frasertweedale:fix/6609-ca-add-permissions branch from 908c93f to bde856a Dec 13, 2017
@freeipa-pr-ci freeipa-pr-ci removed the re-run label Dec 13, 2017
@tiran

This comment has been minimized.

Copy link
Member

tiran commented Dec 15, 2017

@frasertweedale We had some issues with Travis CI recently. Please rebase on master to fix Travis tests.

@frasertweedale frasertweedale force-pushed the frasertweedale:fix/6609-ca-add-permissions branch from bde856a to 03e5869 Dec 16, 2017
The ca-add command pre_callback uses ldap.can_add() to check whether
the user has permission to add CAs.  Alas, the GetEffectiveRights
control used by ldap.can_add() doesn't correctly interpret ACIs with
'targetfilter' constraints, and returns a false-negative for
non-admin users, even when they have the 'System: Add CA'
permission.

To work around this, add the CA object to FreeIPA before attempting
to create the CA in Dogtag.  If the CA creation in Dogtag succeds,
the user then updates the FreeIPA object with the Authority ID and
other authoritative data returned by Dogtag.  If the CA creation in
Dogtag fails, the user cleans up by deleting the newly-created CA
object from FreeIPA.

This modified procedure ensures that the user certainly has the
'System: Add CA' permission before the CA creation in Dogtag is
attempted.  But it also means that the user must have 'write' and
'delete' permission on 'ipaca' objects in FreeIPA, so that it can
complete the object after CA creation in Dogtag, or clean up if that
step fails.  Therefore, update the 'System: Add CA' permission to
confer 'write' and 'delete' access on 'ipaca' objects, as well as
'add' access.

The GetEffectiveRights problem is being tracked upstream as
https://pagure.io/389-ds-base/issue/49278.  When that ticket has
been fixed, this workaround can and should be reverted.

Fixes: https://pagure.io/freeipa/issue/6609
@frasertweedale frasertweedale force-pushed the frasertweedale:fix/6609-ca-add-permissions branch from 03e5869 to 2867c38 Jan 3, 2018
@frasertweedale

This comment has been minimized.

Copy link
Contributor Author

frasertweedale commented Feb 2, 2018

The 389ds fix seems to have landed. I'll test it; this patch may be needed no longer.

@frasertweedale

This comment has been minimized.

Copy link
Contributor Author

frasertweedale commented Feb 2, 2018

Seems that there are still issue in 389. Hopefully they will be fixed soon...
https://pagure.io/389-ds-base/issue/49278#comment-491731

@frasertweedale

This comment has been minimized.

Copy link
Contributor Author

frasertweedale commented Feb 5, 2018

The 389ds fix does work, I just wasn't using it right :)

Closing this PR and I'll have a new one soon with changes to make can_add work properly.

@frasertweedale frasertweedale deleted the frasertweedale:fix/6609-ca-add-permissions branch Mar 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.