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

Add answerfile SELinux mode option #61

Closed

Conversation

lbgracioso
Copy link
Collaborator

This PR gives the user the ability to select a SELinux mode on the headnode.

Copy link

sonarcloud bot commented Jun 4, 2024

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@lbgracioso lbgracioso added the enhancement New feature or request label Jun 5, 2024
@viniciusferrao
Copy link
Owner

Ok for adding the option to change, but adding the option requires that SELinux actually works.

It should also include the SELinux policies for all the packages that the system installs, so finally you could add a switch to enable it.

For now this one is incomplete.

@viniciusferrao viniciusferrao marked this pull request as draft June 18, 2024 17:09
@lbgracioso
Copy link
Collaborator Author

@viniciusferrao Please check it again.

@lbgracioso lbgracioso marked this pull request as ready for review July 8, 2024 18:16
@@ -169,6 +169,19 @@ void AnswerFile::loadHostnameSettings()
= m_ini.getValue("hostname", "domain_name", false, false);
}

Cluster::SELinuxMode AnswerFile::checkSELinuxMode(const std::string& mode)
{
Copy link
Owner

Choose a reason for hiding this comment

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

This should be a switch case statement.

return Cluster::SELinuxMode::Enforcing;
} else if (mode == "permissive") {
return Cluster::SELinuxMode::Permissive;
} else if (mode == "disabled") {
Copy link
Owner

Choose a reason for hiding this comment

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

We should have a method to ignore case when comparing strings. It will avoid bugs where the user selects Disabled and disabled. That should not be an issue in a TUI interface, but on an answerfile it is.

// SELinux policies
if (getSELinux() != SELinuxMode::Disabled) {
SELinux SELinuxConfigurer;
SELinuxConfigurer.configureProvisioner(getProvisioner());
Copy link
Owner

Choose a reason for hiding this comment

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

Configurer is a very bad word. Usually the term used is Setup.

@@ -6,6 +6,7 @@
#ifndef CLOYSTERHPC_ANSWERFILE_H_
#define CLOYSTERHPC_ANSWERFILE_H_

#include "cluster.h"
Copy link
Owner

Choose a reason for hiding this comment

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

< >

#ifndef CLOYSTERHPC_SELINUX_H_
#define CLOYSTERHPC_SELINUX_H_

#include "cloysterhpc/cluster.h"
Copy link
Owner

Choose a reason for hiding this comment

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

< >

@@ -20,6 +20,7 @@
#include <regex>

#ifndef NDEBUG
#include "cloysterhpc/selinux/selinux.h"
Copy link
Owner

Choose a reason for hiding this comment

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

Why it's inside NDEBUG?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IDE did it when included. I'll change.

"restorecon -R /install && "
"rm -rf /root/xCAT-httpd-read-tftpdir*";

cloyster::runCommand(combinedCommands);
Copy link
Owner

Choose a reason for hiding this comment

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

Does this work? && is a shell thing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whats wrong with it?

cloyster::setFilePermissions(file);

// Install SELinux policy file for xCAT
std::string combinedCommands
Copy link
Owner

Choose a reason for hiding this comment

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

Missing constexpr.

void SELinux::configurexCATPolicyFile()
{
// Create SELinux policy file for xCAT
const std::string file = "/root/xCAT-httpd-read-tftpdir.te";
Copy link
Owner

Choose a reason for hiding this comment

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

Do we have a /opt/cloysterhpc directory right? We should use it instead of /root.

cloyster::runCommand(combinedCommands);
}

void SELinux::configurexCATrsyncPolicyFile()
Copy link
Owner

Choose a reason for hiding this comment

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

I really dislike this naming, it looks like procedural code. Because the function name is too long and too specific it's looks like a function, and not a method in the way we refer in OOP.

To be clear, the issue isn't exactly the name, the name is the result of the issue. That's not done as an OOP code.

@lbgracioso
Copy link
Collaborator Author

This project should be discussed in a audio call in order to fix some doubts and troubles. When possible, please contact me and let's call.

@lbgracioso lbgracioso closed this Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants