-
Notifications
You must be signed in to change notification settings - Fork 748
Remove Codenotary integrity check #6236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
mdegat01
left a comment
There was a problem hiding this 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( |
There was a problem hiding this comment.
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:
supervisor/supervisor/api/security.py
Lines 18 to 24 in 3c21a8b
| SCHEMA_OPTIONS = vol.Schema( | |
| { | |
| vol.Optional(ATTR_PWNED): vol.Boolean(), | |
| vol.Optional(ATTR_CONTENT_TRUST): vol.Boolean(), | |
| vol.Optional(ATTR_FORCE_SECURITY): vol.Boolean(), | |
| } | |
| ) |
supervisor/supervisor/api/security.py
Lines 39 to 53 in 3c21a8b
| @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:
supervisor/supervisor/validate.py
Lines 230 to 237 in 3c21a8b
| 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:
supervisor/supervisor/api/security.py
Lines 31 to 37 in 3c21a8b
| 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.
There was a problem hiding this comment.
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.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
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. |
b1d24e2 to
671ece2
Compare
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.
671ece2 to
0bb0b51
Compare
Since we have "remove extra" in voluptuous, we can remove the codenotary field from the addon schema.
Remove the Codenotary based content-trust security option. The replacement usign cosign will likely require different options. Related to: home-assistant/supervisor#6236
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
There was a problem hiding this 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
Remove the Codenotary based content-trust security option. The replacement usign cosign will likely require different options. Related to: home-assistant/supervisor#6236
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
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed: