Confidential computing attestation container #8428

mackdk · 2024-03-08T11:23:33Z

What does this PR change?

This PR defines a new microservice in java to handle attestation results and process them. It runs in a new separate container which is as well defined in this PR. The design is modular and the service can handle multiple attestation modules dynamically loaded at runtime. Each module is able to handle a specific attestation result type.

The java project is defined in Maven. I created a parent pom uyuni-java-parent where all the dependency versions are centralized. Then there are the following modules:

uyuni-java-common, which contains some basic functionality meant to be reusable across multiple java projects
coco-attestation, root project for the real attestation service divided into:
- attestation-core, the main component handling the queue processor logic, module loading and process result handling.
- attestation-module, contains the classes and the interfaces needed to implement an attestation module.
- attestation-module-snpguest, the attestation module for snpguest (real implementation is still WIP).

Additional attestation modules will be implemented. The current implementation uses Postgresql notification to avoid polling the database table for data.

Open discussion points

Naming and modularization: The modularization was created in this way because this was meant to be a base that can be reused to write new services in java when they do not require the full uyuni codebase, but it's not currently strictly needed (a part from the separation attestation core/modules). The whole code is now contained in a microservices subfolder, which is not ideal. The right place should be of course java, but to my understanding it's not possible without moving the existing spacewalk-java package to a subfolder (which I guess we strongly don't want to do 🙂). Any suggestion regarding better naming and modularization is welcome.
Container: I tried to follow the structure and naming consistent with what we have in the containers folder. Please double check it.
Packaging: this is currently split into 3 OBS projects. All of them build with maven with a minor tweaking on the pom configuration.
- uyuni-java-parent: which only contains the parent pom for building
- uyuni-java-common: which packages the corresponding maven project
- uyuni-coco-attestation: packages the actual service. The main package contains attestation-core and attestation-module, all attestation module will be separated into sub-packages. Currently only uyuni-coco-attestation-module-snpguest exists.
Versioning: for now I kept the default 1.0.0-SNAPSHOT. We need to figure out a versioning scheme and how to handle versioning when tagging.
Java dependencies: the code only uses libraries that were already part of uyuni. Currently the project depends on log4j 2, c3p0 for connection pooling, mybatis for database access and postgresql-jdbc for the database driver. Currently, uyuni relies on pgjdbc-ng for receiving notifications from the database, but the current driver offers a similar functionality and the former is stale and has been basically discontinued since three years ago since the authors are no longer using jdbc so they have no motivation to maintain it (What's the status and direction of the project? impossibl/pgjdbc-ng#579). Tests are using junit 5 and mockito (for which I'm using version 5.10.0 that is not provided as a package in OBS but it's not needed by the package build process nor of course at runtime). Additional dependencies might be considered to simplify further developments. In particular a dependency injection mechanism could greatly improve the code. Currently, class dependencies are injected through constructors, but a framework could be used to enforce the practice. We could even implement something basic on our own (something like feather). Another nice thing to have IMO would be @CheckForNull/@Nonnull annotations to help static analysis.
GitHub actions: further configuration will be needed to run unit tests and static analysis on this code. That's still not part of this PR.

This PR is currently NOT targeting master. It will be merged in the coco attestation feature branch.

GUI diff

No difference.

DONE

Documentation

Documentation to be discussed
DONE

Test coverage

Unit tests were added
DONE

Links

Issue(s): https://github.com/SUSE/spacewalk/issues/23552

DONE

Changelogs

Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository

If you don't need a changelog check, please mark this checkbox:

No changelog needed

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Re-run a test

If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:

Re-run test "changelog_test"
Re-run test "backend_unittests_pgsql"
Re-run test "java_pgsql_tests"
Re-run test "schema_migration_test_pgsql"
Re-run test "susemanager_unittests"
Re-run test "javascript_lint"
Re-run test "spacecmd_unittests"

Before you merge

Check How to branch and merge properly!

github-actions · 2024-03-08T11:24:00Z

Suggested tests to cover this Pull Request

mcalmer

just a few points for now

containers/server-attestation-image/Dockerfile

...services/coco-attestation/attestation-core/src/main/resources/mappers/attestation-result.xml

...co-attestation/attestation-module/src/main/java/com/suse/coco/modules/AttestationModule.java

microservices/uyuni-java-parent/uyuni-java-parent.spec

microservices/uyuni-java-common/uyuni-java-common.spec

microservices/uyuni-java-parent/uyuni-java-parent.spec

rel-eng/packages/server-attestation-image

microservices/coco-attestation/uyuni-coco-attestation.spec

containers/server-attestation-image/server-attestation-image.changes

rjmateus

First round about the code structure and maven. So far looks realy good from my point of view. Congrants on all the effort put on this

rjmateus · 2024-03-22T11:24:30Z

microservices/uyuni-java-parent/pom.xml

+            <dependency>
+                <groupId>org.mockito</groupId>
+                <artifactId>mockito-core</artifactId>
+                <version>5.10.0</version>


Shouldn't this one have <scope>test</scope>

This is just the <depedencyManagement> declaration, so I did not include the scope, but yes we can define it directly here as test as well. I will fix it. Same applies to the other 2 entries.

rjmateus · 2024-03-22T11:24:36Z

microservices/uyuni-java-parent/pom.xml

+            <dependency>
+                <groupId>org.junit.jupiter</groupId>
+                <artifactId>junit-jupiter</artifactId>
+                <version>5.8.2</version>


Shouldn't this one have <scope>test</scope>

rjmateus · 2024-03-22T11:24:43Z

microservices/uyuni-java-parent/pom.xml

+            <dependency>
+                <groupId>org.mockito</groupId>
+                <artifactId>mockito-junit-jupiter</artifactId>
+                <version>5.10.0</version>


Shouldn't this one have <scope>test</scope>

rjmateus · 2024-03-22T11:31:38Z

microservices/uyuni-java-common/pom.xml

+        <dependency>
+            <groupId>org.apache.logging.log4j</groupId>
+            <artifactId>log4j-core</artifactId>
+            <scope>test</scope>


Is the log4j needed just for testing? It's not needed at runtime? Is it provided at the runtime?

For compilation of the uyuni-java-common library, only the log4j-api is needed. Of course you will need to provide an implementation at runtime to actually get any logging. Since this is only a common jar, though, I let the decision of that dependency to the consumer of the library. On second thought, I could probably define it as runtime optional.

Or maybe add it as "provided". This way it would be used to compile and test, but not for packaging

rjmateus · 2024-03-22T11:54:32Z

microservices/coco-attestation/pom.xml

+    </parent>
+
+    <artifactId>coco-attestation</artifactId>
+    <version>5.0.0</version>


This version is specified in here and all the sub-projects. This means if we want to change the version we must update the version number for the next release we would need to change in all places.
Do you think we could apply what is described in here, with revision: https://maven.apache.org/maven-ci-friendly.html

I actually wanted the version to be defined in a single place, but I probably made something wrong and it wasn't working. I have a look at the link you provided.

rjmateus · 2024-03-22T12:03:32Z

microservices/coco-attestation/uyuni-coco-attestation.spec

+%pom_disable_module '../uyuni-java-common'
+
+# Make sure there are not dependencies on the modules projects
+%pom_remove_dep 'org.uyuni-project.coco-attestation.module:' attestation-core/pom.xml


Is this line removing the dependency for the core module only or for all?

It's only for attestation-core. This lines make sure the maven pom for attestation-core.jar does not declare any dependency on a module implementation. This is because if the pom contains such dependency, the uyuni-coco-attestation package we create in OBS will require the corresponding module package, and we don't want that, as the modules should be pluggable in any combination (it should be the module that requires the core, not the opposite).

On the other hand, during development it's convenient to declare such runtime dependency, otherwise by default you won't have any module on the classpath when you run the app through an IDE. I could probably define a configuration for the maven exec plugin to achieve the same though. I will look into that.

rjmateus

Just some final comments, but over all I really like the code structure and organization. thank you for the great work

rjmateus · 2024-03-22T12:30:20Z

...s/uyuni-java-common/src/main/java/com/suse/common/configuration/FileConfigurationSource.java

+
+        fileSourcesIn.stream()
+            .flatMap(path -> getConfigurationFiles(path))
+            .sorted(CONFIG_PATH_COMPARATOR)


I think we are not using this yet, but I don't like this comparator based on the path length.
I think we should not sort the files at all. The last file location should be the last to be loaded and overwrite values.

No, this is currently not used. It's a re-implementation of the Config class we use in Uyuni and it's almost backwards compatible (a part for a slightly different management of the missing values). Initially I thought we might need to read the same configuration files, hence I implemented this class. In the end, it turned out we didn't need it at all.

Anyway, this is implementing the same sorting logic we currently have, see the Config class for reference.

rjmateus · 2024-03-22T12:33:41Z

...s/uyuni-java-common/src/main/java/com/suse/common/configuration/FileConfigurationSource.java

+        LOGGER.debug("Adding namespace: {} for file: {}", namespace, file.toAbsolutePath());
+        for (Object key : properties.keySet()) {
+            String propertyName = (String) key;
+            if (!propertyName.startsWith(namespace)) {


what if the property doesn't start with the namespace? Will it get ignored?

Not fully sure to understand. This code is meant to ensure all the properties defined in a file named for example rhn_web.conf starts with web.. In case the property does not start in the correct way, the if branch removes it from the set and then reinsert the value with the correct key. Otherwise, if the property has already the correct prefix, it is kept as is.

rjmateus · 2024-03-22T12:34:41Z

...s/uyuni-java-common/src/main/java/com/suse/common/configuration/FileConfigurationSource.java

+        }
+
+        LOGGER.debug("getString() - Getting property: {}", propertyName);
+        String result = configValues.getProperty(propertyName);


Since we are removing the namespace in here and look only for the property name, can this cause conflicts?

If a property is defined without a namespace, it will always override the namespaced one. That's the current behaviour of our configuration logic. Check the test testOverride() in original ConfigTest.

rjmateus · 2024-03-22T13:46:31Z

microservices/uyuni-java-common/pom.xml

+        <dependency>
+            <groupId>org.apache.logging.log4j</groupId>
+            <artifactId>log4j-core</artifactId>
+            <scope>test</scope>


Or maybe add it as "provided". This way it would be used to compile and test, but not for packaging

rjmateus · 2024-03-22T15:07:51Z

microservices/coco-attestation/attestation-module/pom.xml

+            <artifactId>log4j-api</artifactId>
+        </dependency>
+        <dependency>
+            <groupId>org.mybatis</groupId>


Is mybatis used in here?

Yes, because the SqlSession object is passed to the module to allow querying the database within the same session where the attestation result row is locked.

We could consider abstracting the MyBatis object behind an interface and hide the implementation details. This would allow the code to not depend on a specific DAO layer. It would essentially mean to rewrite some MyBatis interfaces, and I'm not sure if it's worth.

The search server code does something similar though, by providing the DatabaseManager and Query.

rjmateus · 2024-03-22T15:12:46Z

.../attestation-module-snpguest/src/main/java/com/suse/coco/module/snpguest/SNPGuestWorker.java

+                return false;
+            }
+
+            try (var workingDir = directoryProvider.createDirectoryFor(result.getId(), report)) {


Does this directory need to be clean/removed after processing?

The cleanup is currently done in the close() method of VerificationDirectory and called automatically by the try-with-resource.

rjmateus · 2024-03-22T15:38:59Z

...tion/attestation-core/src/main/java/com/suse/coco/attestation/AttestationQueueProcessor.java

+    }
+
+    private Thread createListenerThread() {
+        Thread listenerThread = new Thread(this::listenForAttestationResults, "attestation-processor-listener");


In here we are starting a new thread to listen to changes and notify.
What will happen if for any reason this thread dies? Will the program wait forever for a notification and stop processing stuff silently?

If the listener thread dies, the main thread should stop as the loop is guarded by the listenerRunning flag which is toggled when the listener stops. Also there is a test case checking that.

I have to expand the coverage of this class though, to verify all the possible edge cases betweem the two threads.

rjmateus · 2024-03-22T15:41:43Z

...tion/attestation-core/src/main/java/com/suse/coco/attestation/AttestationQueueProcessor.java

+                    synchronized (dataAvailable) {
+                        dataAvailable.wait();
+                    }
+                    continue;


great trick :)

mackdk · 2024-03-26T16:26:37Z

I will merge these changes in order to have them available in beta 2. I will address the remaining review comments in a later PR.

mackdk requested review from cbosdo, lucidd, mcalmer and rjmateus March 8, 2024 11:23

mackdk requested review from a team as code owners March 8, 2024 11:23

mcalmer reviewed Mar 8, 2024

View reviewed changes

lucidd approved these changes Mar 14, 2024

View reviewed changes