Rewrite and fix CynaraAdmin::SetPolicies

Method CynaraAdmin::Setpolicies, updating Cynara policy for an application,
was previously written to accept two vectors of privileges:
previously enabled privileges and privileges that should be enabled.
Vectors were used to calculate privileges to disable and privileges to
enable in Cynara. It required that both vectors are sorted and without
duplicates. Callers of this method fetched privileges from data base, which
provides sorting and unification.

This was broken in commit 626f947e0bb6fd90d4c20fd914981d5b752ab1e6
(Change smack labeling to be appId based). The second vector was taken
directly from application installation request, that wasn't necessarily
sorted or unique.

This method can be simplified now withot the need for sorted vectors. In
fact only one vector is necessarry now, because cynara-admin provides
support for listing policies (it didn't when the method was initially
written). Now it only takes vector of privileges that should be enabled,
in arbitrary order, that may contain duplicates. It lists previously enabled
privileges directly from Cynara, calculates the difference and sends
updated policies back to Cynara.

Change-Id: I15ca331cf5f46ae43c7665977df7eb4d3c7e986c
Signed-off-by: Rafal Krypa <r.krypa@samsung.com>

diff --git a/src/common/cynara.cpp b/src/common/cynara.cpp
index 30f0777..d1bbef0 100644 (file)
--- a/src/common/cynara.cpp
+++ b/src/common/cynara.cpp
@@ -22,6 +22,7 @@
  */
 
 #include <cstring>
+#include <unordered_set>
 #include "cynara.h"
 
 #include <dpl/log/log.h>
