Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
63 changes: 63 additions & 0 deletions pkg/pb/synthetic_monitoring/checks_extra.go
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,69 @@ func (c Check) ConfigVersion() string {
return strconv.FormatInt(int64(c.Modified*1000000000), 10)
}

var (
// This pattern matches "k6/secrets" or 'k6/secrets'. This is the basic canary to tell us if
// a script might be using secrets.
secretsCanary = `(?:'k6/secrets'|"k6/secrets")`

usesSecretsModuleRe = regexp.MustCompile(
`(?m:` +
// Default import: import secrets from 'k6/secrets'
`^\s*import\s+secrets\s+from\s+` + secretsCanary +
`|` +
// Named import: import { } from 'k6/secrets'
// Named import: import { secrets } from 'k6/secrets'
// Named import: import { secrets, foo } from 'k6/secrets'
// Named import: import { foo, secrets } from 'k6/secrets'
// Named import: import { foo, secrets, bar } from 'k6/secrets'
// Named import: import { secrets as s } from 'k6/secrets'
`^\s*import\s*{[^}]*}\s+from\s+` + secretsCanary +
`|` +
// Namespace import: import * as secrets from 'k6/secrets
`^\s*import\s+\*\s+as\s+\S+\s+from\s+` + secretsCanary +
`|` +
// Require syntax: const secrets = require('k6/secrets')
`^\s*const\s+secrets\s*=\s*require\s*\(\s*` + secretsCanary + `\s*\)` +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I have a test for this, do I? How is this actually used?

`)`)
)
Comment on lines +426 to +445
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the bit that is actually interesting.

I have tried to keep the regular expression simple.

It's matching lines (the ?m: bit at the start, so it's possible to write ^\s*import, meaning it's OK to have blanks before import, but import must be the first thing on the line.


// IsUsingSecrets returns true if the check uses secrets, false otherwise.
//
// Depending on the specific check type, the result is completely accurate or just a guess.
func (c Check) IsUsingSecrets() bool {
switch {
case c.Settings.Dns != nil:
return false

case c.Settings.Grpc != nil:
return false

case c.Settings.Ping != nil:
return false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this a good example of the intention: ping will never use secrets, so it will always return false, unconditionally.

OTOH, HTTP will support secrets, so this needs to be updated in the future.


case c.Settings.Tcp != nil:
return false

case c.Settings.Traceroute != nil:
return false

case c.Settings.Http != nil:
return false

case c.Settings.Scripted != nil:
return usesSecretsModuleRe.Match(c.Settings.Scripted.Script)

case c.Settings.Multihttp != nil:
return false

case c.Settings.Browser != nil:
return usesSecretsModuleRe.Match(c.Settings.Browser.Script)

default:
panic("unhandled check type")
}
}

func (c AdHocCheck) Type() CheckType {
switch {
case c.Settings.Dns != nil:
Expand Down
140 changes: 140 additions & 0 deletions pkg/pb/synthetic_monitoring/checks_extra_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,15 @@ import (
"encoding/json"
"errors"
"flag"
"io/fs"
"os"
"path/filepath"
"reflect"
"strings"
"testing"

"github.com/rs/zerolog"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -2045,3 +2049,139 @@ func TestRemoteInfoMarshalJSON(t *testing.T) {
actual := strings.TrimSpace(buf.String())
require.Equal(t, expected, actual, "JSON encoding of RemoteInfo did not match expected output")
}

// TestUsesSecretsModuleRe tests that the regular expression used to detect the use of secrets works as expected.
//
// When modifying the regular expression, add a case in testdata, following the following naming convention:
//
// * script_using_secrets_<description>.js: should be reported as using secrets
// * script_not_using_secrets_<description>.js: should NOT be reported as using secrets
func TestUsesSecretsModuleRe(t *testing.T) {
t.Parallel()

testdataDir := `testdata`
testdataDirFS := os.DirFS(testdataDir)
filenames, err := fs.Glob(testdataDirFS, "script_*.js")
require.NoError(t, err)

for _, filename := range filenames {
switch {
case strings.HasPrefix(filename, `script_using_secrets_`):
script, err := os.ReadFile(filepath.Join(testdataDir, filename))
require.NoError(t, err)
assert.Truef(t, usesSecretsModuleRe.Match(script), "%s should report true", filename)

case strings.HasPrefix(filename, `script_not_using_secrets_`):
script, err := os.ReadFile(filepath.Join(testdataDir, filename))
require.NoError(t, err)
assert.False(t, usesSecretsModuleRe.Match(script), "%s should report false", filename)

default: // skip
}
}
}

func TestCheckIsUsingSecrets(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is not trying to be exhaustive (that's why the above test exists). This test is just trying to exercise the switch.

t.Parallel()

generateCheck := func(target string, settings CheckSettings) Check {
return Check{
Id: 1,
TenantId: 1000,
Target: target,
Job: "job",
Frequency: 60000,
Timeout: 10000,
Probes: []int64{1},
Settings: settings,
}
}

mustReadFile := func(filename string) []byte {
contents, err := os.ReadFile(filename)
require.NoError(t, err)

return contents
}

testcases := map[string]struct {
check Check
expected bool
}{
"ping": {
check: generateCheck(
"127.0.0.1",
CheckSettings{Ping: &PingSettings{}},
),
expected: false,
},
"dns": {
check: generateCheck(
"127.0.0.1",
CheckSettings{Dns: &DnsSettings{}},
),
expected: false,
},
"http": {
check: generateCheck(
"http://127.0.0.1/",
CheckSettings{Http: &HttpSettings{}},
),
expected: false,
},
"tcp": {
check: generateCheck(
"127.0.0.1:80",
CheckSettings{Tcp: &TcpSettings{}},
),
expected: false,
},
"grpc": {
check: generateCheck(
"127.0.0.1",
CheckSettings{Grpc: &GrpcSettings{}},
),
expected: false,
},
"traceroute": {
check: generateCheck(
"127.0.0.1",
CheckSettings{Traceroute: &TracerouteSettings{}},
),
expected: false,
},
"multihttp": {
check: generateCheck(
"https://grafana.com/",
CheckSettings{Multihttp: &MultiHttpSettings{}},
),
expected: false,
},
"scripted": {
check: generateCheck(
"https://grafana.com/",
CheckSettings{Scripted: &ScriptedSettings{
Script: mustReadFile(`testdata/script_using_secrets_default_import_1.js`),
}},
),
expected: true,
},
"browser": {
check: generateCheck(
"https://grafana.com/",
CheckSettings{Browser: &BrowserSettings{
Script: mustReadFile(`testdata/script_using_secrets_browser_default_import_1.js`),
}},
),
expected: true,
},
}

for name, testcase := range testcases {
t.Run(name, func(t *testing.T) {
t.Parallel()

require.Equal(t, testcase.expected, testcase.check.IsUsingSecrets())
})
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@d0ugal sorry to make this PR harder to understand.

What's important here is the name of the file. That should provide a hint as to the intention. For example this one is NOT using secrets, and it has the import bit commented out.

Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { check, fail } from 'k6'
import http from 'k6/http'
// import secrets from 'k6/secrets'

export default async () => {
const my_secret = await secrets.get('secret_name');
const result = http.get('https://grafana.com/', {
headers: { 'X-My-Header': my_secret },
});

const pass = check(result, {
'is status 200': (r) => r.status === 200,
});

if (!pass) {
fail(`non 200 result ${result.status}`);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { check, fail } from 'k6'
import http from 'k6/http'

export default function() {
const result = http.get('https://grafana.com/');

const pass = check(result, {
'is status 200': (r) => r.status === 200,
});

if (!pass) {
fail(`non 200 result ${result.status}`);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { check, fail } from 'k6'
import http from 'k6/http'

// k6/secrets does not have documented side-effects, the following code shouldn't do anything. For
// that reason, this script is not considered as using secrets.
import 'k6/secrets'

export default function() {
const result = http.get('https://grafana.com/');

const pass = check(result, {
'is status 200': (r) => r.status === 200,
});

if (!pass) {
fail(`non 200 result ${result.status}`);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { check, fail } from 'k6'
import http from 'k6/http'

// k6/secrets does not have documented side-effects, the following code shouldn't do anything. For
// that reason, this script is not considered as using secrets.
import "k6/secrets"

export default function() {
const result = http.get('https://grafana.com/');

const pass = check(result, {
'is status 200': (r) => r.status === 200,
});

if (!pass) {
fail(`non 200 result ${result.status}`);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { check } from 'k6'
import { browser } from 'k6/browser'
import secrets from 'k6/secrets'

export const options = {
scenarios: {
ui: {
options: {
browser: {
type: 'chromium',
},
},
},
},
};

export default async function() {
const my_secret = await secrets.get('secret_name');
const context = await browser.newContext();
const page = await context.newPage();

try {
await page.goto("https://test.k6.io/my_messages.php");

await page.locator('input[name="login"]').type("admin");
await page.locator('input[name="password"]').type(my_secret);

await Promise.all([
page.waitForNavigation(),
page.locator('input[type="submit"]').click(),
]);

await check(page.locator("h2"), {
header: async (locator) => (await locator.textContent()) == "Welcome, admin!",
});
} catch (e) {
console.log('Error during execution:', e);
throw e;
} finally {
await page.close();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { check, fail } from 'k6'
import http from 'k6/http'
import secrets from 'k6/secrets'

export default async () => {
const my_secret = await secrets.get('secret_name');
const result = http.get('https://grafana.com/', {
headers: { 'X-My-Header': my_secret },
});

const pass = check(result, {
'is status 200': (r) => r.status === 200,
});

if (!pass) {
fail(`non 200 result ${result.status}`);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { check, fail } from 'k6'
import http from 'k6/http'
import secrets from "k6/secrets"

export default async () => {
const my_secret = await secrets.get('secret_name');
const result = http.get('https://grafana.com/', {
headers: { 'X-My-Header': my_secret },
});

const pass = check(result, {
'is status 200': (r) => r.status === 200,
});

if (!pass) {
fail(`non 200 result ${result.status}`);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { check, fail } from 'k6'
import http from 'k6/http'
// *shrug*
// import secrets from 'k6/secrets'
import secrets from 'k6/secrets'

export default async () => {
const my_secret = await secrets.get('secret_name');
const result = http.get('https://grafana.com/', {
headers: { 'X-My-Header': my_secret },
});

const pass = check(result, {
'is status 200': (r) => r.status === 200,
});

if (!pass) {
fail(`non 200 result ${result.status}`);
}
}
Loading
Loading