Thank you! I will incorporate your comments and push. /Hans > -----Original Message----- > From: Anders Björnerstedt > Sent: den 22 augusti 2014 15:08 > To: Hans Feldt > Cc: [email protected] > Subject: Re: [PATCH 1 of 2] immsv: add configurability of access control > [#938] > > 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
