topdown+wasm: Verifying host based on allow_net allowlist in built-in functions #4152
Conversation
|
This implementation only applies to the |
johanfylling
force-pushed
the
feature/3665/http_send_allow_net
branch
from
5f22d52 to
161cdbe
Compare
… functions Adding host allow-listing based on the allow_net capability in the http.send()- and net.lookup_ip_addr() built-in functions when running the eval command. Fixes: open-policy-agent#3665 Signed-off-by: Johan Fylling <johan.dev@fylling.se>
johanfylling
force-pushed
the
feature/3665/http_send_allow_net
branch
from
161cdbe to
de8875c
Compare
johanfylling
changed the title
ast/compile.go
Outdated
| @@ -86,6 +86,9 @@ type Compiler struct { | |||
| // with the key being the generated name and value being the original. | |||
| RewrittenVars map[Var]Var | |||
|
|
|||
| // Capabilities is the user-supplied capabilities or features allowed for OPA. | |||
| Capabilities *Capabilities | |||
There was a problem hiding this comment.
Exposing this technically means it could be messed with. However, WithCapabilities() already allows that, so I guess we're OK. Alternatively, we could do
func (c *Compiler) Capabilities() *Capabilities {
return c.capabilities
}There was a problem hiding this comment.
There already is precedence for direct access for other fields, but if a function is preferred, I can add one. The only drawback would be that it might look haphazard to a user.
There was a problem hiding this comment.
I've been thinking about this, too, it's the only part of this PR that worries me. Once we expose this as a field, we can never go back or change or structures or something.
Having a getter function would leave us done wiggle room. I'd prefer we go with that.
There was a problem hiding this comment.
Alright! Will fix.
rego/rego_wasmtarget_test.go
Outdated
|
|
||
| serverUrl, err := url.Parse(ts.URL) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
[nit]
| panic(err) | |
| t.Fatal(err) |
topdown/http_test.go
Outdated
|
|
||
| resultObj, err := ast.InterfaceToValue(expectedResult) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
[nit] Let's either do t.Fatal(err) here, or use resultObj := ast.MustInterfaceToValue(expectedResult) above
There was a problem hiding this comment.
Right, will fix.
| "allow_net match + additional host": {addr, "example.com"}, | ||
| } { | ||
| t.Run(name, func(t *testing.T) { | ||
| ctx, cancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
[nit] Do we somehow need the cancel here? We night just do ctx := context.Background(); but I might be missing something.
There was a problem hiding this comment.
No, I thing you're right. This is a copy/paste I didn't clean up properly.
There was a problem hiding this comment.
Hm, all other test cases use cancel. I'll keep this as-is, just to be in line with the rest of them. However, this might be unnecessary in all cases.
| @@ -305,7 +335,7 @@ func createHTTPRequest(bctx BuiltinContext, obj ast.Object) (*http.Request, *htt | |||
| var strVal string | |||
|
|
|||
| if s, ok := obj.Get(val).Value.(ast.String); ok { | |||
| strVal = string(s) | |||
| strVal = strings.Trim(string(s), "\"") | |||
There was a problem hiding this comment.
It's OK given the existing code, but something here doesn't seem correct to me. Why are there extra "?
There was a problem hiding this comment.
Yea, I don't get this either; but I didn't want to change existing functionality.
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Good point! Will find a suitable place to add to the docs. |
Adding documentation. Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Adding host allow-listing based on the allow_net capability in the http.send()- and
net.lookup_ip_addr() built-in functions when running the eval command.
Fixes: #3665
Signed-off-by: Johan Fylling johan.dev@fylling.se