Skip to content

Conversation

@agners
Copy link
Member

@agners agners commented Oct 7, 2025

Proposed change

Formally deprecate CodeNotary build config.

Remove CodeNotary specific integrity checking. The current code is specific to how Codenotary's integrity checking. The cosign signing mechanism only signs the container image digest (sha256). The container image digest in turn references the image manifest (which references the layers). This setup does not allow integrity checks on disk, but only during installation/download.

To ensure container integrity on disk other mechanism like EROFS with dm-verity can be used. Integrity is validated on the fly with such mechanism. Hence no Supervisor level implementation would be needed, but correct error handling at runtime if errors are detected.

In any case, a future container image integrity likely will look different then how Codenotary looked. Remove the current code to make way for a future implementations.

This can be seen as a follow up of #4217.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to the supervisor)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Ruff (ruff format supervisor tests)
  • Tests have been added to verify that the new code works.

If API endpoints or add-on configuration are added/changed:

Copy link
Contributor

@mdegat01 mdegat01 left a comment

Choose a reason for hiding this comment

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

Looks like we missed a few key things in the API here that should go if we want to fully remove this. This will also require a follow up PR in basically all the other repos Supervisor changes normally impact (cli, client library, dev doc, doc and core).

Also we have code notary worked into our CI don't we? And in the image for supervisor and supervisor devcontainer. So we'll need to get it removed from there as well as other follow-up PRs.

_LOGGER: logging.Logger = logging.getLogger(__name__)

