add `allow_net` to capabilities, use it to disable fetching remote schemas #3748

srenatus · 2021-08-18T09:41:46Z

Introducing a package-level var to gojsonschema isn't the prettiest solution,
but since we want this in an all-or-nothing way right now anyways, it does
the trick. And it's more ergonomic than adding extra parameters all over the
place.

Fixes #3746.

Also:

move some profiling-related default params into newEvalCommandParams
replace some errors.Wrap by fmt.Errorf in loader pkg
remove some != nil handling where it didn't make a difference when working on the schema set
reduces indentation in code examples in opa eval -h and opa check -h by replacing tabs by four spaces.

It feels to me like the addition to capabilities is a bit dangling right now. We should perhaps tackle #3665 soon.

srenatus · 2021-08-18T11:42:26Z

While adding that 18K JSON file is somewhat annoying, it's nice to know that we can read it as-is, and stuff works. So, I'm torn, but leaning towards including it 🤔

tsandall

This look good to me. The only question in my head is whether we should make the CLI tooling default to fetch remote schemas. It's obviously more consistent with the implementation in the compiler to disable it by default, however, I'm just concerned about the UX when people are actually making use of real-world JSON schemas. IOW, to use OPA CLI tooling and real-world JSON schemas, are users always going to have to specify --fetch-remote-schemas? That seems cumbersome. WDYT?

While adding that 18K JSON file is somewhat annoying, it's nice to know that we can read it as-is, and stuff works. So, I'm torn, but leaning towards including it 🤔

This sounds fine to me--we'll forget it's there after this PR is merged.

srenatus · 2021-08-19T08:52:15Z

IOW, to use OPA CLI tooling and real-world JSON schemas, are users always going to have to specify --fetch-remote-schemas? That seems cumbersome. WDYT?

You're probably right. I'll flip the default here, and the flag name accordingly.

