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.

Sign up for GitHub

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

Jump to bottom

fix(templates): disable use of jsonnet with /api/v2/templates/apply #23030

Merged
merged 1 commit into from
Dec 30, 2021
Merged

fix(templates): disable use of jsonnet with /api/v2/templates/apply #23030

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion pkger/doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ templates for what will eventually come to support all influxdb
resources.

The parser supports JSON, Jsonnet, and YAML encodings as well as a number
of different ways to read the file/reader/string as it may.
of different ways to read the file/reader/string as it may. While the parser
supports Jsonnet, due to issues in the go-jsonnet implementation, only trusted
input should be given to the parser and therefore the EnableJsonnet() option
must be used with Parse() to enable Jsonnet support.

As an example, you can use the following to parse and validate a YAML
file and see a summary of its contents:
Expand Down
35 changes: 35 additions & 0 deletions pkger/http_server_template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"io"
"net/http"
"net/url"
"path"
"strings"

Expand Down Expand Up @@ -332,6 +333,40 @@ func (s *HTTPServerTemplates) apply(w http.ResponseWriter, r *http.Request) {
return
}

// Reject use of server-side jsonnet with /api/v2/templates/apply
if encoding == EncodingJsonnet {
s.api.Err(w, r, &errors.Error{
Code: errors.EUnprocessableEntity,
Msg: fmt.Sprintf("template from source(s) had an issue: %s", ErrInvalidEncoding.Error()),
})
return
}

var remotes []string
for _, rem := range reqBody.Remotes {
remotes = append(remotes, rem.URL)
}
remotes = append(remotes, reqBody.RawTemplate.Sources...)

for _, rem := range remotes {
// While things like '.%6Aonnet' evaluate to the default encoding (yaml), let's unescape and catch those too
decoded, err := url.QueryUnescape(rem)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to return the error here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or does that let too much security information escape?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be some security concern, although there shouldn't be much of an issue since our api.Err(...) methods return sanitized errors to clients. Mostly I am keeping these consistent with the IDPE error messages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see @jdstrand has already merged the code in IDPE, so probably we shouldn't become incompatible.

Copy link
Contributor

@jdstrand jdstrand Jan 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're referring to the specific error from url.QueryUnescape() instead of a generic error, I erred on the side of caution since I felt legitimate use of the API should not be causing this error and so giving the attacker a generic error was fine. If we find this to be problematic in practice, we can return a more specific error.

if err != nil {
s.api.Err(w, r, &errors.Error{
Code: errors.EInvalid,
Msg: fmt.Sprintf("template from url[%q] had an issue", rem),
})
return
}
if len(decoded) > 0 && strings.HasSuffix(strings.ToLower(decoded), "jsonnet") {
s.api.Err(w, r, &errors.Error{
Code: errors.EUnprocessableEntity,
Msg: fmt.Sprintf("template from url[%q] had an issue: %s", rem, ErrInvalidEncoding.Error()),
})
return
}
}

