feat: Enforce strict crypto only: remove DisableWeakAlgorithms option#2
feat: Enforce strict crypto only: remove DisableWeakAlgorithms option#2
Conversation
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
There was a problem hiding this comment.
Pull request overview
This PR removes the disable_weak_algorithms configuration knob and makes the project’s cryptographic posture strictly “secure-only” by default, ensuring weak algorithms (notably MD5) and legacy TLS settings cannot be re-enabled.
Changes:
- Introduces a centralized
security.CryptoConfigthat always enforces TLS 1.2+ and a strict cipher policy. - Removes MD5 support from the encode tool (schema/docs + runtime behavior + tests).
- Wires the strict TLS policy into outbound clients (HTTP via
httputil, and IMAP via the email tool), and updates docs/examples accordingly.
Reviewed changes
Copilot reviewed 41 out of 42 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/tools/encodetool/provider.go | Changes provider construction to accept security.CryptoConfig. |
| pkg/tools/encodetool/encodetool_test.go | Updates tests to assert MD5 is rejected; adjusts provider construction. |
| pkg/tools/encodetool/encodetool.go | Removes MD5 from supported ops/schema and rejects MD5 requests with a policy error. |
| pkg/tools/email/email.go | Adds optional TLS config injection for IMAP and clones before dialing. |
| pkg/security/crypto.go | Adds centralized crypto/TLS policy (TLSConfig, cipher suite list, NIST constants). |
| pkg/security/config.go | Extends security config to include crypto policy alongside secrets config. |
| pkg/httputil/client.go | Adds global default TLS config wiring for HTTP clients/transports. |
| pkg/app/app.go | Applies the strict TLS config at bootstrap; wires IMAP TLS config and updated encode provider. |
| examples/ticket-to-pr/AGENTS.md | Updates an architecture diagram (contains a new typo). |
| docs/js/config-builder.js | Adjusts TOML emission logic for [security.secrets]. |
| config.toml.example | Updates security section comments to reflect secure-only crypto policy. |
| SECURITY.md | Updates vulnerability handling expectations and adds delivery/crypto practice notes. |
| README.md | Adds badges and formatting updates. |
| Agents.md | Adds mandatory crypto policy rules/checklist entries referencing the new centralized policy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sks
force-pushed
the
feature/remove_weak_algo
branch
from
0f0e542 to
7af932a
Compare
|
Addressed all Copilot review comments:
All review threads have been resolved. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
pkg/httputil/client.go:40
GetClient()hard-codes a 10shttp.Client.Timeout. Several callers in this PR (e.g., Ollama model pulls, webfetch) previously relied on longer timeouts or caller-provided context deadlines, so this fixed timeout can cause requests to be canceled prematurely. Consider adding aGetClientWithTimeout(timeout time.Duration, enhancers ...RequestEnhancer)(or an options struct) and/or settingTimeout: 0by default and enforcing timeouts viacontext.WithTimeoutat call sites that need a cap.
func GetClient(requestEnhancer ...RequestEnhancer) *http.Client {
return &http.Client{
Timeout: time.Second * 10,
Transport: NewRoundTripper(requestEnhancer...),
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func newFetchTools() *fetchTools { | ||
| return &fetchTools{ | ||
| client: &http.Client{ | ||
| Timeout: defaultTimeout, | ||
| }, | ||
| client: httputil.GetClient(), | ||
| } |
There was a problem hiding this comment.
The package comment and defaultTimeout constant state a 30s HTTP timeout, but newFetchTools() now uses httputil.GetClient() which currently sets a 10s http.Client.Timeout. This makes the documented behavior incorrect and may cause premature timeouts on slower sites. Either restore a 30s timeout for this tool (e.g., a GetClientWithTimeout(defaultTimeout) helper) or enforce the 30s limit via context.WithTimeout inside fetch().
pkg/tools/google/oauth/token.go
Outdated
| // Use oauth2.NewClient so the client gets refreshed tokens from savingTS; | ||
| // wrap with httputil's round tripper for consistent TLS (NIST 2030). | ||
| baseTransport := httputil.NewRoundTripper() | ||
| client := &http.Client{Transport: &oauth2.Transport{Source: savingTS, Base: baseTransport}} | ||
| return client, nil |
There was a problem hiding this comment.
The comment says “Use oauth2.NewClient…”, but the code constructs &http.Client{Transport: &oauth2.Transport{...}} directly. This is misleading for future maintainers (especially around how token refresh is wired). Either update the comment to reflect the actual approach (oauth2.Transport with Base round tripper) or switch to oauth2.NewClient and then wrap/replace the transport as intended.
sks
force-pushed
the
feature/remove_weak_algo
branch
from
c2201f4 to
2f62549
Compare
sks
force-pushed
the
feature/remove_weak_algo
branch
from
2f62549 to
4788b56
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
pkg/httputil/client.go:40
GetClient()always constructs a brand-newhttp.Clientwith a brand-newhttp.Transport. Several call sites invokeGetClient()per request (e.g. web search / model reachability), which defeats connection pooling/keep-alives and can increase resource usage under load. Consider makingGetClientreturn a shared client (or at least a shared underlying Transport) and/or providing an option-based constructor so callers can reuse clients while still applying the default TLS policy.
func GetClient(requestEnhancer ...RequestEnhancer) *http.Client {
return &http.Client{
Timeout: time.Second * 10,
Transport: NewRoundTripper(requestEnhancer...),
}
pkg/expert/modelprovider/ollama.go:105
httputil.GetClient()enforces a fixed 10shttp.Client.Timeout, butPullModelis documented to take many minutes and relies on a long-lived context. With the current client, pulls will reliably fail after ~10s regardless of the context deadline. Consider using a client/transport without a hard timeout for this call (rely on the context), or extendhttputilto support a caller-specified timeout and use a long timeout here.
req.Header.Set("Content-Type", "application/json")
// Pull can take many minutes for large models.
client := &http.Client{Timeout: 15 * time.Minute}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/tools/google/oauth/token.go
Outdated
| // Use oauth2.NewClient so the client gets refreshed tokens from savingTS; | ||
| // wrap with httputil's round tripper for consistent TLS (NIST 2030). | ||
| baseTransport := httputil.NewRoundTripper() | ||
| client := &http.Client{Transport: &oauth2.Transport{Source: savingTS, Base: baseTransport}} | ||
| return client, nil |
There was a problem hiding this comment.
The comment says “Use oauth2.NewClient…”, but the implementation builds &http.Client{Transport: &oauth2.Transport{...}} directly. Either switch to oauth2.NewClient(ctx, savingTS) (then wrap its transport) or adjust the comment so it accurately reflects the current approach.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 48 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| client.Client = httputil.GetClient(func(req *http.Request) { | ||
| req.Header.Set("Authorization", "Bearer "+cfg.Token) | ||
| }) |
There was a problem hiding this comment.
Switching Bitbucket’s underlying http.Client from a custom transport with no overall timeout to httputil.GetClient() introduces a fixed 10s http.Client.Timeout. SCM operations can legitimately exceed 10s on slower networks or large responses, so this may cause unexpected failures. Consider setting an explicit, more suitable timeout here (or none) while still using httputil.NewRoundTripper() for centralized TLS settings and the Authorization header enhancer.
| client := httputil.GetClient() | ||
| resp, err := client.Do(httpReq) |
There was a problem hiding this comment.
httputil.GetClient() constructs a new http.Client with a new http.Transport each time. Calling it on every search prevents connection reuse and can leak resources (transports keep idle goroutines/conns unless reused/closed). Consider storing a client on bingTool (created once in NewBingTool) and reusing it for all requests.
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, url+"/api/tags", nil) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| client := &http.Client{Timeout: 2 * time.Second} | ||
| resp, err := client.Do(req) | ||
| resp, err := httputil.GetClient().Do(req) | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
httputil.GetClient() creates a new http.Client + http.Transport each call; using it inside OllamaReachable/ListModels prevents connection reuse and can leave idle transports around. Consider reusing a shared client (package-level or passed in) so repeated setup/health checks don’t allocate a new transport each time.
Summary
Remove the
disable_weak_algorithmsconfiguration option and make secure development the only mode. Weak algorithms (e.g. MD5, TLS < 1.2, weak ciphers) are always disabled; there is no way to enable them.Changes
Security
pkg/security/crypto.go: DroppedDisableWeakAlgorithmsfromCryptoConfig.TLSConfig()always uses TLS 1.2+ and the strict cipher list.pkg/security/config.go: Updated comment to state that weak algorithms are always disabled.Encode tool
pkg/tools/encodetool: MD5 is always disabled. The encode tool no longer offers or acceptsmd5; requests formd5return an error directing users to SHA-256. Removedcrypto/md5and thedisableMD5flag from the implementation.pkg/tools/encodetool/provider.go:NewToolProviderstill acceptsCryptoConfigfor API compatibility; behavior is always strict.Config & docs
docs/js/config-builder.js: Removed the "Disable weak algorithms" toggle and alldisable_weak_algorithmsstate/TOML/YAML handling.config.toml.example: Removed the commented[security.crypto]/disable_weak_algorithmsblock; added a single line noting that weak algorithms are always disabled.AGENTS.md: Updated the [crypto_keylength] rule and checklist so that weak algorithms are always disabled with no config option.Behavior after this PR
md5requests get a security-policy error and a hint to use SHA-256.disable_weak_algorithms(or equivalent) anywhere; secure defaults are the only mode.