Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

type-checking with jsonschema should not fetch remote schema refs without opt-in #3746

Closed
srenatus opened this issue Aug 17, 2021 · 2 comments · Fixed by #3748
Closed

type-checking with jsonschema should not fetch remote schema refs without opt-in #3746

srenatus opened this issue Aug 17, 2021 · 2 comments · Fixed by #3748
Assignees

Comments

@srenatus
Copy link
Contributor

Tests have made it obvious that schemas that reference remote schemas cause network calls:

--- FAIL: TestSetTypesWithSchemaRef (10.03s)
    schema_test.go:53: unexpected error: compile failed: unable to compile the schema due to: Get "https://kubernetesjsonschema.dev/v1.14.0/_definitions.json": net/http: TLS handshake timeout
FAIL

The offending http.Get is here.

  1. This should be something disabled by default, and opted into voluntarily.
  2. ❓ It's a naked Get -- no context parameters, no concern for any REST auth plugins.

I'd think fixing (2.) isn't as urgent as fixing (1.), though.

@tsandall
Copy link
Member

👍 let's get (1) solved for the next release. I'm not sure if anyone is relying on the remote schema loading feature today. @aavarghese do you know if any of your users are relying on this?

@tsandall
Copy link
Member

@vazirim said that many of the schemas they have include remote refs but the feature is not actively used so it should be fine to disable for now.

@srenatus srenatus self-assigned this Aug 18, 2021
srenatus added a commit to srenatus/opa that referenced this issue Aug 24, 2021
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>
srenatus added a commit that referenced this issue Aug 24, 2021
…hemas (#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 #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>
dolevf pushed a commit to dolevf/opa that referenced this issue Nov 4, 2021
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants