Skip to content

Commit be7f4c0

Browse files
rmuirwoodruffw
rmuir
and
woodruffw
authored
feat(secrets-outside-env): allowlist secrets in config (#1759)
* feat(secrets-outside-env): allowlist secrets in config The `secrets-outside-env` audit gains an `allow` list in the configuration file of secret names that should be ignored by the audit. Signed-off-by: Robert Muir <rmuir@apache.org> * test(secrets-outside-env): add test for base config as well * docs: fix grammatical error * test(secrets-outside-env): add some corner-case tests around secret name Test that GITHUB_TOKEN works as both an index access and a property dereference. Also test that case is ignored when comparing strings. This exception works correctly, since it uses ContextPattern. Test that allowlist works as an index access. This currently works because `single_tail()` takes care. Case-insensitivity does NOT yet work. * fix(secrets-outside-env): use case-insensitive matching Simple solution that uses ascii lowercasing. There's a TODO to use a fancier approach via ContextPattern... * Single-source the allowlist Signed-off-by: William Woodruff <william@yossarian.net> * Clippy Signed-off-by: William Woodruff <william@yossarian.net> * Record changes Signed-off-by: William Woodruff <william@yossarian.net> * Formatting Signed-off-by: William Woodruff <william@yossarian.net> * Bump schema Signed-off-by: William Woodruff <william@yossarian.net> --------- Signed-off-by: Robert Muir <rmuir@apache.org> Signed-off-by: William Woodruff <william@yossarian.net> Co-authored-by: William Woodruff <william@yossarian.net>
1 parent 3b81688 commit be7f4c0

File tree

14 files changed

+417
-10
lines changed

14 files changed

+417
-10
lines changed

‎.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,3 +14,4 @@
1414

1515
# benchmarks
1616
.codspeed/
17+
bench/__pycache__/

‎crates/zizmor/src/audit/secrets_outside_env.rs‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ impl Audit for SecretsOutsideEnvironment {
3333
async fn audit_normal_job<'doc>(
3434
&self,
3535
job: &NormalJob<'doc>,
36-
_config: &Config,
36+
config: &Config,
3737
) -> Result<Vec<Finding<'doc>>, AuditError> {
3838
if job.parent().has_workflow_call() {
3939
// Reusable workflows and environments don't interact well, and are more or less
@@ -71,9 +71,14 @@ impl Audit for SecretsOutsideEnvironment {
7171
continue;
7272
}
7373

74-
if context.matches("secrets.GITHUB_TOKEN") {
75-
// GITHUB_TOKEN is always latently available, so we don't
76-
// flag its usage outside of a dedicated environment.
74+
// Check to see whether we allow this secret. The policy always includes
75+
// GITHUB_TOKEN, since it's always latently available.
76+
if let Some(secret_name) = context.single_tail()
77+
&& config
78+
.secrets_outside_env_policy
79+
.allow
80+
.contains(&secret_name.to_ascii_lowercase())
81+
{
7782
continue;
7883
}
7984

‎crates/zizmor/src/config/mod.rs‎

Lines changed: 57 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,10 @@
1-
use std::{collections::HashMap, fs, num::NonZeroUsize, ops::Deref, str::FromStr};
1+
use std::{
2+
collections::{HashMap, HashSet},
3+
fs,
4+
num::NonZeroUsize,
5+
ops::Deref,
6+
str::FromStr,
7+
};
28

39
use anyhow::{Context as _, anyhow};
410
use camino::Utf8Path;
@@ -16,7 +22,7 @@ use crate::{
1622
App, CollectionOptions,
1723
audit::{
1824
AuditCore, dependabot_cooldown::DependabotCooldown, forbidden_uses::ForbiddenUses,
19-
unpinned_uses::UnpinnedUses,
25+
secrets_outside_env::SecretsOutsideEnvironment, unpinned_uses::UnpinnedUses,
2026
},
2127
finding::Finding,
2228
github::{Client, ClientError},
@@ -228,6 +234,47 @@ pub(crate) enum ForbiddenUsesConfigInner {
228234
Deny(Vec<RepositoryUsesPattern>),
229235
}
230236

237+
/// Configuration for the `secrets-outside-env` audit.
238+
#[derive(Clone, Debug, Default, Deserialize)]
239+
#[serde(rename_all = "kebab-case", deny_unknown_fields)]
240+
#[serde(default)]
241+
#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))]
242+
pub(crate) struct SecretsOutsideEnvConfig {
243+
/// List of secret names excluded from the audit
244+
pub(crate) allow: Vec<String>,
245+
}
246+
247+
/// Policy to evaluate secrets references
248+
#[derive(Clone, Debug)]
249+
pub(crate) struct SecretsOutsideEnvPolicy {
250+
/// List of secret names excluded from the audit
251+
pub(crate) allow: HashSet<String>,
252+
}
253+
254+
impl Default for SecretsOutsideEnvPolicy {
255+
fn default() -> Self {
256+
let mut allow = HashSet::new();
257+
allow.insert("github_token".into());
258+
259+
Self { allow }
260+
}
261+
}
262+
263+
impl From<SecretsOutsideEnvConfig> for SecretsOutsideEnvPolicy {
264+
fn from(value: SecretsOutsideEnvConfig) -> Self {
265+
let mut allow = value
266+
.allow
267+
.iter()
268+
.map(|item| item.to_ascii_lowercase())
269+
.collect::<HashSet<_>>();
270+
271+
let default = Self::default();
272+
allow.extend(default.allow);
273+
274+
Self { allow }
275+
}
276+
}
277+
231278
/// # Configuration for the `unpinned-uses` audit.
232279
///
233280
/// This configuration is reified into an `UnpinnedUsesPolicies`.
@@ -396,6 +443,7 @@ pub(crate) struct Config {
396443
raw: RawConfig,
397444
pub(crate) dependabot_cooldown_config: DependabotCooldownConfig,
398445
pub(crate) forbidden_uses_config: Option<ForbiddenUsesConfig>,
446+
pub(crate) secrets_outside_env_policy: SecretsOutsideEnvPolicy,
399447
pub(crate) unpinned_uses_policies: UnpinnedUsesPolicies,
400448
}
401449

@@ -410,6 +458,12 @@ impl Config {
410458

411459
let forbidden_uses_config = raw.rule_config(ForbiddenUses::ident())?;
412460

461+
let secrets_outside_env_config =
462+
raw.rule_config::<SecretsOutsideEnvConfig>(SecretsOutsideEnvironment::ident())?;
463+
let secrets_outside_env_policy = secrets_outside_env_config
464+
.map(Into::into)
465+
.unwrap_or_default();
466+
413467
let unpinned_uses_policies = {
414468
if let Some(unpinned_uses_config) =
415469
raw.rule_config::<UnpinnedUsesConfig>(UnpinnedUses::ident())?
@@ -424,6 +478,7 @@ impl Config {
424478
raw,
425479
dependabot_cooldown_config,
426480
forbidden_uses_config,
481+
secrets_outside_env_policy,
427482
unpinned_uses_policies,
428483
})
429484
}

‎crates/zizmor/src/config/schema.rs‎

Lines changed: 51 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@
1111

1212
use schemars::JsonSchema;
1313

14-
use super::{DependabotCooldownConfig, ForbiddenUsesConfig, UnpinnedUsesConfig, WorkflowRule};
14+
use super::{
15+
DependabotCooldownConfig, ForbiddenUsesConfig, SecretsOutsideEnvConfig, UnpinnedUsesConfig,
16+
WorkflowRule,
17+
};
1518

1619
/// Base configuration for all audit rules.
1720
#[derive(Clone, Debug, Default, JsonSchema)]
@@ -46,6 +49,17 @@ struct ForbiddenUsesRuleConfig {
4649
config: Option<ForbiddenUsesConfig>,
4750
}
4851

52+
/// Configuration for the `secrets-outside-env` audit.
53+
#[derive(Clone, Debug, Default, JsonSchema)]
54+
#[serde(deny_unknown_fields)]
55+
struct SecretsOutsideEnvRuleConfig {
56+
#[serde(flatten)]
57+
base: BaseRuleConfig,
58+
59+
#[serde(default)]
60+
config: Option<SecretsOutsideEnvConfig>,
61+
}
62+
4963
/// Configuration for the `unpinned-uses` audit.
5064
#[derive(Debug, Default, JsonSchema)]
5165
#[serde(deny_unknown_fields)]
@@ -106,11 +120,11 @@ define_audit_rules! {
106120
concurrency_limits,
107121
archived_uses,
108122
misfeature,
109-
secrets_outside_env,
110123
superfluous_actions;
111124

112125
[DependabotCooldownRuleConfig] dependabot_cooldown,
113126
[ForbiddenUsesRuleConfig] forbidden_uses,
127+
[SecretsOutsideEnvRuleConfig] secrets_outside_env,
114128
[UnpinnedUsesRuleConfig] unpinned_uses,
115129
}
116130

@@ -246,6 +260,41 @@ mod tests {
246260
.expect("forbidden uses deny config should be valid");
247261
}
248262

263+
#[test]
264+
fn test_secrets_outside_env_config() {
265+
let secrets_allow = r#"
266+
rules:
267+
secrets-outside-env:
268+
config:
269+
allow:
270+
- NOT_SO_SECRET
271+
- ALSO_NOT_SO_SECRET
272+
"#;
273+
274+
let instance = serde_yaml::from_str::<serde_json::Value>(secrets_allow).unwrap();
275+
SCHEMA_VALIDATOR
276+
.validate(&instance)
277+
.expect("secrets-outside-env allow config should be valid");
278+
}
279+
280+
#[test]
281+
fn test_secrets_outside_env_base_plus_config() {
282+
let secrets_allow = r#"
283+
rules:
284+
secrets-outside-env:
285+
config:
286+
allow:
287+
- NOT_SO_SECRET
288+
ignore:
289+
- foo.yml
290+
"#;
291+
292+
let instance = serde_yaml::from_str::<serde_json::Value>(secrets_allow).unwrap();
293+
SCHEMA_VALIDATOR
294+
.validate(&instance)
295+
.expect("secrets-outside-env allow config with base config should be valid");
296+
}
297+
249298
#[test]
250299
fn test_unpinned_uses_config() {
251300
let valid = r#"

‎crates/zizmor/tests/integration/audit/secrets_outside_env.rs‎

Lines changed: 125 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use anyhow::Result;
22

3-
use crate::common::{input_under_test, zizmor};
3+
use crate::common::{OutputMode, input_under_test, zizmor};
44

55
#[test]
66
fn test_secrets_outside_env() -> Result<()> {
@@ -27,3 +27,127 @@ fn test_secrets_outside_env() -> Result<()> {
2727

2828
Ok(())
2929
}
30+
31+
#[test]
32+
fn test_config_invalid_variant() -> Result<()> {
33+
insta::assert_snapshot!(
34+
zizmor()
35+
.expects_failure(1)
36+
.input(input_under_test("neutral.yml"))
37+
.config(input_under_test("secrets-outside-env/configs/invalid-variant.yml"))
38+
.output(OutputMode::Stderr)
39+
.run()?,
40+
@"
41+
🌈 zizmor v@@VERSION@@
42+
fatal: no audit was performed
43+
error: configuration error in @@CONFIG@@
44+
|
45+
= help: check the configuration for the 'secrets-outside-env' rule
46+
= help: see: https://docs.zizmor.sh/audits/#secrets-outside-env-configuration
47+
48+
Caused by:
49+
0: configuration error in @@CONFIG@@
50+
1: invalid syntax for audit `secrets-outside-env`
51+
2: unknown field `mystery-variant`, expected `allow`
52+
"
53+
);
54+
Ok(())
55+
}
56+
57+
#[test]
58+
fn test_config_allow_none() -> Result<()> {
59+
insta::assert_snapshot!(
60+
zizmor()
61+
.args(["--persona=auditor"])
62+
.input(input_under_test("secrets-outside-env/multiple-secrets.yml"))
63+
.config(input_under_test("secrets-outside-env/configs/allow-none.yml"))
64+
.run()?,
65+
@"
66+
warning[secrets-outside-env]: secrets referenced without a dedicated environment
67+
--> @@INPUT@@:20:30
68+
|
69+
12 | test:
70+
| ---- this job
71+
...
72+
20 | NOT_SO_SECRET: ${{ secrets.NOT_SO_SECRET }}
73+
| ^^^^^^^^^^^^^^^^^^^^^ secret is accessed outside of a dedicated environment
74+
|
75+
= note: audit confidence → High
76+
77+
warning[secrets-outside-env]: secrets referenced without a dedicated environment
78+
--> @@INPUT@@:25:30
79+
|
80+
12 | test:
81+
| ---- this job
82+
...
83+
25 | NOT_SO_SECRET: ${{ secrets.not_so_secret }}
84+
| ^^^^^^^^^^^^^^^^^^^^^ secret is accessed outside of a dedicated environment
85+
|
86+
= note: audit confidence → High
87+
88+
warning[secrets-outside-env]: secrets referenced without a dedicated environment
89+
--> @@INPUT@@:30:30
90+
|
91+
12 | test:
92+
| ---- this job
93+
...
94+
30 | NOT_SO_SECRET: ${{ secrets['not_so_secret'] }}
95+
| ^^^^^^^^^^^^^^^^^^^^^^^^ secret is accessed outside of a dedicated environment
96+
|
97+
= note: audit confidence → High
98+
99+
warning[secrets-outside-env]: secrets referenced without a dedicated environment
100+
--> @@INPUT@@:35:35
101+
|
102+
12 | test:
103+
| ---- this job
104+
...
105+
35 | ALSO_NOT_SO_SECRET: ${{ secrets.ALSO_NOT_SO_SECRET }}
106+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ secret is accessed outside of a dedicated environment
107+
|
108+
= note: audit confidence → High
109+
110+
4 findings: 0 informational, 0 low, 4 medium, 0 high
111+
"
112+
);
113+
Ok(())
114+
}
115+
116+
#[test]
117+
fn test_config_allow_one() -> Result<()> {
118+
insta::assert_snapshot!(
119+
zizmor()
120+
.args(["--persona=auditor"])
121+
.input(input_under_test("secrets-outside-env/multiple-secrets.yml"))
122+
.config(input_under_test("secrets-outside-env/configs/allow-one.yml"))
123+
.run()?,
124+
@"
125+
warning[secrets-outside-env]: secrets referenced without a dedicated environment
126+
--> @@INPUT@@:35:35
127+
|
128+
12 | test:
129+
| ---- this job
130+
...
131+
35 | ALSO_NOT_SO_SECRET: ${{ secrets.ALSO_NOT_SO_SECRET }}
132+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ secret is accessed outside of a dedicated environment
133+
|
134+
= note: audit confidence → High
135+
136+
1 finding: 0 informational, 0 low, 1 medium, 0 high
137+
"
138+
);
139+
Ok(())
140+
}
141+
142+
#[test]
143+
fn test_config_allow_some() -> Result<()> {
144+
insta::assert_snapshot!(
145+
zizmor()
146+
.args(["--persona=auditor"])
147+
.input(input_under_test("secrets-outside-env/multiple-secrets.yml"))
148+
.config(input_under_test("secrets-outside-env/configs/allow-some.yml"))
149+
.run()?,
150+
@"No findings to report. Good job!"
151+
);
152+
Ok(())
153+
}

‎crates/zizmor/tests/integration/test-data/secrets-outside-env/configs/allow-none.yml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
# zizmor.yml config file allowing empty list of secrets for 'secrets-outside-env' rule
2+
3+
rules:
4+
secrets-outside-env:
5+
config:
6+
allow: []

‎crates/zizmor/tests/integration/test-data/secrets-outside-env/configs/allow-one.yml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
# zizmor.yml config file allowing individual secret for the 'secrets-outside-env' rule
2+
3+
rules:
4+
secrets-outside-env:
5+
config:
6+
allow:
7+
- NOT_SO_SECRET

‎crates/zizmor/tests/integration/test-data/secrets-outside-env/configs/allow-some.yml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
# zizmor.yml config file allowing multiple secrets for the 'secrets-outside-env' rule
2+
3+
rules:
4+
secrets-outside-env:
5+
config:
6+
allow:
7+
- NOT_SO_SECRET
8+
- ALSO_NOT_SO_SECRET

‎crates/zizmor/tests/integration/test-data/secrets-outside-env/configs/invalid-variant.yml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
# zizmor.yml config file with an invalid schema for the 'secrets-outside-env' rule.
2+
3+
rules:
4+
secrets-outside-env:
5+
config:
6+
mystery-variant: 42

0 commit comments

Comments
 (0)