Add CA ProfileService to v2 API #4811

fmarco76 · 2024-07-26T18:12:15Z

Implementing the Profile in v2 APIs I have done replaced the check for the user authorisation from the hardcoded to the authorisation manager (see method isProfileAccessLimited(Principal) in ProfileBase).

The logic is that user with modify access can access the not visible profiles. @edewata Is this OK or do we need a different ACL?

edewata · 2024-07-27T01:25:48Z

base/ca/src/main/java/org/dogtagpki/server/ca/rest/base/ProfileBase.java

+        if (authToken == null)
+            return true;


Could we move this into the if clause (between line 1197 and 1198) since in the else clause it cannot be null?

edewata · 2024-07-27T01:58:45Z

IIUC the original code lets members of Certificate Manager Agents or Certificate Manager Administrators groups to see invisible profiles, and the new code grants that permission to users who have modify access to certServer.profile.configuration resource, is that correct?

Here's the default ACL for that resource (formatted for readability):

resourceACLS: certServer.profile.configuration:read,modify:
    allow (read) group="Administrators"
        || group="Certificate Manager Agents"
        || group="Registration Manager Agents"
        || group="Auditors";
    allow (modify) group="Administrators":
    Administrators, agents, and auditors are allowed to read profile configuration but only administrators allowed to modify

So the new code will grant access to Administrators instead, unless we update the ACL or define a new one. Updating existing ACL might be more complicated since it might have been customized or some people might not want to change it. Adding a new ACL might be easier, but we don't really have a mechanism to do that right now.

@ladycfu What do you think?

The rest of the code looks fine to me.

fmarco76 · 2024-07-27T09:05:42Z

So the new code will grant access to Administrators

A more restrictive operation would be to use the ACL:

resourceACLS: certServer.ca.profiles:list:allow (list) group="Certificate Manager Agents":Certificate Manager agents may list profiles

In this case only the Certificate Manager Agents can access the profiles.

If we would be more flexible we could allow all the people with read access:

resourceACLS: certServer.profile.configuration:read,modify:allow (read) group="Administrators" || group="Certificate Manager Agents" || group="Registration Manager Agents" || group="Auditors"

Currently, the ProfileService uses ACL with the following mapping:

profiles.approve = certServer.ca.profile,approve
profiles.create = certServer.profile.configuration,modify
profiles.delete = certServer.profile.configuration,modify
profiles.list = certServer.ee.profiles,list
profiles.modify = certServer.profile.configuration,modify
profiles.read = certServer.profile.configuration,read

edewata · 2024-07-29T15:17:14Z

I'll let @ladycfu decide on how the ACL should look like, but regardless, this change will require performing an ldapmodify to the existing database. Should that be done automatically in some way, or should that be done manually by the admin?

@fmarco76 If you want, we can merge the PR with the original (i.e. hard-coded) ACL first, then do the ACL change in a separate PR.

fmarco76 · 2024-07-29T15:37:22Z

this change will require performing an ldapmodify to the existing database.

Is this needed only if we define a new ACL? In my test, with existing ACL I had no need to modify the stored information.

edewata · 2024-07-29T15:51:45Z

To my understanding the ACL is imported from LDIF file into LDAP during installation, and the server will only use the ACL in LDAP, not in LDIF. So if we only modify the ACL in LDIF (either adding new ACL or modifying an existing one) the changes will only appear in new installations, and existing installations will still have the old ACL in LDAP unless we explicitly modify the LDAP as well.

ladycfu · 2024-07-29T16:44:00Z

base/ca/src/main/java/org/dogtagpki/server/ca/rest/base/ProfileBase.java

+        }
+        if (authToken == null)
+            return true;
+


could the checkRealm() method in AuthzSubsystem.java be used here instead?

ladycfu · 2024-07-29T17:24:21Z

I'll let @ladycfu decide on how the ACL should look like, but regardless, this change will require performing an ldapmodify to the existing database. Should that be done automatically in some way, or should that be done manually by the admin?

@fmarco76 If you want, we can merge the PR with the original (i.e. hard-coded) ACL first, then do the ACL change in a separate PR.

Not looking at the code, but just a quick reply to how I envision the policy works: 1. Both CA admins and agents should be allowed to view/list all profiles, regardless of visibility. Visibility setting should only affect non-admin and non-agent users (actually, I don't think there is any security impact on letting users see disabled profiles, but I think it'll be less messy when list of profiles are presented to users). 2. CA admins can only modify disabled profiles and cannot enable/disable a profile 3. CA agents cannot modify any profiles, but could disable/enable a profile.

ladycfu · 2024-07-29T17:29:56Z

I should add that CA admins are also allowed to add profiles, but in a disabled state. An agent is expected to review and enable the new profile to show approval. I'm not sure about deletion of profiles, but I supposed it should be fine to leave that to the admins.

