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

[BugFix] [RHEL/6] Fixes scapval failures related to xccdf:check (issue #1037, issue #1038) #1050

Merged
merged 6 commits into from
Feb 24, 2016

Conversation

iankko
Copy link

@iankko iankko commented Feb 21, 2016

This is version 2 of a patchset to fix the following issues:

Basically is a replacement of / reimplementation of #1039 using @isimluk 's hint from #1039 (comment) and reducing the count of necessary changes to the minimum, so the produced XCCDF / DataStream files would contain official OCIL-2.0 as a check system.

Please review.

Thank you, Jan.

Jan Lieskovsky added 4 commits February 21, 2016 21:35
compare XCCDF ID for match with both OVAL and OCIL IDs
with EXSLT date:date-time() which returns timestamp in the format
of xs:dateTime (see http://exslt.org/date/functions/date-time/ )

This fixes errors like:

File '/root/scap-security-guide/RHEL/6/output/ssg-rhel6-ds.xml'
line 37333: Element '{http://scap.nist.gov/schema/ocil/2.0}timestamp':
'2016-02-11-05:00' is not a valid value of the atomic type 'xs:dateTime'.
script:

* Replace 'ocil-transitional' check system in 'get_ovalfiles()'
  routine with official OCIL-2.0 check system,

* When performing the verification if all XCCDF rules reference
  valid OVAL checks skip <check-content-ref> elements having
  OCIL-2.0 as the check-system (since we are verifying sanity
  of XCCDF vs OVAL IDs here)
from intermediary XML having OCIL checks already expanded for official
OCIL-2.0

Fixes:
* ComplianceAsCode#1037
* ComplianceAsCode#1038
@iankko
Copy link
Author

iankko commented Feb 21, 2016

Again, it's expected this change to fail on el6 Jenkins node (since RHEL/7 make validate target is failing with current master on that system).

Change:
    #1048

needs to be merged first prior this can start passing on RHEL-6.

Once #1048 is merged, the testing of this one should be rescheduled && can be reviewed (in that moment Jenkins test will start reporting behaviour of changes relevant to this PR).

@iankko
Copy link
Author

iankko commented Feb 22, 2016

re[test this please]

@iankko
Copy link
Author

iankko commented Feb 22, 2016

Simon, could you have a look @ this one? (this if v_2 of #1039 containing just necessary changes on top of using ocilrefs.xml with expanded OCILs).

Thank you, Jan!

@isimluk isimluk changed the title [BugFix] [RHEL/6] Fixes for issue #1037 and issue #1038 [BugFix] [RHEL/6] Fixes scapval failures related to xccdf:check (issue #1037, issue #1038) Feb 22, 2016
@@ -88,7 +88,7 @@ $(OUT)/bash-remediations.xml: $(SHARED)/$(TRANS)/combineremediations.py $(bash_r
$(SHARED)/$(TRANS)/combineremediations.py $(PROD) $(BUILD_REMEDIATIONS) $(OUT)/bash-remediations.xml

# Common build targets - an XCCDF with remediations but not linked to OVAL yet
$(OUT)/xccdf-unlinked-withremediations.xml: $(OUT)/xccdf-unlinked-resolved.xml $(OUT)/xccdf-unlinked-ocilrefs.xml $(OUT)/bash-remediations.xml $(SHARED)/$(TRANS)/xccdf-addremediations.xslt
$(OUT)/xccdf-unlinked-withremediations.xml: $(OUT)/xccdf-unlinked-ocilrefs.xml $(OUT)/xccdf-unlinked-resolved.xml $(OUT)/bash-remediations.xml $(SHARED)/$(TRANS)/xccdf-addremediations.xslt
Copy link
Member

Choose a reason for hiding this comment

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

Why is the order of the 2 deps switched around? I am probably missing something.

Copy link
Author

Choose a reason for hiding this comment

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

Because $(OUT)/xccdf-unlinked-resolved.xml doesn't contain ocil checks resolved to official OCIL-2.0 ones (there's still this ocil-transitional check system). While $(OUT)/xccdf-unlinked-ocilrefs.xml contains them already resolved (there's this $ocil_cs URL used as checksystem).

The point of this fix being NIST SCAP content validation test suite doesn't allow different check systems other than OVAL and OCIL. Since ocil-transitional is just SSG construct, we change the order to include remediations into that XCCDF, which is using official OCIL-2.0 check system (to silent the NIST suite).

Of course, we could simply remove the mentions about ocil-transitional, but why to do that, when people might be accustomed to use it somehow. And at some point in the future, we in any case might want to support / produce valid OCIL.

So that's the reason (see also #1039 (comment) for the history how this evolved. [Original plan was to replace ocil-transitional with official OCIL-2.0. But this has shown not to be necessary, since SSG already produces XCCDF with valid OCIL -- so couple of patches from previous version have shown to be worthless]).

Copy link
Member

Choose a reason for hiding this comment

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

Hi Jan, I understand that but it doesn't answer my question.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, so is the question why $(OUT)/xccdf-unlinked-resolved.xml wasn't deleted? (rather than moving backward)

Copy link
Member

Choose a reason for hiding this comment

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

Even if we keep it at this stage this whole hunk does nothing.

It used to be X has to be newer than A and B. You changed it to X has to be newer than B and A.

That's my question.

Copy link
Author

Choose a reason for hiding this comment

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

Hi Martin,

@mpreisler

Even if we keep it at this stage this whole hunk does nothing.

That's not completely true. Let me explain below.

It used to be X has to be newer than A and B. You changed it to X has to be newer than B and A.

Correct. But this is not the sole effect of that change (and actually isn't the motivation why that change was performed).

What the change does also is it changes the source file, from which the $(OUT)/xccdf-unlinked-withremediations.xml intermediary target will be created. Before first prerequisite ($<) was set to $(OUT)/xccdf-unlinked-resolved.xml. The aforementioned change is moving it to be $(OUT)/xccdf-unlinked-ocilrefs.xml just by flipping the order of prerequisites (I didn't want to delete the other $(OUT)/xccdf-unlinked-resolved.xml just for the case it might come handy in the future due some reason).

This automatic Makefile variables syntax:
    [1] https://www.gnu.org/software/make/manual/html_node/Automatic-Variables.html

was already present as convention in shared/product-make.include thus when making this change I didn't want to move away from it and use direct $(OUT)/ocil-unlinked.xml reference / name instead.

But I will add a comment (probably even replace $< automatic variable with explicit name of first prerequisite) so the purpose of this change is more explicit / clear.

That's my question.

Thanks for explanation. Makes sense now.

Jan.

@isimluk isimluk removed their assignment Feb 23, 2016
Jan Lieskovsky added 2 commits February 24, 2016 12:10
…unlinked-ocilrefs.xml"

so the motivation behind the change is immediately clear

(Update WRT to ComplianceAsCode#1050 (comment) )
Modify the output error message shown depending on the fact if OVAL
or OCIL ID didn't match the XCCDF ID

(Update per: ComplianceAsCode#1050 (diff))
@iankko
Copy link
Author

iankko commented Feb 24, 2016

@mpreisler
Both of the changes / updates should be performed now. Would you mind re(review) this one?

Thanks a lot! Jan

@mpreisler
Copy link
Member

There are some very minor issues in the PR still but I think it's better merged and fixed later.

LGTM, ACK.

mpreisler added a commit that referenced this pull request Feb 24, 2016
[BugFix] [RHEL/6] Fixes scapval failures related to xccdf:check (issue #1037, issue #1038)
@mpreisler mpreisler merged commit 5ba7a7f into ComplianceAsCode:master Feb 24, 2016
@iankko
Copy link
Author

iankko commented Feb 24, 2016

@mpreisler

There are some very minor issues in the PR still but I think it's better merged and fixed later.
LGTM, ACK.

Thanks, Martin! Much appreciated.

Once you found time, please file those issues as new tickets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes to reported bugs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants