-
Notifications
You must be signed in to change notification settings - Fork 177
Load TLS root CA directly from /config instead of /persist/certs #5553
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
base: master
Are you sure you want to change the base?
Conversation
This change fixes the security issue where TLS root certificates were copied from the integrity-protected /config partition to the mutable /persist/certs directory and referenced via SHA256 indirection. Changes: - Remove certificate copy logic from InitializeCertDir() - now only creates the certs directory without copying files - Update GetTLSConfig() to read certificates directly from types.V2TLSBaseFile (/config/v2tlsbaseroot-certificates.pem) - Update UpdateTLSProxyCerts() to read from V2TLSBaseFile instead of V2TLSCertShaFilename - Remove V2TLSCertShaFilename constant from locationconsts.go as it's no longer needed - Update getSecurityInfo() in handlemetrics.go to compute SHA256 hash directly from the V2TLSBaseFile content instead of reading a pre-computed SHA from persist Signed-off-by: Shahriyar Jalayeri <shahriyar@posteo.de>
eriknordmark
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.
Given that soon we should figure out how to deliver a new root CA file to deployed devices, do you have ideas how to update it?
We could do the eve config mount and then to a writerename to types.V2TLSBaseFile perhaps with a .bak file to handle a power outage? Or something different?
If we start storing these in rootfs, then we don't have to change the process, a system update brings the new certs too, but if we want to allow surgical update of the pinned certs, then I think storing in config is good approach. But let's discuss all the options and have a security assessment of each before commiting. |
Not part of this PR, but we do have /etc/ssl/certs/ca-certificates.crt in the rootfs so we could have EVE compare and update /config/v2tlsbaseroot-certificates.pem from that file (e.g., by introducing some additional onboot container). That will ensure that we update it as we update the rootfs to new (Alpine) versions. |
eriknordmark
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.
LGTM but CI/CD has issues. Can you rebase on master to see if those issues go away?
eriknordmark
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.
I wonder whether we should just drop this file from /config and instead read from the rootfs i.e., read directly from /etc/ssl/certs/ca-certificates.crt and feed that to AppendCertsFromPEM().
|
@eriknordmark OK, I marked this as draft for now, let me finish the Flexible PCR feature and then get back to this one. |
Description
This change fixes the security issue where TLS root certificates were copied from the integrity-protected /config partition to the mutable /persist/certs directory and referenced via SHA256 indirection.
Changes:
PR dependencies
None
How to test and validate this PR
Changelog notes
Fixed security issue where TLS root certificates could be tampered with via physical disk access, now loading certificates directly from integrity-protected /config partition.
PR Backports
Possibly all :
Checklist
For backport PRs (remove it if it's not a backport):
And the last but not least:
check them.
Please, check the boxes above after submitting the PR in interactive mode.