Skip to content

authz: add OpContext::for_saga_action #672

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 8, 2022
Merged

authz: add OpContext::for_saga_action #672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 7 additions & 4 deletions nexus/src/authn/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,16 @@
//! authentication, but they'd all produce the same [`Context`] struct.

pub mod external;
pub mod saga;

pub use crate::db::fixed_data::user_builtin::USER_DB_INIT;
pub use crate::db::fixed_data::user_builtin::USER_INTERNAL_API;
pub use crate::db::fixed_data::user_builtin::USER_SAGA_RECOVERY;
pub use crate::db::fixed_data::user_builtin::USER_TEST_PRIVILEGED;
pub use crate::db::fixed_data::user_builtin::USER_TEST_UNPRIVILEGED;

use serde::Deserialize;
use serde::Serialize;
use uuid::Uuid;

/// Describes how the actor performing the current operation is authenticated
Expand Down Expand Up @@ -156,8 +159,8 @@ mod test {

/// Describes whether the user is authenticated and provides more information
/// that's specific to whether they're authenticated (or not)
#[derive(Debug)]
pub enum Kind {
#[derive(Clone, Debug, Deserialize, Serialize)]
enum Kind {
/// Client successfully authenticated
Authenticated(Details),
/// Client did not attempt to authenticate
Expand All @@ -168,14 +171,14 @@ pub enum Kind {
///
/// This could eventually include other information used during authorization,
/// like a remote IP, the time of authentication, etc.
#[derive(Debug)]
#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct Details {
/// the actor performing the request
actor: Actor,
}

/// Who is performing an operation
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
#[derive(Clone, Copy, Debug, Deserialize, Eq, PartialEq, Serialize)]
pub struct Actor(pub Uuid);

/// Label for a particular authentication scheme (used in log messages and
Expand Down
31 changes: 31 additions & 0 deletions nexus/src/authn/saga.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

//! Serialization of authentication context for sagas
// TODO-security: This is not fully baked. Right now, we serialize the actor
// id. We should think through that a little more. Should we instead preload a
// bunch of roles and then serialize that, for example?

use crate::authn;
use crate::context::OpContext;
use serde::Deserialize;
use serde::Serialize;

/// Serialized form of an `OpContext`
// NOTE: This structure must be versioned carefully. (That's true of all saga
// structures, but this one has a particularly large blast radius.)
#[derive(Debug, Deserialize, Serialize)]
pub struct Serialized {
kind: authn::Kind,
}
Comment on lines +15 to +21
Copy link
Collaborator

Choose a reason for hiding this comment

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

What was the rationale behind creating a new struct instead of:

  • Allowing a caller to get authn::Kind from an authn::Context
  • Implementing From<authn::Kind> for authn::Context?

(I know the comment says OpContext, but this seems much more specifically targeting authn::Context)

That's basically what this is right? Something which should stay 1:1 with the "current authentication information", or some scheme to re-acquire it? Since Kind includes "all the possible ways the user may be authenticated", that seems like a reasonable proxy.

Either way - could we change some of the names being used here?

  • I know this is intended to be used by sagas, but this data structure had nothing directly to do with sagas. If a caller wanted to record "who authenticated an operation at a point-in-time", they could use this code, but that seems like it would be a divergence from the name.
  • If we stick with a separate struct - Serialized seems a bit generic, no? Tons of structures which are serializable exist in the authn module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree with your feeling that there's something off about the structure and name here. Let me address your specific points first:

What was the rationale behind creating a new struct instead of:

* Allowing a caller to get `authn::Kind` from an `authn::Context`

* Implementing `From<authn::Kind>` for `authn::Context`?

...

(I know the comment says OpContext, but this seems much more specifically targeting authn::Context)

That's basically what this is right? Something which should stay 1:1 with the "current authentication information", or some scheme to re-acquire it? Since Kind includes "all the possible ways the user may be authenticated", that seems like a reasonable proxy.

I've been trying to keep the internal details of authn::Context as private as possible because I expect them to change as we flesh out authn. What you said is currently true. I don't know if that will be true going forward.

Either way - could we change some of the names being used here?

* I know this is intended to be used _by_ sagas, but this data structure had nothing directly to do with sagas. If a caller wanted to record "who authenticated an operation at a point-in-time", they _could_ use this code, but that seems like it would be a divergence from the name.

There are different reasons that someone might want a serialized idea of "who authenticated an operation". These use cases might have very different security properties. You might want this information for a debug log, a formal audit log, or to do an authz check in the context of the request, or to propagate the authentication to some other context, possibly at a later time (as in the case of a saga action, but could also be true if we passed this information in a request to some other control plane service). Some of these representations could involve actual secrets (e.g., a bearer token), while others absolutely should never include a secret (the debug log entry). This change creates one representation that's (barely) suitable for one of these purposes. As you said it's simple enough to be used for many of them today. But it seems unlikely that will be true in the future.

Now, we could go either way on this. If we expect to have multiple representations in the future, we could label this structure as general-purpose today and trust that our future selves will be careful enough to recognize when security demands a different representation and build new representations as needed. Or we could label it today as "suitable for use in this specific case only" and trust our future selves, with a second use case in hand, to decide whether it's suitable and refactor as needed. I generally tend towards the latter -- I don't like to describe something as generic when I only really know about one use case.

* If we stick with a separate struct - `Serialized` seems a bit generic, no? Tons of structures which are serializable exist in the `authn` module.

Yeah, as we've previously discussed, I'd been trying to let the module name describe it. It's authn::saga::Serialized -- a serialized authn [context] for use in sagas. authn::saga::SerializedIdentity? authn::SerializedIdentityForSaga? authn::SagaIdentity?

So like I said, I agree with you that there's something funny about the structure or naming here. But I'm not sure how better to phrase it. To restate my goals:

  • have a type that can be easily incorporated directly into a saga's parameters
  • have it be very easy to get that type from an OpContext and to reconstruct an OpContext from it [along with other information that's also part of the OpContext]
  • so far, that type could be authn::Context directly or as you said authn::Kind. That sort of ties the in-memory form (which is mainly intended for the use case of authorizing an operation in the context of this request) to the serialized form (which is intended for storing to disk and reusing at a later time in some other context).
  • I also want to make it hard for someone to (accidentally) serialize the internal implementation details of an authn::Context to somewhere else (e.g., the public API or some other database record or a log file) without considering the consequences -- i.e., if somebody wants to do that, I want them to ask "hmm, maybe authn::Context [or authn::Kind] doesn't impl Serialize for a reason?"


impl Serialized {
pub fn for_opctx(opctx: &OpContext) -> Serialized {
Serialized { kind: opctx.authn.kind.clone() }
}

pub fn to_authn(&self) -> authn::Context {
authn::Context { kind: self.kind.clone(), schemes_tried: vec![] }
}
}
72 changes: 63 additions & 9 deletions nexus/src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use crate::authn::Actor;
use crate::authz::AuthorizedResource;
use crate::db::model::ConsoleSession;
use crate::db::DataStore;
use crate::saga_interface::SagaContext;
use async_trait::async_trait;
use authn::external::session_cookie::HttpAuthnSessionCookie;
use authn::external::spoof::HttpAuthnSpoof;
Expand Down Expand Up @@ -202,6 +203,8 @@ enum OpKind {
ExternalApiRequest,
/// Handling an internal API request
InternalApiRequest,
/// Executing a saga activity
Saga,
/// Background operations in Nexus
Background,
/// Automated testing (unit tests and integration tests)
Expand All @@ -225,8 +228,9 @@ impl OpContext {
datastore,
);

let (log, metadata) =
OpContext::log_and_metadata_for_request(rqctx, &authn).await;
let (log, mut metadata) =
OpContext::log_and_metadata_for_authn(&rqctx.log, &authn);
OpContext::load_request_metadata(rqctx, &mut metadata).await;

Ok(OpContext {
log,
Expand Down Expand Up @@ -255,8 +259,9 @@ impl OpContext {
datastore,
);

let (log, metadata) =
OpContext::log_and_metadata_for_request(rqctx, &authn).await;
let (log, mut metadata) =
OpContext::log_and_metadata_for_authn(&rqctx.log, &authn);
OpContext::load_request_metadata(rqctx, &mut metadata).await;

OpContext {
log,
Expand All @@ -269,8 +274,8 @@ impl OpContext {
}
}

async fn log_and_metadata_for_request<T: Send + Sync + 'static>(
rqctx: &dropshot::RequestContext<T>,
fn log_and_metadata_for_authn(
log: &slog::Logger,
authn: &authn::Context,
) -> (slog::Logger, BTreeMap<String, String>) {
let mut metadata = BTreeMap::new();
Expand All @@ -279,22 +284,71 @@ impl OpContext {
metadata
.insert(String::from("authenticated"), String::from("true"));
metadata.insert(String::from("actor"), actor_id.to_string());
rqctx.log.new(
log.new(
o!("authenticated" => true, "actor" => actor_id.to_string()),
)
} else {
metadata
.insert(String::from("authenticated"), String::from("false"));
rqctx.log.new(o!("authenticated" => false))
log.new(o!("authenticated" => false))
};

(log, metadata)
}

async fn load_request_metadata<T: Send + Sync + 'static>(
rqctx: &dropshot::RequestContext<T>,
metadata: &mut BTreeMap<String, String>,
) {
let request = rqctx.request.lock().await;
metadata.insert(String::from("request_id"), rqctx.request_id.clone());
metadata
.insert(String::from("http_method"), request.method().to_string());
metadata.insert(String::from("http_uri"), request.uri().to_string());
}

(log, metadata)
pub fn for_saga_action<T>(
sagactx: &steno::ActionContext<T>,
serialized_authn: &authn::saga::Serialized,
) -> OpContext
where
T: steno::SagaType<ExecContextType = Arc<SagaContext>>,
{
let created_instant = Instant::now();
let created_walltime = SystemTime::now();
let osagactx = sagactx.user_data();
let nexus = osagactx.nexus();
let datastore = Arc::clone(nexus.datastore());
let authn = Arc::new(serialized_authn.to_authn());
let authz = authz::Context::new(
Arc::clone(&authn),
Arc::clone(&osagactx.authz()),
datastore,
);
let (log, mut metadata) =
OpContext::log_and_metadata_for_authn(osagactx.log(), &authn);

// TODO-debugging This would be a good place to put the saga template
// name, but we don't have it available here. This log maybe should
// come from steno, prepopulated with useful metadata similar to the
// way dropshot::RequestContext does.
let log = log.new(o!(
"saga_node" => sagactx.node_label().to_string(),
));
metadata.insert(
String::from("saga_node"),
sagactx.node_label().to_string(),
);

OpContext {
log,
authz,
authn,
created_instant,
created_walltime,
metadata,
kind: OpKind::Saga,
}
}

/// Returns a context suitable for use in background operations in Nexus
Expand Down
35 changes: 13 additions & 22 deletions nexus/src/db/datastore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1376,13 +1376,23 @@ impl DataStore {

/// See `disk_update_runtime()`. This version should only be used from
/// sagas, which do not currently have authn contexts.
// TODO-security Like project_delete_disk_no_auth(), this should be removed
// once we have support for authz checks from sagas.
pub async fn disk_update_runtime_no_auth(
pub async fn disk_update_runtime(
&self,
opctx: &OpContext,
authz_disk: &authz::Disk,
new_runtime: &DiskRuntimeState,
) -> Result<bool, Error> {
// TODO-security This permission might be overloaded here. The way disk
// runtime updates work is that the caller in Nexus first updates the
// Sled Agent to make a change, then updates to the database to reflect
// that change. So by the time we get here, we better have already done
// an authz check, or we will have already made some unauthorized change
// to the system! At the same time, we don't want just anybody to be
// able to modify the database state. So we _do_ still want an authz
// check here. Arguably it's for a different kind of action, but it
// doesn't seem that useful to split it out right now.
opctx.authorize(authz::Action::Modify, authz_disk).await?;

let disk_id = authz_disk.id();
use db::schema::disk::dsl;
let updated = diesel::update(dsl::disk)
Expand All @@ -1407,25 +1417,6 @@ impl DataStore {
Ok(updated)
}

pub async fn disk_update_runtime(
&self,
opctx: &OpContext,
authz_disk: &authz::Disk,
new_runtime: &DiskRuntimeState,
) -> Result<bool, Error> {
// TODO-security This permission might be overloaded here. The way disk
// runtime updates work is that the caller in Nexus first updates the
// Sled Agent to make a change, then updates to the database to reflect
// that change. So by the time we get here, we better have already done
// an authz check, or we will have already made some unauthorized change
// to the system! At the same time, we don't want just anybody to be
// able to modify the database state. So we _do_ still want an authz
// check here. Arguably it's for a different kind of action, but it
// doesn't seem that useful to split it out right now.
opctx.authorize(authz::Action::Modify, authz_disk).await?;
self.disk_update_runtime_no_auth(authz_disk, new_runtime).await
}

/// Fetches information about a Disk that the caller has previously fetched
///
/// The principal difference from `disk_fetch` is that this function takes
Expand Down
15 changes: 12 additions & 3 deletions nexus/src/nexus.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,9 @@ pub struct Nexus {
/** persistent storage for resources in the control plane */
db_datastore: Arc<db::DataStore>,

/** handle to global authz information */
authz: Arc<authz::Authz>,

/** saga execution coordinator */
sec_client: Arc<steno::SecClient>,

Expand Down Expand Up @@ -200,6 +203,7 @@ impl Nexus {
log: log.new(o!()),
api_rack_identity: db::model::RackIdentity::new(*rack_id),
db_datastore: Arc::clone(&db_datastore),
authz: Arc::clone(&authz),
sec_client: Arc::clone(&sec_client),
recovery_task: std::sync::Mutex::new(None),
populate_status,
Expand All @@ -210,7 +214,7 @@ impl Nexus {
let nexus = Arc::new(nexus);
let opctx = OpContext::for_background(
log.new(o!("component" => "SagaRecoverer")),
authz,
Arc::clone(&authz),
authn::Context::internal_saga_recovery(),
Arc::clone(&db_datastore),
);
Expand All @@ -221,6 +225,7 @@ impl Nexus {
Arc::new(Arc::new(SagaContext::new(
Arc::clone(&nexus),
saga_logger,
Arc::clone(&authz),
))),
db_datastore,
Arc::clone(&sec_client),
Expand Down Expand Up @@ -442,8 +447,11 @@ impl Nexus {
let saga_id = SagaId(Uuid::new_v4());
let saga_logger =
self.log.new(o!("template_name" => template_name.to_owned()));
let saga_context =
Arc::new(Arc::new(SagaContext::new(Arc::clone(self), saga_logger)));
let saga_context = Arc::new(Arc::new(SagaContext::new(
Arc::clone(self),
saga_logger,
Arc::clone(&self.authz),
)));
let future = self
.sec_client
.saga_create(
Expand Down Expand Up @@ -707,6 +715,7 @@ impl Nexus {
}

let saga_params = Arc::new(sagas::ParamsDiskCreate {
serialized_authn: authn::saga::Serialized::for_opctx(opctx),
project_id: authz_project.id(),
create_params: params.clone(),
});
Expand Down
15 changes: 12 additions & 3 deletions nexus/src/saga_interface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
* Interfaces available to saga actions and undo actions
*/

use crate::db;
use crate::external_api::params;
use crate::Nexus;
use crate::{authz, db};
use omicron_common::api::external::Error;
use sled_agent_client::Client as SledAgentClient;
use slog::Logger;
Expand All @@ -24,6 +24,7 @@ use uuid::Uuid;
pub struct SagaContext {
nexus: Arc<Nexus>,
log: Logger,
authz: Arc<authz::Authz>,
}

impl fmt::Debug for SagaContext {
Expand All @@ -33,8 +34,12 @@ impl fmt::Debug for SagaContext {
}

impl SagaContext {
pub fn new(nexus: Arc<Nexus>, log: Logger) -> SagaContext {
SagaContext { nexus, log }
pub fn new(
nexus: Arc<Nexus>,
log: Logger,
authz: Arc<authz::Authz>,
) -> SagaContext {
SagaContext { authz, nexus, log }
}

pub fn log(&self) -> &Logger {
Expand All @@ -59,6 +64,10 @@ impl SagaContext {
self.nexus.sled_allocate().await
}

pub fn authz(&self) -> &Arc<authz::Authz> {
&self.authz
}

pub fn nexus(&self) -> &Arc<Nexus> {
&self.nexus
}
Expand Down
Loading