Add: snaps confinement test (new)

[patliuu]

Resolved issues

Jira task https://warthogs.atlassian.net/browse/CQT-2907

Rational

Murcia project's customer asked us to include 2 snaps in the GM image. One of the snap is strict confined and installed in devmode, the other one is devmode confined and also installed in devmode.[1]

Recently this problem was found. For security reason, this should not be allowed on the image we delivered.
So we were asked to create a test case to catch this in the future.

[1] snap confinement document https://snapcraft.io/docs/snap-confinement

Information

We originally have a test case called snappy/test-snap-confinement-mode. It checks the confinement and sandbox features of the system, if the system confinement is not strict, the test case will print out the missing requirements (e.g. sandbox features or cgroup version). Therefore, this test is testing the confinement of the system. We don't actually test the snap confinements of each installed snap.

In the problematic image of Murcia project, there are 2 snaps sideloaded at the last stage of image installation.

Name	Version	Revision		Confinement	Devmode
gcap	1.0.0	x1		strict	True		July 14, 2023
litebmc-fwupd	2.6	x1		devmode	True		July 14, 2023

Requirement

After checking with OEM SWE team, the requriments of this test are checking the following things for each installed snap,

1. Check the revision should NOT be sideloaded like x1.
2. Check the confinement should be strict. classic and devmode confinement are not allowed.
3. Check the installed in devmode should be False.
4. This test should be executed on all desktop, server, and core images.

What are implemented in this pull request?

1. Refactor the original script. Splitting them to SystemConfinement class and SnapsConfinement class corresponding to the system confinement test and snap confinement test(the new test created)
2. Whitelisted the snaps that are installed in devmode for testing. Including bugit, checkboxXX, mir-test-tools, graphic-test-tools.
3. Implement the requirements above.
4. Add snappy/test-system-confinement and snappy/test-snaps-confinement to snappy-snap-automated test plan.
5. Add snappy-snap-automated test plan to desktop test plans. As we are thinking desktop image should also test the snap features including install, refresh, revert ...etc.

Sideload test results

• Tested on a Ubuntu Desktop 22.04 laptop
  https://pastebin.canonical.com/p/hsyFyRVtVQ/
• Tested on a Ubuntu Core 22 device with sideloading stress-ng snap
  https://pastebin.canonical.com/p/CngKh9fNyD/
• Tested on that problematic machine (Desktop image on IoT device)
  https://pastebin.canonical.com/p/gT8WNzbjbt/

[kissiel]

Fantastic stuff!
The code below is quite testable, but there's no tests. I added some comments below that could be helpful with that.
See other comments for other tweaks.

[kissiel]

we're following modern naming schemes in which "allowlist" is preferred.

[patliuu]

Good suggestion! I'll modify the naming in the next commit.

[kissiel]

Both classes in this testing program have only one method and they don't benefit from being a class. Simple function would be cleaner (for instance the indentation would be better allowing cleaner code).

Furthermore the function could be decomposed further to facilitate unit testing. For instance the "check_snap_confinment" could accept snap's data as an argument, and do the checks. Then in the main scope in the iterating over snaps could be done and this function could be invoked for each.
In turn the unit test becomes simple - the snap data would have to be prepared with the conditions you want, and so on...

[patliuu]

Honestly, I was referring to the structure snappy tests.

I think the idea is to separate the test cases by classes, and the data like features_should_include or allowlist_snaps could be located in that class. Re-write it to only functions are OK, but I think both ways have their own advantages.

Regarding decomposing the functions to apply unit testing, there are 2 things I'd like to discuss with you.

1. For this test case, I feel the unit tests could be implemented for functions in snapd.py. As this test is calling the functions in snapd.py and get data in the dictionaries, it seems to me that unit tests for this test case are not necessary.
2. In my opinion, wrapping those if conditions to functions or not are fine. If the functions will not be reused or extended, keeping these simple if conditions will be easier to understand the test steps. If we are going to wrap them in functions like check_snap_confinement() or check_snap_demode(), it will be better to keep the class structure and implement these functions in classes. Something like,

class SnapsConfinement:
    def __init__(self):
        data = Snapd.list()

    def check_snap_confinement(self, snap, snap_name):
        confinement = snap.get("confinement")
        if confinement != "strict":
            logging.error(
                "Snap '%s' confinement is expected to be 'strict' "
                "but got '%s'", snap_name, confinement,
            )
            return False
        return True

And iterate the snaps self.data and get the snap name in invoked() functions.

Which way do you prefer?

[kissiel]

keeping these simple if conditions will be easier to understand the test steps.

I agree with this statement.

If what you want to have is a class that reflects one idea of a test, like "check snaps' confinement", then I think natural decomposition would be to iterate over the snaps in invoked and call the is_snap_confined check for each (btw. I changed the name on purpose - for checks that end in booleans the is_something is clearer tbh).
I hope this answers your question.

[patliuu]

Thank you for the clarification.

I rethought it a little bit. Since these 2 test classes are simple, I would prefer just to flatten them into functions test_system_confinement() and test_snaps_confinement(). And the attributes features_should_include and allowlist_snaps for each class become a local variable in each function.

To keep the test steps easy to understand, I would leave the if conditions as they are.
Feel free to give any comments in case I misunderstand anything.

[kissiel]

Fantastic, very Pythonic approach, <3

[kissiel]

Quick answer about the RE below. I'll answer more soon.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (main@7ae6ae1). Click here to learn what that means.
The diff coverage is n/a.

@@          Coverage Diff           @@
##             main    #663   +/-   ##
======================================
  Coverage        ?   2.10%           
======================================
  Files           ?     127           
  Lines           ?   15031           
  Branches        ?    2596           
======================================
  Hits            ?     316           
  Misses          ?   14652           
  Partials        ?      63           

[patliuu]

Hi @kissiel ,
I've applied the suggestion except for the discussion of using classes or not and wapping "if conditions" into other functions or not.

[patliuu]

Updated the code. Flatten the test classes to test functions to make them simpler.

[kissiel]

Looks great! +1
Thank you for all the changes!

I really hope that the CI will not choke on the test_ function prefix (it may look like a unit test ;) )