💭 I wonder if a general "don't do anything network-y" flag would be a good idea. This could be even more relevant when imports come from URLs one day. (Akin to deno's network capability, maybe, https://deno.land/manual/getting_started/permissions#permissions-list)

srenatus · 2021-08-19T12:20:44Z

Flipped all the things 🙃

tsandall · 2021-08-19T15:26:38Z

💭 I wonder if a general "don't do anything network-y" flag would be a good idea. This could be even more relevant when imports come from URLs one day. (Akin to deno's network capability, maybe, https://deno.land/manual/getting_started/permissions#permissions-list)

I've wondered whether we could use the capabilities feature for this...

srenatus · 2021-08-20T09:35:08Z

I've wondered whether we could use the capabilities feature for this...

Actually, yeah, I think we should. So, to keep this backwards-compatible, we'd introduce some key to capabilities that lets you disable networking, default to true. Or we could try to mimic deno even more and allow for specifying hostnames, like https://deno.land/manual/getting_started/permissions#network-access. Deno's default is disallow, so we'd be in the slightly odd but acceptable position that

specifying nothing allows all networking
specifying something only allows those hosts, denying the rest.

This would probably be the same user interface that could support #3665.

What do you think?

Introducing a package-level var to gojsonschema isn't the prettiest solution, but since we want this in an all-or-nothing way right now anyways, it does the trick. And it's more ergonomic than adding extra parameters all over the place. Fixes open-policy-agent#3746. Also: * move some profiling-related default params into newEvalCommandParams * replace some errors.Wrap by fmt.Errorf in loader pkg * remove some != nil handling where it didn't make a difference when working on the schema set Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

It would be nice to ensure that the remote refs feature actually works, without introducing a network dependency into our tests. This commit adds the kube 1.14 definitions into ast/testdata, and uses that from a httptest.Server instance in the unit tests. Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

srenatus · 2021-08-24T08:36:39Z

So, this requires adding the --capabilities flag to opa eval. I had actually been surprised that it wasn't there. It would make sense to for example restrict the capabilities of an opa eval call, although it can only subset the builtins available; not change them, of course.

When adding an allow_net capability key to capabilities.json, it could be deceiving in opa build: it would not be baked into the bundle. The capabilities that matter would still be the one used with opa eval.

This renders them exactly as they look in the multiline strings, not with the excessive indentation that a tab might bring. Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

There are follow-up tasks once capabilities are available to `opa eval`, but this commit restricts its being put to use to the type checker's remote json schema refs. Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

srenatus · 2021-08-24T10:37:05Z

cmd/eval.go

@@ -248,6 +273,7 @@ Loads a single JSON file, applying it to the input document; or all the schema f
 	evalCommand.Flags().BoolVarP(&params.failDefined, "fail-defined", "", false, "exits with non-zero exit code on defined/non-empty result and errors")

 	// Shared flags
+	addCapabilitiesFlag(evalCommand.Flags(), params.capabilities)


A fun side effect of this is that we can now use opa eval to eval like it's 20202:

$ opa eval -fpretty --capabilities capabilities/v0.17.0.json 'base64.is_valid("foo")' 1 error occurred: 1:1: rego_type_error: undefined function base64.is_valid

tsandall

Nice! ~~Can you file a follow-up issue to add support into the http.send() built-in function? We'll need to be careful to ensure that's implemented correctly (e.g., we'll have to deal with redirects...)~~

Nevermind! We have this one #3665

…hemas (open-policy-agent#3748) This adds a new top-level key to the capabilities structure, `allow_net`. It currently is only used for restricting the typechecker's ability to fetch remote refs in JSON schemas, but could be used more widely in the future. It works like this: - If it's not present, any host can be contacted - If it's present, the items will be the hosts or IP addresses that may be contacted; anything not in the list is prohibited. - As a consequence, If it's present and empty (`[]`), no host can be contacted Introducing a package-level var to gojsonschema isn't the prettiest solution, but since we want this in an all-or-nothing way right now anyways, it does the trick. And it's more ergonomic than adding extra parameters all over the place. Fixes open-policy-agent#3746. Also: * move some profiling-related default params into newEvalCommandParams * replace some errors.Wrap by fmt.Errorf in loader pkg * remove some != nil handling where it didn't make a difference when working on the schema set * reduces indentation in code examples in `opa eval -h` and `opa check -h` by replacing tabs by four spaces. * ast: allow testing with remote refs without networking It would be nice to ensure that the remote refs feature actually works, without introducing a network dependency into our tests. This commit adds the kube 1.14 definitions into ast/testdata, and uses that from a httptest.Server instance in the unit tests. Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com> Signed-off-by: Dolev Farhi <farhi.dolev@gmail.com>

srenatus force-pushed the sr/typechecker/dont-fetch-remote-schemas-without-opt-in branch from c073455 to b674f1c Compare August 18, 2021 11:40

srenatus force-pushed the sr/typechecker/dont-fetch-remote-schemas-without-opt-in branch from b674f1c to 846c657 Compare August 18, 2021 11:54

srenatus marked this pull request as ready for review August 18, 2021 11:59

tsandall reviewed Aug 18, 2021

View reviewed changes

srenatus marked this pull request as draft August 19, 2021 08:52

srenatus force-pushed the sr/typechecker/dont-fetch-remote-schemas-without-opt-in branch 3 times, most recently from aaa69bd to 2409174 Compare August 19, 2021 12:08

srenatus changed the title ~~cmd: add flag to enable fetching remote schemas~~ cmd: add flag to disable fetching remote schemas Aug 19, 2021

srenatus marked this pull request as ready for review August 19, 2021 12:20

srenatus added 3 commits August 24, 2021 10:27

[squash] flip flag and default

e72b7b2

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

cmd: replace tabs by four spaces in help texts

7f381c4

This renders them exactly as they look in the multiline strings, not with the excessive indentation that a tab might bring. Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

srenatus force-pushed the sr/typechecker/dont-fetch-remote-schemas-without-opt-in branch 2 times, most recently from 9360d52 to 4176f9b Compare August 24, 2021 10:05

srenatus force-pushed the sr/typechecker/dont-fetch-remote-schemas-without-opt-in branch from 4176f9b to a381a58 Compare August 24, 2021 10:14

srenatus changed the title ~~cmd: add flag to disable fetching remote schemas~~ add allow_net to capabilities, use it to disable fetching remote schemas Aug 24, 2021

srenatus commented Aug 24, 2021

View reviewed changes

tsandall approved these changes Aug 24, 2021

View reviewed changes

tsandall mentioned this pull request Aug 24, 2021

Add support for AllowNet capability in http.send #3665

Closed

srenatus merged commit 0efa2f0 into open-policy-agent:main Aug 24, 2021

srenatus deleted the sr/typechecker/dont-fetch-remote-schemas-without-opt-in branch August 24, 2021 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `allow_net` to capabilities, use it to disable fetching remote schemas #3748

add `allow_net` to capabilities, use it to disable fetching remote schemas #3748

srenatus commented Aug 18, 2021 •

edited

srenatus commented Aug 18, 2021

tsandall left a comment •

edited

srenatus commented Aug 19, 2021

srenatus commented Aug 19, 2021

tsandall commented Aug 19, 2021

srenatus commented Aug 20, 2021

srenatus commented Aug 24, 2021 •

edited

srenatus Aug 24, 2021 •

edited

tsandall left a comment •

edited

add allow_net to capabilities, use it to disable fetching remote schemas #3748

add allow_net to capabilities, use it to disable fetching remote schemas #3748

Conversation

srenatus commented Aug 18, 2021 • edited

srenatus commented Aug 18, 2021

tsandall left a comment • edited

Choose a reason for hiding this comment

srenatus commented Aug 19, 2021

srenatus commented Aug 19, 2021

tsandall commented Aug 19, 2021

srenatus commented Aug 20, 2021

srenatus commented Aug 24, 2021 • edited

srenatus Aug 24, 2021 • edited

Choose a reason for hiding this comment

tsandall left a comment • edited

Choose a reason for hiding this comment

add `allow_net` to capabilities, use it to disable fetching remote schemas #3748

add `allow_net` to capabilities, use it to disable fetching remote schemas #3748

srenatus commented Aug 18, 2021 •

edited

tsandall left a comment •

edited

srenatus commented Aug 24, 2021 •

edited

srenatus Aug 24, 2021 •

edited

tsandall left a comment •

edited