var stackID platform.ID
if reqBody.StackID != nil {
if err := stackID.DecodeFromString(*reqBody.StackID); err != nil {
Expand Down
220 changes: 207 additions & 13 deletions pkger/http_server_template_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@ import (
"github.com/influxdata/influxdb/v2"
pcontext "github.com/influxdata/influxdb/v2/context"
"github.com/influxdata/influxdb/v2/kit/platform"
influxerror "github.com/influxdata/influxdb/v2/kit/platform/errors"
kithttp "github.com/influxdata/influxdb/v2/kit/transport/http"
"github.com/influxdata/influxdb/v2/mock"
"github.com/influxdata/influxdb/v2/pkg/testttp"
"github.com/influxdata/influxdb/v2/pkger"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/zap"
"go.uber.org/zap/zaptest/observer"
"gopkg.in/yaml.v3"
)

Expand Down Expand Up @@ -106,6 +108,138 @@ func TestPkgerHTTPServerTemplate(t *testing.T) {
})

t.Run("dry run pkg", func(t *testing.T) {
t.Run("jsonnet disabled", func(t *testing.T) {
tests := []struct {
name string
contentType string
reqBody pkger.ReqApply
}{
{
name: "app jsonnet disabled",
contentType: "application/x-jsonnet",
reqBody: pkger.ReqApply{
DryRun: true,
OrgID: platform.ID(9000).String(),
RawTemplate: bucketPkgKinds(t, pkger.EncodingJsonnet),
},
},
{
name: "retrieves package from a URL (jsonnet disabled)",
reqBody: pkger.ReqApply{
DryRun: true,
OrgID: platform.ID(9000).String(),
Remotes: []pkger.ReqTemplateRemote{{
URL: newPkgURL(t, filesvr.URL, "testdata/bucket_associates_labels_one.jsonnet"),
}},
},
},
{
name: "app json with jsonnet disabled remote",
contentType: "application/json",
reqBody: pkger.ReqApply{
DryRun: true,
OrgID: platform.ID(9000).String(),
RawTemplate: bucketPkgJsonWithJsonnetRemote(t),
},
},
}

for _, tt := range tests {
fn := func(t *testing.T) {
svc := &fakeSVC{
dryRunFn: func(ctx context.Context, orgID, userID platform.ID, opts ...pkger.ApplyOptFn) (pkger.ImpactSummary, error) {
var opt pkger.ApplyOpt
for _, o := range opts {
o(&opt)
}
pkg, err := pkger.Combine(opt.Templates)
if err != nil {
return pkger.ImpactSummary{}, err
}

if err := pkg.Validate(); err != nil {
return pkger.ImpactSummary{}, err
}
sum := pkg.Summary()
var diff pkger.Diff
for _, b := range sum.Buckets {
diff.Buckets = append(diff.Buckets, pkger.DiffBucket{
DiffIdentifier: pkger.DiffIdentifier{
MetaName: b.Name,
},
})
}
return pkger.ImpactSummary{
Summary: sum,
Diff: diff,
}, nil
},
}

core, sink := observer.New(zap.InfoLevel)
pkgHandler := pkger.NewHTTPServerTemplates(zap.New(core), svc)
svr := newMountedHandler(pkgHandler, 1)

ctx := context.Background()
testttp.
PostJSON(t, "/api/v2/templates/apply", tt.reqBody).
Headers("Content-Type", tt.contentType).
WithCtx(ctx).
Do(svr).
ExpectStatus(http.StatusUnprocessableEntity).
ExpectBody(func(buf *bytes.Buffer) {
var resp pkger.RespApply
decodeBody(t, buf, &resp)

assert.Len(t, resp.Summary.Buckets, 0)
assert.Len(t, resp.Diff.Buckets, 0)
})

// Verify logging when jsonnet is disabled
entries := sink.TakeAll() // resets to 0
if tt.contentType == "application/x-jsonnet" {
require.Equal(t, 1, len(entries))
// message 0
require.Equal(t, zap.ErrorLevel, entries[0].Entry.Level)
require.Equal(t, "api error encountered", entries[0].Entry.Message)
assert.ElementsMatch(t, []zap.Field{
zap.Error(&influxerror.Error{
Code: influxerror.EUnprocessableEntity,
Msg: "template from source(s) had an issue: invalid encoding provided",
},
)}, entries[0].Context)
} else if len(tt.reqBody.Remotes) == 1 && strings.HasSuffix(tt.reqBody.Remotes[0].URL, "jsonnet") {
require.Equal(t, 1, len(entries))
// message 0
require.Equal(t, zap.ErrorLevel, entries[0].Entry.Level)
require.Equal(t, "api error encountered", entries[0].Entry.Message)
expMsg := fmt.Sprintf("template from url[\"%s\"] had an issue: invalid encoding provided", tt.reqBody.Remotes[0].URL)
assert.ElementsMatch(t, []zap.Field{
zap.Error(&influxerror.Error{
Code: influxerror.EUnprocessableEntity,
Msg: expMsg,
},
)}, entries[0].Context)
} else if len(tt.reqBody.RawTemplate.Sources) == 1 && strings.HasSuffix(tt.reqBody.RawTemplate.Sources[0], "jsonnet") {
require.Equal(t, 1, len(entries))
// message 0
require.Equal(t, zap.ErrorLevel, entries[0].Entry.Level)
require.Equal(t, "api error encountered", entries[0].Entry.Message)
expMsg := fmt.Sprintf("template from url[\"%s\"] had an issue: invalid encoding provided", tt.reqBody.RawTemplate.Sources[0])
assert.ElementsMatch(t, []zap.Field{
zap.Error(&influxerror.Error{
Code: influxerror.EUnprocessableEntity,
Msg: expMsg,
},
)}, entries[0].Context)
} else {
require.Equal(t, 0, len(entries))
}
}
t.Run(tt.name, fn)
}
})

t.Run("json", func(t *testing.T) {
tests := []struct {
name string
Expand All @@ -130,7 +264,7 @@ func TestPkgerHTTPServerTemplate(t *testing.T) {
},
},
{
name: "retrieves package from a URL",
name: "retrieves package from a URL (json)",
reqBody: pkger.ReqApply{
DryRun: true,
OrgID: platform.ID(9000).String(),
Expand All @@ -139,15 +273,6 @@ func TestPkgerHTTPServerTemplate(t *testing.T) {
}},
},
},
{
name: "app jsonnet",
contentType: "application/x-jsonnet",
reqBody: pkger.ReqApply{
DryRun: true,
OrgID: platform.ID(9000).String(),
RawTemplate: bucketPkgKinds(t, pkger.EncodingJsonnet),
},
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -182,7 +307,8 @@ func TestPkgerHTTPServerTemplate(t *testing.T) {
},
}

pkgHandler := pkger.NewHTTPServerTemplates(zap.NewNop(), svc)
core, _ := observer.New(zap.InfoLevel)
pkgHandler := pkger.NewHTTPServerTemplates(zap.New(core), svc)
svr := newMountedHandler(pkgHandler, 1)

testttp.
Expand All @@ -198,7 +324,6 @@ func TestPkgerHTTPServerTemplate(t *testing.T) {
assert.Len(t, resp.Diff.Buckets, 1)
})
}

t.Run(tt.name, fn)
}
})
Expand Down Expand Up @@ -566,6 +691,48 @@ func TestPkgerHTTPServerTemplate(t *testing.T) {
})
})
})

