rego: fail closed on external_data provider errors#705
rego: fail closed on external_data provider errors#705
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the Rego external_data builtin to fail closed by propagating provider/cache/TLS/HTTP/JSON failures as builtin errors, preventing policies from bypassing constraints by ignoring system_error.
Changes:
- Change
external_databuiltin to returnerror(instead of encoding failures into a normal Rego response term). - Remove the now-unused
net/httpimport from the builtin implementation. - Update
TestDriver_ExternalDataexpectations for error scenarios.
Show a summary per file
| File | Description |
|---|---|
constraint/pkg/client/drivers/rego/builtin.go |
Propagates provider/TLS/request errors as builtin errors (fail closed) and removes unused import. |
constraint/pkg/client/drivers/rego/driver_unit_test.go |
Adjusts external_data unit test expectations for failure scenarios. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
| if tt.errorExpected { | ||
| if err == nil { | ||
| t.Fatalf("got Query() error = nil, want non-nil") | ||
| } | ||
| return | ||
| } | ||
| if tt.errorExpected && len(qr.Results) == 0 { | ||
| t.Fatalf("got 0 errors on normal query; want 1") | ||
| if err != nil { | ||
| t.Fatalf("got Query() error = %v, want nil", err) | ||
| } | ||
| if !tt.errorExpected && len(qr.Results) > 0 { | ||
| if len(qr.Results) > 0 { |
There was a problem hiding this comment.
Test expectations no longer match Driver.Query behavior. Query() intentionally converts Rego eval errors into violation results (see driver.go) and still returns a nil error; with external_data now returning an error from the builtin, Query() will likely return err == nil and qr.Results > 0. Update this test to assert on qr.Results (and possibly the msg contents) for failure cases, or change Driver.Query to propagate eval errors if the new contract is intentional.
| provider, err := d.providerCache.Get(regoReq.ProviderName) | ||
| if err != nil { | ||
| return externaldata.HandleError(http.StatusBadRequest, err) | ||
| return nil, err | ||
| } | ||
|
|
||
| clientCert, err := d.getTLSCertificate() | ||
| if err != nil { | ||
| return externaldata.HandleError(http.StatusBadRequest, err) | ||
| return nil, err | ||
| } | ||
|
|
||
| externaldataResponse, statusCode, err := d.sendRequestToProvider(bctx.Context, &provider, providerRequestKeys, clientCert) | ||
| if err != nil { | ||
| return externaldata.HandleError(statusCode, err) | ||
| return nil, err |
There was a problem hiding this comment.
Now that external_data errors are propagated as builtin errors (and ultimately surfaced via Query()'s error-to-violation conversion), consider wrapping these returned errors with context like provider name and (for request failures) the HTTP status code. Returning the raw underlying error strings (e.g. "key is not found in provider cache") makes diagnosing which provider/operation failed harder once it reaches users/logs.
JaydipGabani
left a comment
JaydipGabani
left a comment
There was a problem hiding this comment.
This should not be changed, returning rego response was intentional, changing this may break existing behavior.
IMO close this pr without merging.
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
sozercan
force-pushed
the
codex/fix-external_data-error-handling
branch
from
cd40586 to
db39515
Compare
Motivation
external_databuiltin converted provider/cache/TLS/HTTP/JSON errors into a normal Rego response term, allowing constraints to be bypassed if they ignoresystem_error.Description
external_dataimplementation instead of encoding failures into aRegoResponse, by replacingexternaldata.HandleError(...)withreturn nil, errinconstraint/pkg/client/drivers/rego/builtin.go.net/httpimport fromconstraint/pkg/client/drivers/rego/builtin.go.TestDriver_ExternalDatainconstraint/pkg/client/drivers/rego/driver_unit_test.goso failure scenarios assert thatQueryreturns a non-nil error while successful responses keep the existing behavior.Testing
go test ./pkg/client/drivers/rego -run TestDriver_ExternalData -count=1, but the run timed out in this environment (EXIT:124).go test -v ./pkg/client/drivers/rego -run 'TestDriver_ExternalData/provider_not_found' -count=1, which also timed out in this environment (EXIT:124).Codex Task