@@ -286,53 +287,35 @@ void CynaraAdmin::SetPolicies(const std::vector<CynaraAdminPolicy> &policies)
 void CynaraAdmin::UpdateAppPolicy(
     const std::string &label,
     const std::string &user,
-    const std::vector<std::string> &oldPrivileges,
-    const std::vector<std::string> &newPrivileges)
+    const std::vector<std::string> &privileges)
 {
-    std::vector<CynaraAdminPolicy> policies;
+    std::unordered_set<std::string> privilegesSet(privileges.begin(), privileges.end());
 
-    // Perform sort-merge join on oldPrivileges and newPrivileges.
-    // Assume that they are already sorted and without duplicates.
-    auto oldIter = oldPrivileges.begin();
-    auto newIter = newPrivileges.begin();
+    std::vector<CynaraAdminPolicy> policies;
+    CynaraAdmin::getInstance().ListPolicies(
+        CynaraAdmin::Buckets.at(Bucket::MANIFESTS),
+        label, user, CYNARA_ADMIN_ANY, policies);
 
-    while (oldIter != oldPrivileges.end() && newIter != newPrivileges.end()) {
-        int compare = oldIter->compare(*newIter);
-        if (compare == 0) {
+    // Compare previous policies with set of new requested privileges
+    for (auto &policy : policies) {
+        if (privilegesSet.erase(policy.privilege)) {
+            // privilege was found and removed from the set, keeping policy
             LogDebug("(user = " << user << " label = " << label << ") " <<
-                "keeping privilege " << *newIter);
-            ++oldIter;
-            ++newIter;
-            continue;
-        } else if (compare < 0) {
-            LogDebug("(user = " << user << " label = " << label << ") " <<
-                "removing privilege " << *oldIter);
-            policies.push_back(CynaraAdminPolicy(label, user, *oldIter,
-                    static_cast<int>(CynaraAdminPolicy::Operation::Delete),
-                    Buckets.at(Bucket::MANIFESTS)));
-            ++oldIter;
+                "keeping privilege " << policy.privilege);
         } else {
+            // privilege was not found in the set, deleting policy
+            policy.result = static_cast<int>(CynaraAdminPolicy::Operation::Delete);
             LogDebug("(user = " << user << " label = " << label << ") " <<
-                "adding privilege " << *newIter);
-            policies.push_back(CynaraAdminPolicy(label, user, *newIter,
-                    static_cast<int>(CynaraAdminPolicy::Operation::Allow),
-                    Buckets.at(Bucket::MANIFESTS)));
-            ++newIter;
+                "removing privilege " << policy.privilege);
         }
     }
 
-    for (; oldIter != oldPrivileges.end(); ++oldIter) {
-        LogDebug("(user = " << user << " label = " << label << ") " <<
-            "removing privilege " << *oldIter);
-        policies.push_back(CynaraAdminPolicy(label, user, *oldIter,
-                    static_cast<int>(CynaraAdminPolicy::Operation::Delete),
-                    Buckets.at(Bucket::MANIFESTS)));
-    }
-
-    for (; newIter != newPrivileges.end(); ++newIter) {
+    // Add policies for privileges that weren't previously enabled
+    // Those that were previously enabled are now removed from privilegesSet
+    for (const auto &privilege : privilegesSet) {
         LogDebug("(user = " << user << " label = " << label << ") " <<
-            "adding privilege " << *newIter);
-        policies.push_back(CynaraAdminPolicy(label, user, *newIter,
+            "adding privilege " << privilege);
+        policies.push_back(CynaraAdminPolicy(label, user, privilege,
                     static_cast<int>(CynaraAdminPolicy::Operation::Allow),
                     Buckets.at(Bucket::MANIFESTS)));
     }

diff --git a/src/common/include/cynara.h b/src/common/include/cynara.h
index aa0ed60..c6a77f3 100644 (file)
--- a/src/common/include/cynara.h
+++ b/src/common/include/cynara.h
@@ -118,26 +118,18 @@ public:
     void SetPolicies(const std::vector<CynaraAdminPolicy> &policies);
 
     /**
-     * Update Cynara policies for the package and the user, using two vectors
-     * of privileges: privileges set before (and already enabled in Cynara)
-     * and new privileges, to be set in Cynara.
+     * Update Cynara policies for the application and the user.
      * Difference will be calculated, removing old unneeded privileges and
      * adding new, previously not enabled privileges.
      * Caller must have permission to access Cynara administrative socket.
      *
      * @param label application Smack label
      * @param user user identifier
-     * @param oldPrivileges previously enabled privileges for the package.
-     *        Must be sorted and without duplicates.
-     * @param newPrivileges currently enabled privileges for the package.
-     *        Must be sorted and without duplicates.
+     * @param privileges currently enabled privileges
      *
-     * TODO: drop oldPrivileges argument and get them directly from Cynara.
-     * Appropriate Cynara interface is needed first.
      */
     void UpdateAppPolicy(const std::string &label, const std::string &user,
-        const std::vector<std::string> &oldPrivileges,
-        const std::vector<std::string> &newPrivileges);
+        const std::vector<std::string> &privileges);
 
     /**
      * Depending on user type, create link between MAIN bucket and appropriate

diff --git a/src/common/include/master-req.h b/src/common/include/master-req.h
index 9890728..15b1fdc 100644 (file)
--- a/src/common/include/master-req.h
+++ b/src/common/include/master-req.h
@@ -39,16 +39,12 @@ namespace MasterReq {
  *
  * @param[in]  appID            Application ID
  * @param[in]  uidstr           String containing user identifier
- * @param[in]  oldPkgPrivileges Previously enabled privileges for the package,
- *                              Must be sorted and without duplicates
- * @param[in]  newPkgPrivileges Currently enabled privileges for the package,
- *                              Must be sorted and without duplicates
+ * @param[in]  privileges       Currently enabled privileges for the application
  *
  * @see CynaraAdmin::UpdateAppPolicy
  */
 int CynaraPolicyUpdate(const std::string &appId, const std::string &uidstr,
-                       const std::vector<std::string> &oldPkgPrivileges,
-                       const std::vector<std::string> &newPkgPrivileges);
+                       const std::vector<std::string> &privileges);
 
 /**
  * Forwards Cynara user initialization to Master service.

diff --git a/src/common/master-req.cpp b/src/common/master-req.cpp
index f6526b3..b2c2783 100644 (file)
--- a/src/common/master-req.cpp
+++ b/src/common/master-req.cpp
@@ -32,16 +32,15 @@
 namespace SecurityManager {
 namespace MasterReq {
 
-int CynaraPolicyUpdate(const std::string &appId,  const std::string &uidstr,
-                       const std::vector<std::string> &oldPkgPrivileges,
-                       const std::vector<std::string> &newPkgPrivileges)
+int CynaraPolicyUpdate(const std::string &appId, const std::string &uidstr,
+                       const std::vector<std::string> &privileges)
 {
     int ret;
     MessageBuffer sendBuf, retBuf;
 
     Serialization::Serialize(sendBuf,
         static_cast<int>(MasterSecurityModuleCall::CYNARA_UPDATE_POLICY),
-        appId, uidstr, oldPkgPrivileges, newPkgPrivileges);
+        appId, uidstr, privileges);
 
     ret = sendToServer(MASTER_SERVICE_SOCKET, sendBuf.Pop(), retBuf);
     if (ret == SECURITY_MANAGER_API_SUCCESS)

diff --git a/src/common/service_impl.cpp b/src/common/service_impl.cpp
index 469f6a4..fb0a0e6 100644 (file)
--- a/src/common/service_impl.cpp
+++ b/src/common/service_impl.cpp
@@ -292,8 +292,6 @@ int ServiceImpl::appInstall(const app_inst_req &req, uid_t uid, bool isSlave)
     }
 
     try {
-        std::vector<std::string> oldAppPrivileges;
-
         appLabel = zoneSmackLabelGenerate(SmackLabels::generateAppLabel(req.appId), zoneId);
         /* NOTE: we don't use pkgLabel here, but generate it for pkgId validation */
         pkgLabel = zoneSmackLabelGenerate(SmackLabels::generatePkgLabel(req.pkgId), zoneId);
@@ -309,23 +307,21 @@ int ServiceImpl::appInstall(const app_inst_req &req, uid_t uid, bool isSlave)
             PrivilegeDb::getInstance().RollbackTransaction();
             return SECURITY_MANAGER_API_ERROR_INPUT_PARAM;
         }
