Skip to content

Commit 5e1c805

Browse files
gustavoavenameta-codesync[bot]
authored andcommitted
restricted_paths: run shadow path comparison logging
Summary: ## This stack This stack moves restricted paths toward AclManifest-backed restriction lookup and Shadow-mode comparison logging while keeping existing `RestrictedPaths` public APIs and config-backed enforcement behavior stable. The end state is that path and manifest access checks can evaluate both the legacy config/manifest-id-store source and the new AclManifest source, log source-prefixed comparison results to Scuba, and still return config-authoritative authorization results while Shadow mode is being validated. Before this stack, restricted path logging was effectively single-source: path access read `path_acls` from config, manifest access read restricted paths from the manifest-id store, and Scuba rows only described the aggregate config-backed result. That made it hard to prove whether AclManifest-derived restrictions matched production behavior before flipping traffic to them. After this stack, lookup and logging are split into source-specific primitives: config and AclManifest restriction lookup live behind explicit source selectors, authorization result construction is shared, and Shadow mode dispatch runs both sources side by side for supported path and HgAugmented manifest accesses. Scuba rows now include aggregate config-authoritative fields plus `config_*`, `acl_manifest_*`, `acl_manifest_mode`, errors, and `considered_restricted_by`, so we can compare behavior without changing enforcement. ```text Before: path access -> config path_acls -> aggregate auth/log row manifest access -> manifest-id store + config -> aggregate auth/log row After: path access -> config path_acls -> authoritative result -> AclManifest at changeset -> shadow comparison fields -> Scuba: aggregate + config_* + acl_manifest_* + considered_restricted_by HgAugmented manifest access -> config manifest-id store -> authoritative result -> manifest acl_manifest pointer -> shadow comparison fields -> Scuba: aggregate + config_* + acl_manifest_* + considered_restricted_by ``` This makes the codebase better by separating lookup, authorization, and logging responsibilities; making source choice explicit in tests and implementation; and giving us production observability for AclManifest parity before changing enforcement. Follow-up work can use the Shadow data to fix mismatches, expand support beyond HgAugmented manifest logging, move repos toward `Both`/`Authoritative` modes, and eventually remove legacy manifest-type handling and config-only fallbacks once AclManifest is proven safe. ## This diff (behaviour change) This diff wires Shadow-mode path access through config and AclManifest source checks. This is a behavior change in Shadow mode only: path access now logs source comparison rows when either source finds a restriction or errors, while the returned authorization result remains config-authoritative. Reviewed By: lmvasquezg Differential Revision: D103436517 fbshipit-source-id: b4797cb3c4ce7c7eec937e06f9a1f71fb96c2b8b
1 parent 6b0c9a9 commit 5e1c805

5 files changed

Lines changed: 232 additions & 53 deletions

File tree

eden/mononoke/repo_attributes/restricted_paths/src/access_log.rs

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -122,13 +122,6 @@ pub(crate) struct SourceRestrictionCheckResult {
122122
pub(crate) is_rollout_allowlisted: bool,
123123
}
124124

