Add validation of cert file path #195
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #195 +/- ##
========================================
- Coverage 9.50% 9.39% -0.12%
========================================
Files 34 35 +1
Lines 3326 3365 +39
========================================
Hits 316 316
- Misses 2988 3027 +39
Partials 22 22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: n4mlz <[email protected]>
n4mlz
force-pushed
the
fix/validate-file-path
branch
from
f40d0b6 to
da47ac4
Compare
Signed-off-by: n4mlz <[email protected]>
Signed-off-by: n4mlz <[email protected]>
Signed-off-by: n4mlz <[email protected]>
Signed-off-by: n4mlz <[email protected]>
Signed-off-by: n4mlz <[email protected]>
There was a problem hiding this comment.
Try to include changes only for the validation logic.
If you think some part of code requires some fix, leave TODOs as a comment instead, rather than fixing them within the same PR.
Also was wondering if you would apply the same logic for the RoleCert as well? Thanks
Signed-off-by: n4mlz <[email protected]>
Signed-off-by: n4mlz <[email protected]>
n4mlz
force-pushed
the
fix/validate-file-path
branch
from
8b53fed to
c0dc6db
Compare
|
Regarding the location for certificate path validation processing The k8s-athenz-sia/pkg/config/config.go Line 262 in cdb74dd idCfg.ServiceCert.CopperArgos.Cert.Paths. Looking at pkg/certificate/service.go, these files exist in use cases where only reading occurs, with no write operations performed. If we were to perform write permission validation simultaneously with reading the configuration file, we would end up verifying write capabilities even when write operations aren't actually required. Furthermore, performing actual write permission validation as part of the configuration file processing seems beyond its intended responsibility. This could potentially create implicit dependencies.Additionally, the requirement specified was "to verify actual write permissions before performing any file write operations. For these reasons, performing write permission validation within pkg/certificate/service.go appears more appropriate.
|
pkg/certificate/service.go
Outdated
| isValidFiles := func() error { | ||
| if !idCfg.ServiceCert.LocalCert.Use { | ||
| for _, certFile := range idCfg.ServiceCert.CopperArgos.Cert.Paths { | ||
| err := isValidFile(certFile) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| for _, keyFile := range idCfg.ServiceCert.CopperArgos.Key.Paths { | ||
| err := isValidFile(keyFile) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| err := isValidFile(idCfg.CaCertFile) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Do the following:
- Do early return as much as possible => gives less indent = more readable code = less buggy as easier to fix
- In go, you can return error without defining them in each line.
| isValidFiles := func() error { | |
| if !idCfg.ServiceCert.LocalCert.Use { | |
| for _, certFile := range idCfg.ServiceCert.CopperArgos.Cert.Paths { | |
| err := isValidFile(certFile) | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| for _, keyFile := range idCfg.ServiceCert.CopperArgos.Key.Paths { | |
| err := isValidFile(keyFile) | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| err := isValidFile(idCfg.CaCertFile) | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| return nil | |
| } | |
| isValidFiles := func() error { | |
| // TODO: Write a reason why LocalCert Mode does not require the cert path validation!! | |
| if idCfg.ServiceCert.LocalCert.Use { | |
| return nil | |
| } | |
| for _, certFile := range idCfg.ServiceCert.CopperArgos.Cert.Paths { | |
| if err := isValidFile(certFile); err != nil { | |
| return err | |
| } | |
| } | |
| for _, keyFile := range idCfg.ServiceCert.CopperArgos.Key.Paths { | |
| if err := isValidFile(keyFile); err != nil { | |
| return err | |
| } | |
| } | |
| return isValidFile(idCfg.CaCertFile) // will return error if idCfg.CaCertFile is not a valid filepath | |
| } |
Signed-off-by: n4mlz <[email protected]>
Signed-off-by: n4mlz <[email protected]>
Signed-off-by: n4mlz <[email protected]>
Signed-off-by: n4mlz <[email protected]>
…te reloader Signed-off-by: n4mlz <[email protected]>
Signed-off-by: n4mlz <[email protected]>
| err := validFilePath(keyFile) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Nits but:
| err := validFilePath(keyFile) | |
| if err != nil { | |
| return err | |
| } | |
| if err := validFilePath(keyFile); err != nil { | |
| return err | |
| } |
| err := validFilePath(certFile) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
| } | ||
|
|
||
| // Verify that the certificate file paths are in writable locations | ||
| func (idCfg *IdentityConfig) ValidateCertFilePath() error { |
There was a problem hiding this comment.
Can we have a background why we do this
(Can we have more comments for "why" this function exists)
| err := unix.Access(target_path, unix.W_OK) | ||
| if err != nil { | ||
| return fmt.Errorf("file permission error: %w", err) | ||
| } |
There was a problem hiding this comment.
|
|
||
| // Verify that the certificate file paths are in writable locations | ||
| func (idCfg *IdentityConfig) ValidateCertFilePath() error { | ||
| // When idCfg.ServiceCert.LocalCert.Use is true, skip file writing and return early |
There was a problem hiding this comment.
Add a why please thanks. Why do we skip for LocalCert Mode?
| "golang.org/x/sys/unix" | ||
| ) | ||
|
|
||
| func validFilePath(file_path string) error { |
There was a problem hiding this comment.
Write a comment please.
- What it does
- Why we have this
| dir_path := filepath.Dir(file_path) | ||
| var target_path string | ||
| _, file_err := os.Stat(file_path) | ||
| _, dir_err := os.Stat(dir_path) | ||
|
|
||
| if file_err == nil { | ||
| // validate file path | ||
| target_path = file_path | ||
| } else if os.IsNotExist(file_err) && dir_err == nil { | ||
| // validate dir path | ||
| target_path = dir_path | ||
| } else { | ||
| return fmt.Errorf("file path not exist: %w, %w", file_err, dir_err) | ||
| } |
There was a problem hiding this comment.
was wondering if you can write a separate function for this, instead of including within the validFilePath()
func getPath(path string) error, string {
_, err:= os.Stat(filePath)
if err == nil {
return nil, filePath
}
if !os.IsNotExist(err) {
return "", fmt.Errorf("error accessing path '%s': %w", filePath, err)
}
dirPath := filepath.Dir(filePath)
if _, dirErr := osStat(dirPath); dirErr != nil { return "", fmt.Errorf("todo error"); }
return dirPath, nil
}Then just do
if err := unix.Access(getPath(path), unix.W_OK); err != nil {
return fmt.Errorf("file permission error: %w", err)
}There was a problem hiding this comment.
Did not write ocmments or good function name so please do so.
There was a problem hiding this comment.
Pull Request Overview
This PR adds validation of certificate file paths before the SIA agent requests certificates from ZTS, preventing inconsistent states when file write operations fail. The implementation checks that certificate, CA certificate, and key file paths are writable before attempting to fetch new certificates.
- Pre-validates write permissions for X.509 certificate files, CA certificate files, and key files
- Prevents agent from entering inconsistent state when certificate writes fail
- Returns early if LocalCert.Use is true to skip file operations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/config/validate-file-path.go | New file implementing file path validation logic with write permission checks |
| pkg/certificate/service.go | Added validation call in the certificate service run function before certificate requests |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // validate dir path | ||
| target_path = dir_path | ||
| } else { | ||
| return fmt.Errorf("file path not exist: %w, %w", file_err, dir_err) |
There was a problem hiding this comment.
The error message has grammatical issues. Change 'file path not exist' to 'file path does not exist'.
| return fmt.Errorf("file path not exist: %w, %w", file_err, dir_err) | |
| return fmt.Errorf("file path does not exist: %w, %w", file_err, dir_err) |
4 participants
Description
Added implementation to verify path before saving cert file.
Background
The current SIA agent fetches a new certificate from ZTS before attempting to write it to the filesystem. If the file write operation fails for any reason (e.g., incorrect permissions, non-existent directory), the agent enters a state of inconsistency: the new certificate exists in memory, but the old one remains on disk.
This inconsistency prevents future certificate renewals and requires complex manual intervention, such as deleting files and hard-rebooting the instance, to recover the agent.
What's done?
This PR introduces a robust pre-verification check that runs before the agent requests a certificate from ZTS. This check ensures that the followings are writable:
CERT_FILE)CA_CERT_FILE)KEY_FILE)This PR does not include the following:
If file path is not ready, it will output the following as a log:
Assignees
Assigneesis setType of changes
labelsof the following that fits:bug: Bug fixdependencies: Dependency upgradesdocumentation: Documentation changesenhancement: New Featuregood first issue: First contributionlogging: Log changesrefactor: Refactoring (no functional changes, no api changes)Flags
Checklist
Checklist for maintainer