-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat: disable file:// urls when hardening enabled #24858
Conversation
Stacks and templates allow specifying file:// URLs. Add option to disable their use for people who don't require them.
Fix tests on Darwin / MacOS. Also fixes spelling issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A first pass; I can do a deeper review another day. Mostly about file names in errors...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One question, not blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Most of my in-line comments are observations, but there are a few places where it would be good to update comments (eg, remocal and zero size files in /proc).
I also did some manual testing (using the amd64 deb from https://app.circleci.com/pipelines/github/influxdata/influxdb/38891/workflows/ec8d697d-ee31-46a3-9f15-2f1238f2eb8b/jobs/363718/artifacts) for /api/v2/stacks:
$ cat ~/stuff/ok.json
[
{
"apiVersion": "influxdata.com/v2alpha1",
"kind": "Label",
"metadata": {
"name": "label-1"
},
"spec": {
"color": "#eee888",
"description": "desc_1"
}
},
{
"apiVersion": "influxdata.com/v2alpha1",
"kind": "Bucket",
"metadata": {
"name": "rucket-1"
},
"spec": {
"associations": [
{
"kind": "Label",
"name": "label-1"
}
],
"description": "desc_1",
"retentionRules": [
{
"everySeconds": 10000,
"type": "expire"
}
]
}
}
]
# in one terminal, start a http server to serve this over http://
$ cd ~/stuff
$ python3 -m http.server
# via http://
$ curl --insecure -H "Authorization: Token $TOKEN" --request POST "$URL/api/v2/stacks" --data "{\"orgID\":\"$ORGID\", \"name\":\"some name\", \"description\": \"some desc\", \"urls\": [\"http://localhost:8000/ok.json\"]}"
{
"id": "0ce72523377b4000",
"orgID": "697959a536e728a2",
"createdAt": "2024-04-17T10:05:13.181224325-05:00",
...
$ influx apply -o $ORG --stack-id 0ce72523377b4000
LABELS +add | -remove | unchanged
+-----+---------------+------------------+---------------+---------+-------------+
| +/- | METADATA NAME | ID | RESOURCE NAME | COLOR | DESCRIPTION |
+-----+---------------+------------------+---------------+---------+-------------+
| + | label-1 | 0000000000000000 | label-1 | #eee888 | desc_1 |
+-----+---------------+------------------+---------------+---------+-------------+
| TOTAL | 1 |
+-----+---------------+------------------+---------------+---------+-------------+
...
$ influx stacks rm --stack-id 0ce72523377b4000
# now try with file://
$ curl --insecure -H "Authorization: Token $TOKEN" --request POST "$URL/api/v2/stacks" --data "{\"orgID\":\"$ORGID\", \"name\":\"some name\", \"description\": \"some desc\", \"urls\": [\"file:///home/ubuntu/stuff/ok.json\"]}"
{
"id": "0ce725ef413b4000",
"orgID": "697959a536e728a2",
"createdAt": "2024-04-17T10:08:42.116599762-05:00",
...
$ influx apply -o $ORG --stack-id 0ce725ef413b4000
LABELS +add | -remove | unchanged
+-----+---------------+------------------+---------------+---------+-------------+
| +/- | METADATA NAME | ID | RESOURCE NAME | COLOR | DESCRIPTION |
+-----+---------------+------------------+---------------+---------+-------------+
| + | label-1 | 0000000000000000 | label-1 | #eee888 | desc_1 |
+-----+---------------+------------------+---------------+---------+-------------+
| TOTAL | 1 |
+-----+---------------+------------------+---------------+---------+-------------+
...
$ influx stacks rm --stack-id 0ce725ef413b4000
Now try stacks with 'hardening-enabled = true':
# the curl is expected to pass
$ curl --insecure -H "Authorization: Token $TOKEN" --request POST "$URL/api/v2/stacks" --data "{\"orgID\":\"$ORGID\", \"name\":\"some name\", \"description\": \"some desc\", \"urls\": [\"file:///home/ubuntu/stuff/ok.json\"]}"
{
"id": "0ce72690ee408000",
"orgID": "697959a536e728a2",
"createdAt": "2024-04-17T10:11:27.673418946-05:00",
...
# the apply is not
$ influx apply -o $ORG --stack-id 0ce72690ee408000
Error: failed to check template impact: 400 Bad Request: invalid URL scheme: invalid URL scheme
$ influx stacks rm --stack-id 0ce72690ee408000
Now try stacks with just 'template-file-urls-disabled = true':
# the curl is expected to pass
$ curl --insecure -H "Authorization: Token $TOKEN" --request POST "$URL/api/v2/stacks" --data "{\"orgID\":\"$ORGID\", \"name\":\"some name\", \"description\": \"some desc\", \"urls\": [\"file:///home/ubuntu/stuff/ok.json\"]}"
{
"id": "0ce7271e9d86b000",
"orgID": "697959a536e728a2",
"createdAt": "2024-04-17T10:11:27.673418946-05:00",
...
# the apply is not
$ influx apply -o $ORG --stack-id 0ce7271e9d86b000
Error: failed to check template impact: 400 Bad Request: invalid URL scheme: invalid URL scheme
$ influx stacks rm --stack-id 0ce7271e9d86b000
Now try /api/v2/templates/apply directly with neither hardening-enabled nor template-file-urls-disabled set:
# test http:// still works with /api/v2/templates/apply
$ curl --insecure -H "Authorization: Token $TOKEN" -H "Content-Type: application/json" --request POST "$URL/api/v2/templates/apply" --data "{\"orgID\": \"$ORGID\", \"dryRun\": true, \"remotes\": [{\"url\": \"http://localhost:8000/ok.json\"}]}"
{
"sources": [
"http://localhost:8000/ok.json"
],
"stackID": "",
...
# influx apply --encoding json -f works too # the -f path is on the client, not server
$ influx apply --encoding json -f ~/stuff/ok.json
LABELS +add | -remove | unchanged
+-----+---------------+------------------+---------------+---------+-------------+
| +/- | METADATA NAME | ID | RESOURCE NAME | COLOR | DESCRIPTION |
+-----+---------------+------------------+---------------+---------+-------------+
| + | label-1 | 0000000000000000 | label-1 | #eee888 | desc_1 |
+-----+---------------+------------------+---------------+---------+-------------+
| TOTAL | 1 |
+-----+---------------+------------------+---------------+---------+-------------+
...
# now try with file:///home/ubuntu/stuff/ok.json
# Curiously, I was thinking should've been allowed since neither hardening-enabled nor
# template-file-urls-disabled was specified in the config file
$ curl --insecure -H "Authorization: Token $TOKEN" -H "Content-Type: application/json" --request POST "$URL/api/v2/templates/apply" --data "{\"orgID\": \"$ORGID\", \"dryRun\": true, \"remotes\": [{\"url\": \"file:///home/ubuntu/stuff/ok.json\"}]}"
{
"code": "unprocessable entity",
"message": "template from url[file:///home/ubuntu/stuff/ok.json] had an issue: Get \"file:///home/ubuntu/stuff/ok.json\": unsupported protocol scheme \"file\""
I then tried with the official 2.7.6-1 amd64 deb, and it also fails to process file:///home/ubuntu/stuff/ok.json
with /api/v2/templates/apply. I then tried 2.1.1 (which predates #23030, which I though might've changed the behavior) and it fails in the same way.
So, /api/v2/stacks is working as designed by the PR. /api/v2/templates/apply unconditionally denies use of file:/// URLs and has for years. The difference in behavior is odd and makes me want to say that we should unconditionally disallow file:/// URLs in both for consistency, but we know that file:/// URLs are used with stacks, so I don't think we can do that.
Long story short, this PR is functionally correct.
Improve special file system exception checking for tmpfs on Linux to cover most common tmpfs mounts. Improve help for "--hardening-enabled" command line option. Improve some comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably fine as is, but I have a small pedantic suggestion for /dev/shm
and a few comment update suggestions. I'm not sure if the tests are flaky (I didn't check), but fyi, not all the tests are passing.
Add `FromFile` tests that involve symlinks to both special file systems and to files in tmpfs on Linux.
Improve error reporting for `IsSpecialFSFromFileInfo`. This will help debug issues that users might have with the template hardening.
Handle `limitReadFileSize` so that a 0 or negative value implies that file size limits are not applicable. This has no impact with `limitReadFileSize` as a constant, but prepares for this potentially being changed to a configuration in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! LGTM. I like how IsSpecialFSFromFileInfo()
fails closed. One teensy nitpick in-line that you can feel free to ignore.
} | ||
var actualDirs []string | ||
for _, dir := range candidateDirs { | ||
if dir == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: either here or the os.Getenv("XDG_RUNTIME_DIR")
might benefit from a comment that this is for the other. Not a blocker.
Stacks and templates allow specifying file:// URLs. Add option to disable their use for people who don't require them.