🌱 Add support for SSL env vars to cert pool watcher #1672
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1672 +/- ##
==========================================
+ Coverage 67.42% 67.49% +0.07%
==========================================
Files 55 57 +2
Lines 4632 4630 -2
==========================================
+ Hits 3123 3125 +2
+ Misses 1284 1278 -6
- Partials 225 227 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
internal/httputil/certpoolwatcher.go
Outdated
| // specified, this means that we have some control over the system root | ||
| // location, thus they may change, thus we should watch those locations. | ||
| if d := os.Getenv("SSL_CERT_DIR"); d != "" { | ||
| dirs := strings.Split(d, ":") |
There was a problem hiding this comment.
| dirs := strings.Split(d, ":") | |
| dirs := filepath.SplitList(d) |
There was a problem hiding this comment.
The separater is explicitly ":", and this is exactly how the golang library crypto/x509 does it, so I'm a bit reluctant to change it.
There was a problem hiding this comment.
Ok, if crypto/x509 does it with simple strings.Split, I'm good.
| if err = watcher.Add(f); err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: collect the paths in step 1, slices.DeleteFunc to stat and remove in step 2, loop over remaining to call watcher.Add in step 3?
There was a problem hiding this comment.
watchPaths := []string{caDir, os.Getenv("SSL_CERT_DIR"), os.Getenv("SSL_CERT_FILE")}
slices.DeleteFunc(watchPaths, func(p string) bool { /* do stat here */})
for _, p := range watchPaths {
if err := watcher.Add(p); err != nil {
return nil, err
}
}
The SystemRoot store looks at the SSL_CERT_DIR and SSL_CERT_FILE environment variables for certificate locations. Because these variables are under control of the user, we should assume that the user wants to control the contents of the SystemRoot, and subsequently that those contents could change (as compared to certs located in the default /etc/pki location). Thus, we should watch those locations if they exist. Signed-off-by: Todd Short <[email protected]>
|
New changes are detected. LGTM label has been removed. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
The SystemRoot store looks at the SSL_CERT_DIR and SSL_CERT_FILE environment variables for certificate locations. Because these variables are under control of the user, we should assume that the user wants to control the contents of the SystemRoot, and subsequently that those contents could change (as compared to certs located in the default /etc/pki location).
Thus, we should watch those locations if they exist.