Ack from me on PATCH 1 of 2 (first of two patches) with the following
adjustments
Hans Feldt wrote:
> osaf/libs/common/immsv/include/immsv_api.h | 8 +++
> osaf/services/saf/immsv/immloadd/imm_loader.cc | 23 +++++++-
> osaf/services/saf/immsv/immnd/ImmModel.cc | 69
> ++++++++++++++++++++++++++
> osaf/services/saf/immsv/immnd/ImmModel.hh | 3 +
> osaf/services/saf/immsv/immnd/immnd_cb.h | 1 -
> osaf/services/saf/immsv/immnd/immnd_evt.c | 49 +++++++++++++-----
> osaf/services/saf/immsv/immnd/immnd_init.h | 3 +
> osaf/services/saf/immsv/immnd/immnd_main.c | 2 -
> samples/immsv/OpensafImm_Upgrade_4.5.xml | 13 ++++
> 9 files changed, 152 insertions(+), 19 deletions(-)
>
>
> A new int attribute accessControlMode is added to the OpensafImm class.
> Supported values are:
> 0 - DISABLED
> 1 - PERMISSIVE
> 2 - ENFORCING
>
> Its default value is DISABLED meaning no access control. This is to be
> backwards compatible for upgrade of existing systems.
>
> Changes to the mode needs to be done as root e.g. using sudo.
>
> Permissive access control is done with:
> sudo immcfg -a accessControlMode=1 opensafImm=opensafImm,safApp=safImmService
>
> Enforcing access control is done with:
> sudo immcfg -a accessControlMode=2 opensafImm=opensafImm,safApp=safImmService
>
> Disabling access control is done with:
> sudo immcfg -a accessControlMode=0 opensafImm=opensafImm,safApp=safImmService
>
> An additional UNIX group that allows IMM access can be configured with the
> adminGroupName attribute in the OpensafImm class. For example:
> sudo immcfg -a adminGroupName=osafimmadm
> opensafImm=opensafImm,safApp=safImmService
>
> diff --git a/osaf/libs/common/immsv/include/immsv_api.h
> b/osaf/libs/common/immsv/include/immsv_api.h
> --- a/osaf/libs/common/immsv/include/immsv_api.h
> +++ b/osaf/libs/common/immsv/include/immsv_api.h
> @@ -59,6 +59,14 @@ extern "C" {
> #define IMMSV_MAX_OBJS_IN_SYNCBATCH (IMMSV_DEFAULT_MAX_SYNC_BATCH_SIZE / 10)
>
> #define OPENSAF_IMM_LONG_DNS_ALLOWED "longDnsAllowed"
> +#define OPENSAF_IMM_ACCESS_CONTROL_MODE "accessControlMode"
> +#define OPENSAF_IMM_ADMIN_GROUP_NAME "adminGroupName"
> +
> +typedef enum {
> + ACCESS_CONTROL_DISABLED = 0,
> + ACCESS_CONTROL_PERMISSIVE = 1,
> + ACCESS_CONTROL_ENFORCING = 2
> +} ImmAccessControlModeT;
>
Above enum should be named either OsafImmAccessControlModeT or
OpenSafImmAccessControlModeT,
or OsafAccessControlModeT or OpenSafAccessControlModeT.
This both for consistency in naming and to clearly identify that the
enum is defined in OpenSAF.
The 'Imm' part depends on if this *type* (and attribute) is to only
control access control. for the imm service.
If the type is to be used as an OpenSAF global enablement type, for
access control for all OpensAF services
(the ones that support access control) then the 'Imm' part of the name
should be dropped. This would be either
if all services are controlled by this one attribute, or if each service
has its own config attribute, but all use the
same enum type. In the latter case I guess the type should be defined in
a generic OpenSAF include file.
Finally, I guess this type really should be publicly available somehow.
It is used as the value type for a config attribute that is visible to
"users".
So it really should be in some public include file visible to all users
of OpenSAF.
This would be regardless of if it was only used for imm access control
or if it is
used for access control of all OpenSAF services that currently support
access control.
>
> /*Max # of outstanding fevs messages towards director.*/
> /*Note max-max is 255. cb->fevs_replies_pending is an uint8_t*/
> diff --git a/osaf/services/saf/immsv/immloadd/imm_loader.cc
> b/osaf/services/saf/immsv/immloadd/imm_loader.cc
> --- a/osaf/services/saf/immsv/immloadd/imm_loader.cc
> +++ b/osaf/services/saf/immsv/immloadd/imm_loader.cc
> @@ -31,10 +31,17 @@
> #include <sys/stat.h>
> #include <errno.h>
> #include <ncsgl_defs.h>
> +#include <config.h>
>
> #include "saAis.h"
> #include "osaf_extended_name.h"
>
> +// Default value of accessControlMode attribute in the OpensafImm class
> +// Can be changed at build time using configure
> +#ifndef IMM_ACCESS_CONTROL_MODE
> +#define IMM_ACCESS_CONTROL_MODE ACCESS_CONTROL_DISABLED
> +#endif
> +
> #define MAX_DEPTH 10
> #define MAX_CHAR_BUFFER_SIZE 8192 //8k
>
> @@ -286,10 +293,11 @@ void opensafClassCreate(SaImmHandleT imm
> {
> SaAisErrorT err = SA_AIS_OK;
> int retries=0;
> - SaImmAttrDefinitionT_2 d1, d2, d3, d4, d5, d6;
> + SaImmAttrDefinitionT_2 d1, d2, d3, d4, d5, d6, d7, d8;
> SaUint32T nost_flags_default = 0;
> SaUint32T batch_size_default = IMMSV_DEFAULT_MAX_SYNC_BATCH_SIZE;
> SaUint32T extended_names_enabled_default = 0;
> + SaUint32T access_control_mode_default = IMM_ACCESS_CONTROL_MODE;
>
> d1.attrName = (char *) OPENSAF_IMM_ATTR_RDN;
> d1.attrValueType = SA_IMM_ATTR_SANAMET;
> @@ -322,7 +330,18 @@ void opensafClassCreate(SaImmHandleT imm
> d6.attrFlags = SA_IMM_ATTR_CONFIG | SA_IMM_ATTR_WRITABLE;
> d6.attrDefaultValue = &extended_names_enabled_default;
>
> - const SaImmAttrDefinitionT_2* attrDefs[7] = {&d1, &d2, &d3, &d4, &d5,
> &d6, 0};
> + d7.attrName = (char *) OPENSAF_IMM_ACCESS_CONTROL_MODE;
> + d7.attrValueType = SA_IMM_ATTR_SAUINT32T;
> + d7.attrFlags = SA_IMM_ATTR_CONFIG | SA_IMM_ATTR_WRITABLE;
> + d7.attrDefaultValue = &access_control_mode_default;
> +
> + d8.attrName = (char *) OPENSAF_IMM_ADMIN_GROUP_NAME;
> + d8.attrValueType = SA_IMM_ATTR_SASTRINGT;
> + d8.attrFlags = SA_IMM_ATTR_CONFIG | SA_IMM_ATTR_WRITABLE;
> + d8.attrDefaultValue = NULL;
> +
> + const SaImmAttrDefinitionT_2* attrDefs[] =
> + {&d1, &d2, &d3, &d4, &d5, &d6, &d7, &d8, 0};
>
>
> do {/* Create the class */
> diff --git a/osaf/services/saf/immsv/immnd/ImmModel.cc
> b/osaf/services/saf/immsv/immnd/ImmModel.cc
> --- a/osaf/services/saf/immsv/immnd/ImmModel.cc
> +++ b/osaf/services/saf/immsv/immnd/ImmModel.cc
> @@ -443,6 +443,8 @@ static const std::string immAttrNostFlag
> static const std::string immSyncBatchSize(OPENSAF_IMM_SYNC_BATCH_SIZE);
> static const std::string immPbeBSlaveName(OPENSAF_IMM_2PBE_APPL_NAME);
> static const std::string immLongDnsAllowed(OPENSAF_IMM_LONG_DNS_ALLOWED);
> +static const std::string
> immAccessControlMode(OPENSAF_IMM_ACCESS_CONTROL_MODE);
> +static const std::string immAdminGroupName(OPENSAF_IMM_ADMIN_GROUP_NAME);
>
> static const std::string immMngtClass("SaImmMngt");
> static const std::string
> immManagementDn("safRdn=immManagement,safApp=safImmService");
> @@ -899,6 +901,17 @@ immModel_protocol45Allowed(IMMND_CB *cb)
> SA_TRUE : SA_FALSE;
> }
>
> +ImmAccessControlModeT
> +immModel_accessControlMode(IMMND_CB *cb)
> +{
> + return ImmModel::instance(&cb->immModel)->accessControlMode();
> +}
> +
> +const char*
> +immModel_adminGroupName(IMMND_CB *cb)
> +{
> + return ImmModel::instance(&cb->immModel)->adminGroupName();
> +}
>
> SaBoolT
> immModel_purgeSyncRequest(IMMND_CB *cb, SaUint32T clientId)
> @@ -2454,6 +2467,15 @@ ImmModel::setLoader(int pid)
> LOG_NO("RepositoryInitModeT is SA_IMM_INIT_FROM_FILE");
> immInitMode = SA_IMM_INIT_FROM_FILE; /* Ensure valid value*/
> }
> +
> + if (accessControlMode() == ACCESS_CONTROL_DISABLED) {
> + LOG_WA("IMM Access Control mode is DISABLED!");
> + } else if (accessControlMode() == ACCESS_CONTROL_PERMISSIVE) {
> + LOG_WA("IMM Access Control mode is PERMISSIVE");
> + } else {
> + LOG_NO("IMM Access Control mode is ENFORCING");
> + }
> +
> } else {
> TRACE_5("Loading starts, pid:%u", pid);
> }
> @@ -3438,6 +3460,53 @@ ImmModel::schemaChangeAllowed()
> return noStdFlags & OPENSAF_IMM_FLAG_SCHCH_ALLOW;
> }
>
> +ImmAccessControlModeT
> +ImmModel::accessControlMode()
> +{
> + TRACE_ENTER();
> + ObjectMap::iterator oi = sObjectMap.find(immObjectDn);
> + if(oi == sObjectMap.end()) {
> + TRACE_LEAVE();
> + return ACCESS_CONTROL_DISABLED;
> + }
> +
> + ObjectInfo* immObject = oi->second;
> + ImmAttrValueMap::iterator avi =
> + immObject->mAttrValueMap.find(immAccessControlMode);
> + if (avi == immObject->mAttrValueMap.end())
> + return ACCESS_CONTROL_DISABLED;
> + osafassert(!(avi->second->isMultiValued()));
> + ImmAttrValue* valuep = avi->second;
> + ImmAccessControlModeT accessControlMode =
> + static_cast<ImmAccessControlModeT>(valuep->getValue_int());
> +
> + TRACE_LEAVE2("%u", accessControlMode);
> + return accessControlMode;
> +}
> +
> +const char*
> +ImmModel::adminGroupName()
> +{
> + TRACE_ENTER();
> + ObjectMap::iterator oi = sObjectMap.find(immObjectDn);
> + if(oi == sObjectMap.end()) {
> + TRACE_LEAVE();
> + return false;
>
Above line should really be: "return NULL; "
> + }
> +
> + ObjectInfo* immObject = oi->second;
> + ImmAttrValueMap::iterator avi =
> + immObject->mAttrValueMap.find(immAdminGroupName);
> + if (avi == immObject->mAttrValueMap.end())
> + return NULL;
> + osafassert(!(avi->second->isMultiValued()));
> + ImmAttrValue* valuep = avi->second;
> + const char *adminGroupName = valuep->getValueC_str();
> +
> + TRACE_LEAVE();
> + return adminGroupName;
> +}
> +
> /**
> * Verify that a class create with same name as an existing class is a legal
> * schema upgrade.
> diff --git a/osaf/services/saf/immsv/immnd/ImmModel.hh
> b/osaf/services/saf/immsv/immnd/ImmModel.hh
> --- a/osaf/services/saf/immsv/immnd/ImmModel.hh
> +++ b/osaf/services/saf/immsv/immnd/ImmModel.hh
> @@ -23,6 +23,7 @@
> #include <string>
> #include <vector>
> #include <map>
> +#include "immsv_api.h"
>
Hmm a bit strange that the above include is required just for this ticket.
Again I think this reflects that the AccessControlModeT type should be
defined in a larger scope.
In any case, if the above include is done here in ImmModel.hh then the
same include can be removed
from ImmModel.cc since it becomes redundant there.
>
> struct ClassInfo;
> struct CcbInfo;
> @@ -102,6 +103,8 @@ public:
>
> bool nocaseCompare(const std::string& s1,
> const std::string& s2) const;
> + const char* adminGroupName();
> + ImmAccessControlModeT accessControlMode();
> bool schemaChangeAllowed();
> bool protocol41Allowed();
> bool protocol43Allowed();
> diff --git a/osaf/services/saf/immsv/immnd/immnd_cb.h
> b/osaf/services/saf/immsv/immnd/immnd_cb.h
> --- a/osaf/services/saf/immsv/immnd/immnd_cb.h
> +++ b/osaf/services/saf/immsv/immnd/immnd_cb.h
> @@ -170,7 +170,6 @@ typedef struct immnd_cb_tag {
> NCS_SEL_OBJ usr1_sel_obj; /* Selection object for USR1 signal
> events */
> SaSelectionObjectT amf_sel_obj; /* Selection Object for AMF events */
> int nid_started; /* true if started by NID */
> - const char *admin_group_name; // linux group name for admins
> } IMMND_CB;
>
> /* CB prototypes */
> diff --git a/osaf/services/saf/immsv/immnd/immnd_evt.c
> b/osaf/services/saf/immsv/immnd/immnd_evt.c
> --- a/osaf/services/saf/immsv/immnd/immnd_evt.c
> +++ b/osaf/services/saf/immsv/immnd/immnd_evt.c
> @@ -25,6 +25,8 @@
>
> ******************************************************************************/
>
> #define _GNU_SOURCE
> +#include <sys/types.h>
> +#include <pwd.h>
> #include <osaf_secutil.h>
> #include "immnd.h"
> #include "immsv_api.h"
> @@ -743,20 +745,39 @@ static uint32_t immnd_evt_proc_imm_init(
> goto agent_rsp;
> }
>
> - /* allow access using white list approach */
> - if (sinfo->uid == 0) {
> - TRACE("superuser");
> - } else if (getgid() == sinfo->gid) {
> - TRACE("same group");
> - } else if ((immnd_cb->admin_group_name != NULL) &&
> - (osaf_user_is_member_of_group(sinfo->uid,
> immnd_cb->admin_group_name) == true)) {
> - TRACE("configured group");
> - } else {
> - syslog(LOG_AUTH, "access denied, uid:%d, pid:%d", sinfo->uid,
> sinfo->pid);
> - TRACE_2("access denied, uid:%d, pid:%d, group_name:%s",
> sinfo->uid, sinfo->pid,
> - immnd_cb->admin_group_name);
> - error = SA_AIS_ERR_ACCESS_DENIED;
> - goto agent_rsp;
> + ImmAccessControlModeT mode = immModel_accessControlMode(immnd_cb);
> + if (mode != ACCESS_CONTROL_DISABLED) {
> + /* allow access using white list approach */
> + if (sinfo->uid == 0) {
> + TRACE("superuser");
> + } else if (getgid() == sinfo->gid) {
> + TRACE("same group");
> + } else {
> + const char *admin_group_name =
> immModel_adminGroupName(immnd_cb);
> + if ((admin_group_name != NULL) &&
> + (osaf_user_is_member_of_group(sinfo->uid,
> admin_group_name) == true)) {
> + TRACE("configured group");
> + } else {
> + if (mode == ACCESS_CONTROL_PERMISSIVE) {
> + struct passwd *pwd =
> getpwuid(sinfo->uid);
> + if (pwd != NULL)
> + syslog(LOG_AUTH, "access
> violation by %s(uid=%d)",
> + pwd->pw_name,
> sinfo->uid);
> + TRACE_2("access denied, uid:%d, pid:%d,
> group_name:%s",
> + sinfo->uid, sinfo->pid,
> admin_group_name);
>
Above trace should say "access violation" rather than "access denied"
> + } else {
> + // mode ENFORCING
> + struct passwd *pwd =
> getpwuid(sinfo->uid);
> + if (pwd != NULL)
> + syslog(LOG_AUTH, "access denied
> for %s(uid=%d)",
> + pwd->pw_name,
> sinfo->uid);
> + TRACE_2("access denied, uid:%d, pid:%d,
> group_name:%s",
> + sinfo->uid, sinfo->pid,
> admin_group_name);
> + error = SA_AIS_ERR_ACCESS_DENIED;
> + goto agent_rsp;
> + }
> + }
> + }
> }
>
> cl_node = calloc(1, sizeof(IMMND_IMM_CLIENT_NODE));
> diff --git a/osaf/services/saf/immsv/immnd/immnd_init.h
> b/osaf/services/saf/immsv/immnd/immnd_init.h
> --- a/osaf/services/saf/immsv/immnd/immnd_init.h
> +++ b/osaf/services/saf/immsv/immnd/immnd_init.h
> @@ -31,6 +31,7 @@
>
> #include <saClm.h>
> #include "immsv_evt_model.h"
> +#include "immsv_api.h"
>
> extern IMMND_CB *immnd_cb;
>
> @@ -296,6 +297,8 @@ extern "C" {
> SaBoolT immModel_protocol43Allowed(IMMND_CB *cb);
> SaBoolT immModel_protocol45Allowed(IMMND_CB *cb);
> SaBoolT immModel_oneSafe2PBEAllowed(IMMND_CB *cb);
> + ImmAccessControlModeT immModel_accessControlMode(IMMND_CB *cb);
> + const char *immModel_adminGroupName(IMMND_CB *cb);
>
> SaBoolT immModel_purgeSyncRequest(IMMND_CB *cb, SaUint32T clientId);
>
> diff --git a/osaf/services/saf/immsv/immnd/immnd_main.c
> b/osaf/services/saf/immsv/immnd/immnd_main.c
> --- a/osaf/services/saf/immsv/immnd/immnd_main.c
> +++ b/osaf/services/saf/immsv/immnd/immnd_main.c
> @@ -134,8 +134,6 @@ static uint32_t immnd_initialize(char *p
> /* unset so that forked processes (e.g. loader) does not create MDS
> server */
> unsetenv("MDS_SOCK_SERVER_CREATE");
>
> - immnd_cb->admin_group_name = getenv("IMM_ADMIN_GROUP_NAME");
> -
> /* Initialize immnd control block */
> immnd_cb->ha_state = SA_AMF_HA_ACTIVE;
> immnd_cb->cli_id_gen = 1;
> diff --git a/samples/immsv/OpensafImm_Upgrade_4.5.xml
> b/samples/immsv/OpensafImm_Upgrade_4.5.xml
> --- a/samples/immsv/OpensafImm_Upgrade_4.5.xml
> +++ b/samples/immsv/OpensafImm_Upgrade_4.5.xml
> @@ -9,6 +9,19 @@
> <flag>SA_INITIALIZED</flag>
> </rdn>
> <attr>
> + <name>accessControlMode</name>
> + <type>SA_UINT32_T</type>
> + <category>SA_CONFIG</category>
> + <flag>SA_WRITABLE</flag>
> + <default-value>0</default-value>
> + </attr>
> + <attr>
> + <name>adminGroupName</name>
> + <type>SA_STRING_T</type>
> + <category>SA_CONFIG</category>
> + <flag>SA_WRITABLE</flag>
> + </attr>
> + <attr>
> <name>longDnsAllowed</name>
> <type>SA_UINT32_T</type>
> <category>SA_CONFIG</category>
>
------------------------------------------------------------------------------
Slashdot TV.
Video for Nerds. Stuff that matters.
http://tv.slashdot.org/
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
