Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HPCC-32452 Time security manager auth calls #19007

Closed
wants to merge 3 commits into from
Closed

HPCC-32452 Time security manager auth calls #19007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c31582b
HPCC-32465 Add ESP support for trace level
Aug 22, 2024
3a034a6
HPCC-32452 Add conditional tracing for ESP security manager authoriza…
Aug 22, 2024
f23b4e4
Review comments
Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions common/workunit/workunit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
#include "daqueue.hpp"
#include "workunit.ipp"
#include "digisign.hpp"
#include "secmanagertracedecorator.hpp"

#include <list>
#include <string>
Expand Down Expand Up @@ -113,7 +114,7 @@ static bool checkWuScopeSecAccess(const char *wuscope, ISecManager *secmgr, ISec
{
if (!secmgr || !secuser)
return true;
bool ret = secmgr->authorizeEx(RT_WORKUNIT_SCOPE, *secuser, wuscope)>=required;
bool ret = CSecManagerTraceDecorator(*secmgr).authorizeEx(RT_WORKUNIT_SCOPE, *secuser, wuscope)>=required;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concern that each time you use CSecManagerTraceDecorator that it is constructed and destructed. Any thought on making the decorated calls static?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be a best practice to use a single security manager configuration (implying a single implementation) within an ESP process. ESPs can be configured to use multiple managers and configurations.

A static interface would require either passing the manager to each decorated call, negating the benefit of extending ISecManager, or a threaded security manager (similar to the threaded active span). The latter option would likely complicate the implementation by requiring manager checks in every decorated method.

I'm not sure which is preferred. Also, one of @rpastrana's comments might indirectly justify a threaded active decorator, so only one decorator would be created per service.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about the overhead of creating a new decorator every time a secmanager method is called. What's the overall overhead added and what is the overriding reason to implement this way.

if (!ret && (log || excpt))
wuAccessError(secuser->getName(), action, wuscope, NULL, excpt, log);
return ret;
Expand Down Expand Up @@ -146,7 +147,7 @@ static bool checkWuSecAccess(IConstWorkUnit &cw, ISecManager *secmgr, ISecUser *
{
if (!secmgr || !secuser)
return true;
bool ret=secmgr->authorizeEx(RT_WORKUNIT_SCOPE, *secuser, cw.queryWuScope())>=required;
bool ret=CSecManagerTraceDecorator(*secmgr).authorizeEx(RT_WORKUNIT_SCOPE, *secuser, cw.queryWuScope())>=required;
if (!ret && (log || excpt))
{
wuAccessError(secuser->getName(), action, cw.queryWuScope(), cw.queryWuid(), excpt, log);
Expand All @@ -159,7 +160,7 @@ static bool checkWuSecAccess(const char *wuid, ISecManager *secmgr, ISecUser *se
return true;
Owned<IWorkUnitFactory> factory = getWorkUnitFactory();
Owned<IConstWorkUnit> cw = factory->openWorkUnit(wuid);
bool ret=secmgr->authorizeEx(RT_WORKUNIT_SCOPE, *secuser, cw->queryWuScope())>=required;
bool ret=CSecManagerTraceDecorator(*secmgr).authorizeEx(RT_WORKUNIT_SCOPE, *secuser, cw->queryWuScope())>=required;
if (!ret && (log || excpt))
{
wuAccessError(secuser->getName(), action, cw->queryWuScope(), cw->queryWuid(), excpt, log);
Expand Down Expand Up @@ -5219,7 +5220,7 @@ class CSecureConstWUIterator : public CInterfaceOf<IConstWorkUnitIterator>
SecAccessFlags perm;
if (!perms)
{
perm = secuser.get() ? secmgr->authorizeWorkunitScope(*secuser, scopeName) : SecAccess_Unavailable;
perm = secuser.get() ? CSecManagerTraceDecorator(*secmgr).authorizeWorkunitScope(*secuser, scopeName) : SecAccess_Unavailable;
scopePermissions.setValue(scopeName, perm);
}
else
Expand Down
17 changes: 16 additions & 1 deletion esp/bindings/bind_ng.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "jliball.hpp"
#include "esp.hpp"
#include "soapesp.hpp"
#include "secmanagertracedecorator.hpp"

class CEspNgContext : implements IEspContext, public CInterface
{
Expand All @@ -31,6 +32,7 @@ class CEspNgContext : implements IEspContext, public CInterface

Owned<ISecUser> secuser;
Owned<ISecManager> secmgr;
Owned<ISecManager> tracingSecMgrDecorator;
Owned<ISecResourceList> reslist;
Owned<IAuthMap> authMap;
Owned<ISecPropertyList> secprops;
Expand Down Expand Up @@ -78,7 +80,20 @@ class CEspNgContext : implements IEspContext, public CInterface
}


void setSecManger(ISecManager * mgr){secmgr.setown(mgr);}
void setSecManger(ISecManager * mgr)
{
setSecManager(mgr, nullptr);
}
virtual void setSecManager(ISecManager* mgr, ISecManager * _tracingSecMgrDecorator)
{
secmgr.setown(mgr);
if (!mgr)
tracingSecMgrDecorator.clear();
else if (!_tracingSecMgrDecorator)
tracingSecMgrDecorator.setown(new CSecManagerTraceDecorator(*mgr));
else
tracingSecMgrDecorator.set(_tracingSecMgrDecorator);
}
ISecManager * querySecManager(){return secmgr.get();}

void setContextPath(const char * path){contextPath.clear().append(path);}
Expand Down
15 changes: 11 additions & 4 deletions esp/bindings/http/platform/httpbinding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
#include <memory>

#include "esdl_def_helper.hpp"

#include "secmanagertracedecorator.hpp"

#define FILE_UPLOAD "FileUploadAccess"
#define DEFAULT_HTTP_PORT 80
Expand Down Expand Up @@ -406,6 +406,9 @@ EspHttpBinding::EspHttpBinding(IPropertyTree* tree, const char *bindname, const
//This is a Pluggable Security Manager
setBndCfgServiceType(tree, procname, bnd_cfg);
m_secmgr.setown(SecLoader::loadPluggableSecManager<ISecManager>(bindname, bnd_cfg, secMgrCfg));
if (!m_secmgr)
throw makeStringException(-1, "error loading pluggable security manager");
m_tracingSecMgrDecorator.setown(new CSecManagerTraceDecorator(*m_secmgr));
m_authmap.setown(m_secmgr->createAuthMap(authcfg));
m_feature_authmap.setown(m_secmgr->createFeatureMap(authcfg));
m_setting_authmap.setown(m_secmgr->createSettingMap(authcfg));
Expand Down Expand Up @@ -434,6 +437,7 @@ EspHttpBinding::EspHttpBinding(IPropertyTree* tree, const char *bindname, const
{
throw MakeStringException(-1, "error generating SecManager");
}
m_tracingSecMgrDecorator.setown(new CSecManagerTraceDecorator(*m_secmgr));;

StringBuffer basednbuf;
authcfg->getProp("@resourcesBasedn", basednbuf);
Expand All @@ -449,6 +453,9 @@ EspHttpBinding::EspHttpBinding(IPropertyTree* tree, const char *bindname, const
else if(stricmp(m_authmethod.str(), "Local") == 0)
{
m_secmgr.setown(SecLoader::loadSecManager("Local", "EspHttpBinding", NULL));
if (m_secmgr.get() == NULL)
throw MakeStringException(-1, "error loading local security manager");
m_tracingSecMgrDecorator.setown(new CSecManagerTraceDecorator(*m_secmgr));
m_authmap.setown(m_secmgr->createAuthMap(authcfg));
}
IRestartManager* restartManager = dynamic_cast<IRestartManager*>(m_secmgr.get());
Expand Down Expand Up @@ -847,7 +854,7 @@ void EspHttpBinding::populateRequest(CHttpRequest *request)
{
IEspContext* ctx = request->queryContext();

ctx->setSecManger(m_secmgr.getLink());
ctx->setSecManager(m_secmgr.getLink(), m_tracingSecMgrDecorator.get());
ctx->setFeatureAuthMap(m_feature_authmap.getLink());

StringBuffer userid, password,realm,peer;
Expand Down Expand Up @@ -972,7 +979,7 @@ bool EspHttpBinding::basicAuth(IEspContext* ctx)
return false;
}

bool authenticated = m_secmgr->authorize(*user, rlist, ctx->querySecureContext());
bool authenticated = m_tracingSecMgrDecorator->authorize(*user, rlist, ctx->querySecureContext());
if(!authenticated)
{
VStringBuffer err("User %s : ", user->getName());
Expand Down Expand Up @@ -1029,7 +1036,7 @@ bool EspHttpBinding::basicAuth(IEspContext* ctx)
if(securitySettings == NULL)
return authorized;

m_secmgr->updateSettings(*user,securitySettings, ctx->querySecureContext());
m_tracingSecMgrDecorator->updateSettings(*user,securitySettings, ctx->querySecureContext());

ctx->addTraceSummaryTimeStamp(LogMin, "basicAuth", TXSUMMARY_GRP_CORE);
return authorized;
Expand Down
2 changes: 2 additions & 0 deletions esp/bindings/http/platform/httpbinding.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ class esp_http_decl EspHttpBinding :
StringBuffer m_filespath;
StringBuffer m_wsdlAddress;
Owned<ISecManager> m_secmgr;
Owned<ISecManager> m_tracingSecMgrDecorator;
Owned<IAuthMap> m_authmap;
Owned<IAuthMap> m_feature_authmap;
Owned<IAuthMap> m_setting_authmap;
Expand Down Expand Up @@ -372,6 +373,7 @@ class esp_http_decl EspHttpBinding :
return false;
}
ISecManager* querySecManager() const { return m_secmgr.get(); }
ISecManager* queryTracingSecManager() const { return m_tracingSecMgrDecorator.get(); }
IAuthMap* queryAuthMAP() const { return m_authmap.get();}
const char* queryAuthMethod() const { return m_authmethod.str(); }
void setProcessName(const char* name) { processName.set(name); }
Expand Down
35 changes: 30 additions & 5 deletions esp/bindings/http/platform/httpservice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,36 @@ int CEspHttpServer::processRequest()
else //user ID is in HTTP header
ESPLOG(LogMin, "%s %s, from %s@%s", method.str(), m_request->getPath(pathStr).str(), userid, m_request->getPeer(peerStr).str());

// The user auth check may create trace output, in which case case the root server span
// must be created first. Since the span was intentionally created after dispatching a
// variety of requests, span creation is conditional.
bool wantTracing = true;
if (!queryTraceManager().isTracingEnabled())
wantTracing = false;
else if (streq(method, GET_METHOD))
{
if (sub_serv_root == stype)
wantTracing = false;
else if (strieq(serviceName, "esp"))
{
wantTracing = !(methodName.isEmpty() ||
strieq(methodName, "files") ||
strieq(methodName, "xslt") ||
strieq(methodName, "frame") ||
strieq(methodName, "titlebar") ||
strieq(methodName, "nav") ||
strieq(methodName, "navdata") ||
strieq(methodName, "navmenuevent") ||
strieq(methodName, "soapreq"));
}
}
else if (!m_apport)
wantTracing = false;
//The context will be destroyed when this request is destroyed. So initialise a SpanScope in the context to
//ensure the span is also terminated at the same time.
Owned<ISpan> serverSpan = (wantTracing ? m_request->createServerSpan(serviceName, methodName) : getNullSpan());
ctx->setRequestSpan(serverSpan);

authState = checkUserAuth();
if ((authState == authTaskDone) || (authState == authFailed))
return 0;
Expand Down Expand Up @@ -500,11 +530,6 @@ int CEspHttpServer::processRequest()
return 0;
}

//The context will be destroyed when this request is destroyed. So initialise a SpanScope in the context to
//ensure the span is also terminated at the same time.
Owned<ISpan> serverSpan = m_request->createServerSpan(serviceName, methodName);
ctx->setRequestSpan(serverSpan);

if (thebinding!=NULL)
{
if(stricmp(method.str(), POST_METHOD)==0)
Expand Down
15 changes: 15 additions & 0 deletions esp/platform/application_config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include "espcfg.ipp"
#include "esplog.hpp"
#include "espcontext.hpp"
#include "esptrace.h"

enum class LdapType { LegacyAD, AzureAD };

Expand Down Expand Up @@ -436,6 +437,18 @@ void setLDAPSecurityInWSAccess(IPropertyTree *legacyEsp, IPropertyTree *legacyLd
}
}

// Copy trace flags from appEsp to legacyEsp. The source is expected to be an application's `esp`
// configuration object. The destination is expected to be the `EspProcess` element.
void copyTraceFlags(IPropertyTree *legacyEsp, IPropertyTree *appEsp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when would this method be useful?
Is it only capitalizing the traceflags name?
Perhaps a method comment header would help

{
IPropertyTree *traceFlags = appEsp->queryPropTree(propTraceFlags);
if (traceFlags)
{
IPropertyTree *legacyTraceFlags = legacyEsp->addPropTree(propTraceFlags);
copyAttributes(legacyTraceFlags, traceFlags);
}
}

IPropertyTree *buildApplicationLegacyConfig(const char *application, const char* argv[])
{
Owned<IPropertyTree> appEspConfig = loadApplicationConfig(application, argv);
Expand Down Expand Up @@ -468,5 +481,7 @@ IPropertyTree *buildApplicationLegacyConfig(const char *application, const char*
IPropertyTree *legacyDirectories = legacy->queryPropTree("Software/Directories");
IPropertyTree *appDirectories = appEspConfig->queryPropTree("directories");
copyDirectories(legacyDirectories, appDirectories);

copyTraceFlags(legacyEsp, appEspConfig);
return legacy.getClear();
}
2 changes: 2 additions & 0 deletions esp/platform/esp.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,9 @@ interface IEspContext : extends IInterface
virtual void setResources(ISecResourceList * rlist) = 0;
virtual ISecResourceList * queryResources() = 0;
virtual void setSecManger(ISecManager * mgr) = 0;
virtual void setSecManager(ISecManager * mgr, ISecManager * tracingSecMgrDecoratorDecorator) = 0;
virtual ISecManager * querySecManager() = 0;
virtual ISecManager * queryTracingSecManager() = 0;
virtual void setContextPath(const char * path) = 0;
virtual const char * getContextPath() = 0;
virtual void setBindingValue(void * value) = 0;
Expand Down
20 changes: 19 additions & 1 deletion esp/platform/espcontext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include "espsecurecontext.hpp"
#include "ldapsecurity.ipp"
#include "dasds.hpp"
#include "secmanagertracedecorator.hpp"

class CEspContext : public CInterface, implements IEspContext
{
Expand All @@ -50,6 +51,7 @@ class CEspContext : public CInterface, implements IEspContext
Owned<ISecUser> m_user;
Owned<ISecResourceList> m_resources;
Owned<ISecManager> m_secmgr;
Owned<ISecManager> m_tracingSecMgrDecorator;
Owned<IAuthMap> m_feature_authmap;
Owned<ISecPropertyList> m_sec_settings;

Expand Down Expand Up @@ -289,16 +291,32 @@ class CEspContext : public CInterface, implements IEspContext
}

virtual void setSecManger(ISecManager* mgr)
{
setSecManager(mgr, nullptr);
}

virtual void setSecManager(ISecManager* mgr, ISecManager* tracingSecMgrDecorator) override
{
m_secmgr.setown(mgr);
m_SecurityHandler.setSecManger(mgr);
if (!mgr)
m_tracingSecMgrDecorator.clear();
else if (!tracingSecMgrDecorator)
m_tracingSecMgrDecorator.setown(new CSecManagerTraceDecorator(*mgr));
else
m_tracingSecMgrDecorator.set(tracingSecMgrDecorator);
m_SecurityHandler.setSecManager(mgr, tracingSecMgrDecorator);
}

virtual ISecManager* querySecManager()
{
return m_secmgr.get();
}

virtual ISecManager* queryTracingSecManager() override
{
return m_tracingSecMgrDecorator.get();
}

virtual void setBindingValue(void * value)
{
m_bindingValue=value;
Expand Down
Loading