From 195b5d8aeadd180413ee4557efc6519cb1d4a6e8 Mon Sep 17 00:00:00 2001 From: Terrence Asselin Date: Wed, 8 Jan 2025 15:29:51 -0600 Subject: [PATCH 1/2] HPCC-32847 Require WsLogAccess to include logs in ZAP Require the same WsLogAccess:READ permission to include log files in a ZAP as would be required to view them. When log files are excluded from the ZAP due to lack of permission, the ZAP info file (.txt) includes a line explaining their absence. Signed-off-by: Terrence Asselin --- .../ws_workunits/ws_workunitsHelpers.cpp | 53 ++++++++++++------- .../ws_workunits/ws_workunitsHelpers.hpp | 17 ++++-- 2 files changed, 47 insertions(+), 23 deletions(-) diff --git a/esp/services/ws_workunits/ws_workunitsHelpers.cpp b/esp/services/ws_workunits/ws_workunitsHelpers.cpp index 2389e899d96..30f812f10bb 100644 --- a/esp/services/ws_workunits/ws_workunitsHelpers.cpp +++ b/esp/services/ws_workunits/ws_workunitsHelpers.cpp @@ -45,6 +45,9 @@ namespace ws_workunits { +// Same feature access as in ws_logaccess.ecm +#define WS_LOGACCESS "WsLogAccess" + const char * const timerFilterText = "measure[time],source[global],depth[1,]"; // Does not include hthor subgraph timings const char* zipFolder = "tempzipfiles" PATHSEPSTR; @@ -4096,8 +4099,7 @@ void CWsWuFileHelper::createThorSlaveLogfile(IConstWorkUnit* cwu, WsWuInfo& winf } #endif -void CWsWuFileHelper::createZAPInfoFile(const char* url, const char* esp, const char* thor, const char* problemDesc, - const char* whatChanged, const char* timing, IConstWorkUnit* cwu, const char* tempDirName) +void CWsWuFileHelper::createZAPInfoFile(CWsWuZAPInfoReq &request, IConstWorkUnit* cwu, const char* tempDirName) { VStringBuffer fileName("%s%c%s.txt", tempDirName, PATHSEPCHAR, cwu->queryWuid()); Owned outFile = createBufferedIOStreamFromFile(fileName.str(), IFOcreate); @@ -4109,11 +4111,11 @@ void CWsWuFileHelper::createZAPInfoFile(const char* url, const char* esp, const sb.append("User: ").append(cwu->queryUser()).append("\r\n"); sb.append("Build Version:").append(getBuildVersion()).append("\r\n"); sb.append("Cluster: ").append(cwu->queryClusterName()).append("\r\n"); - sb.append("ESP: ").append(esp).append("\r\n"); - if (!isEmptyString(url)) - sb.append("URL: ").append(url).append("\r\n"); - if (!isEmptyString(thor)) - sb.append("Thor: ").append(thor).append("\r\n"); + sb.append("ESP: ").append(request.esp).append("\r\n"); + if (!isEmptyString(request.url)) + sb.append("URL: ").append(request.url).append("\r\n"); + if (!isEmptyString(request.thor)) + sb.append("Thor: ").append(request.thor).append("\r\n"); outFile->write(sb.length(), sb.str()); //Exceptions/Warnings/Info @@ -4147,9 +4149,14 @@ void CWsWuFileHelper::createZAPInfoFile(const char* url, const char* esp, const } //User provided Information - writeZAPWUInfoToIOStream(outFile, "Problem: ", problemDesc); - writeZAPWUInfoToIOStream(outFile, "What Changed: ", whatChanged); - writeZAPWUInfoToIOStream(outFile, "Timing: ", timing); + writeZAPWUInfoToIOStream(outFile, "Problem: ", request.problemDesc); + writeZAPWUInfoToIOStream(outFile, "What Changed: ", request.whatChanged); + writeZAPWUInfoToIOStream(outFile, "Timing: ", request.whereSlow); + if (request.logsExcludedDueToNoAccess()) + { + writeZAPWUInfoToIOStream(outFile, "Logs: ", "Excluded due to no access, need WsLogAccess:Read"); + LOG(MCuserWarning, "ZAP report for workunit %s excludes logs due to access restrictions. Need WsLogAccess:READ", request.wuid.str()); + } } void CWsWuFileHelper::writeZAPWUInfoToIOStream(IFileIOStream* outFile, const char* name, SCMStringBuffer& value) @@ -4517,14 +4524,14 @@ int CWsWuFileHelper::zipAFolder(const char* folder, bool gzip, const char* zipFi void CWsWuFileHelper::createWUZAPFile(IEspContext& context, IConstWorkUnit* cwu, CWsWuZAPInfoReq& request, const char* tempDirName, StringBuffer& zapFileName, StringBuffer& zipFileNameWithFullPath, unsigned _thorSlaveLogThreadPoolSize) { + request.hasLogsAccess = context.validateFeatureAccess(WS_LOGACCESS, SecAccess_Read, false); setZAPReportName(request.zapFileName, cwu->queryWuid(), zapFileName); zipFileNameWithFullPath.set(tempDirName).append(PATHSEPCHAR).append("zapreport.zip"); thorSlaveLogThreadPoolSize = _thorSlaveLogThreadPoolSize; - //create WU ZAP files - createZAPInfoFile(request.url.str(), request.esp.str(), request.thor.str(), request.problemDesc.str(), request.whatChanged.str(), - request.whereSlow.str(), cwu, tempDirName); +//create WU ZAP files + createZAPInfoFile(request, cwu, tempDirName); createZAPECLQueryArchiveFiles(cwu, tempDirName); WsWuInfo winfo(context, cwu); @@ -4532,21 +4539,24 @@ void CWsWuFileHelper::createWUZAPFile(IEspContext& context, IConstWorkUnit* cwu, createZAPWUGraphProgressFile(request.wuid.str(), tempDirName); StringArray localFiles; - createZAPWUQueryAssociatedFiles(cwu, tempDirName, localFiles); + createZAPWUQueryAssociatedFiles(cwu, tempDirName, localFiles, request.hasLogsAccess); + if (request.hasLogsAccess) + { #ifndef _CONTAINERIZED - createProcessLogfile(cwu, winfo, "EclAgent", tempDirName); - createProcessLogfile(cwu, winfo, "Thor", tempDirName); - if (request.includeThorSlaveLog.isEmpty() || strieq(request.includeThorSlaveLog.str(), "on")) - createThorSlaveLogfile(cwu, winfo, tempDirName); + createProcessLogfile(cwu, winfo, "EclAgent", tempDirName); + createProcessLogfile(cwu, winfo, "Thor", tempDirName); + if (request.includeThorSlaveLog.isEmpty() || strieq(request.includeThorSlaveLog.str(), "on")) + createThorSlaveLogfile(cwu, winfo, tempDirName); #else - readWULogToFiles(cwu, winfo, tempDirName, request); + readWULogToFiles(cwu, winfo, tempDirName, request); #endif + } //Write out to ZIP file zipAllZAPFiles(tempDirName, localFiles, request.password, zipFileNameWithFullPath); } -void CWsWuFileHelper::createZAPWUQueryAssociatedFiles(IConstWorkUnit* cwu, const char* tempDirName, StringArray& localFiles) +void CWsWuFileHelper::createZAPWUQueryAssociatedFiles(IConstWorkUnit* cwu, const char* tempDirName, StringArray& localFiles, bool hasLogsAccess) { Owned query = cwu->getQuery(); if (!query) @@ -4563,6 +4573,9 @@ void CWsWuFileHelper::createZAPWUQueryAssociatedFiles(IConstWorkUnit* cwu, const cur.getName(name); cur.getIp(ip); + if (!hasLogsAccess && endsWith(name.str(), ".eclcc.log")) + continue; + RemoteFilename rfn; SocketEndpoint ep(ip.str()); rfn.setPath(ep, name.str()); diff --git a/esp/services/ws_workunits/ws_workunitsHelpers.hpp b/esp/services/ws_workunits/ws_workunitsHelpers.hpp index d8b68ce724b..15009d727f2 100644 --- a/esp/services/ws_workunits/ws_workunitsHelpers.hpp +++ b/esp/services/ws_workunits/ws_workunitsHelpers.hpp @@ -369,6 +369,7 @@ struct CWsWuZAPInfoReq bool sendEmail, attachZAPReportToEmail; bool includeRelatedLogs = true, includePerComponentLogs = false; unsigned maxAttachmentSize, port; + bool hasLogsAccess = false; WUComponentLogOptions logFilter; @@ -381,6 +382,17 @@ struct CWsWuZAPInfoReq { logFilter.populateLogFilter(wuid.str(), httpRequest); } + + // True when logs _would have been included_ if the user had access + bool logsExcludedDueToNoAccess() + { +#ifdef _CONTAINERIZED + return (includeRelatedLogs || includePerComponentLogs) && !hasLogsAccess; +#else + // Bare metal historically always includes Thor and EclAgent logs + return !hasLogsAccess; +#endif + } }; class WsWuInfo @@ -890,11 +902,10 @@ class CWsWuFileHelper IFile *createWorkingFolder(IEspContext &context, const char *wuid, const char *namePrefix, StringBuffer &namePrefixStr, StringBuffer &folderName); - void createZAPInfoFile(const char *url, const char *espIP, const char *thorIP, const char *problemDesc, - const char *whatChanged, const char *timing, IConstWorkUnit *cwu, const char *pathNameStr); + void createZAPInfoFile(CWsWuZAPInfoReq &request, IConstWorkUnit *cwu, const char *pathNameStr); void createZAPWUXMLFile(WsWuInfo &winfo, const char *pathNameStr); void createZAPECLQueryArchiveFiles(IConstWorkUnit *cwu, const char *pathNameStr); - void createZAPWUQueryAssociatedFiles(IConstWorkUnit *cwu, const char *pathToCreate, StringArray &localFiles); + void createZAPWUQueryAssociatedFiles(IConstWorkUnit *cwu, const char *pathToCreate, StringArray &localFiles, bool hasLogsAccess); void createZAPWUGraphProgressFile(const char *wuid, const char *pathNameStr); #ifndef _CONTAINERIZED void createProcessLogfile(IConstWorkUnit *cwu, WsWuInfo &winfo, const char *process, const char *path); From 915e318795888d297fd6752a8c2d5074c1dad1cf Mon Sep 17 00:00:00 2001 From: Terrence Asselin Date: Wed, 15 Jan 2025 16:25:27 -0600 Subject: [PATCH 2/2] HPCC-32847 Code review comment changes * Vary the permission feature required to access logs to match the one used for the cluster deployment- containerized or bare metal. * The WsLogAccess permission is used when pulling all containerized log files, including eclcc, because the remote log accessor is required to pull them all. * Because users are used to seeing errors in log file stubs, instead of actual log entries, continue that behavior here. When access to log files is denied, insert a message into each (otherwise empty) log file included in the ZAP. * Some code paths don't currently replace files with error messages, so to maintain current behavior and user expectations don't insert error messages into any file other than a log file. Signed-off-by: Terrence Asselin --- .../ws_workunits/ws_workunitsHelpers.cpp | 73 ++++++++++++------- .../ws_workunits/ws_workunitsHelpers.hpp | 14 +--- 2 files changed, 49 insertions(+), 38 deletions(-) diff --git a/esp/services/ws_workunits/ws_workunitsHelpers.cpp b/esp/services/ws_workunits/ws_workunitsHelpers.cpp index 30f812f10bb..1192ac70634 100644 --- a/esp/services/ws_workunits/ws_workunitsHelpers.cpp +++ b/esp/services/ws_workunits/ws_workunitsHelpers.cpp @@ -45,8 +45,11 @@ namespace ws_workunits { -// Same feature access as in ws_logaccess.ecm -#define WS_LOGACCESS "WsLogAccess" +#ifdef _CONTAINERIZED +static const char* LOG_ACCESS_FEATURE = "WsLogAccess"; +#else +static const char* LOG_ACCESS_FEATURE = "ClusterTopologyAccess"; +#endif const char * const timerFilterText = "measure[time],source[global],depth[1,]"; // Does not include hthor subgraph timings const char* zipFolder = "tempzipfiles" PATHSEPSTR; @@ -3994,13 +3997,12 @@ void CWsWuFileHelper::cleanFolder(IFile* folder, bool removeFolder) } #ifndef _CONTAINERIZED -void CWsWuFileHelper::createProcessLogfile(IConstWorkUnit* cwu, WsWuInfo& winfo, const char* process, const char* path) +void CWsWuFileHelper::createProcessLogfile(IConstWorkUnit* cwu, WsWuInfo& winfo, const char* process, const char* path, bool hasLogsAccess) { BoolHash uniqueProcesses; Owned procs = cwu->getProcesses(process, NULL); ForEach (*procs) { - StringBuffer logSpec; IPropertyTree& proc = procs->query(); const char* processName = proc.queryName(); if (isEmpty(processName)) @@ -4024,11 +4026,17 @@ void CWsWuFileHelper::createProcessLogfile(IConstWorkUnit* cwu, WsWuInfo& winfo, pid.appendf("%d", proc.getPropInt("@pid")); fileName.append("_eclagent.log"); + if (!hasLogsAccess) + throw makeStringExceptionV(ECLWATCH_ACCESS_TO_FILE_DENIED, "Access to log files is denied. You are missing permission %s:READ - check with your system admin.", LOG_ACCESS_FEATURE); + winfo.getWorkunitEclAgentLog(processName, nullptr, pid.str(), mb, fileName.str()); } else if (strieq(process, "Thor")) { fileName.append("_thormaster.log"); + if (!hasLogsAccess) + throw makeStringExceptionV(ECLWATCH_ACCESS_TO_FILE_DENIED, "Access to log files is denied. You are missing permission %s:READ - check with your system admin.", LOG_ACCESS_FEATURE); + winfo.getWorkunitThorMasterLog(processName, nullptr, mb, fileName.str()); } } @@ -4036,14 +4044,14 @@ void CWsWuFileHelper::createProcessLogfile(IConstWorkUnit* cwu, WsWuInfo& winfo, { StringBuffer s; e->errorMessage(s); - IERRLOG("Error accessing Process Log file %s: %s", logSpec.str(), s.str()); + IERRLOG("Error accessing Process Log file %s: %s", processName, s.str()); writeToFile(fileName.str(), s.length(), s.str()); e->Release(); } } } -void CWsWuFileHelper::createThorSlaveLogfile(IConstWorkUnit* cwu, WsWuInfo& winfo, const char* path) +void CWsWuFileHelper::createThorSlaveLogfile(IConstWorkUnit* cwu, WsWuInfo& winfo, const char* path, bool hasLogsAccess) { if (cwu->getWuidVersion() == 0) return; @@ -4058,6 +4066,18 @@ void CWsWuFileHelper::createThorSlaveLogfile(IConstWorkUnit* cwu, WsWuInfo& winf return; } + // Since there is not already a handler inserting exceptions into the thor slave log, + // make a simple case here to insert a message about permission failure and write it to + // a file that could be recognized as a thor slave log. + if (!hasLogsAccess) + { + StringBuffer msg; + msg.appendf("Access to log files is denied. You are missing permission %s:READ - check with your system admin.", LOG_ACCESS_FEATURE); + VStringBuffer fileName("%s%cthorslave.log", path, PATHSEPCHAR); + writeToFile(fileName.str(), msg.length(), msg.str()); + return; + } + Owned threadFactory = new CGetThorSlaveLogToFileThreadFactory(); Owned threadPool = createThreadPool("WsWuFileHelper GetThorSlaveLogToFile Thread Pool", threadFactory, false, nullptr, thorSlaveLogThreadPoolSize, INFINITE); @@ -4152,11 +4172,6 @@ void CWsWuFileHelper::createZAPInfoFile(CWsWuZAPInfoReq &request, IConstWorkUnit writeZAPWUInfoToIOStream(outFile, "Problem: ", request.problemDesc); writeZAPWUInfoToIOStream(outFile, "What Changed: ", request.whatChanged); writeZAPWUInfoToIOStream(outFile, "Timing: ", request.whereSlow); - if (request.logsExcludedDueToNoAccess()) - { - writeZAPWUInfoToIOStream(outFile, "Logs: ", "Excluded due to no access, need WsLogAccess:Read"); - LOG(MCuserWarning, "ZAP report for workunit %s excludes logs due to access restrictions. Need WsLogAccess:READ", request.wuid.str()); - } } void CWsWuFileHelper::writeZAPWUInfoToIOStream(IFileIOStream* outFile, const char* name, SCMStringBuffer& value) @@ -4288,6 +4303,8 @@ void CWsWuFileHelper::readWULogToFile(const char *logFileName, WsWuInfo &winfo, { try { + if (!zapLogFilterOptions.hasLogsAccess) + throw makeStringExceptionV(ECLWATCH_ACCESS_TO_FILE_DENIED, "Access to log files is denied. You are missing permission %s:READ - check with your system admin.", LOG_ACCESS_FEATURE); winfo.readWorkunitComponentLogs(logFileName, zapLogFilterOptions); } catch(IException* e) @@ -4524,7 +4541,7 @@ int CWsWuFileHelper::zipAFolder(const char* folder, bool gzip, const char* zipFi void CWsWuFileHelper::createWUZAPFile(IEspContext& context, IConstWorkUnit* cwu, CWsWuZAPInfoReq& request, const char* tempDirName, StringBuffer& zapFileName, StringBuffer& zipFileNameWithFullPath, unsigned _thorSlaveLogThreadPoolSize) { - request.hasLogsAccess = context.validateFeatureAccess(WS_LOGACCESS, SecAccess_Read, false); + request.hasLogsAccess = context.validateFeatureAccess(LOG_ACCESS_FEATURE, SecAccess_Read, false); setZAPReportName(request.zapFileName, cwu->queryWuid(), zapFileName); zipFileNameWithFullPath.set(tempDirName).append(PATHSEPCHAR).append("zapreport.zip"); @@ -4540,17 +4557,14 @@ void CWsWuFileHelper::createWUZAPFile(IEspContext& context, IConstWorkUnit* cwu, StringArray localFiles; createZAPWUQueryAssociatedFiles(cwu, tempDirName, localFiles, request.hasLogsAccess); - if (request.hasLogsAccess) - { #ifndef _CONTAINERIZED - createProcessLogfile(cwu, winfo, "EclAgent", tempDirName); - createProcessLogfile(cwu, winfo, "Thor", tempDirName); + createProcessLogfile(cwu, winfo, "EclAgent", tempDirName, request.hasLogsAccess); + createProcessLogfile(cwu, winfo, "Thor", tempDirName, request.hasLogsAccess); if (request.includeThorSlaveLog.isEmpty() || strieq(request.includeThorSlaveLog.str(), "on")) - createThorSlaveLogfile(cwu, winfo, tempDirName); + createThorSlaveLogfile(cwu, winfo, tempDirName, request.hasLogsAccess); #else readWULogToFiles(cwu, winfo, tempDirName, request); #endif - } //Write out to ZIP file zipAllZAPFiles(tempDirName, localFiles, request.password, zipFileNameWithFullPath); @@ -4573,12 +4587,24 @@ void CWsWuFileHelper::createZAPWUQueryAssociatedFiles(IConstWorkUnit* cwu, const cur.getName(name); cur.getIp(ip); - if (!hasLogsAccess && endsWith(name.str(), ".eclcc.log")) - continue; - RemoteFilename rfn; SocketEndpoint ep(ip.str()); rfn.setPath(ep, name.str()); + StringBuffer fileName(name.str()); + getFileNameOnly(fileName, false); + + VStringBuffer outFileName("%s%c%s", tempDirName, PATHSEPCHAR, fileName.str()); + + // Since users are only accustomed to seeing error messages replacing log file contents, + // special case an access denied error message for the one log file processed here. + if (!hasLogsAccess && endsWith(name.str(), ".eclcc.log")) + { + StringBuffer msg; + msg.appendf("Access to log files is denied. You are missing permission %s:READ - check with your system admin.", LOG_ACCESS_FEATURE); + writeToFile(outFileName.str(), msg.length(), msg.str()); + continue; + } + if (rfn.isLocal()) { localFiles.append(name.str()); @@ -4592,11 +4618,6 @@ void CWsWuFileHelper::createZAPWUQueryAssociatedFiles(IConstWorkUnit* cwu, const continue; } - StringBuffer fileName(name.str()); - getFileNameOnly(fileName, false); - - VStringBuffer outFileName("%s%c%s", tempDirName, PATHSEPCHAR, fileName.str()); - OwnedIFile outFile = createIFile(outFileName); if (!outFile) { diff --git a/esp/services/ws_workunits/ws_workunitsHelpers.hpp b/esp/services/ws_workunits/ws_workunitsHelpers.hpp index 15009d727f2..7dde648c6c1 100644 --- a/esp/services/ws_workunits/ws_workunitsHelpers.hpp +++ b/esp/services/ws_workunits/ws_workunitsHelpers.hpp @@ -383,16 +383,6 @@ struct CWsWuZAPInfoReq logFilter.populateLogFilter(wuid.str(), httpRequest); } - // True when logs _would have been included_ if the user had access - bool logsExcludedDueToNoAccess() - { -#ifdef _CONTAINERIZED - return (includeRelatedLogs || includePerComponentLogs) && !hasLogsAccess; -#else - // Bare metal historically always includes Thor and EclAgent logs - return !hasLogsAccess; -#endif - } }; class WsWuInfo @@ -908,8 +898,8 @@ class CWsWuFileHelper void createZAPWUQueryAssociatedFiles(IConstWorkUnit *cwu, const char *pathToCreate, StringArray &localFiles, bool hasLogsAccess); void createZAPWUGraphProgressFile(const char *wuid, const char *pathNameStr); #ifndef _CONTAINERIZED - void createProcessLogfile(IConstWorkUnit *cwu, WsWuInfo &winfo, const char *process, const char *path); - void createThorSlaveLogfile(IConstWorkUnit *cwu, WsWuInfo &winfo, const char *path); + void createProcessLogfile(IConstWorkUnit *cwu, WsWuInfo &winfo, const char *process, const char *path, bool hasLogsAccess); + void createThorSlaveLogfile(IConstWorkUnit *cwu, WsWuInfo &winfo, const char *path, bool hasLogsAccess); #endif LogAccessLogFormat getComponentLogFormatFromLogName(const char *log); void readWULogToFiles(IConstWorkUnit *cwu, WsWuInfo &winfo, const char *path, CWsWuZAPInfoReq &zapLogFilterOptions);