# pylint: disable=no-value-for-parameter
SCHEMA_OPTIONS = vol.Schema(
Copy link
Contributor

@mdegat01 mdegat01 Oct 10, 2025

Choose a reason for hiding this comment

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

The schema for security/options includes content_trust and force_security:

SCHEMA_OPTIONS = vol.Schema(
{
vol.Optional(ATTR_PWNED): vol.Boolean(),
vol.Optional(ATTR_CONTENT_TRUST): vol.Boolean(),
vol.Optional(ATTR_FORCE_SECURITY): vol.Boolean(),
}
)

@api_process
async def options(self, request: web.Request) -> None:
"""Set options for Security."""
body = await api_validate(SCHEMA_OPTIONS, request)
if ATTR_PWNED in body:
self.sys_security.pwned = body[ATTR_PWNED]
if ATTR_CONTENT_TRUST in body:
self.sys_security.content_trust = body[ATTR_CONTENT_TRUST]
if ATTR_FORCE_SECURITY in body:
self.sys_security.force = body[ATTR_FORCE_SECURITY]
await self.sys_security.save_data()
await self.sys_resolution.evaluate.evaluate_system()

Neither of these options make sense anymore. They should be removed from here, the python properties removed from security module and removed from the schema for the persistent config file for the security module here:

SCHEMA_SECURITY_CONFIG = vol.Schema(
{
vol.Optional(ATTR_CONTENT_TRUST, default=True): vol.Boolean(),
vol.Optional(ATTR_PWNED, default=True): vol.Boolean(),
vol.Optional(ATTR_FORCE_SECURITY, default=False): vol.Boolean(),
},
extra=vol.REMOVE_EXTRA,
)

That will also require a change in the client library, cli and documentation to remove those options.

The security/info API should not return their values either:

async def info(self, request: web.Request) -> dict[str, Any]:
"""Return Security information."""
return {
ATTR_CONTENT_TRUST: self.sys_security.content_trust,
ATTR_PWNED: self.sys_security.pwned,
ATTR_FORCE_SECURITY: self.sys_security.force,
}

That will require a change in the client library and documentation.

Should be able to toss those constants after as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially, my intention was to leave the outside API intact, so we can reuse it for the cosign implementation if it feels right. But after a bit more consideration, I think it is better to remove the API as well. With cosign I'd like to use cosign specific wording, e.g. image signature verification for the verification process etc. Content verification will work quite a bit different with EROFS/fsverity, so the API does not really match.

The force_security flag is also used by HIBP, so I did not remove that part.

@home-assistant home-assistant bot marked this pull request as draft October 10, 2025 18:18
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@pvizeli
Copy link
Member

pvizeli commented Oct 22, 2025

The reason why I did not removed it was, that wanted to just make a swap to cosign. It should be simple to replace it with using the cosign tool but I never found the time

@agners
Copy link
Member Author

agners commented Oct 23, 2025

The reason why I did not removed it was, that wanted to just make a swap to cosign. It should be simple to replace it with using the cosign tool but I never found the time

We could come up with a way to use cosign as a drop-in replacement where we would check the file system content at runtime (similar to how it worked on cosign). But the next step then would be how can we act on it if there are issues... And unfortunately Docker is a bit annoying in that domain: If a layer is used by another image, and the corruption happened in a file which is part of a shared part, then removing and pulling the image does not fix the integrity issue (since the shared layer will not get fetched/written again) 😞 . In a way, the current approach would allow to detect integrity issues, but we have no way to fix it 😢 .

I'd like to leverage containerd integrated integrity checks, namely EROFS and fsverity (got introduced with containerd/containerd#11352). But integration of this will be different, so I think it is simpler to remove the current code and then implement support for EROFS/fsverity from ground up.

@agners agners marked this pull request as ready for review October 23, 2025 14:16
@home-assistant home-assistant bot requested a review from mdegat01 October 23, 2025 14:16
@agners agners force-pushed the remove-codenotary-integrity-check branch from b1d24e2 to 671ece2 Compare October 28, 2025 14:45
agners added 13 commits October 28, 2025 15:48
The current code is specific to how CodeNotary was doing integrity
checking. A future integrity checking mechanism likely will work
differently (e.g. through EROFS based containers). Remove the current
code to make way for a future implementation.
Remove CodeNotary related exceptions and handling from the Docker
interface.
Introduce a new exception class APIGone to indicate that certain API
features have been removed and are no longer available. Update the
security integrity check endpoint to raise this new exception instead
of a generic APIError, providing clearer communication to clients that
the feature has been intentionally removed.
A cosign based signature verification will likely be named differently
to avoid confusion with existing implementations. For now, remove the
content trust option entirely.
@agners agners force-pushed the remove-codenotary-integrity-check branch from 671ece2 to 0bb0b51 Compare October 28, 2025 14:48
Since we have "remove extra" in voluptuous, we can remove the
codenotary field from the addon schema.
agners added a commit to home-assistant/cli that referenced this pull request Oct 28, 2025
Remove the Codenotary based content-trust security option. The
replacement usign cosign will likely require different options.

Related to: home-assistant/supervisor#6236
agners added a commit to home-assistant-libs/python-supervisor-client that referenced this pull request Oct 28, 2025
Remove constants used for Codenotary related security features. A future
security feature will be cosign based and likely use different
termology.

Related to: home-assistant/supervisor#6236
@agners agners added the needs-core Pull request needs Home Assistant Core changes but none are linked label Oct 28, 2025
Copy link
Contributor

@mdegat01 mdegat01 left a comment

Choose a reason for hiding this comment

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

Looks ready to me now. We'll need a PR in cli, client library and doc for the removal of that API option though.

EDIT: Ok I see the first two are done, whoops. Last thing is remove it from the doc here: https://developers.home-assistant.io/docs/api/supervisor/endpoints#security

@mdegat01 mdegat01 added the missing-documentation Added to pull requests that needs a docs, but none is linked label Oct 28, 2025
sairon pushed a commit to home-assistant/cli that referenced this pull request Oct 29, 2025
Remove the Codenotary based content-trust security option. The
replacement usign cosign will likely require different options.

Related to: home-assistant/supervisor#6236
@agners agners merged commit 1448a33 into main Nov 3, 2025
23 checks passed
@agners agners deleted the remove-codenotary-integrity-check branch November 3, 2025 19:13
@github-actions github-actions bot locked and limited conversation to collaborators Nov 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

breaking-change cla-signed Hacktoberfest missing-documentation Added to pull requests that needs a docs, but none is linked needs-core Pull request needs Home Assistant Core changes but none are linked

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants