-
Notifications
You must be signed in to change notification settings - Fork 28
feat: Add IsUsingSecrets to synthetic_monitoring.Check #1473
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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*\)` + | ||
`)`) | ||
) | ||
Comment on lines
+426
to
+445
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
// 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
) | ||
|
||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()) | ||
}) | ||
} | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}`); | ||
} | ||
} |
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.
I don't think I have a test for this, do I? How is this actually used?