Ack with a few comments.
1) Using a config attribute changeable at runtime instead of a build
time configuration
such as an environment variable, is more flexible but less secure. That
is, it is more
risky from a security point of view. Anyone that gets temporary access
to a terminal
with session already started (authenticated) could disable all imm
accessControl at runtime.
I am not sure how important this is but we have until the 4.5 release to
think about it.
Once the config attribute is out there, it can not be removed. It could
be deprecated
i.e. dis-used but currently IMMSV does not support removal of attribute
as a schema change,
since it inherently be an incompatible upgrade.
2) minor code adjustment, change from assert to if statement needed in
some places.
See inline below.
Hans Feldt wrote:
> osaf/libs/common/immsv/include/immsv_api.h | 2 +
> osaf/services/saf/immsv/immloadd/imm_loader.cc | 23 ++++++++-
> osaf/services/saf/immsv/immnd/ImmModel.cc | 65
> ++++++++++++++++++++++++++
> osaf/services/saf/immsv/immnd/ImmModel.hh | 2 +
> osaf/services/saf/immsv/immnd/immnd_cb.h | 1 -
> osaf/services/saf/immsv/immnd/immnd_evt.c | 34 ++++++++-----
> osaf/services/saf/immsv/immnd/immnd_init.h | 2 +
> osaf/services/saf/immsv/immnd/immnd_main.c | 2 -
> samples/immsv/OpensafImm_Upgrade_4.5.xml | 13 +++++
> 9 files changed, 125 insertions(+), 19 deletions(-)
>
>
> A new boolean attribute accessControlEnabled is added to the OpensafImm class.
> Its default value is OFF meaning no access control. This is to be backwards
> compatible for upgrade of existing systems.
>
> Access control is in runtime enabled with:
> immcfg -a accessControlEnabled=1 opensafImm=opensafImm,safApp=safImmService
>
> And disabled with:
> immcfg -a accessControlEnabled=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:
>
> 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,8 @@ 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_ENABLED "accessControlEnabled"
> +#define OPENSAF_IMM_ADMIN_GROUP_NAME "adminGroupName"
>
> /*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 accessControlEnabled attribute in the OpensafImm class
> +// Can be changed at build time using configure
> +#ifndef IMM_ACCESS_CONTROL
> +#define IMM_ACCESS_CONTROL 0
> +#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_enabled_default = IMM_ACCESS_CONTROL;
>
> 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_ENABLED;
> + d7.attrValueType = SA_IMM_ATTR_SAUINT32T;
> + d7.attrFlags = SA_IMM_ATTR_CONFIG | SA_IMM_ATTR_WRITABLE;
> + d7.attrDefaultValue = &access_control_enabled_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
> immAccessControlEnabled(OPENSAF_IMM_ACCESS_CONTROL_ENABLED);
> +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,18 @@ immModel_protocol45Allowed(IMMND_CB *cb)
> SA_TRUE : SA_FALSE;
> }
>
> +SaBoolT
> +immModel_accessControlEnabled(IMMND_CB *cb)
> +{
> + return (ImmModel::instance(&cb->immModel)->accessControlEnabled()) ?
> + SA_TRUE : SA_FALSE;
> +}
> +
> +const char*
> +immModel_adminGroupName(IMMND_CB *cb)
> +{
> + return ImmModel::instance(&cb->immModel)->adminGroupName();
> +}
>
> SaBoolT
> immModel_purgeSyncRequest(IMMND_CB *cb, SaUint32T clientId)
> @@ -2454,6 +2468,13 @@ ImmModel::setLoader(int pid)
> LOG_NO("RepositoryInitModeT is SA_IMM_INIT_FROM_FILE");
> immInitMode = SA_IMM_INIT_FROM_FILE; /* Ensure valid value*/
> }
> +
> + if (accessControlEnabled() == 0) {
> + LOG_WA("IMM Access Control is OFF!");
> + } else {
> + LOG_NO("IMM Access Control is ON");
> + }
> +
> } else {
> TRACE_5("Loading starts, pid:%u", pid);
> }
> @@ -3438,6 +3459,50 @@ ImmModel::schemaChangeAllowed()
> return noStdFlags & OPENSAF_IMM_FLAG_SCHCH_ALLOW;
> }
>
> +bool
> +ImmModel::accessControlEnabled()
> +{
> + TRACE_ENTER();
> + ObjectMap::iterator oi = sObjectMap.find(immObjectDn);
> + if(oi == sObjectMap.end()) {
> + TRACE_LEAVE();
> + return false;
> + }
> +
> + ObjectInfo* immObject = oi->second;
> + ImmAttrValueMap::iterator avi =
> + immObject->mAttrValueMap.find(immAccessControlEnabled);
> + osafassert(avi != immObject->mAttrValueMap.end());
>
The above assert should be changed to return false if the attribute does
not exist.
The problem is if a system is upgraded to 4.5 but the schema change, of
adding the
new 4.5 attributes to this class, is skipped. Then this assert would
crash all IMMNDs
and cause a cluster restart. Omitting the schema change that adds these
attributes
is possible and may be intentional for deployments not interested in the
support for
access control or long DNs.
Check the implementation of ImmModel::getLongDnsAllowed().
I thought I had done the same error but I had not. Which surprised
and pleased me :-)
The subsequent assert is ok because if this attribute does exist, it
should not be multivalued.
> + osafassert(!(avi->second->isMultiValued()));
> + ImmAttrValue* valuep = avi->second;
> + unsigned int accessControlEnabled = valuep->getValue_int();
> +
> + TRACE_LEAVE2("%u", accessControlEnabled);
> + return accessControlEnabled;
> +}
> +
> +const char*
> +ImmModel::adminGroupName()
> +{
> + TRACE_ENTER();
> + ObjectMap::iterator oi = sObjectMap.find(immObjectDn);
> + if(oi == sObjectMap.end()) {
> + TRACE_LEAVE();
> + return false;
> + }
> +
> + ObjectInfo* immObject = oi->second;
> + ImmAttrValueMap::iterator avi =
> + immObject->mAttrValueMap.find(immAdminGroupName);
> + osafassert(avi != immObject->mAttrValueMap.end());
>
Same here. The above assert should be an if.
Below assert is ok ... but .. perhaps we should make it multivalued?,
allowing more than one group ?
Upgrade from single valued to multivalued is supported though so this is
not as irreversible as the addition of the attributes as such.
/AndersBj
> + 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
> @@ -102,6 +102,8 @@ public:
>
> bool nocaseCompare(const std::string& s1,
> const std::string& s2) const;
> + const char* adminGroupName();
> + bool accessControlEnabled();
> 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
> @@ -736,20 +736,26 @@ 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;
> + if (immModel_accessControlEnabled(immnd_cb)) {
> + /* 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 {
> + 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,
> 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
> @@ -296,6 +296,8 @@ extern "C" {
> SaBoolT immModel_protocol43Allowed(IMMND_CB *cb);
> SaBoolT immModel_protocol45Allowed(IMMND_CB *cb);
> SaBoolT immModel_oneSafe2PBEAllowed(IMMND_CB *cb);
> + SaBoolT immModel_accessControlEnabled(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
> @@ -127,8 +127,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>accessControlEnabled</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>
>
------------------------------------------------------------------------------
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
