Compliance/procedures pt1 #881

wupeka · 2025-02-21T08:08:18Z

Description

First set of compliance procedures - testing procedures, ensureFilesystemOptions, ensureFilePermissions.
FilePermissions procedures are missing tests.

Checklist

I have read the contribution guidelines.
I added unit-tests to validate my changes. All unit tests are passing.
I have merged the latest dev branch prior to this PR submission.
I ran pre-commit on my changes prior to this PR submission.
I submitted this PR against the dev branch.

github-actions · 2025-02-21T08:13:35Z

Test Results

44 files 44 suites 40m 53s ⏱️
4 tests 4 ✅ 0 💤 0 ❌
176 runs 154 ✅ 22 💤 0 ❌

Results for commit 791ffa3.

♻️ This comment has been updated with latest results.

robertwoj-microsoft · 2025-02-21T09:36:28Z

src/modules/compliance/src/lib/CMakeLists.txt

@@ -16,6 +18,13 @@ add_library(compliancelib STATIC

 set_property(TARGET compliancelib PROPERTY POSITION_INDEPENDENT_CODE ON)

+include(FindPerl)
+add_custom_command(
+    OUTPUT ProcedureMap.cpp ProcedureMap.h


make clean will drop these files

src/modules/compliance/src/lib/GenProcedureMap.py

robertwoj-microsoft · 2025-02-21T09:43:16Z

src/modules/compliance/src/lib/procedures/ensureFilePermissions.cpp

+
+namespace compliance {
+
+AUDIT_FN(ensureFilePermissions)


Could we get rid of these macros? it reads as

Suggested change

AUDIT_FN(ensureFilePermissions)

Result<bool> Audit_ensureFilePermissions(std::map<std::string, std::string> args, std::ostringstream &logstream)

robertwoj-microsoft · 2025-02-21T09:43:35Z

src/modules/compliance/src/lib/procedures/ensureFilePermissions.cpp

+
+AUDIT_FN(ensureFilePermissions)
+{
+    struct stat statbuf;


I think we can skip the struct keyword here

src/modules/compliance/src/lib/procedures/ensureFilePermissions.cpp

robertwoj-microsoft · 2025-02-21T12:22:31Z

src/modules/compliance/src/lib/procedures/testingProcedures.cpp

+#include <CommonUtils.h>
+
+namespace compliance {
+REMEDIATE_FN(remediationFailure)


We have the following define:

if (BUILD_TESTS) add_compile_options(-D TEST_CODE) add_subdirectory(tests) endif()

Do you think it makes sense add these functions only when TEST_CODE is defined?

We will be using those procedures for end-to-end tests etc, the costs of having them compiled in is neglible, so I'd keep them.

src/modules/compliance/src/lib/CMakeLists.txt

AhmedBM · 2025-02-24T21:46:29Z

src/modules/compliance/src/lib/GenProcedureMap.pl

@@ -0,0 +1,99 @@
+#!/usr/bin/env perl


Can we not just use bash for this? Bringing in Perl just for this seems like overkill

Perl is currently in all of our build environments. (unfortunately, Python is not).

We haven't decided finally yet, but we'll probably pre-generate the ProcedureMap and put it in the repo, so this script won't be launched at build time - only manually when someone adds/removes a procedure. For now, with the amount of churn, it's easier to just have it automated.

Ohh nice, i didn't realize the environments already contained Perl, i just validated that ubun14.04/debian-10 have it 👌

src/modules/compliance/src/lib/procedures/ensureFilesystemOption.cpp

AhmedBM · 2025-02-24T21:54:30Z

src/modules/compliance/src/lib/procedures/ensureFilesystemOption.cpp

+
+        fstabMap[mnt->mnt_dir] = entry;
+    }
+    fclose(file);


ignoring fclose return value? Do we care if we cannot close file?

It's open for read only, so we don't care (and it should never fail in those circumstances).

AhmedBM · 2025-02-24T21:56:05Z

src/modules/compliance/src/lib/procedures/ensureFilePermissions.cpp

+        struct passwd *pwd = getpwuid(statbuf.st_uid);
+        if (nullptr == pwd)
+        {
+            logstream << "No user with uid " << statbuf.st_uid;


why are we not using the OSCONFIG_LOG_INFO/OSCONFIG_LOG_ERROR macros here?

The logstream is used to create a string with the result of the audit for the UI. It is then also logged to OSCONFIG_LOG.

When you say the UI, do you mean this is whats displayed in the non-compliance reason in the portal? In the end however, wouldn't this string be a very long concatenated string, im not seeing any newlines?

Unfortunately - yes, that's the only thing that we can (at least for now) present to the user.
It usually won't be that complex though.

AhmedBM · 2025-02-24T22:01:30Z

src/modules/compliance/tests/CMakeLists.txt

@@ -5,6 +5,7 @@ project(compliancetests)

 include(CTest)
 find_package(GTest REQUIRED)
+FILE(GLOB PROCEDURES procedures/*.cpp)


Im generally against CMake file GLOBS because CMake cannot track changes and re-run if there are new/removed files but i believe these procedures are generated dynamically? If so, i understand the need to

Procedures are not generated dynamically, but at this stage we expect to have a lot of churn both here and in src/modules/compliance/src/lib/procedures, and we want to parallelilze the work on different procedures in different files. Having a glob means we don't need to change CMakeLists.txt with every new file, so we won't have conflicts. In the end we'll just put a static list there. (The same rationale applies to generating ProcedureMap at build time)

i see, can you add a comment above to state that the GLOB is temporary until we stabilize?

AhmedBM

If Perl is being added as a build requirement, we would need to update every build container to include it. These are located in devops/docker

Makes me wonder though, can we not just do this in bash?

Signed-off-by: Krzysztof Kanas <kkanas@microsoft.com>

Fix logs from: MmiGet(0x7ecdec017510, Compliance, auditEnsureAccessToEtcMotdIsConfigured, "{ allOf: [{ ensureFilePermissions: ensureFilePermissions for /etc/motdStat errorNo such file or directory } == FALSE] == FALSE }") BaselineMmiGet(Compliance, auditEnsureAccessToEtcMotdIsConfigured): '"{ allOf: [{ ensureFilePermissions: ensureFilePermissions for /etc/motdStat errorNo such file or directory } == FALSE] == FALSE }"' (130) To: MmiGet(0x7cf35c02bf50, Compliance, auditEnsureAccessToEtcMotdIsConfigured, "{ allOf: [{ ensureFilePermissions: ensureFilePermissions for '/etc/motd' Stat error 'No such file or directory' } == FALSE] == FALSE }") BaselineMmiGet(Compliance, auditEnsureAccessToEtcMotdIsConfigured): '"{ allOf: [{ ensureFilePermissions: ensureFilePermissions for '/etc/motd' Stat error 'No such file or directory' } == FALSE] == FALSE }"' (136) Signed-off-by: Krzysztof Kanas <kkanas@microsoft.com>

Signed-off-by: Krzysztof Kanas <kkanas@microsoft.com>

This reverts commit 94c6b78.

wupeka requested a review from a team as a code owner February 21, 2025 08:08

wupeka force-pushed the compliance/procedures-pt1 branch 2 times, most recently from fa2fa36 to b1ace1a Compare February 21, 2025 09:02

robertwoj-microsoft reviewed Feb 21, 2025

View reviewed changes

src/modules/compliance/src/lib/GenProcedureMap.py Outdated Show resolved Hide resolved

robertwoj-microsoft reviewed Feb 21, 2025

View reviewed changes

src/modules/compliance/src/lib/procedures/ensureFilePermissions.cpp Outdated Show resolved Hide resolved

robertwoj-microsoft reviewed Feb 21, 2025

View reviewed changes

AhmedBM reviewed Feb 24, 2025

View reviewed changes

src/modules/compliance/src/lib/CMakeLists.txt Outdated Show resolved Hide resolved

AhmedBM reviewed Feb 24, 2025

View reviewed changes

src/modules/compliance/src/lib/procedures/ensureFilesystemOption.cpp Outdated Show resolved Hide resolved

AhmedBM reviewed Feb 24, 2025

View reviewed changes

wupeka force-pushed the compliance/procedures-pt1 branch 3 times, most recently from e369a13 to ca00f6e Compare February 25, 2025 13:40

wupeka and others added 10 commits February 28, 2025 12:49

Basic procedures for testing

c393aad

ensureFilePermissions audit and remediation procedures

1b11a4f

ensureFilesystemOption procedures and tests

d03a4fd

WiP packageInstalled audit

db68ec5

An example MOF and ZIP generator

4206805

Use static ComplianceExample.mof, but keep the generator

cb5ea43

Evaluator: add log key to log when parser json error

0381df9

Signed-off-by: Krzysztof Kanas <kkanas@microsoft.com>

Post-review fix

0607aba

Fix permissions/mask in ensureFilePermissions

9069c74

wupeka and others added 6 commits February 28, 2025 12:50

Allow for multiple groups in ensureFilePermissions

dfde583

base64: impove error reporting

82deab1

Signed-off-by: Krzysztof Kanas <kkanas@microsoft.com>

Add a test for non-alnum characters in base64

c172916

Revert "base64: impove error reporting"

0986cda

This reverts commit 94c6b78.

Handle json-encoded string as a parameter to MmiSet properly

836bd09

Test for procedure containing '/' in b64 encoded form

791ffa3

wupeka force-pushed the compliance/procedures-pt1 branch from ae9d7f5 to 791ffa3 Compare February 28, 2025 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compliance/procedures pt1 #881

Compliance/procedures pt1 #881

wupeka commented Feb 21, 2025

github-actions bot commented Feb 21, 2025 •

edited

Loading

robertwoj-microsoft Feb 21, 2025

robertwoj-microsoft Feb 21, 2025

robertwoj-microsoft Feb 21, 2025

robertwoj-microsoft Feb 21, 2025

wupeka Feb 25, 2025

AhmedBM Feb 24, 2025

wupeka Feb 25, 2025

AhmedBM Feb 25, 2025

AhmedBM Feb 24, 2025 •

edited

Loading

wupeka Feb 25, 2025

AhmedBM Feb 24, 2025

wupeka Feb 25, 2025

AhmedBM Feb 25, 2025

wupeka Feb 27, 2025

AhmedBM Feb 24, 2025

wupeka Feb 25, 2025

AhmedBM Feb 25, 2025

wupeka Feb 27, 2025

AhmedBM left a comment

	AUDIT_FN(ensureFilePermissions)
	Result<bool> Audit_ensureFilePermissions(std::map<std::string, std::string> args, std::ostringstream &logstream)

Compliance/procedures pt1 #881

Are you sure you want to change the base?

Compliance/procedures pt1 #881

Conversation

wupeka commented Feb 21, 2025

Description

Checklist

github-actions bot commented Feb 21, 2025 • edited Loading

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AhmedBM Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AhmedBM left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 21, 2025 •

edited

Loading

AhmedBM Feb 24, 2025 •

edited

Loading