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

More precise modified time comparison in "configure_crypto_policy" #6437

Conversation

yuumasato
Copy link
Member

@yuumasato yuumasato commented Nov 27, 2020

Description:

  • Change the way we compare the timestamps of /etc/crypto-policies/config and /etc/crypto-policies/state/current to be a bit more precise.
    • Instead of comparing the difference between each file and current time, and ensuring that time difference to config file is greater (i.e. config file was changed before current file).
    • Let's directly compare their timestamp (in seconds since epoch), and ensure that config timestamp is less than current timestamp.

Rationale:

  • There is no way to calculate the time_difference against the same moment in time.

Calculating the time difference of both files against current time and
comparing them is imprecise, as both calculations of the time difference
can not be done simultaneously.
One of the files will always appear to be a bit older than the other.

If both files were generated one right after the other (i.e. on the
same second), but calculation of difference against current time was
done in different units of "second". The test will result in a false
negative.

Lets just compare their age in seconds since epoch directly.
@yuumasato yuumasato changed the title More precise modified time comparison in "configure_crypto_polic" More precise modified time comparison in "configure_crypto_policy" Nov 27, 2020
@openscap-ci
Copy link
Collaborator

openscap-ci commented Nov 27, 2020

Changes identified:
Rules:
 configure_crypto_policy
 enable_fips_mode

Show details

Rule configure_crypto_policy:
 Node moved within OVAL check.
 Deleted attribute from OVAL check.
 Text changed in OVAL check.
 Attribute value changed in OVAL check.
 Node deleted from OVAL check.
Rule enable_fips_mode:
 enable_fips_mode is affected by configure_crypto_policy change.

Recommended tests to execute:
 build_product rhel8
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using bash --datastream build/ssg-rhel8-ds.xml enable_fips_mode
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using bash --datastream build/ssg-rhel8-ds.xml configure_crypto_policy

#!/bin/bash
# platform = Red Hat Enterprise Linux 8

# This is false negative scenario
Copy link
Member Author

Choose a reason for hiding this comment

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

@matusmarhefka @mildas Do you see value in this test scenario?
Strictly speaking, this should be a fail scenario. But AFAIK, there is no way to detect this, as smallest time unit in OVAL is one second.

Copy link
Member

Choose a reason for hiding this comment

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

I see, if we want to fake the files to have the same modification timestamps I would do the following trick:

update-crypto-policies --set "DEFAULT"
CDATE=$(date +%Y%m%d%H%M)
touch -a -m -t "$CDATE" /etc/crypto-policies/config /etc/crypto-policies/state/current

Copy link
Member

Choose a reason for hiding this comment

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

This is strictly to just test the situation when file modification time is the same, but I understand that this is OVAL limitation and we cannot really catch situations when modification time difference is less than 1s.

Copy link
Member Author

@yuumasato yuumasato Nov 30, 2020

Choose a reason for hiding this comment

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

Thanks for the tip for the timestamp, I'll add this to the test scenario.
But my concern was more about whether we should keep a test like this, to illustrate the situation we cannot cover well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what you mean with false negative scenario in this case (shouldn't it be false positive? but probably I'm just confused). However, I like this test scenario and I see value in it because we can test that this specific case is passing and not causing failures from <1sec roundings.

The next comment about precision of seconds is very important there and probably I would add to it something like WARNING: or IMPORTANT: just to emphasize its importance.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mildas Hmm, I think it is a false negative, as it reports a no-issue situation where there is one.

However, I like this test scenario and I see value in it because we can test that this specific case is passing and not causing failures from <1sec roundings.

I think the rounding issue is solved in 3df3833, it was caused by comparing the timestamp of the file to the current time.

In this test scenario, the <1sec difference is an actual issue.

In this scenario, the test should return fail, but returns pass.
The smallest unit of time in OVAL is one second, there is no way to
differentiate files that were changed less than one second appart
from each other.
@yuumasato yuumasato force-pushed the more_precise_time_diff_in_crypto_policies branch from 1291277 to 48e2b97 Compare December 4, 2020 14:02
@jan-cerny
Copy link
Collaborator

/test e2e-aws-ocp4-cis-node

1 similar comment
@redhatrises
Copy link
Contributor

/test e2e-aws-ocp4-cis-node

@carlosmmatos carlosmmatos merged commit c8c061d into ComplianceAsCode:master Dec 8, 2020
@yuumasato yuumasato deleted the more_precise_time_diff_in_crypto_policies branch December 8, 2020 14:35
@yuumasato yuumasato added this to the 0.1.54 milestone Jan 12, 2021
This pull request was closed.
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.

7 participants