Skip to content

clean up "test-privileged", "test-unprivileged" users #1087

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 6 commits into from
May 19, 2022
Merged

clean up "test-privileged", "test-unprivileged" users #1087

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
561536c
test users should be silo users
davepacheco May 18, 2022
2506f89
all tests pass
davepacheco May 19, 2022
fa164ce
fixup
davepacheco May 19, 2022
d40daeb
self-review nits
davepacheco May 19, 2022
1cc1a91
don't support authenticating as built-in user any more
davepacheco May 19, 2022
1c39fcd
clippy
davepacheco May 19, 2022
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
3 changes: 1 addition & 2 deletions nexus/examples/config.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ session_absolute_timeout_minutes = 480
# List of authentication schemes to support.
#
# This is not fleshed out yet and the only reason to change it now is for
# working on authentication or authorization. Neither is really implemented
# yet.
# working on authentication or authorization.
[authn]
# TODO(https://github.com/oxidecomputer/omicron/issues/372): Remove "spoof".
schemes_external = ["spoof", "session_cookie"]
Expand Down
2 changes: 0 additions & 2 deletions nexus/src/authn/external/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,6 @@ mod test {
user_builtin_id: "1c91bab2-4841-669f-cc32-de80da5bbf39"
.parse()
.unwrap(),
silo_id: *crate::db::fixed_data::silo::SILO_ID,
};
let grunt1 = Box::new(GruntScheme {
name: name1,
Expand All @@ -208,7 +207,6 @@ mod test {
user_builtin_id: "799684af-533a-cb66-b5ac-ab55a791d5ef"
.parse()
.unwrap(),
silo_id: *crate::db::fixed_data::silo::SILO_ID,
};
let grunt2 = Box::new(GruntScheme {
name: name2,
Expand Down
9 changes: 2 additions & 7 deletions nexus/src/authn/external/session_cookie.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,8 @@ where
}
};

// TODO-security Once we have real SiloUsers and we can have
// "test-privileged" and "test-unprivileged" be Silo Users, this should
// be changed accordingly. Until then, the integration tests attempt to
// use this scheme with "test-privileged", which requires that we
// interpret these as BuiltinUsers.
let actor = Actor::UserBuiltin {
user_builtin_id: session.silo_user_id(),
let actor = Actor::SiloUser {
silo_user_id: session.silo_user_id(),
silo_id: session.silo_id(),
};

Expand Down
117 changes: 30 additions & 87 deletions nexus/src/authn/external/spoof.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,8 @@ use uuid::Uuid;
// _authentication_ information. Similarly, the "Unauthorized" HTTP response
// code usually describes an _authentication_ error.)
//
// The above example treats the specified user as a silo user. It's also
// possible to use this mechanism to specify a built-in user. In that case, the
// bearer token format looks like this:
//
// Authorization: Bearer oxide-spoof-builtin-<uuid>
//
// Either way, this mechanism trusts (without verification) that the client is
// whoever they say they are. That's true of any bearer token, but this one is
// This mechanism trusts (without verification) that the client is whoever
// they say they are. That's true of any bearer token, but this one is
// particularly dangerous because the tokens are long-lived and not secret.

pub const SPOOF_SCHEME_NAME: authn::SchemeName = authn::SchemeName("spoof");
Expand All @@ -63,16 +57,13 @@ lazy_static! {
/// Actor (id) used for the special "bad credentials" error
static ref SPOOF_RESERVED_BAD_CREDS_ACTOR: Actor = Actor::UserBuiltin {
user_builtin_id: "22222222-2222-2222-2222-222222222222".parse().unwrap(),
silo_id: *crate::db::fixed_data::silo::SILO_ID,
};
/// Complete HTTP header value to trigger the "bad actor" error
pub static ref SPOOF_HEADER_BAD_ACTOR: Authorization<Bearer> =
make_header_value_str(ActorType::Builtin, SPOOF_RESERVED_BAD_ACTOR)
.unwrap();
make_header_value_str(SPOOF_RESERVED_BAD_ACTOR).unwrap();
/// Complete HTTP header value to trigger the "bad creds" error
pub static ref SPOOF_HEADER_BAD_CREDS: Authorization<Bearer> =
make_header_value_str(ActorType::Builtin, SPOOF_RESERVED_BAD_CREDS)
.unwrap();
make_header_value_str(SPOOF_RESERVED_BAD_CREDS).unwrap();
}

/// Implements a (test-only) authentication scheme where the client simply
Expand Down Expand Up @@ -102,36 +93,19 @@ where
}
}

/// Specifies whether the spoof header explicitly specifies a built-in user or a
/// normal (silo) user
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub enum ActorType {
Builtin,
Silo,
}

fn authn_spoof(raw_value: Option<&Authorization<Bearer>>) -> SchemeResult {
let token = match raw_value {
None => return SchemeResult::NotRequested,
Some(bearer) => bearer.token(),
};

let str_value = match token.strip_prefix(SPOOF_PREFIX) {
None => {
// This is some other kind of bearer token. Maybe another scheme
// knows how to deal with it.
return SchemeResult::NotRequested;
}
Some(s) => s,
};

let (actor_kind, str_value) = {
match str_value.strip_prefix("builtin-") {
Some(s) => (ActorType::Builtin, s),
None => (ActorType::Silo, str_value),
}
};
if !token.starts_with(SPOOF_PREFIX) {
// This is some other kind of bearer token. Maybe another scheme knows
// how to deal with it.
return SchemeResult::NotRequested;
}

let str_value = &token[SPOOF_PREFIX.len()..];
if str_value == SPOOF_RESERVED_BAD_ACTOR {
return SchemeResult::Failed(Reason::UnknownActor {
actor: str_value.to_owned(),
Expand All @@ -146,16 +120,10 @@ fn authn_spoof(raw_value: Option<&Authorization<Bearer>>) -> SchemeResult {
}

match Uuid::parse_str(str_value).context("parsing header value as UUID") {
Ok(actor_id) => {
Ok(silo_user_id) => {
// TODO This should really look up the silo for the given Silo User.
let silo_id = *crate::db::fixed_data::silo::SILO_ID;
let actor = match actor_kind {
ActorType::Builtin => {
Actor::UserBuiltin { user_builtin_id: actor_id, silo_id }
}
ActorType::Silo => {
Actor::SiloUser { silo_user_id: actor_id, silo_id }
}
};
let actor = Actor::SiloUser { silo_id, silo_user_id };
SchemeResult::Authenticated(Details { actor })
}
Err(source) => SchemeResult::Failed(Reason::BadFormat { source }),
Expand All @@ -164,8 +132,8 @@ fn authn_spoof(raw_value: Option<&Authorization<Bearer>>) -> SchemeResult {

/// Returns a value of the `Authorization` header for this actor that will be
/// accepted using this scheme
pub fn make_header_value(kind: ActorType, id: Uuid) -> Authorization<Bearer> {
make_header_value_str(kind, &id.to_string()).unwrap()
pub fn make_header_value(id: Uuid) -> Authorization<Bearer> {
make_header_value_str(&id.to_string()).unwrap()
}

/// Returns a value of the `Authorization` header with `str` in the place where
Expand All @@ -176,14 +144,9 @@ pub fn make_header_value(kind: ActorType, id: Uuid) -> Authorization<Bearer> {
/// this returns a typed value and so cannot be used to make various kinds of
/// invalid headers.
fn make_header_value_str(
kind: ActorType,
s: &str,
) -> Result<Authorization<Bearer>, anyhow::Error> {
let suffix = match kind {
ActorType::Builtin => format!("builtin-{}", s),
ActorType::Silo => String::from(s),
};
Authorization::bearer(&format!("{}{}", SPOOF_PREFIX, suffix))
Authorization::bearer(&format!("{}{}", SPOOF_PREFIX, s))
.context("not a valid HTTP header value")
}

Expand All @@ -209,8 +172,6 @@ mod test {
use super::make_header_value;
use super::make_header_value_raw;
use super::make_header_value_str;
use super::ActorType;
use crate::db;
use headers::authorization::Bearer;
use headers::authorization::Credentials;
use headers::Authorization;
Expand All @@ -221,18 +182,12 @@ mod test {
fn test_make_header_value() {
let test_uuid_str = "37b56e4f-8c60-453b-a37e-99be6efe8a89";
let test_uuid = test_uuid_str.parse::<Uuid>().unwrap();
let header_value = make_header_value(ActorType::Builtin, test_uuid);
let header_value = make_header_value(test_uuid);
// Other formats are valid here (e.g., changes in case or spacing).
// However, a black-box test that accounted for those would be at least
// as complicated as `make_header_value()` itself and wouldn't be so
// convincing in demonstrating that that function does what we think it
// does.
assert_eq!(
header_value.0.encode().to_str().unwrap(),
"Bearer oxide-spoof-builtin-37b56e4f-8c60-453b-a37e-99be6efe8a89"
);

let header_value = make_header_value(ActorType::Silo, test_uuid);
assert_eq!(
header_value.0.encode().to_str().unwrap(),
"Bearer oxide-spoof-37b56e4f-8c60-453b-a37e-99be6efe8a89"
Expand All @@ -245,7 +200,7 @@ mod test {
let test_uuid = test_uuid_str.parse::<Uuid>().unwrap();
assert_eq!(
make_header_value_raw(test_uuid_str.as_bytes()).unwrap(),
make_header_value(ActorType::Silo, test_uuid).0.encode(),
make_header_value(test_uuid).0.encode(),
);

assert_eq!(
Expand All @@ -272,29 +227,18 @@ mod test {
fn test_spoof_header_valid() {
let test_uuid_str = "37b56e4f-8c60-453b-a37e-99be6efe8a89";
let test_uuid = test_uuid_str.parse::<Uuid>().unwrap();
let test_header = make_header_value(test_uuid);

// Success casees: the client provided a valid uuid in the header.
for actor_type in [ActorType::Builtin, ActorType::Silo] {
let test_header = make_header_value(actor_type, test_uuid);
let success_case = authn_spoof(Some(&test_header));
match success_case {
SchemeResult::Authenticated(details) => {
assert_eq!(details.actor.actor_id(), test_uuid);
let found_type = details.actor.actor_type();
match found_type {
db::model::IdentityType::UserBuiltin => {
assert_eq!(actor_type, ActorType::Builtin);
}
db::model::IdentityType::SiloUser => {
assert_eq!(actor_type, ActorType::Silo);
}
}
}
result => {
panic!("unexpectedly failed authentication: {:?}", result);
}
};
}
// Success case: the client provided a valid uuid in the header.
let success_case = authn_spoof(Some(&test_header));
match success_case {
SchemeResult::Authenticated(details) => {
assert_eq!(details.actor.actor_id(), test_uuid);
}
_ => {
assert!(false);
}
};
}

#[test]
Expand Down Expand Up @@ -335,8 +279,7 @@ mod test {
];

for input in &bad_inputs {
let test_header =
make_header_value_str(ActorType::Builtin, input).unwrap();
let test_header = make_header_value_str(input).unwrap();
let result = authn_spoof(Some(&test_header));
if let SchemeResult::Failed(error) = result {
assert!(error.to_string().starts_with(
Expand Down
Loading