api: adding current status field to plan api #3065

dheyay · 2024-04-17T22:06:35Z

Why is this needed?

This PR adds a field displaying the current_status of the cve to show if a system is currently affected by a cve/usn before the fix plan is executed.
The field is returned as a part of the plan api.

Test Steps

Checklist

I have updated or added any unit tests accordingly
I have updated or added any integration tests accordingly
Changes here need to be documented, and this was done in:

Does this PR require extra reviews?

Yes
No

github-actions · 2024-04-17T22:06:50Z

Jira: This PR is not related to a Jira item. (The PR title does not include a SC-#### reference)

GitHub Issues: No GitHub issues are fixed by this PR. (No commits have Fixes: #### references)

Launchpad Bugs: No Launchpad bugs are fixed by this PR. (No commits have LP: #### references)

Documentation: The changes in this PR do not require documentation changes.

👍 this comment to confirm that this is correct.

lucasmoura

Just a python style suggestion, but if you disagree, no worries at all

lucasmoura · 2024-04-22T13:20:58Z

uaclient/api/u/pro/security/fix/_common/plan/v1.py

+                self.fix_steps[0].data.status == "system-not-affected"
+                or self.fix_steps[0].data.status == "cve-already-fixed"
+                or self.fix_steps[0].data.status == "cve-fixed-by-livepatch"


just a style suggestion, but we can update this for a in check:
self.fix_steps[0].data.status in ("system-not-affected", "cve-already-fixed", "cve-fixed-by-livepatch")

Looks better, updated!

orndorffgrant

Let's re-use the existing enum values for FixStatus

More importantly, I think we should expose the current state as it is rather than try to derive the current state from the plan and expected state after the fact (that is exactly what we said was too messy/hacky to recommend to users).

So in _generate_fix_plan there is a check if count == 0:. If that is true then we know the system is not affected, so if true it can do something like fix_plan.current_status = FixStatus.SYSTEM_NOT_AFFECTED.

Then later, there is a if not binary_pkgs: if src_packages: I think in that situation we'd want fix_plan.current_status = FixStatus.SYSTEM_NON_VULNERABLE.

It looks like we're missing logic to determine if the fixed package is already installed but we haven't rebooted yet, so I don't think we'll be able to set fix_plan.current_status = FixStatus.SYSTEM_VULNERABLE_UNTIL_REBOOT until we solve that - which might warrant it's own bug.

WDYT @lucasmoura ?

dheyay · 2024-04-23T19:13:14Z

@orndorffgrant I agree, the current status would be more effective if not derived from the plan and can be set in the _generate_fix_plan function.

For the logic regarding FixStatus.SYSTEM_VULNERABLE_UNTIL_REBOOT, I think we can use the system.should_reboot() function along with the fixed packages to check for this?

dheyay · 2024-04-26T21:58:21Z

Updated @orndorffgrant, let me know if this works?

lucasmoura · 2024-05-02T20:06:39Z

@orndorffgrant I thinks this is a good idea

orndorffgrant

Looking good! Some nits

Since #3050 will depend on this new feature, it may need to be split into a PR that can go live immediately and a separate PR that will only be published with version 33

Also you'll need to add documentation for this API endpoint change

orndorffgrant · 2024-05-03T17:56:48Z

uaclient/api/u/pro/security/fix/_common/__init__.py

@@ -52,6 +52,7 @@ def __init__(self, value: int, msg: str):
    SYSTEM_NOT_AFFECTED = _Value(0, "not-affected")
    SYSTEM_STILL_VULNERABLE = _Value(1, "still-affected")
    SYSTEM_VULNERABLE_UNTIL_REBOOT = _Value(2, "affected-until-reboot")
+    SYSTEM_VULNERABLE = _Value(3, "affected")


How is this different from "still-affected"?

The rationale for having this as a different message was that if the user would like to see if their system is affected, a still-affected status could be confusing compared to a simple binary - affected / not-affected. Also, the still-affected message is used to convey the expected_status where it makes sense since we provide with a reason as to why the fix/plan did not resolve the cve.

But I understand if having this only adds to complexity and can be substituted by the already existing still-affected message. What do you suggest @orndorffgrant ?

I see your point about user-facing messages, but this is an enum and not intended to be a user-facing message. If it is currently being used as a user-facing message then that is arguably a defect.

I see that perhaps "affected" would've been a better enum value when we defined these originally, but changing it now would break backwards compatibility.

uaclient/api/u/pro/security/fix/_common/plan/v1.py

uaclient/cli/tests/test_cli_fix.py

dheyay · 2024-05-03T19:59:38Z

Updated @orndorffgrant, will also update the documentation and create a PR for the same

orndorffgrant · 2024-05-07T17:08:34Z

uaclient/api/u/pro/security/fix/_common/plan/v1.py

@@ -395,10 +396,12 @@ def __init__(
        error: Optional[FixPlanError],
        additional_data: AdditionalData,
        description: Optional[str] = None,
-        affected_packages: Optional[List[str]] = None
+        affected_packages: Optional[List[str]] = None,
+        current_status: Optional[str] = None,


Suggested change

current_status: Optional[str] = None,

current_status: Optional[str] = None

This extra comma is a syntax error in Python 3.5 (see xenial build package CI failure)

orndorffgrant

One last nit then I think we're good

orndorffgrant · 2024-05-24T19:24:05Z

Weird, my comment from my last review didn't post... I'll find it

orndorffgrant · 2024-05-24T19:24:28Z

uaclient/api/u/pro/security/fix/_common/plan/v1.py

@@ -942,6 +958,7 @@ def _generate_fix_plan(
                apt.update_esm_caches(cfg)
                esm_cache_updated = True
            except Exception as e:
+                fix_plan.current_status = "error"


I think we don't need this anymore?

Fixed @orndorffgrant

…d by the CVE

dheyay force-pushed the cve-affected-status branch from 3cdbae5 to b3b03db Compare April 17, 2024 22:08

dheyay changed the title ~~CVE affected status~~ api: adding current status field to plan api Apr 18, 2024

dheyay force-pushed the cve-affected-status branch 7 times, most recently from 478f9c7 to c59686d Compare April 19, 2024 20:16

lucasmoura approved these changes Apr 22, 2024

View reviewed changes

dheyay force-pushed the cve-affected-status branch 3 times, most recently from 243fff3 to 3189860 Compare April 23, 2024 14:02

orndorffgrant requested changes Apr 23, 2024

View reviewed changes

dheyay force-pushed the cve-affected-status branch 6 times, most recently from 238dc6c to ba401ac Compare April 24, 2024 21:09

orndorffgrant reviewed May 3, 2024

View reviewed changes

dheyay force-pushed the cve-affected-status branch from ba401ac to fe1fbad Compare May 3, 2024 19:59

orndorffgrant reviewed May 7, 2024

View reviewed changes

dheyay force-pushed the cve-affected-status branch 2 times, most recently from 56d8593 to 99cb30f Compare May 13, 2024 14:37

dheyay force-pushed the cve-affected-status branch from 99cb30f to 912a83b Compare May 13, 2024 14:38

dheyay mentioned this pull request May 16, 2024

Updated plan api docs to include the current status field #3111

Merged

5 tasks

orndorffgrant approved these changes May 20, 2024

View reviewed changes

orndorffgrant reviewed May 24, 2024

View reviewed changes

Added current status field to plan api to show if a system is affecte…

3929d28

…d by the CVE

dheyay force-pushed the cve-affected-status branch from 912a83b to 3929d28 Compare May 28, 2024 15:08

dheyay mentioned this pull request May 28, 2024

docs: Adding howto guides for CVEs/USN #3116

Open

5 tasks

orndorffgrant merged commit 9e374dd into next-v33 May 28, 2024
17 of 24 checks passed

orndorffgrant deleted the cve-affected-status branch May 28, 2024 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: adding current status field to plan api #3065

api: adding current status field to plan api #3065

dheyay commented Apr 17, 2024 •

edited

github-actions bot commented Apr 17, 2024 •

edited

lucasmoura left a comment

lucasmoura Apr 22, 2024

dheyay Apr 22, 2024

orndorffgrant left a comment

dheyay commented Apr 23, 2024 •

edited

dheyay commented Apr 26, 2024

lucasmoura commented May 2, 2024

orndorffgrant left a comment

orndorffgrant May 3, 2024

dheyay May 3, 2024

orndorffgrant May 7, 2024

dheyay commented May 3, 2024

orndorffgrant May 7, 2024

orndorffgrant left a comment

orndorffgrant commented May 24, 2024

orndorffgrant May 24, 2024

dheyay May 28, 2024

	current_status: Optional[str] = None,
	current_status: Optional[str] = None

api: adding current status field to plan api #3065

api: adding current status field to plan api #3065

Conversation

dheyay commented Apr 17, 2024 • edited

Why is this needed?

Test Steps

Checklist

Does this PR require extra reviews?

github-actions bot commented Apr 17, 2024 • edited

lucasmoura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orndorffgrant left a comment

Choose a reason for hiding this comment

dheyay commented Apr 23, 2024 • edited

dheyay commented Apr 26, 2024

lucasmoura commented May 2, 2024

orndorffgrant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dheyay commented May 3, 2024

Choose a reason for hiding this comment

orndorffgrant left a comment

Choose a reason for hiding this comment

orndorffgrant commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dheyay commented Apr 17, 2024 •

edited

github-actions bot commented Apr 17, 2024 •

edited

dheyay commented Apr 23, 2024 •

edited