-        PrivilegeDb::getInstance().GetAppPrivileges(req.appId, uid, oldAppPrivileges);
+
         PrivilegeDb::getInstance().AddApplication(req.appId, req.pkgId, uid);
         PrivilegeDb::getInstance().UpdateAppPrivileges(req.appId, uid, req.privileges);
         /* Get all application ids in the package to generate rules withing the package */
         PrivilegeDb::getInstance().GetAppIdsForPkgId(req.pkgId, pkgContents);
 
         if (isSlave) {
-            int ret = MasterReq::CynaraPolicyUpdate(req.appId, uidstr, oldAppPrivileges,
-                                                    req.privileges);
+            int ret = MasterReq::CynaraPolicyUpdate(req.appId, uidstr, req.privileges);
             if (ret != SECURITY_MANAGER_API_SUCCESS) {
                 PrivilegeDb::getInstance().RollbackTransaction();
                 LogError("Error while processing request on master: " << ret);
                 return ret;
             }
         } else {
-            CynaraAdmin::getInstance().UpdateAppPolicy(appLabel, uidstr, oldAppPrivileges,
-                                                       req.privileges);
+            CynaraAdmin::getInstance().UpdateAppPolicy(appLabel, uidstr, req.privileges);
         }
 
         PrivilegeDb::getInstance().CommitTransaction();