t.Run("Templates()", func(t *testing.T) {
tests := []struct {
name string
reqBody pkger.ReqApply
encoding pkger.Encoding
}{
{
name: "jsonnet disabled",
reqBody: pkger.ReqApply{
OrgID: platform.ID(9000).String(),
RawTemplate: bucketPkgKinds(t, pkger.EncodingJsonnet),
},
encoding: pkger.EncodingJsonnet,
},
{
name: "jsonnet remote disabled",
reqBody: pkger.ReqApply{
OrgID: platform.ID(9000).String(),
Remotes: []pkger.ReqTemplateRemote{{
URL: newPkgURL(t, filesvr.URL, "testdata/bucket_associates_labels_one.jsonnet"),
}},
},
encoding: pkger.EncodingJsonnet,
},
{
name: "jsonnet disabled remote source",
reqBody: pkger.ReqApply{
OrgID: platform.ID(9000).String(),
RawTemplate: bucketPkgJsonWithJsonnetRemote(t),
},
encoding: pkger.EncodingJSON,
},
}

for _, tt := range tests {
tmpl, err := tt.reqBody.Templates(tt.encoding)
assert.Nil(t, tmpl)
require.Error(t, err)
assert.Equal(t, "unprocessable entity", influxerror.ErrorCode(err))
}
})
}

func assertNonZeroApplyResp(t *testing.T, resp pkger.RespApply) {
Expand Down Expand Up @@ -644,7 +811,7 @@ spec:
require.FailNow(t, "invalid encoding provided: "+encoding.String())
}

pkg, err := pkger.Parse(encoding, pkger.FromString(fmt.Sprintf(pkgStr, pkger.APIVersion)))
pkg, err := pkger.Parse(encoding, pkger.FromString(fmt.Sprintf(pkgStr, pkger.APIVersion)), pkger.EnableJsonnet())
require.NoError(t, err)

b, err := pkg.Encode(encoding)
Expand All @@ -656,6 +823,33 @@ spec:
}
}

func bucketPkgJsonWithJsonnetRemote(t *testing.T) pkger.ReqRawTemplate {
pkgStr := `[
{
"apiVersion": "%[1]s",
"kind": "Bucket",
"metadata": {
"name": "rucket-11"
},
"spec": {
"description": "bucket 1 description"
}
}
]
`
// Create a json template and then add a jsonnet remote raw template
pkg, err := pkger.Parse(pkger.EncodingJSON, pkger.FromString(fmt.Sprintf(pkgStr, pkger.APIVersion)))
require.NoError(t, err)

b, err := pkg.Encode(pkger.EncodingJSON)
require.NoError(t, err)
return pkger.ReqRawTemplate{
ContentType: pkger.EncodingJsonnet.String(),
Sources: []string{"file:///nonexistent.jsonnet"},
Template: b,
}
}

func newReqApplyYMLBody(t *testing.T, orgID platform.ID, dryRun bool) *bytes.Buffer {
t.Helper()

Expand Down
23 changes: 20 additions & 3 deletions pkger/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,16 @@ func parseJSON(r io.Reader, opts ...ValidateOptFn) (*Template, error) {
}

func parseJsonnet(r io.Reader, opts ...ValidateOptFn) (*Template, error) {
return parse(jsonnet.NewDecoder(r), opts...)
opt := &validateOpt{}
for _, o := range opts {
o(opt)
}
// For security, we'll default to disabling parsing jsonnet but allow callers to override the behavior via
// EnableJsonnet(). Enabling jsonnet might be useful for client code where parsing jsonnet could be acceptable.
if opt.enableJsonnet {
return parse(jsonnet.NewDecoder(r), opts...)
}
return nil, fmt.Errorf("%s: jsonnet", ErrInvalidEncoding)
}

func parseSource(r io.Reader, opts ...ValidateOptFn) (*Template, error) {
Expand Down Expand Up @@ -536,14 +545,22 @@ func Combine(pkgs []*Template, validationOpts ...ValidateOptFn) (*Template, erro

type (
validateOpt struct {
minResources bool
skipValidate bool
minResources bool
skipValidate bool
enableJsonnet bool
}

// ValidateOptFn provides a means to disable desired validation checks.
ValidateOptFn func(*validateOpt)
)

// Jsonnet parsing is disabled by default. EnableJsonnet turns it back on.
func EnableJsonnet() ValidateOptFn {
return func(opt *validateOpt) {
opt.enableJsonnet = true
}
}

// ValidWithoutResources ignores the validation check for minimum number
// of resources. This is useful for the service Create to ignore this and
// allow the creation of a pkg without resources.
Expand Down
Loading