Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions objectstore-server/src/auth/error.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use objectstore_types::auth::Permission;
use thiserror::Error;

/// Error type for different authorization failure scenarios.
Expand Down Expand Up @@ -25,3 +26,25 @@ pub enum AuthError {
#[error("operation not allowed")]
NotPermitted,
}

impl AuthError {
/// Return a shortname for the failure reason that can be used to tag metrics.
pub fn code(&self) -> &'static str {
match self {
Self::BadRequest(_) => "bad_request",
Self::InternalError(_) => "internal_error",
Self::ValidationFailure(_) => "validation_failure",
Self::VerificationFailure => "verification_failure",
Self::NotPermitted => "not_permitted",
}
}

/// Increment a counter and emit a debug log for this auth failure.
pub fn log(&self, permission: Option<Permission>, usecase: Option<&str>) {
objectstore_metrics::counter!(
"server.auth.failure": 1,
"code" => self.code(),
);
tracing::debug!(?permission, ?usecase, ?self, "Authorization failure")
}
}
42 changes: 33 additions & 9 deletions objectstore-server/src/auth/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use objectstore_service::{PayloadStream, StorageService};
use objectstore_types::auth::Permission;
use objectstore_types::metadata::Metadata;

use crate::auth::AuthContext;
use crate::auth::{AuthContext, AuthError};
use crate::endpoints::common::ApiResult;

/// Wrapper around [`StorageService`] that ensures each operation is authorized.
Expand Down Expand Up @@ -32,23 +32,47 @@ use crate::endpoints::common::ApiResult;
pub struct AuthAwareService {
service: StorageService,
context: Option<AuthContext>,
enforce: bool,
}

impl AuthAwareService {
/// Creates a new `AuthAwareService` using the given service and auth context.
/// Creates a new `AuthAwareService` using the given [`StorageService`], [`AuthContext`], and
/// enforcement setting.
///
/// If no auth context is provided, authorization is disabled and all operations will be
/// permitted.
pub fn new(service: StorageService, context: Option<AuthContext>) -> Self {
Self { service, context }
/// If enforcement is enabled, an `AuthContext` must be provided and its checks must succeed
/// for an operation to be permitted.
///
/// If enforcement is disabled, an `AuthContext` is not required. If one is provided, its
/// checks will be run but their results ignored. All operations will be permitted.
pub fn new(
service: StorageService,
context: Option<AuthContext>,
enforce: bool,
) -> ApiResult<Self> {
if enforce && context.is_none() {
let err = AuthError::InternalError("Missing auth context".into());
err.log(None, None);
Err(err.into())
} else {
Ok(Self {
service,
context,
enforce,
})
}
}

fn assert_authorized(&self, perm: Permission, context: &ObjectContext) -> ApiResult<()> {
if let Some(auth) = &self.context {
auth.assert_authorized(perm, context)?;
let auth_result = match &self.context {
Some(auth) => auth.assert_authorized(perm, context),
None => Ok(()),
}
.inspect_err(|err| err.log(Some(perm), Some(context.usecase.as_str())));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double debug logging on authorization check failures

Low Severity

When AuthContext::assert_authorized in context.rs fails, it emits a tracing::debug! with message "Authorization failed". Then inspect_err in assert_authorized calls err.log(), which emits a second tracing::debug! with message "Authorization failure" and increments a counter. The counter is new and valuable, but the debug log is redundant — every per-operation auth failure now produces two debug entries with overlapping information.

Additional Locations (1)

Fix in Cursor Fix in Web


Ok(())
match self.enforce {
true => Ok(auth_result?),
false => Ok(()),
}
}

/// Checks whether the request is authorized for the given permission on the given context.
Expand Down
24 changes: 16 additions & 8 deletions objectstore-server/src/extractors/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,29 @@ impl FromRequestParts<ServiceState> for AuthAwareService {
parts: &mut Parts,
state: &ServiceState,
) -> Result<Self, Self::Rejection> {
let service = state.service.clone();
if !state.config.auth.enforce {
return Ok(AuthAwareService::new(service, None));
}

let encoded_token = parts
.headers
.get(header::AUTHORIZATION)
.and_then(|v| v.to_str().ok())
.and_then(strip_bearer);

let context = AuthContext::from_encoded_jwt(encoded_token, &state.key_directory)
.inspect_err(|err| tracing::debug!("Authorization rejected: `{:?}`", err))?;
// Attempt to decode / verify the JWT, logging failure
let auth_result = AuthContext::from_encoded_jwt(encoded_token, &state.key_directory)
.inspect_err(|err| err.log(None, None));

// If auth enforcement is enabled, `from_encoded_jwt()` must have succeeded.
// If auth enforcement is disabled, we'll pass the context along if it succeeded but will
// still proceed with `None` if it failed.
let auth_context = match state.config.auth.enforce {
true => Some(auth_result?),
false => auth_result.ok(),
};

Ok(AuthAwareService::new(service, Some(context)))
AuthAwareService::new(
state.service.clone(),
auth_context,
state.config.auth.enforce,
)
}
}

Expand Down
13 changes: 12 additions & 1 deletion objectstore-types/src/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use std::collections::HashSet;
use serde::{Deserialize, Serialize};

/// Permissions that control whether different operations are authorized.
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, Hash)]
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, Hash, Copy)]
pub enum Permission {
/// Read / download objects (serialized as `"object.read"`).
#[serde(rename = "object.read")]
Expand All @@ -33,3 +33,14 @@ impl Permission {
])
}
}

impl std::fmt::Display for Permission {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
let s = match self {
Self::ObjectRead => "object.read",
Self::ObjectWrite => "object.write",
Self::ObjectDelete => "object.delete",
};
f.write_str(s)
}
}