From 2ce5d8137d85aa21f817e24eb534b756bbe278b1 Mon Sep 17 00:00:00 2001 From: Egor Suvorov Date: Thu, 11 Aug 2022 03:44:44 +0300 Subject: [PATCH] Separate permission checks for Pageserver and Safekeeper There will be different scopes for those two, so authorization code should be different. The `check_permission` function is now not in the shared library. Its implementation is very similar to the one which will be added for Safekeeper. In fact, we may reuse the same existing root-like 'PageServerApi' scope, but I would prefer to have separate root-like scopes for services. Also, generate_management_token in tests is generate_pageserver_token now. --- libs/utils/src/auth.rs | 18 +----------------- libs/utils/src/http/endpoint.rs | 13 ++++++++----- pageserver/src/auth.rs | 19 +++++++++++++++++++ pageserver/src/http/routes.rs | 8 +++++++- pageserver/src/lib.rs | 1 + pageserver/src/page_service.rs | 5 +++-- safekeeper/src/auth.rs | 18 ++++++++++++++++++ safekeeper/src/http/routes.rs | 8 +++++++- safekeeper/src/lib.rs | 1 + test_runner/fixtures/neon_fixtures.py | 2 +- test_runner/regress/test_auth.py | 10 +++++----- test_runner/regress/test_pageserver_api.py | 4 ++-- 12 files changed, 73 insertions(+), 34 deletions(-) create mode 100644 pageserver/src/auth.rs create mode 100644 safekeeper/src/auth.rs diff --git a/libs/utils/src/auth.rs b/libs/utils/src/auth.rs index b190b0d1c5..a5a93eaad9 100644 --- a/libs/utils/src/auth.rs +++ b/libs/utils/src/auth.rs @@ -7,7 +7,7 @@ use serde; use std::fs; use std::path::Path; -use anyhow::{bail, Result}; +use anyhow::Result; use jsonwebtoken::{ decode, encode, Algorithm, DecodingKey, EncodingKey, Header, TokenData, Validation, }; @@ -40,22 +40,6 @@ impl Claims { } } -pub fn check_permission(claims: &Claims, tenant_id: Option) -> Result<()> { - match (&claims.scope, tenant_id) { - (Scope::Tenant, None) => { - bail!("Attempt to access management api with tenant scope. Permission denied") - } - (Scope::Tenant, Some(tenant_id)) => { - if claims.tenant_id.unwrap() != tenant_id { - bail!("Tenant id mismatch. Permission denied") - } - Ok(()) - } - (Scope::PageServerApi, None) => Ok(()), // access to management api for PageServerApi scope - (Scope::PageServerApi, Some(_)) => Ok(()), // access to tenant api using PageServerApi scope - } -} - pub struct JwtAuth { decoding_key: DecodingKey, validation: Validation, diff --git a/libs/utils/src/http/endpoint.rs b/libs/utils/src/http/endpoint.rs index 7a519929cf..fecbbb945b 100644 --- a/libs/utils/src/http/endpoint.rs +++ b/libs/utils/src/http/endpoint.rs @@ -1,6 +1,5 @@ -use crate::auth::{self, Claims, JwtAuth}; +use crate::auth::{Claims, JwtAuth}; use crate::http::error; -use crate::id::TenantId; use anyhow::anyhow; use hyper::header::AUTHORIZATION; use hyper::{header::CONTENT_TYPE, Body, Request, Response, Server}; @@ -144,10 +143,14 @@ pub fn auth_middleware( }) } -pub fn check_permission(req: &Request, tenant_id: Option) -> Result<(), ApiError> { +pub fn check_permission_with( + req: &Request, + check_permission: impl Fn(&Claims) -> Result<(), anyhow::Error>, +) -> Result<(), ApiError> { match req.context::() { - Some(claims) => Ok(auth::check_permission(&claims, tenant_id) - .map_err(|err| ApiError::Forbidden(err.to_string()))?), + Some(claims) => { + Ok(check_permission(&claims).map_err(|err| ApiError::Forbidden(err.to_string()))?) + } None => Ok(()), // claims is None because auth is disabled } } diff --git a/pageserver/src/auth.rs b/pageserver/src/auth.rs new file mode 100644 index 0000000000..65021da21c --- /dev/null +++ b/pageserver/src/auth.rs @@ -0,0 +1,19 @@ +use anyhow::{bail, Result}; +use utils::auth::{Claims, Scope}; +use utils::id::TenantId; + +pub fn check_permission(claims: &Claims, tenant_id: Option) -> Result<()> { + match (&claims.scope, tenant_id) { + (Scope::Tenant, None) => { + bail!("Attempt to access management api with tenant scope. Permission denied") + } + (Scope::Tenant, Some(tenant_id)) => { + if claims.tenant_id.unwrap() != tenant_id { + bail!("Tenant id mismatch. Permission denied") + } + Ok(()) + } + (Scope::PageServerApi, None) => Ok(()), // access to management api for PageServerApi scope + (Scope::PageServerApi, Some(_)) => Ok(()), // access to tenant api using PageServerApi scope + } +} diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs index c823fa7bf1..2d1f945bc8 100644 --- a/pageserver/src/http/routes.rs +++ b/pageserver/src/http/routes.rs @@ -21,7 +21,7 @@ use crate::{config::PageServerConf, tenant_mgr}; use utils::{ auth::JwtAuth, http::{ - endpoint::{self, attach_openapi_ui, auth_middleware, check_permission}, + endpoint::{self, attach_openapi_ui, auth_middleware, check_permission_with}, error::{ApiError, HttpErrorBody}, json::{json_request, json_response}, request::parse_request_param, @@ -79,6 +79,12 @@ fn get_config(request: &Request) -> &'static PageServerConf { get_state(request).conf } +fn check_permission(request: &Request, tenant_id: Option) -> Result<(), ApiError> { + check_permission_with(request, |claims| { + crate::auth::check_permission(claims, tenant_id) + }) +} + // Helper function to construct a TimelineInfo struct for a timeline async fn build_timeline_info( state: &State, diff --git a/pageserver/src/lib.rs b/pageserver/src/lib.rs index c0c167c2ac..e3112223e0 100644 --- a/pageserver/src/lib.rs +++ b/pageserver/src/lib.rs @@ -1,3 +1,4 @@ +mod auth; pub mod basebackup; pub mod config; pub mod http; diff --git a/pageserver/src/page_service.rs b/pageserver/src/page_service.rs index eb9416a482..656bb21d3c 100644 --- a/pageserver/src/page_service.rs +++ b/pageserver/src/page_service.rs @@ -31,7 +31,7 @@ use tokio_util::io::SyncIoBridge; use tracing::*; use utils::id::ConnectionId; use utils::{ - auth::{self, Claims, JwtAuth, Scope}, + auth::{Claims, JwtAuth, Scope}, id::{TenantId, TimelineId}, lsn::Lsn, postgres_backend::AuthType, @@ -39,6 +39,7 @@ use utils::{ simple_rcu::RcuReadGuard, }; +use crate::auth::check_permission; use crate::basebackup; use crate::config::{PageServerConf, ProfilingConfig}; use crate::import_datadir::import_wal_from_tar; @@ -670,7 +671,7 @@ impl PageServerHandler { .claims .as_ref() .expect("claims presence already checked"); - auth::check_permission(claims, tenant_id) + check_permission(claims, tenant_id) } } diff --git a/safekeeper/src/auth.rs b/safekeeper/src/auth.rs new file mode 100644 index 0000000000..89040d69f6 --- /dev/null +++ b/safekeeper/src/auth.rs @@ -0,0 +1,18 @@ +use anyhow::{bail, Result}; +use utils::auth::{Claims, Scope}; +use utils::id::TenantId; + +pub fn check_permission(claims: &Claims, tenant_id: Option) -> Result<()> { + match (&claims.scope, tenant_id) { + (Scope::Tenant, None) => { + bail!("Attempt to access management api with tenant scope. Permission denied") + } + (Scope::Tenant, Some(tenant_id)) => { + if claims.tenant_id.unwrap() != tenant_id { + bail!("Tenant id mismatch. Permission denied") + } + Ok(()) + } + (Scope::PageServerApi, _) => bail!("PageServerApi scope makes no sense for Safekeeper"), + } +} diff --git a/safekeeper/src/http/routes.rs b/safekeeper/src/http/routes.rs index d1213baf38..9343611959 100644 --- a/safekeeper/src/http/routes.rs +++ b/safekeeper/src/http/routes.rs @@ -20,7 +20,7 @@ use etcd_broker::subscription_value::SkTimelineInfo; use utils::{ auth::JwtAuth, http::{ - endpoint::{self, auth_middleware, check_permission}, + endpoint::{self, auth_middleware, check_permission_with}, error::ApiError, json::{json_request, json_response}, request::{ensure_no_body, parse_request_param}, @@ -103,6 +103,12 @@ struct TimelineStatus { remote_consistent_lsn: Lsn, } +fn check_permission(request: &Request, tenant_id: Option) -> Result<(), ApiError> { + check_permission_with(request, |claims| { + crate::auth::check_permission(claims, tenant_id) + }) +} + /// Report info about timeline. async fn timeline_status_handler(request: Request) -> Result, ApiError> { let ttid = TenantTimelineId::new( diff --git a/safekeeper/src/lib.rs b/safekeeper/src/lib.rs index 395a29c9ed..fdea284cb7 100644 --- a/safekeeper/src/lib.rs +++ b/safekeeper/src/lib.rs @@ -12,6 +12,7 @@ use utils::{ logging::LogFormat, }; +mod auth; pub mod broker; pub mod control_file; pub mod control_file_upgrade; diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index e1e5eae309..f68849ea2e 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/test_runner/fixtures/neon_fixtures.py @@ -432,7 +432,7 @@ class AuthKeys: return token - def generate_management_token(self) -> str: + def generate_pageserver_token(self) -> str: return self.generate_token(scope="pageserverapi") def generate_tenant_token(self, tenant_id: TenantId) -> str: diff --git a/test_runner/regress/test_auth.py b/test_runner/regress/test_auth.py index 8443aa029f..5bb64af770 100644 --- a/test_runner/regress/test_auth.py +++ b/test_runner/regress/test_auth.py @@ -16,13 +16,13 @@ def test_pageserver_auth(neon_env_builder: NeonEnvBuilder): invalid_tenant_token = env.auth_keys.generate_tenant_token(TenantId.generate()) invalid_tenant_http_client = env.pageserver.http_client(invalid_tenant_token) - management_token = env.auth_keys.generate_management_token() - management_http_client = env.pageserver.http_client(management_token) + pageserver_token = env.auth_keys.generate_pageserver_token() + pageserver_http_client = env.pageserver.http_client(pageserver_token) # this does not invoke auth check and only decodes jwt and checks it for validity # check both tokens ps.safe_psql("set FOO", password=tenant_token) - ps.safe_psql("set FOO", password=management_token) + ps.safe_psql("set FOO", password=pageserver_token) new_timeline_id = env.neon_cli.create_branch( "test_pageserver_auth", tenant_id=env.initial_tenant @@ -33,7 +33,7 @@ def test_pageserver_auth(neon_env_builder: NeonEnvBuilder): tenant_id=env.initial_tenant, ancestor_timeline_id=new_timeline_id ) # console can create branches for tenant - management_http_client.timeline_create( + pageserver_http_client.timeline_create( tenant_id=env.initial_tenant, ancestor_timeline_id=new_timeline_id ) @@ -46,7 +46,7 @@ def test_pageserver_auth(neon_env_builder: NeonEnvBuilder): ) # create tenant using management token - management_http_client.tenant_create() + pageserver_http_client.tenant_create() # fail to create tenant using tenant token with pytest.raises( diff --git a/test_runner/regress/test_pageserver_api.py b/test_runner/regress/test_pageserver_api.py index ab321eeb02..eb22ac5f99 100644 --- a/test_runner/regress/test_pageserver_api.py +++ b/test_runner/regress/test_pageserver_api.py @@ -181,7 +181,7 @@ def test_pageserver_http_api_client_auth_enabled(neon_env_builder: NeonEnvBuilde neon_env_builder.auth_enabled = True env = neon_env_builder.init_start() - management_token = env.auth_keys.generate_management_token() + pageserver_token = env.auth_keys.generate_pageserver_token() - with env.pageserver.http_client(auth_token=management_token) as client: + with env.pageserver.http_client(auth_token=pageserver_token) as client: check_client(client, env.initial_tenant)