125-
#[cfg_attr(
126-
not(test),
127-
expect(
128-
dead_code,
129-
reason = "implemented before Shadow dispatch wires production callers"
130-
)
131-
)]
132125
impl SourceRestrictionCheckResult {
133126
pub(crate) fn new(
134127
has_authorization: bool,
@@ -254,13 +247,6 @@ pub async fn is_part_of_group(
254247
/// config-authoritative, while source disagreement is summarized with compact
255248
/// mismatch fields. If every available source completed unrestricted, no row
256249
/// is written.
257-
#[cfg_attr(
258-
not(test),
259-
expect(
260-
dead_code,
261-
reason = "implemented before Shadow dispatch wires production callers"
262-
)
263-
)]
264250
pub(crate) fn log_source_results_to_scuba(
265251
ctx: &CoreContext,
266252
repo_id: RepositoryId,

eden/mononoke/repo_attributes/restricted_paths/src/lib.rs

Lines changed: 70 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,12 @@ use tokio::task;
3838

3939
pub use crate::access_log::ACCESS_LOG_SCUBA_TABLE;
4040
pub use crate::access_log::RestrictionCheckResult;
41+
use crate::access_log::SourceRestrictionResult;
4142
pub use crate::access_log::has_read_access_to_repo_region_acls;
4243
use crate::access_log::is_member_of_groups;
4344
use crate::access_log::log_access_to_restricted_path;
45+
use crate::access_log::log_source_results_to_scuba;
46+
use crate::restriction_check::PathRestrictionSource;
4447
pub use crate::restriction_info::ManifestRestrictionInfo;
4548
pub use crate::restriction_info::PathRestrictionInfo;
4649

@@ -133,7 +136,9 @@ impl RestrictedPaths {
133136
/// from `.slacl` files in the repo, so callers cannot treat a false
134137
/// config-only lookup as proof that no restricted paths exist.
135138
pub fn may_have_restricted_paths(&self) -> bool {
136-
self.use_acl_manifest || self.config_based.has_restricted_paths()
139+
self.use_acl_manifest
140+
|| self.config_based.has_restricted_paths()
141+
|| self.config().acl_manifest_mode == AclManifestMode::Shadow
137142
}
138143

139144
/// Returns the soft path ACLs configuration.
@@ -321,6 +326,12 @@ impl RestrictedPaths {
321326
path: NonRootMPath,
322327
cs_id: Option<ChangesetId>,
323328
) -> Result<RestrictionCheckResult> {
329+
if !self.use_acl_manifest && self.config().acl_manifest_mode == AclManifestMode::Shadow {
330+
return self
331+
.log_shadow_access_by_path_if_restricted(ctx, path, cs_id)
332+
.await;
333+
}
334+
324335
// Return early if the repo doesn't have any restricted paths.
325336
let _ = cs_id; // Will be used in a follow-up diff for ACL manifest checks
326337
if self.config().is_empty() {
@@ -362,6 +373,53 @@ impl RestrictedPaths {
362373
.await
363374
}
364375

376+
async fn log_shadow_access_by_path_if_restricted(
377+
&self,
378+
ctx: &CoreContext,
379+
path: NonRootMPath,
380+
cs_id: Option<ChangesetId>,
381+
) -> Result<RestrictionCheckResult> {
382+
let config_result = source_result(
383+
restriction_check::check_path_restriction(
384+
ctx,
385+
self,
386+
path.clone(),
387+
PathRestrictionSource::Config,
388+
)
389+
.await,
390+
);
391+
let acl_manifest_result = match cs_id {
392+
Some(cs_id) => Some(source_result(
393+
restriction_check::check_path_restriction(
394+
ctx,
395+
self,
396+
path.clone(),
397+
PathRestrictionSource::AclManifest(cs_id),
398+
)
399+
.await,
400+
)),
401+
None => None,
402+
};
403+
404+
log_source_results_to_scuba(
405+
ctx,
406+
self.config_based.manifest_id_store().repo_id(),
407+
&config_result,
408+
acl_manifest_result.as_ref(),
409+
AclManifestMode::Shadow,
410+
crate::access_log::RestrictedPathAccessData::FullPath { full_path: path },
411+
self.scuba.clone(),
412+
)?;
413+
414+
let config = config_result
415+
.as_ref()
416+
.map_err(|err| anyhow::anyhow!("{:#}", err))?;
417+
Ok(RestrictionCheckResult {
418+
has_authorization: config.has_authorization,
419+
restriction_roots: config.restriction_paths.clone().unwrap_or_default(),
420+
})
421+
}
422+
365423
/// Check if any client identity matches the enforcement conditions config.
366424
/// Returns true if enforcement should be applied.
367425
async fn should_enforce_restriction(&self, ctx: &CoreContext) -> Result<bool> {
@@ -375,6 +433,15 @@ impl RestrictedPaths {
375433
}
376434
}
377435

436+
fn source_result<T>(result: Result<T>) -> SourceRestrictionResult
437+
where
438+
T: Into<crate::access_log::SourceRestrictionCheckResult>,
439+
{
440+
result
441+
.map(|result| Arc::new(result.into()))
442+
.map_err(Arc::new)
443+
}
444+
378445
/// Helper function to spawn an async task that logs access to restricted paths.
379446
///
380447
/// This function checks if restricted paths access logging is enabled via justknobs,
@@ -408,8 +475,8 @@ pub fn spawn_log_restricted_path_access(
408475
return Ok(None);
409476
}
410477

411-
// Early return if config is empty - no restricted paths to check
412-
if restricted_paths.config().is_empty() {
478+
// Early return if no source can report restricted paths.
479+
if !restricted_paths.may_have_restricted_paths() {
413480
return Ok(None);
414481
}
415482

eden/mononoke/repo_attributes/restricted_paths/src/restriction_check.rs

Lines changed: 101 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,10 @@
88
//! Restriction check helpers that turn restriction lookup results into
99
//! authorization results.
1010
11+
use std::str::FromStr;
1112
use std::sync::Arc;
1213

14+
use anyhow::Context;
1315
use anyhow::Result;
1416
use context::CoreContext;
1517
use futures::future;
@@ -22,11 +24,13 @@ use crate::ManifestId;
2224
use crate::ManifestType;
2325
use crate::RestrictedPaths;
2426
use crate::RestrictionCheckResult;
27+
use crate::access_log::SourceRestrictionCheckResult;
2528
use crate::access_log::has_read_access_to_repo_region_acls;
2629
use crate::access_log::is_part_of_group;
30+
use crate::restriction_info;
31+
use crate::restriction_info::PathRestrictionInfo;
2732

2833
/// Source to use for path-side restriction checks.
29-
#[expect(dead_code, reason = "interface skeleton for the split stack")]
3034
#[derive(Clone, Debug, Eq, PartialEq)]
3135
pub(crate) enum PathRestrictionSource {
3236
/// Config-backed path ACLs.
@@ -102,14 +106,30 @@ pub(crate) fn build_restriction_check_result(
102106
}
103107

104108
/// Check a path against one restriction source.
105-
#[expect(dead_code, reason = "interface skeleton for the split stack")]
106109
pub(crate) async fn check_path_restriction(
107-
_ctx: &CoreContext,
108-
_restricted_paths: &RestrictedPaths,
109-
_path: NonRootMPath,
110-
_source: PathRestrictionSource,
111-
) -> Result<RestrictionCheckResult> {
112-
unimplemented!("implemented by the follow-up extraction diff")
110+
ctx: &CoreContext,
111+
restricted_paths: &RestrictedPaths,
112+
path: NonRootMPath,
113+
source: PathRestrictionSource,
114+
) -> Result<SourceRestrictionCheckResult> {
115+
match source {
116+
PathRestrictionSource::Config => {
117+
check_config_path_authorization(ctx, restricted_paths, &path).await
118+
}
119+
PathRestrictionSource::AclManifest(cs_id) => {
120+
let restriction_info = restriction_info::get_path_restriction_info_from_acl_manifest(
121+
restricted_paths,
122+
ctx,
123+
cs_id,
124+
std::slice::from_ref(&path),
125+
)
126+
.await
127+
.context("find AclManifest restrictions for path-side fetch")?;
128+
let restriction_acls = parse_restriction_acls(&restriction_info)?;
129+
check_path_authorization(ctx, restricted_paths, restriction_info, restriction_acls)
130+
.await
131+
}
132+
}
113133
}
114134

115135
/// Check a manifest against one restriction source.
@@ -123,3 +143,76 @@ pub(crate) async fn check_manifest_restriction(
123143
) -> Result<RestrictionCheckResult> {
124144
unimplemented!("implemented by the follow-up extraction diff")
125145
}
146+
147+
async fn check_config_path_authorization(
148+
ctx: &CoreContext,
149+
restricted_paths: &RestrictedPaths,
150+
path: &NonRootMPath,
151+
) -> Result<SourceRestrictionCheckResult> {
152+
let (restriction_info, restriction_acls) = restricted_paths
153+
.config()
154+
.path_acls
155+
.iter()
156+
.filter(|(prefix, _)| prefix.is_prefix_of(path))
157+
.map(|(restriction_root, acl)| {
158+
let repo_region_acl = acl.to_string();
159+
(
160+
PathRestrictionInfo {
161+
restriction_root: restriction_root.clone(),
162+
request_acl: repo_region_acl.clone(),
163+
repo_region_acl,
164+
},
165+
acl.clone(),
166+
)
167+
})
168+
.unzip();
169+
170+
check_path_authorization(ctx, restricted_paths, restriction_info, restriction_acls).await
171+
}
172+
173+
async fn check_path_authorization(
174+
ctx: &CoreContext,
175+
restricted_paths: &RestrictedPaths,
176+
restriction_info: Vec<PathRestrictionInfo>,
177+
restriction_acls: Vec<MononokeIdentity>,
178+
) -> Result<SourceRestrictionCheckResult> {
179+
if restriction_info.is_empty() {
180+
return Ok(SourceRestrictionCheckResult::unrestricted(Some(vec![])));
181+
}
182+
183+
let restriction_paths = restriction_info
184+
.iter()
185+
.map(|info| info.restriction_root.clone())
186+
.collect::<Vec<_>>();
187+
let acl_refs = restriction_acls.iter().collect::<Vec<_>>();
188+
let authorization = check_authorization(
189+
ctx,
190+
restricted_paths.acl_provider(),
191+
&acl_refs,
192+
restricted_paths.config().tooling_allowlist_group.as_deref(),
193+
restricted_paths.config().rollout_allowlist_group.as_deref(),
194+
)
195+
.await?;
196+
197+
Ok(SourceRestrictionCheckResult::new(
198+
authorization.has_authorization(),
199+
authorization.has_acl_access,
200+
restriction_acls,
201+
Some(restriction_paths),
202+
authorization.is_allowlisted_tooling,
203+
authorization.is_rollout_allowlisted,
204+
))
205+
}
206+
207+
fn parse_restriction_acls(
208+
restriction_info: &[PathRestrictionInfo],
209+
) -> Result<Vec<MononokeIdentity>> {
210+
restriction_info
211+
.iter()
212+
.map(|info| {
213+
MononokeIdentity::from_str(&info.repo_region_acl).with_context(|| {
214+
format!("Failed to parse repo_region_acl {}", info.repo_region_acl)
215+
})
216+
})
217+
.collect()
218+
}

0 commit comments

Comments
 (0)