@@ -408,8 +404,6 @@ int ServiceImpl::appUninstall(const std::string &appId, uid_t uid, bool isSlave)
     }
 
     try {
-        std::vector<std::string> oldAppPrivileges;
-
         PrivilegeDb::getInstance().BeginTransaction();
         if (!PrivilegeDb::getInstance().GetAppPkgId(appId, pkgId)) {
             LogWarning("Application " << appId <<
@@ -425,21 +419,18 @@ int ServiceImpl::appUninstall(const std::string &appId, uid_t uid, bool isSlave)
                 that this app belongs to, this will allow us to remove all rules withing the
                 package that the app appears in */
             PrivilegeDb::getInstance().GetAppIdsForPkgId(pkgId, pkgContents);
-            PrivilegeDb::getInstance().GetAppPrivileges(appId, uid, oldAppPrivileges);
             PrivilegeDb::getInstance().UpdateAppPrivileges(appId, uid, std::vector<std::string>());
             PrivilegeDb::getInstance().RemoveApplication(appId, uid, removePkg);
 
             if (isSlave) {
-                int ret = MasterReq::CynaraPolicyUpdate(appId, uidstr, oldAppPrivileges,
-                                                        std::vector<std::string>());
+                int ret = MasterReq::CynaraPolicyUpdate(appId, uidstr, std::vector<std::string>());
                 if (ret != SECURITY_MANAGER_API_SUCCESS) {
                     PrivilegeDb::getInstance().RollbackTransaction();
                     LogError("Error while processing request on master: " << ret);
                     return ret;
                 }
             } else {
-                CynaraAdmin::getInstance().UpdateAppPolicy(smackLabel, uidstr, oldAppPrivileges,
-                                                           std::vector<std::string>());
+                CynaraAdmin::getInstance().UpdateAppPolicy(smackLabel, uidstr, std::vector<std::string>());
             }
 
             PrivilegeDb::getInstance().CommitTransaction();
@@ -883,6 +874,7 @@ int ServiceImpl::getPolicy(const policy_entry &filter, uid_t uid, pid_t pid, con
                 std::vector<std::string> listOfPrivileges;
 
                 // FIXME: also fetch privileges of global applications
+                // FIXME: fetch privileges from cynara, drop PrivilegeDb::GetAppPrivileges
                 PrivilegeDb::getInstance().GetAppPrivileges(appId, uid, listOfPrivileges);
 
                 if (filter.privilege.compare(SECURITY_MANAGER_ANY)) {

diff --git a/src/server/service/master-service.cpp b/src/server/service/master-service.cpp
index 2440419..770820e 100644 (file)
--- a/src/server/service/master-service.cpp
+++ b/src/server/service/master-service.cpp
@@ -170,19 +170,17 @@ void MasterService::processCynaraUpdatePolicy(MessageBuffer &buffer, MessageBuff
     int ret = SECURITY_MANAGER_API_ERROR_SERVER_ERROR;
     std::string appId;
     std::string uidstr;
-    std::vector<std::string> oldAppPrivileges, newAppPrivileges;
-    std::string appLabel, newLabel;
+    std::string appLabel;
+    std::vector<std::string> privileges;
 
     Deserialization::Deserialize(buffer, appId);
     Deserialization::Deserialize(buffer, uidstr);
-    Deserialization::Deserialize(buffer, oldAppPrivileges);
-    Deserialization::Deserialize(buffer, newAppPrivileges);
+    Deserialization::Deserialize(buffer, privileges);
 
     appLabel = zoneSmackLabelGenerate(SmackLabels::generateAppLabel(appId), zoneId);
 
     try {
-        CynaraAdmin::getInstance().UpdateAppPolicy(appLabel, uidstr, oldAppPrivileges,
-                                                   newAppPrivileges);
+        CynaraAdmin::getInstance().UpdateAppPolicy(appLabel, uidstr, privileges);
     } catch (const CynaraException::Base &e) {
         LogError("Error while setting Cynara rules for application: " << e.DumpToString());
         goto out;