3538: Improve the api key of the metrics r=dureuill a=irevoire

Related to https://github.com/meilisearch/meilisearch/pull/3524#discussion_r1115903998
Update: https://github.com/meilisearch/meilisearch/issues/3523

Right after merging the PR, we changed our minds and decided to update the way we handle the API keys on the metrics route.
Now instead of bypassing all the applied rules of the API key, we forbid the usage of the `/metrics` route if you have any restrictions on the indexes.

Co-authored-by: Tamo <tamo@meilisearch.com>
Co-authored-by: Louis Dureuil <louis@meilisearch.com>
This commit is contained in:
bors[bot] 2023-02-27 13:46:57 +00:00 committed by GitHub
commit 6ca7a109b9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 43 additions and 17 deletions

View File

@ -2,13 +2,13 @@ use actix_web::http::header;
use actix_web::web::{self, Data}; use actix_web::web::{self, Data};
use actix_web::HttpResponse; use actix_web::HttpResponse;
use index_scheduler::IndexScheduler; use index_scheduler::IndexScheduler;
use meilisearch_auth::{AuthController, AuthFilter}; use meilisearch_auth::AuthController;
use meilisearch_types::error::ResponseError; use meilisearch_types::error::ResponseError;
use meilisearch_types::keys::actions; use meilisearch_types::keys::actions;
use prometheus::{Encoder, TextEncoder}; use prometheus::{Encoder, TextEncoder};
use crate::extractors::authentication::policies::ActionPolicy; use crate::extractors::authentication::policies::ActionPolicy;
use crate::extractors::authentication::GuardedData; use crate::extractors::authentication::{AuthenticationError, GuardedData};
use crate::routes::create_all_stats; use crate::routes::create_all_stats;
pub fn configure(config: &mut web::ServiceConfig) { pub fn configure(config: &mut web::ServiceConfig) {
@ -19,12 +19,17 @@ pub async fn get_metrics(
index_scheduler: GuardedData<ActionPolicy<{ actions::METRICS_GET }>, Data<IndexScheduler>>, index_scheduler: GuardedData<ActionPolicy<{ actions::METRICS_GET }>, Data<IndexScheduler>>,
auth_controller: GuardedData<ActionPolicy<{ actions::METRICS_GET }>, AuthController>, auth_controller: GuardedData<ActionPolicy<{ actions::METRICS_GET }>, AuthController>,
) -> Result<HttpResponse, ResponseError> { ) -> Result<HttpResponse, ResponseError> {
let response = create_all_stats( let auth_filters = index_scheduler.filters();
(*index_scheduler).clone(), if !auth_filters.all_indexes_authorized() {
(*auth_controller).clone(), let mut error = ResponseError::from(AuthenticationError::InvalidToken);
// we don't use the filters contained in the `ActionPolicy` because the metrics must have the right to access all the indexes. error
&AuthFilter::default(), .message
)?; .push_str(" The API key for the `/metrics` route must allow access to all indexes.");
return Err(error);
}
let response =
create_all_stats((*index_scheduler).clone(), (*auth_controller).clone(), auth_filters)?;
crate::metrics::MEILISEARCH_DB_SIZE_BYTES.set(response.database_size as i64); crate::metrics::MEILISEARCH_DB_SIZE_BYTES.set(response.database_size as i64);
crate::metrics::MEILISEARCH_INDEX_COUNT.set(response.indexes.len() as i64); crate::metrics::MEILISEARCH_INDEX_COUNT.set(response.indexes.len() as i64);

View File

@ -75,6 +75,14 @@ static INVALID_RESPONSE: Lazy<Value> = Lazy::new(|| {
}) })
}); });
static INVALID_METRICS_RESPONSE: Lazy<Value> = Lazy::new(|| {
json!({"message": "The provided API key is invalid. The API key for the `/metrics` route must allow access to all indexes.",
"code": "invalid_api_key",
"type": "auth",
"link": "https://docs.meilisearch.com/errors#invalid_api_key"
})
});
const MASTER_KEY: &str = "MASTER_KEY"; const MASTER_KEY: &str = "MASTER_KEY";
#[actix_rt::test] #[actix_rt::test]
@ -202,15 +210,28 @@ async fn access_authorized_restricted_index() {
let (response, code) = server.dummy_request(method, route).await; let (response, code) = server.dummy_request(method, route).await;
assert_ne!( // The metrics route MUST have no limitation on the indexes
response, if *route == "/metrics" {
INVALID_RESPONSE.clone(), assert_eq!(
"on route: {:?} - {:?} with action: {:?}", response,
method, INVALID_METRICS_RESPONSE.clone(),
route, "on route: {:?} - {:?} with action: {:?}",
action method,
); route,
assert_ne!(code, 403); action
);
assert_eq!(code, 403);
} else {
assert_ne!(
response,
INVALID_RESPONSE.clone(),
"on route: {:?} - {:?} with action: {:?}",
method,
route,
action
);
assert_ne!(code, 403);
}
} }
} }
} }