[PATCH] 315-319 KRA realm related patches
Patch descriptions (in reverse order). The final patch will need some discussion. Please review, Ade *********************************************** commit 4a1fb1e678d0024d9ee51fcda0d83f74f1715f4b Author: Ade Lee <alee@redhat.com> Date: Thu Jun 2 09:41:35 2016 -0400 Modify pki-server db-upgrade to do realm related upgrades Tickets 2320, 2319 commit ed3e2da4c598bf4cec89bec8e20a23ab6d82013c Author: Ade Lee <alee@redhat.com> Date: Fri May 27 14:01:59 2016 -0400 New VLV indexes for KRA including realm commit 1a2947fed2f7cd2cc32fa810ab77d64bf3acb821 Author: Ade Lee <alee@redhat.com> Date: Thu May 26 00:48:39 2016 -0400 Fix legacy servlets to check realm when requesting recovery commit 483f9b2066110c3b8d4598e3afe1a9508bddbbb7 Author: Ade Lee <alee@redhat.com> Date: Wed May 25 18:53:22 2016 -0400 Change legacy requests servlet to check realm The legacy KRA servlet has been modified to check the realm if present in the request, or only return non-realm requests if not present. No attempt is made to fix the error reporting of the servlet. As such, an authz failure due to the realm check is handled in the same way that other authz failures are handled. commit 6c52845955315ca8842290d41c826c26aa037eb3 Author: Ade Lee <alee@redhat.com> Date: Wed May 25 18:10:59 2016 -0400 Fix old KRA servlets to check realm The old KRA servlets to list and display keys do not go through the same code paths as the REST API. Therefore, they do not check the authz realm. This patch adds the relevant code. No attempt is made to fix the error handling of the old servlets. the long term solution for this is to deprecate the old servlets and make the UI use the REST API instead. Therefore, authz failures due to realm checks are handled in the same way as other authz changes.
And now with the patches .. On Thu, 2016-06-02 at 09:50 -0400, Ade Lee wrote:
Patch descriptions (in reverse order).
The final patch will need some discussion. Please review,
Ade
*********************************************** commit 4a1fb1e678d0024d9ee51fcda0d83f74f1715f4b Author: Ade Lee <alee@redhat.com> Date: Thu Jun 2 09:41:35 2016 -0400
Modify pki-server db-upgrade to do realm related upgrades
Tickets 2320, 2319
commit ed3e2da4c598bf4cec89bec8e20a23ab6d82013c Author: Ade Lee <alee@redhat.com> Date: Fri May 27 14:01:59 2016 -0400
New VLV indexes for KRA including realm
commit 1a2947fed2f7cd2cc32fa810ab77d64bf3acb821 Author: Ade Lee <alee@redhat.com> Date: Thu May 26 00:48:39 2016 -0400
Fix legacy servlets to check realm when requesting recovery
commit 483f9b2066110c3b8d4598e3afe1a9508bddbbb7 Author: Ade Lee <alee@redhat.com> Date: Wed May 25 18:53:22 2016 -0400
Change legacy requests servlet to check realm
The legacy KRA servlet has been modified to check the realm if present in the request, or only return non-realm requests if not present.
No attempt is made to fix the error reporting of the servlet. As such, an authz failure due to the realm check is handled in the same way that other authz failures are handled.
commit 6c52845955315ca8842290d41c826c26aa037eb3 Author: Ade Lee <alee@redhat.com> Date: Wed May 25 18:10:59 2016 -0400
Fix old KRA servlets to check realm
The old KRA servlets to list and display keys do not go through the same code paths as the REST API. Therefore, they do not check the authz realm.
This patch adds the relevant code. No attempt is made to fix the error handling of the old servlets. the long term solution for this is to deprecate the old servlets and make the UI use the REST API instead. Therefore, authz failures due to realm checks are handled in the same way as other authz changes.
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
On 6/2/2016 8:51 AM, Ade Lee wrote:
And now with the patches ..
On Thu, 2016-06-02 at 09:50 -0400, Ade Lee wrote:
Patch descriptions (in reverse order).
The final patch will need some discussion. Please review,
Ade
Some comments: 1. In SrchKey and SrchKeyForRecovery the check probably should have been like this (no closing paren): if (filter.contains("(realm=")) { 2. If a realm is specified, I suppose it won't match any of the VLV indexes. Do the queries still work correctly without a matching VLV? 3. Could you document how to run the upgrade command in this page? http://pki.fedoraproject.org/wiki/Database_Upgrade_for_PKI_10.3.x Note that there are still some manual upgrade procedures that also need to be executed as documented in that page. 4. In PKISubsystem.open_database() can we use bind_dn and bind_password parameter names instead of user and password (it's a DN not a user ID)? Also in DBUpgrade should we use -D, -w, and -Z for consistency with DS tools? 5. The gnu_getopt() is called with "server_id=". I think it should be "server-id=". But since it actually contains the DS instance name maybe we should use "ds-instance=" instead? 6. In the DBUpgrade.modify_kra_vlv() the os.unlink(ldif_file) probably should be moved into the finally clause above it to ensure the temporary file is deleted in all cases. Alternatively the code could be written like this: with NamedTemporaryFile() as ldif_file: # do everything with the ldif_file here 7. In DBUpgrade.modify_schema() why not use subsystem.open_database() instead of ldapmodify? That will ensure it uses the same mechanism to connect to the LDAP server. 8. Also the DBUpgrade.modify_schema() is ignoring all schema update failures. I think we can only ignore the case where the new schema is already added. In other cases the upgrade should fail since the subsequent upgrades may depend on it. 9. I think DBUpgrade.create_realm_index() should only execute if there's a KRA subsystem in the instance. The code right now creates the index in the first subsystem's database which may not be a KRA. subsystem = instance.subsystems[0] 10. The DBUpgrade uses db2index.pl to rebuild the indexes which means the LDAP server must run locally. To support remote LDAP server I think we need to use reindex task. We have indextasks.ldif and vlvtasks.ldif that can be used for this. See also: http://pki.fedoraproject.org/wiki/DS_Database_Indexes 11. The DBUpgrade should not ignore reindex failures since subsequent upgrades may depend on it. 12. There's a typo in the last patch's commit description. -- Endi S. Dewata
Made fix 1 below and pushed 315-318 to master. After much discussion with Endi, figured out how best to refactor 319. Will resubmit as 320. Thanks, Ade On Thu, 2016-06-02 at 14:10 -0500, Endi Sukma Dewata wrote:
On 6/2/2016 8:51 AM, Ade Lee wrote:
And now with the patches ..
On Thu, 2016-06-02 at 09:50 -0400, Ade Lee wrote:
Patch descriptions (in reverse order).
The final patch will need some discussion. Please review,
Ade
Some comments:
1. In SrchKey and SrchKeyForRecovery the check probably should have been like this (no closing paren):
if (filter.contains("(realm=")) {
2. If a realm is specified, I suppose it won't match any of the VLV indexes. Do the queries still work correctly without a matching VLV?
3. Could you document how to run the upgrade command in this page? http://pki.fedoraproject.org/wiki/Database_Upgrade_for_PKI_10.3.x
Note that there are still some manual upgrade procedures that also need to be executed as documented in that page.
4. In PKISubsystem.open_database() can we use bind_dn and bind_password parameter names instead of user and password (it's a DN not a user ID)? Also in DBUpgrade should we use -D, -w, and -Z for consistency with DS tools?
5. The gnu_getopt() is called with "server_id=". I think it should be "server-id=". But since it actually contains the DS instance name maybe we should use "ds-instance=" instead?
6. In the DBUpgrade.modify_kra_vlv() the os.unlink(ldif_file) probably should be moved into the finally clause above it to ensure the temporary file is deleted in all cases. Alternatively the code could be written like this:
with NamedTemporaryFile() as ldif_file: # do everything with the ldif_file here
7. In DBUpgrade.modify_schema() why not use subsystem.open_database() instead of ldapmodify? That will ensure it uses the same mechanism to connect to the LDAP server.
8. Also the DBUpgrade.modify_schema() is ignoring all schema update failures. I think we can only ignore the case where the new schema is already added. In other cases the upgrade should fail since the subsequent upgrades may depend on it.
9. I think DBUpgrade.create_realm_index() should only execute if there's a KRA subsystem in the instance. The code right now creates the index in the first subsystem's database which may not be a KRA.
subsystem = instance.subsystems[0]
10. The DBUpgrade uses db2index.pl to rebuild the indexes which means the LDAP server must run locally. To support remote LDAP server I think we need to use reindex task. We have indextasks.ldif and vlvtasks.ldif that can be used for this. See also: http://pki.fedoraproject.org/wiki/DS_Database_Indexes
11. The DBUpgrade should not ignore reindex failures since subsequent upgrades may depend on it.
12. There's a typo in the last patch's commit description.
participants (2)
-
Ade Lee -
Endi Sukma Dewata