Fix SBOM generation inconsistency

[damdo]

Currently SBOMs may have inconsistent hashing of the configuration file.

The representation of the config file in fact may differ at certain stages of the gok commands lifecycle, where at packer running time for example, extra InternalCompatibilityFlags would be added in memory (while that not being the case at gok sbom time), resulting in a differing config and as such differing SBOM hashes. The same would happen due to the differing pointer addresses in the config that resulted in differing hashing results.

This is now fixed by removing the InternalCompatibilityFlags from the config prior to its hashing, as well as converting the config into a string before performing the hash computation, to avoid hashing pointers problems.

[stapelberg]

Currently SBOMs may have inconsistent hashing of the configuration file.

Can you elaborate about where you ran into this?

This is now fixed by removing the InternalCompatibilityFlags from the config prior to its hashing, as well as converting the config into a string before performing the hash computation, to avoid hashing pointers problems.

This approach has the downside of being destructive with regards to InternalCompatibilityFlags, though, right? If I change the config.json and just add another flag in InternalCompatibilityFlags, it would show up as the same SBOM? That does not sound good.

Maybe this should be fixed in a different place instead?

[damdo]

Currently SBOMs may have inconsistent hashing of the configuration file.

Can you elaborate about where you ran into this?

Sure. It is 100% of the time reproducible for me.

# create a gaf disk for the instance
root@00a1d367c15f:~/gokrazy/test# GOARCH=amd64 ../../gok -i test --parent_dir="./.." overwrite --gaf /tmp/disk.gaf 2>&1 >/dev/null

# extract the sbom.json and read the hash
root@00a1d367c15f:~/gokrazy/test# unzip -p /tmp/disk.gaf sbom.json | jq -r '.sbom_hash'
955b4c96886d1e109d8a82e1f2054c67948d4afaab34c374d0c4b09b8ca32d8b

# invoke gok sbom directly
root@00a1d367c15f:~/gokrazy/test# GOARCH=amd64 ../../gok -i test --parent_dir="./.." sbom | jq -r '.sbom_hash'
865418423a176b1caad2e2fae11cc5238ac986a61bed1a15cc3385384c0ba462

# the two hashes differ (while always both staying consistent in value on multiple runs)

Aside from the pointers (which cause diff in the byte representation of the formattedCfg but that can be easily rectified by reconverting that to a string representation before formatting it in hexadecimal. As fixed in the PR (let me know if you are happy with this))

The remaining diff is caused by the InternalCompatibilityFlags. This can be seen by dumping the stringyfied formattedCfg to disk both during sbom generation, and run the overwrite (to gaf) and the sbom separately, inspecting the output.

# output of gok sbom
(string) (len=799) "{\n    \"Hostname\": \"gokrazy-dummy\",\n    \"Update\": {\n        \"Hostname\": \"192.168.64.2\",\n        \"HTTPPort\": \"80\",\n        \"HTTPPassword\":
 \"OMITTED\"\n    },\n    \"Packages\": [\n        \"github.com/gokrazy/serial-busybox\",\n        \"github.com/gokrazy/breakglass\"\n    ],\n    \"PackageConfig\":
 {\n        \"github.com/gokrazy/breakglass\": {\n            \"CommandLineFlags\": [\n                \"-authorized_keys=/etc/breakglass.authorized_keys\"\n            ],\n
        \"ExtraFilePaths\": {\n                \"/etc/breakglass.authorized_keys\": \"breakglass.authorized_keys\"\n            }\n        }\n    },\n    \"SerialConsole\": \"tt
yS0,115200\",\n    \"KernelPackage\": \"github.com/rtr7/kernel\",\n    \"FirmwarePackage\": \"github.com/rtr7/kernel\",\n    \"EEPROMPackage\": \"\",\n    \"InternalCompatibilit
yFlags\": {}\n}\n"

# output of gok overwrite --gaf
(string) (len=827) "{\n    \"Hostname\": \"gokrazy-dummy\",\n    \"Update\": {\n        \"Hostname\": \"192.168.64.2\",\n        \"HTTPPort\": \"80\",\n        \"HTTPPassword\":
 \"OMITTED\"\n    },\n    \"Packages\": [\n        \"github.com/gokrazy/serial-busybox\",\n        \"github.com/gokrazy/breakglass\"\n    ],\n    \"PackageConfig\":
 {\n        \"github.com/gokrazy/breakglass\": {\n            \"CommandLineFlags\": [\n                \"-authorized_keys=/etc/breakglass.authorized_keys\"\n            ],\n
        \"ExtraFilePaths\": {\n                \"/etc/breakglass.authorized_keys\": \"breakglass.authorized_keys\"\n            }\n        }\n    },\n    \"SerialConsole\": \"tt
yS0,115200\",\n    \"KernelPackage\": \"github.com/rtr7/kernel\",\n    \"FirmwarePackage\": \"github.com/rtr7/kernel\",\n    \"EEPROMPackage\": \"\",\n    \"InternalCompatibilit
yFlags\": {\n        \"Sudo\": \"auto\"\n    }\n}\n"

As you can see the difference is in the InternalCompatibilityFlags:{"Sudo": "auto"} setting, defined during overwrite, but not during sbom.

This happens because in the gok packer logic we dynamically set the flag: https://github.com/gokrazy/tools/blob/23cde3b0d858497a63c21e93ad30859bf197995f/internal/packer/packer.go#L1008-L1011

This approach has the downside of being destructive with regards to InternalCompatibilityFlags, though, right? If I change the config.json and just add another flag in InternalCompatibilityFlags, it would show up as the same SBOM? That does not sound good.

Yes good point, given those are internal flags for migrating from gokr-packer to gok (and configure the CLI meta options), I figured they wouldn't necessarily mean anything from an image build perspective, but I see your point.

What would you suggest then? Should we set InternalCompatibilityFlags:{"Sudo": "auto"} also at gok sbom invocation to avoid diff?

[stapelberg]

Thanks for the details.

I think in this particular case it would actually be best to:

1. introduce a SudoOrDefault() accessor to config.InternalCompatibilityFlags that returns "auto" or sudo if non-empty
2. switch the code from .Sudo to .SudoOrDefault()
3. remove the lines that set the option to "auto"

…and avoid the diff that way.

[damdo]

Yes, that makes sense to me.

I've created a PR for the accessor change here: https://github.com/gokrazy/internal/pull/18 And updated this PR accordingly.

Once the accessor PR merges we'll need to remove. the replace directive before merging this PR.

[damdo]

Replace directive removed and newest gokrazy/internal referenced. Ready a review @stapelberg :)