From cce86abf14761d1f782c4ca964aea239eac7641f Mon Sep 17 00:00:00 2001 From: Harvey Hunt Date: Mon, 27 Jul 2020 11:02:26 -0700 Subject: [PATCH] mononoke: lfs_server: Enforce ACL checks on a per repo basis Summary: Update the LFS server to use the `enforce_lfs_acl_check` to enforce ACL checks for specific repos and also reject clients with missing idents. In the next diff, I will use the existing LFS server config's `enforce_acl_check` flag as a killswitch. Reviewed By: krallin Differential Revision: D22762451 fbshipit-source-id: 61d26944127711f3503e04154e8c079ae75dc815 --- eden/mononoke/lfs_server/Cargo.toml | 1 + eden/mononoke/lfs_server/src/config.rs | 2 + .../lfs_server/src/lfs_server_context.rs | 49 ++++++++++++------- eden/mononoke/lfs_server/src/main.rs | 7 +-- eden/mononoke/metaconfig/parser/src/config.rs | 6 +++ eden/mononoke/metaconfig/types/src/lib.rs | 2 + eden/mononoke/tests/integration/library.sh | 6 +++ .../integration/test-lfs-server-acl-check.t | 15 +++++- 8 files changed, 64 insertions(+), 24 deletions(-) diff --git a/eden/mononoke/lfs_server/Cargo.toml b/eden/mononoke/lfs_server/Cargo.toml index f6ca6ba3d3..f3035067b2 100644 --- a/eden/mononoke/lfs_server/Cargo.toml +++ b/eden/mononoke/lfs_server/Cargo.toml @@ -16,6 +16,7 @@ filestore = { path = "../filestore" } gotham_ext = { path = "../gotham_ext" } lfs_protocol = { path = "../lfs_protocol" } metaconfig_parser = { path = "../metaconfig/parser" } +metaconfig_types = { path = "../metaconfig/types" } mononoke_types = { path = "../mononoke_types" } permission_checker = { path = "../permission_checker" } redactedblobstore = { path = "../blobstore/redactedblobstore" } diff --git a/eden/mononoke/lfs_server/src/config.rs b/eden/mononoke/lfs_server/src/config.rs index ff51cc76c4..fc60d47371 100644 --- a/eden/mononoke/lfs_server/src/config.rs +++ b/eden/mononoke/lfs_server/src/config.rs @@ -141,6 +141,8 @@ impl ServerConfig { pub fn acl_check(&self) -> bool { self.raw_server_config.acl_check } + #[allow(dead_code)] + // TODO(harveyhunt) Use this method as a killswitch for ACL checking. pub fn enforce_acl_check(&self) -> bool { self.raw_server_config.enforce_acl_check } diff --git a/eden/mononoke/lfs_server/src/lfs_server_context.rs b/eden/mononoke/lfs_server/src/lfs_server_context.rs index 29e5224752..658a3f57a2 100644 --- a/eden/mononoke/lfs_server/src/lfs_server_context.rs +++ b/eden/mononoke/lfs_server/src/lfs_server_context.rs @@ -5,6 +5,7 @@ * GNU General Public License version 2. */ +use std::borrow::Cow; use std::collections::HashMap; use std::fmt::{Arguments, Write}; use std::sync::{ @@ -36,6 +37,7 @@ use context::CoreContext; use hyper::{client::HttpConnector, Client}; use hyper_openssl::HttpsConnector; use lfs_protocol::{RequestBatch, RequestObject, ResponseBatch}; +use metaconfig_types::RepoConfig; use mononoke_types::hash::Sha256; use mononoke_types::ContentId; @@ -49,7 +51,7 @@ pub type HttpsHyperClient = Client>; const ACL_CHECK_ACTION: &str = "read"; struct LfsServerContextInner { - repositories: HashMap, + repositories: HashMap, client: Arc, server: Arc, always_wait_for_upstream: bool, @@ -65,7 +67,7 @@ pub struct LfsServerContext { impl LfsServerContext { pub fn new( - repositories: HashMap, + repositories: HashMap, server: ServerUris, always_wait_for_upstream: bool, max_upload_size: Option, @@ -98,11 +100,20 @@ impl LfsServerContext { repository: String, identities: Option<&MononokeIdentitySet>, ) -> Result { - let (repo, aclchecker, client, server, always_wait_for_upstream, max_upload_size, config) = { + let ( + repo, + aclchecker, + client, + server, + always_wait_for_upstream, + max_upload_size, + config, + enforce_acl_check, + ) = { let inner = self.inner.lock().expect("poisoned lock"); match inner.repositories.get(&repository) { - Some((repo, aclchecker)) => ( + Some((repo, aclchecker, repo_config)) => ( repo.clone(), aclchecker.clone(), inner.client.clone(), @@ -110,6 +121,7 @@ impl LfsServerContext { inner.always_wait_for_upstream, inner.max_upload_size, inner.config_handle.get(), + repo_config.enforce_lfs_acl_check, ), None => { return Err(LfsServerContextErrorKind::RepositoryDoesNotExist( @@ -120,7 +132,8 @@ impl LfsServerContext { }; if config.acl_check() { - acl_check(aclchecker, identities, config.enforce_acl_check()).await?; + // TODO(harveyhunt): Use the live config's enforce_acl_check() method as a killswitch. + acl_check(aclchecker, identities, enforce_acl_check).await?; } Ok(RepositoryRequestContext { @@ -157,21 +170,19 @@ async fn acl_check( identities: Option<&MononokeIdentitySet>, enforce_acl_check: bool, ) -> Result<(), LfsServerContextErrorKind> { - if let Some(identities) = identities { - // Always make the request for permission even if enforce_acl_check is - // false, so that we get the even logged in our system - let acl_check = aclchecker - .check_set(identities, &[ACL_CHECK_ACTION]) - .await - .map_err(LfsServerContextErrorKind::PermissionCheckFailed)?; - if !acl_check && enforce_acl_check { - return Err(LfsServerContextErrorKind::Forbidden.into()); - } else { - return Ok(()); - } + let identities: Cow = match identities { + Some(idents) => Cow::Borrowed(idents), + None => Cow::Owned(MononokeIdentitySet::new()), + }; + + let acl_check = aclchecker + .check_set(identities.as_ref(), &[ACL_CHECK_ACTION]) + .await + .map_err(LfsServerContextErrorKind::PermissionCheckFailed)?; + + if !acl_check && enforce_acl_check { + return Err(LfsServerContextErrorKind::Forbidden.into()); } else { - // For now, allow clients to connect that don't provide an identity. Once we know - // all clients have idents, we can return Forbidden. return Ok(()); } } diff --git a/eden/mononoke/lfs_server/src/main.rs b/eden/mononoke/lfs_server/src/main.rs index 7cbcb924b5..74d32430ae 100644 --- a/eden/mononoke/lfs_server/src/main.rs +++ b/eden/mononoke/lfs_server/src/main.rs @@ -38,6 +38,7 @@ use cmdlib::{ monitoring::{start_fb303_server, AliveService}, }; use metaconfig_parser::RepoConfigs; +use metaconfig_types::RepoConfig; use crate::lfs_server_context::{LfsServerContext, ServerUris}; use crate::middleware::{ @@ -274,7 +275,7 @@ fn main(fb: FacebookInit) -> Result<(), Error> { &logger, ); - let hipster_acl = config.hipster_acl; + let hipster_acl = config.hipster_acl.as_ref(); let aclchecker = async { if let Some(test_checker) = test_acl_checker { Ok(test_checker.clone()) @@ -298,9 +299,9 @@ fn main(fb: FacebookInit) -> Result<(), Error> { let (repo, aclchecker) = try_join!(builder.build(), aclchecker)?; - Result::<(String, (BlobRepo, ArcPermissionChecker)), Error>::Ok(( + Result::<(String, (BlobRepo, ArcPermissionChecker, RepoConfig)), Error>::Ok(( name, - (repo, aclchecker), + (repo, aclchecker, config), )) } }); diff --git a/eden/mononoke/metaconfig/parser/src/config.rs b/eden/mononoke/metaconfig/parser/src/config.rs index db35ceb285..9d136af61c 100644 --- a/eden/mononoke/metaconfig/parser/src/config.rs +++ b/eden/mononoke/metaconfig/parser/src/config.rs @@ -209,6 +209,7 @@ fn parse_repo_config( scuba_local_path_hooks, hgsql_name, hgsql_globalrevs_name, + enforce_lfs_acl_check, .. } = repo_config; @@ -321,6 +322,8 @@ fn parse_repo_config( } }; + let enforce_lfs_acl_check = enforce_lfs_acl_check.unwrap_or(false); + Ok(RepoConfig { enabled, storage_config, @@ -356,6 +359,7 @@ fn parse_repo_config( derived_data_config, hgsql_name, hgsql_globalrevs_name, + enforce_lfs_acl_check, }) } @@ -1152,6 +1156,7 @@ mod test { }, hgsql_name: HgsqlName("fbsource".to_string()), hgsql_globalrevs_name: HgsqlGlobalrevsName("fbsource".to_string()), + enforce_lfs_acl_check: false, }, ); @@ -1199,6 +1204,7 @@ mod test { derived_data_config: DerivedDataConfig::default(), hgsql_name: HgsqlName("www-foobar".to_string()), hgsql_globalrevs_name: HgsqlGlobalrevsName("www-barfoo".to_string()), + enforce_lfs_acl_check: false, }, ); assert_eq!( diff --git a/eden/mononoke/metaconfig/types/src/lib.rs b/eden/mononoke/metaconfig/types/src/lib.rs index fee0ae016f..832ec8e818 100644 --- a/eden/mononoke/metaconfig/types/src/lib.rs +++ b/eden/mononoke/metaconfig/types/src/lib.rs @@ -178,6 +178,8 @@ pub struct RepoConfig { /// Name of this repository in hgsql ... for globalrevs. This could, in some cases, not be the /// same as HgsqlName. pub hgsql_globalrevs_name: HgsqlGlobalrevsName, + /// Whether to enforce strict LFS ACL checks for this repo. + pub enforce_lfs_acl_check: bool, } /// Config for derived data diff --git a/eden/mononoke/tests/integration/library.sh b/eden/mononoke/tests/integration/library.sh index 98957dc637..c698dc6988 100644 --- a/eden/mononoke/tests/integration/library.sh +++ b/eden/mononoke/tests/integration/library.sh @@ -647,6 +647,12 @@ scuba_local_path="$SCUBA_LOGGING_PATH" CONFIG fi +if [[ -n "${ENFORCE_LFS_ACL_CHECK:-}" ]]; then + cat >> "repos/$reponame/server.toml" < "$LIVE_CONFIG" << EOF > { @@ -25,6 +27,7 @@ $ LFS_LOG="$TESTTMP/lfs.log" $ LFS_ROOT="$(lfs_server --log "$LFS_LOG" --tls --live-config "file:${LIVE_CONFIG}" --allowed-test-identity USER:test --trusted-proxy-identity USER:myusername0)" $ LFS_URI="$LFS_ROOT/repo1" + $ LFS_URI_REPO_ENFORCE_ACL="$LFS_ROOT/repo2" # Setup constants. These headers are normally provided by proxygen, they store # an encoded form of the original client identity. In this case, we have @@ -32,6 +35,7 @@ $ ALLOWED_IDENT="x-fb-validated-client-encoded-identity: %7B%22ai%22%3A%20%22%22%2C%20%22ch%22%3A%20%22%22%2C%20%22it%22%3A%20%22user%22%2C%20%22id%22%3A%20%22test%22%7D" $ DISALLOWED_IDENT="x-fb-validated-client-encoded-identity: %7B%22ai%22%3A%20%22%22%2C%20%22ch%22%3A%20%22%22%2C%20%22it%22%3A%20%22user%22%2C%20%22id%22%3A%20%22invalid%22%7D" $ DOWNLOAD_URL="$LFS_URI/download/d28548bc21aabf04d143886d717d72375e3deecd0dafb3d110676b70a192cb5d" + $ DOWNLOAD_URL_REPO_ENFORCE_ACL="$LFS_URI_REPO_ENFORCE_ACL/download/d28548bc21aabf04d143886d717d72375e3deecd0dafb3d110676b70a192cb5d" # Upload a blob $ yes A 2>/dev/null | head -c 2KiB | ssldebuglfssend "$LFS_URI" @@ -46,15 +50,22 @@ $ sslcurl -s -o /dev/null -w "%{http_code}\n" "$DOWNLOAD_URL" --header "$ALLOWED_IDENT" 200 -# Make a request with an invalid encoded client identity header +# Make a request with an invalid encoded client identity header. As +# enforce_lfs_acl_check is not set, this is allowed. $ sslcurl -s -o /dev/null -w "%{http_code}\n" "$DOWNLOAD_URL" --header "$DISALLOWED_IDENT" - 403 + 200 # Make a request without specifying an identity in the header # NOTE: We allow this whilst we wait for all clients to get certs $ sslcurl -s -o /dev/null -w "%{http_code}\n" "$DOWNLOAD_URL" 200 +# Make a request without specifying an identity in the header, as the repo is +# configured with enforce_lfs_acl_check this request is forbidden as there is +# no ident provided. + $ sslcurl -s -o /dev/null -w "%{http_code}\n" "$DOWNLOAD_URL_REPO_ENFORCE_ACL" + 403 + # Disable ACL checking $ sed -i 's/"enforce_acl_check": true/"enforce_acl_check": false/g' "$LIVE_CONFIG" $ sleep 2