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.

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

NR-78587 Newer versions of semodule tool do not display the SELinux modules version #1940

Merged
merged 11 commits into from
Oct 25, 2024

Conversation

sairaj18
Copy link
Contributor

@sairaj18 sairaj18 commented Oct 23, 2024

Changes in this PR

  • Populating the value of versions as empty string when semodule -l doesn't give versions for the modules in the output
  • Added test to verify the above

Copy link
Contributor

@DavSanchez DavSanchez left a comment

Choose a reason for hiding this comment

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

Hi! As we discussed with @josemore, it might be worth it to get the list of SELinux modules anyway on RHEL 8+, even if we couldn't retrieve the version numbers anymore, as the module list could still be valuable (i.e. it's better to have a SELinux module list with no versions than no list at all).

So, we can instead run the semodule -l command the same as before, but make the version numbers optional.

We can modify the regexp we use in internal/plugins/linux/selinux.go:160 to make the second capture group optional:

	// Output of `semodule -l` might not include the version number in modern versions of RHEL
	// and derivatives. So we make the second capture group optional. The capture that would
	// match the version numbers is either not present or produces an empty string instead.
	moduleRegex := regexp.MustCompile(`(\S+)(?:\s+(\S+))?`)

or like this

        // Capture "zero-or-more elements" of whitespaces and non-whitespaces for the optional version field
	moduleRegex := regexp.MustCompile(`(\S+)\s*(\S*)`)

We might want to guard ourselves against the capture groups not having the number of matches we expect, just to be extra sure. We can do this in parseSemoduleOutput with:

        const captureGroupsMinLen = 2
        // ...
	moduleMatches := moduleRegex.FindAllStringSubmatch(line, -1)
		if moduleMatches != nil {
			id := moduleMatches[0][1]

			// Guard against the second capture group not existing
			version := ""

			if len(moduleMatches[0]) > captureGroupsMinLen {
				version = moduleMatches[0][2]
			}

			result = append(result, SELinuxPolicyModule{id, version})
		}

We should have some tests to cover the new scenarios. Similar to the ones you already added @sairaj18 but this time they should not return errors but valid SELinuxPolicyModules with version == "".

As discussed, printing a log line if the version is not found for each module is not worth it, we might leave this without logs for now or, if we want the logs anyway in the future, we can attempt an additional iteration over result after appending all elements, check if any version number is "", and print a log warn/debug saying that version numbers couldn't be retrieved.

@sairaj18
Copy link
Contributor Author

Hey @DavSanchez, I have made the changes that you have mentioned the above comment and pushed those changes.

@sairaj18 sairaj18 marked this pull request as ready for review October 24, 2024 13:24
@sairaj18 sairaj18 requested a review from a team October 24, 2024 13:24
@sairaj18 sairaj18 force-pushed the NR-78587-fix-semodule-rhel8 branch from 809ed5b to 24c4f52 Compare October 25, 2024 06:25
Copy link
Contributor

@DavSanchez DavSanchez left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@sairaj18 sairaj18 merged commit feac5b8 into master Oct 25, 2024
9 checks passed
@sairaj18 sairaj18 deleted the NR-78587-fix-semodule-rhel8 branch October 25, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants