Generate bash and ansible remediation roles from profiles #143

matejak · 2017-10-11T15:26:53Z

Before this PR, S-W was able to remediate on the fly or remediate after the check using oscap.
This PR aims to enable users to control the remediation by giving them access to remediation scripts (that they can edit and execute by themselves).

EDIT by @mpreisler: Let's only do generate from profiles here and generate from results can be in a separate PR.

openscap-ci · 2017-10-11T15:26:54Z

Can one of the admins verify this patch?

matejak · 2017-10-11T15:29:22Z

The easier part has been done - you should be able to save remediations from the main app window.

mpreisler · 2017-10-11T15:31:00Z

include/MainWindow.h

        void toggleRuleResultsExpanded(bool checked);

+        /// Pops up a save dialog for a bash remediation
+        void genBash();


write a better method name please, it's worth it for self-documentation and editors auto-complete anyway.

also, can't we have just one method that gets "template" passed in? that way we can add even more remediation options easily later on.

I think that it is unavoidable to have multiple methods as one menu item requires one slot method. As noted in the code, there is a potential for code reuse between those methods. I am thinking of polymorphism and I would postpone this until the remediation from results is implemented, since there could be some code reuse from this side as well.

mpreisler · 2017-10-11T15:32:53Z

src/MainWindow.cpp

+    if (filename.isEmpty())
+        return;
+
+    int output_fd = open(filename.toUtf8(), O_CREAT|O_TRUNC|O_NOFOLLOW|O_WRONLY, 0700);


if at all possible use Qt abstractions, AFAIK one of them gives you the fd.

https://doc.qt.io/qt-4.8/qfile.html#handle

mpreisler · 2017-10-11T15:35:49Z

To avoid confusing users even more we have to stick to consistent terms. We use "remediation roles" in SCAP Security Guide and I think we should stick to it across all projects.

mpreisler · 2017-10-11T15:52:39Z

@openscap-ci add to whitelist

mpreisler · 2017-10-11T18:10:16Z

include/MainWindow.h

        void toggleRuleResultsExpanded();
        void toggleRuleResultsExpanded(bool checked);

        /// Pops up a save dialog for a bash remediation


since we talk about rule results just above this, let's make sure the comment mentions that these roles are generated for the currently selected profile, not for results

mpreisler · 2017-10-11T18:14:05Z

src/MainWindow.cpp


    // start centered
    move(QApplication::desktop()->screen()->rect().center() - rect().center());
+


should all of this really be done at the end of the ctor? Could you please improve the variable names? mGenMenu is not even a member and it starts with m. And the variable name is not descriptive.

Same for the other variables.

I basically repeated what I have seen in the ResultsViewer constructor. I don't know where else to put it. It seems that the pattern here is that the appearance of the form is fully determined after the ctor is executed, so from this POV, it makes sense to group the stuff that defines UI in there.

mpreisler · 2017-10-11T18:16:26Z

src/MainWindow.cpp

+
+// TODO: Reuse code between generateBashRemediationRole and generateAnsibleRemediationRole
+void MainWindow::generateBashRemediationRole()
+{


refactor this into a method that takes the short name "bash remediation role", extension ".sh" and OpenSCAP remediation template "urn:xccdf:...". Then the two generate functions can just call that. Shorter code is better.

I took slightly different approach, so I hope you like it too as it has the same effect.

mpreisler · 2017-10-11T18:21:29Z

ui/MainWindow.ui

             </item>
+             <item>
+              <widget class="QPushButton" name="genRemediationButton">
+               <property name="toolTip">


typo. again, please use consistent terms. It's a remediation role. It contains a fix for every single rule in the profile that has a fix.

mpreisler · 2017-10-11T18:21:57Z

ui/MainWindow.ui

+               <property name="toolTip">
+                <string>Generate remediations of all possible failiures that the selected profile can cover.</string>
+               </property>
+               <property name="text">


Generate remediation role?

mpreisler · 2017-10-11T18:25:54Z

src/MainWindow.cpp

+    QFile output_file(filename);
+    output_file.open(QIODevice::WriteOnly);
+    struct xccdf_session * session = mScanningSession->getXCCDFSession();
+    struct xccdf_policy *policy = xccdf_session_get_xccdf_policy(session);


code style for the above 2 lines. it sucks but code style in S-W is different from openscap

mpreisler · 2017-10-11T18:26:06Z

src/MainWindow.cpp

+    const QString filename = QFileDialog::getSaveFileName(this,
+        QObject::tr("Save as Ansible playbook"),
+        QObject::tr("remediate-profile.yml"),
+        QObject::tr("ansible playbooks (*.yml)"), 0


file format is singular: "ansible playbook", not "playbooks"

mpreisler · 2017-10-11T18:26:19Z

src/MainWindow.cpp

+    const QString filename = QFileDialog::getSaveFileName(this,
+        QObject::tr("Save as bash script"),
+        QObject::tr("remediate-profile.sh"),
+        QObject::tr("bash scripts (*.sh)"), 0


file format is singular: "bash script", not "scripts"

mpreisler · 2017-10-11T18:27:13Z

src/MainWindow.cpp

+    output_file.open(QIODevice::WriteOnly);
+    struct xccdf_session * session = mScanningSession->getXCCDFSession();
+    struct xccdf_policy *policy = xccdf_session_get_xccdf_policy(session);
+    xccdf_policy_generate_fix(policy, NULL, "urn:xccdf:fix:script:sh", output_file.handle());


error checking missing

mpreisler · 2017-10-11T18:27:58Z

src/MainWindow.cpp

+    if (filename.isEmpty())
+        return;
+
+    QFile output_file(filename);


code style (variable name)

mpreisler · 2017-10-11T20:11:38Z

src/MainWindow.cpp

+{
+    const QString filename = QFileDialog::getSaveFileName(this,
+        QObject::tr("Save as bash script"),
+        QObject::tr("remediate-profile.sh"),


Could we please base the suggested file name on the benchmark and profile ID? that would increase UX because many times I can just click save without typing anything.

I keep this in mind, but I have postponed it. Do you have any idea what is a good form? I am not sure about benchmark IDs, but those profile IDs alone are quite long.

mpreisler · 2017-10-12T15:06:44Z

The class doesn't follow code conventions and more importantly I think that's a sign of over-engineering. The class doesn't really hold or remember any state, it just runs something. In this case I think we should just use a function, it's going to be easier and shorter.

EDIT: I am willing to compromise on the class if you like that approach, I wouldn't implement it like that but it's fine by me.

mpreisler · 2017-10-12T15:18:55Z

include/RemediationRoleSaver.h

+	    mSaveMessage = QObject::tr("Save remediation role as a bash script");
+	    mFiletypeExtension = "sh";
+	    mFiletypeTemplate = QObject::tr("bash script (*.%1)");
+	    mFixTemplate = QObject::tr("urn:xccdf:fix:script:sh");


use initialization lists, do this in the cpp file, not in the header.

mpreisler · 2017-10-12T16:40:42Z

src/RemediationRoleSaver.cpp

+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors:
+ *      Martin Preisler <mpreisle@redhat.com>


ehm... nope :)

mpreisler · 2017-10-12T16:40:52Z

include/RemediationRoleSaver.h

 {
    public:
-	RemediationSaverBase(QWidget* parentWindow, ScanningSession* session): mParentWindow(parentWindow), mScanningSession(session) {}
+    RemediationSaverBase(QWidget* parentWindow, ScanningSession* session): mParentWindow(parentWindow), mScanningSession(session) {}


indents borked

mpreisler · 2017-10-12T16:41:36Z

src/RemediationRoleSaver.cpp

+
+
+BashRemediationSaver::BashRemediationSaver(QWidget* parentWindow, ScanningSession* session):RemediationSaverBase(parentWindow, session)
+{


split the line above into multiple lines, break after : makes it more readable, indent the initialization list items and keep them one per line.

mpreisler · 2017-10-12T16:41:50Z

src/RemediationRoleSaver.cpp

+    mSaveMessage = QObject::tr("Save remediation role as a bash script");
+    mFiletypeExtension = "sh";
+    mFiletypeTemplate = QObject::tr("bash script (*.%1)");
+    mFixTemplate = QObject::tr("urn:xccdf:fix:script:sh");


any reason why this is not initialized in the initialization list?

It belongs to the base class and the base class doesn't know what should the content be.
Having it in the cpp file like this has the added benefit that If there is a new format of remediation rules, the person who will implement it will immediately know what is the meaning of each individual string thanks to the fact that it is assigned to the variable as opposed to being specified as "the 3rd argument to the constructor".

ah, ok, I didn't realize you are setting parent class members in the ctor

mpreisler · 2017-10-12T16:52:25Z

src/MainWindow.cpp

    );
 }
+
+// TODO: Reuse code between generateBashRemediationRole and generateAnsibleRemediationRole


outdated comment

mpreisler · 2017-10-12T16:52:40Z

src/RemediationRoleSaver.cpp

+    if (filename.isEmpty())
+        return;
+
+    int result = SaveToFile(filename);


can be const

mpreisler · 2017-10-12T16:52:46Z

src/RemediationRoleSaver.cpp

+
+#include "RemediationRoleSaver.h"
+
+void RemediationSaverBase::SelectFilenameAndSaveRole()


method name

mpreisler · 2017-10-12T16:52:56Z

src/RemediationRoleSaver.cpp

+    }
+}
+
+int RemediationSaverBase::SaveToFile(const QString& filename)


method name doesn't follow coding style

mpreisler · 2017-10-12T16:53:07Z

src/RemediationRoleSaver.cpp

+    return result;
+}
+
+QString RemediationSaverBase::guessFilenameStem()


this method can be const

mpreisler · 2017-10-12T17:53:13Z

include/RemediationRoleSaver.h

+{
+    public:
+        RemediationSaverBase(QWidget* parentWindow, ScanningSession* session):
+            mParentWindow(parentWindow), mScanningSession(session) {}


move ctor to cpp

mpreisler · 2017-10-12T17:56:19Z

src/MainWindow.cpp

+    remediationButtonMenu->addAction(genBashRemediation);
+    remediationButtonMenu->addAction(genAnsibleRemediation);
+    // remediationButtonMenu->addAction(mGenPuppetRemediation);
+    // remediationButtonMenu->addAction(mGenAnacondaRemediation);


the puppet command probably makes sense but anaconda roles don't have a use-case. I suggest to uncomment what's supposed to be there and remove anaconda action for good.

mpreisler · 2017-10-12T17:57:20Z

src/RemediationRoleSaver.cpp

+    outputFile.open(QIODevice::WriteOnly);
+    struct xccdf_session* session = mScanningSession->getXCCDFSession();
+    struct xccdf_policy* policy = xccdf_session_get_xccdf_policy(session);
+    int result = xccdf_policy_generate_fix(policy, NULL, mFixTemplate.toUtf8(), outputFile.handle());


since we are writing C++ we should handle C errors where we use C and only there IMO. Let's check the error here and raise an exception. Leaking error codes outside of methods makes it a pain to use the API.

mpreisler · 2017-10-19T15:10:21Z

ACK

Implemented profile-based generation of remediations.

20e6b3d

mpreisler reviewed Oct 11, 2017

View reviewed changes

mpreisler changed the title ~~Implement generation of remediation scripts.~~ Generate bash and ansible remediation roles from profiles Oct 11, 2017

mpreisler added this to the 1.1.6 milestone Oct 11, 2017

Made style fixes, replaced C idioms with QT.

42693c0

mpreisler reviewed Oct 11, 2017

View reviewed changes

src/MainWindow.cpp Outdated

if (filename.isEmpty())

return;

QFile output_file(filename);

Copy link

Member

mpreisler Oct 11, 2017

Choose a reason for hiding this comment

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

code style (variable name)

mpreisler reviewed Oct 11, 2017

View reviewed changes

Improved code structure.

a043ac3

mpreisler reviewed Oct 12, 2017

View reviewed changes

Fixed some issues with code style.

ffefb73

mpreisler reviewed Oct 12, 2017

View reviewed changes

src/RemediationRoleSaver.cpp Outdated

if (filename.isEmpty())

return;

int result = SaveToFile(filename);

Copy link

Member

mpreisler Oct 12, 2017

Choose a reason for hiding this comment

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

can be const

mpreisler reviewed Oct 12, 2017

View reviewed changes

src/RemediationRoleSaver.cpp Outdated

return result;

}

QString RemediationSaverBase::guessFilenameStem()

Copy link

Member

mpreisler Oct 12, 2017

Choose a reason for hiding this comment

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

this method can be const

Introduced another coding style fixes.

eee7704

mpreisler reviewed Oct 12, 2017

View reviewed changes

matejak added 2 commits October 19, 2017 14:28

Added Puppet profile-base remediations, improved code style.

bf382e2

Converted saveToFile to a proper procedure.

661ca24

mpreisler merged commit 892552d into OpenSCAP:v1-1 Oct 19, 2017

mpreisler self-assigned this Oct 19, 2017

mpreisler added the enhancement label Oct 19, 2017

matejak mentioned this pull request Oct 25, 2017

Result-based generation of remediation roles #144

Merged

jan-cerny mentioned this pull request Nov 7, 2017

RFE: Generate remediation roles from profiles #125

Closed


		// start centered
		move(QApplication::desktop()->screen()->rect().center() - rect().center());



		BashRemediationSaver::BashRemediationSaver(QWidget* parentWindow, ScanningSession* session):RemediationSaverBase(parentWindow, session)
		{


		#include "RemediationRoleSaver.h"

		void RemediationSaverBase::SelectFilenameAndSaveRole()

Generate bash and ansible remediation roles from profiles #143

Generate bash and ansible remediation roles from profiles #143

Uh oh!

Conversation

matejak commented Oct 11, 2017 • edited by mpreisler Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openscap-ci commented Oct 11, 2017

Uh oh!

matejak commented Oct 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matejak Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpreisler commented Oct 11, 2017

Uh oh!

mpreisler commented Oct 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpreisler Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpreisler Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpreisler commented Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matejak Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matejak commented Oct 11, 2017 •

edited by mpreisler

Loading

matejak Oct 11, 2017 •

edited

Loading

mpreisler Oct 11, 2017 •

edited

Loading

mpreisler Oct 11, 2017 •

edited

Loading

mpreisler commented Oct 12, 2017 •

edited

Loading

matejak Oct 12, 2017 •

edited

Loading