fmarco76 · 2024-07-29T17:46:04Z

Update the ACL to certServer.profile.configuration,read which should work as expected.

ladycfu · 2024-07-29T21:29:18Z

base/ca/src/main/java/org/dogtagpki/server/ca/rest/base/ProfileBase.java

+        }
+    }
+
+    private boolean isProfileAccessLimited(Principal principal) {


If I read this correctly, this is just to check whether principal has "read" access? I'm just curious why it is named "isProfileAccessLimited", and all callers have the return value assigned to visibleOnly

Yes, the actual check is done by this code below which returns false if the user has a read access:

AuthzToken authzToken = authzSubsystem.authorize( authzMgrName, authToken, "certServer.profile.configuration", "read"); if (authzToken != null) return false;

The visibleOnly variable comes from the original code which actually means "only allowed to access visible profiles" or "not allowed to access invisible profiles":
https://github.com/dogtagpki/pki/blob/master/base/ca/src/main/java/org/dogtagpki/server/ca/rest/v1/ProfileService.java#L126-L134

I believe the isProfileAccessLimited() method name was given to match the meaning of visibleOnly, but I think they are both rather confusing since we'd have to mentally invert them to understand what they do. To make it easier to understand I'd suggest renaming them to something like allowedToAccessInvisibleProfiles and isAllowedToAccessInvisibleProfiles(), respectively, and flip the logic everywhere it's used.

ladycfu · 2024-07-29T22:35:43Z

base/ca/src/main/java/org/dogtagpki/server/ca/rest/base/ProfileBase.java

+            String userid = principal.getName();
+            try {
+                if (ps.checkOwner()) {
+                    if (ps.getProfileEnableBy(profileId).equals(userid)) {


This is checking to see whether the profile was enabled by the same uid that's trying to disable it now. I think the policy we want is just to check whether the person that's trying to disable the profile is a CA agent. Let's say if Joe was a CA agent who enabled a profile, but later his CA agent privilege was removed, then he can't be allowed to disable the profile (although I suspect the ACL was checked before this... once my other question above is answered). Also, as long as the person is a CA agent, they are allowed to disable/enable a profile. They do not need to be the same person.

I believe this code is basically identical to the original code here:
https://github.com/dogtagpki/pki/blob/master/base/ca/src/main/java/org/dogtagpki/server/ca/rest/v1/ProfileService.java#L449-L475

In this PR we're just trying to convert the original RESTEasy-based service into a plain servlet so we can drop the RESTEasy dependency later on, but the functionality should be pretty much identical. I think @fmarco76 is trying to fix the hard-coded ACL too, but the behavior should still be unchanged (which is why it's a bit complicated).

If the behavior of the original code itself is not correct we probably should review how this feature is tested and documented.

@edewata thanks. yeah it's okay to create a new follow-up ticket to change the behavior to meet what we need. Basically "checkOwner" is not needed, as long as the user is a CA agent. Although it does make one wonder why no one has complained about it.

ladycfu · 2024-07-29T22:39:44Z

base/ca/src/main/java/org/dogtagpki/server/ca/rest/base/ProfileBase.java

+                throw new ConflictingOperationException("Profile already enabled");
+            }
+            try {
+                ps.enableProfile(profileId, principal.getName());


@edewata I know Marco is out on pto, so maybe you could help explain to me where the ACL for checking for the CA agent membership of the principal is held?

Please see my comments here:
#4811 (comment)
#4811 (comment)

So the original code checks whether the user is a member of Certificate Manager Agents or Certificate Manager Administrators directly, and the new code checks whether the user has read access to certServer.profile.configuration using ACL, but IIUC they are not identical replacements, that's my concern with this change.

thanks @edewata . So, with the clarification that this task is to replace the rest interface with the same policy, I agree with Endi that the code in this patch here isn't identical. Although I have to add that as I stated earlier, only CA agents should have the privilege to enable or disable a profile. I'll leave it to you two to decide whether to change it to the original behavior for this patch now, then create a separate ticket to change it to the desired behavior I stated, or simply change it to match the desired behavior.

ladycfu

see comments.

ladycfu · 2024-07-30T22:29:37Z

base/ca/src/main/java/org/dogtagpki/server/ca/rest/base/ProfileBase.java

+                throw new ConflictingOperationException("Profile already enabled");
+            }
+            try {
+                ps.enableProfile(profileId, principal.getName());


thanks @edewata . So, with the clarification that this task is to replace the rest interface with the same policy, I agree with Endi that the code in this patch here isn't identical. Although I have to add that as I stated earlier, only CA agents should have the privilege to enable or disable a profile. I'll leave it to you two to decide whether to change it to the original behavior for this patch now, then create a separate ticket to change it to the desired behavior I stated, or simply change it to match the desired behavior.

ladycfu · 2024-07-30T22:45:14Z

base/ca/src/main/java/org/dogtagpki/server/ca/rest/base/ProfileBase.java

+            String userid = principal.getName();
+            try {
+                if (ps.checkOwner()) {
+                    if (ps.getProfileEnableBy(profileId).equals(userid)) {


@edewata thanks. yeah it's okay to create a new follow-up ticket to change the behavior to meet what we need. Basically "checkOwner" is not needed, as long as the user is a CA agent. Although it does make one wonder why no one has complained about it.

fmarco76 · 2024-08-07T10:19:12Z

@edewata @ladycfu I have reverted the code to use the embedded checks and it works exactly as in the current implementation. We can review/modify this behaviour in separate tickets/PRs if needed. Is this OK to merge now?

ACL groups does not match with the embedded checks in v1 code so the code has been reverted to the embedded check leaving the ACL update to future commits.

sonarcloud · 2024-08-07T10:37:26Z

Quality Gate passed

Issues
33 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
41.8% Duplication on New Code

See analysis details on SonarCloud

edewata

Thanks for the update! Yeah, I think we can merge it.

So for the ACL, if we decide later to change it, we need to figure out how to upgrade the existing ACL in LDAP first (if needed) before we can change the ACL.

fmarco76 · 2024-08-07T14:27:12Z

@edewata @ladycfu Thanks! I am merging this now but we can continue to discussion on the ACL

fmarco76 requested a review from edewata July 26, 2024 18:12

edewata reviewed Jul 27, 2024

View reviewed changes

edewata requested a review from ladycfu July 27, 2024 01:58

ladycfu reviewed Jul 29, 2024

View reviewed changes

Add CA ProfileService to v2 API

80f24f8

fmarco76 force-pushed the ProfileService_v2 branch from 0d09dce to 80f24f8 Compare July 29, 2024 17:43

fmarco76 requested review from edewata and ladycfu July 29, 2024 17:45

ladycfu reviewed Jul 29, 2024

View reviewed changes

ladycfu reviewed Jul 30, 2024

View reviewed changes

Revert the ACL check for profile APIs to v1 code.

d9d35c9

ACL groups does not match with the embedded checks in v1 code so the code has been reverted to the embedded check leaving the ACL update to future commits.

fmarco76 force-pushed the ProfileService_v2 branch from f5ec6dd to d9d35c9 Compare August 7, 2024 10:25

edewata approved these changes Aug 7, 2024

View reviewed changes

fmarco76 merged commit b636083 into dogtagpki:master Aug 7, 2024
146 of 156 checks passed

fmarco76 deleted the ProfileService_v2 branch August 8, 2024 07:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CA ProfileService to v2 API #4811

Add CA ProfileService to v2 API #4811

fmarco76 commented Jul 26, 2024

edewata Jul 27, 2024

edewata commented Jul 27, 2024

fmarco76 commented Jul 27, 2024

edewata commented Jul 29, 2024

fmarco76 commented Jul 29, 2024

edewata commented Jul 29, 2024

ladycfu Jul 29, 2024

ladycfu commented Jul 29, 2024

ladycfu commented Jul 29, 2024

fmarco76 commented Jul 29, 2024

ladycfu Jul 29, 2024 •

edited

Loading

edewata Jul 30, 2024

ladycfu Jul 29, 2024 •

edited

Loading

edewata Jul 30, 2024

ladycfu Jul 30, 2024

ladycfu Jul 29, 2024

edewata Jul 30, 2024

ladycfu Jul 30, 2024

ladycfu left a comment

ladycfu Jul 30, 2024

ladycfu Jul 30, 2024

fmarco76 commented Aug 7, 2024

sonarcloud bot commented Aug 7, 2024

edewata left a comment

fmarco76 commented Aug 7, 2024

Add CA ProfileService to v2 API #4811

Add CA ProfileService to v2 API #4811

Conversation

fmarco76 commented Jul 26, 2024

Choose a reason for hiding this comment

edewata commented Jul 27, 2024

fmarco76 commented Jul 27, 2024

edewata commented Jul 29, 2024

fmarco76 commented Jul 29, 2024

edewata commented Jul 29, 2024

Choose a reason for hiding this comment

ladycfu commented Jul 29, 2024

ladycfu commented Jul 29, 2024

fmarco76 commented Jul 29, 2024

ladycfu Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ladycfu Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ladycfu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmarco76 commented Aug 7, 2024

sonarcloud bot commented Aug 7, 2024

Quality Gate passed

edewata left a comment

Choose a reason for hiding this comment

fmarco76 commented Aug 7, 2024

ladycfu Jul 29, 2024 •

edited

Loading

ladycfu Jul 29, 2024 •

edited

Loading