Skip to content
Merged
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
25 changes: 22 additions & 3 deletions common/auth/src/client/provider/openid.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ pub struct OpenIdTokenProviderConfigArguments {
default_value = "false"
)]
pub tls_insecure: bool,
/// OAuth scope(s) to request in the client_credentials token request.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if this is a breaking change ? what happens with existing deployments (and have we tested )

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This PR is essentially building upon #2552, so there should be nothing breaking here. We're bringing back the default of no scope being explicitly requested, with an option to request scopes if necessary. What possible breaking change did you have in mind?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess my request is if we have tested with an existing env which has named scope and if there is any migration path concerns ... if you have none, then I have none ;)

/// Space-separated when multiple scopes are needed. If unset, no scope parameter is sent.
#[arg(id = "oidc_scope", long = "oidc-scope", env = "OIDC_PROVIDER_SCOPE")]
pub scope: Option<String>,
}

impl OpenIdTokenProviderConfigArguments {
Expand All @@ -60,6 +64,7 @@ impl OpenIdTokenProviderConfigArguments {
client_secret: Some(devmode::SSO_CLIENT_SECRET.to_string()),
refresh_before: Duration::from_secs(30).into(),
tls_insecure: false,
scope: None,
}
}
}
Expand Down Expand Up @@ -89,6 +94,7 @@ pub struct OpenIdTokenProviderConfig {
pub issuer_url: String,
pub refresh_before: humantime::Duration,
pub tls_insecure: bool,
pub scope: Option<String>,
}

impl OpenIdTokenProviderConfig {
Expand All @@ -99,6 +105,7 @@ impl OpenIdTokenProviderConfig {
client_secret: devmode::SSO_CLIENT_SECRET.to_string(),
refresh_before: Duration::from_secs(30).into(),
tls_insecure: false,
scope: None,
}
}

Expand Down Expand Up @@ -132,6 +139,7 @@ impl OpenIdTokenProviderConfig {
issuer_url,
refresh_before: arguments.refresh_before,
tls_insecure: arguments.tls_insecure,
scope: arguments.scope,
})
}
_ => None,
Expand All @@ -151,6 +159,7 @@ pub struct OpenIdTokenProvider {
client: Arc<openid::Client>,
current_token: Arc<RwLock<Option<openid::TemporalBearerGuard>>>,
refresh_before: chrono::Duration,
scope: Option<String>,
}

impl Debug for OpenIdTokenProvider {
Expand All @@ -167,11 +176,16 @@ impl Debug for OpenIdTokenProvider {

impl OpenIdTokenProvider {
/// Create a new provider using the provided client.
pub fn new(client: openid::Client, refresh_before: chrono::Duration) -> Self {
pub fn new(
client: openid::Client,
refresh_before: chrono::Duration,
scope: Option<String>,
) -> Self {
Self {
client: Arc::new(client),
current_token: Arc::new(RwLock::new(None)),
refresh_before,
scope,
}
}

Expand All @@ -198,6 +212,7 @@ impl OpenIdTokenProvider {
Ok(Self::new(
client,
chrono::Duration::from_std(config.refresh_before.into())?,
config.scope,
))
}

Expand Down Expand Up @@ -243,7 +258,11 @@ impl OpenIdTokenProvider {
Some(current_token) => {
log::debug!("Refreshing token ... ");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just noticed while in the area - I think this should be converted to tracing call (As per CONVENTIONS.md)

match current_token.as_ref().refresh_token.is_some() {
true => self.client.refresh_token(current_token, None).await?.into(),
true => self
.client
.refresh_token(current_token, self.scope.as_deref())
.await?
.into(),
false => self.initial_token().await?,
}
}
Expand All @@ -262,7 +281,7 @@ impl OpenIdTokenProvider {
async fn initial_token(&self) -> Result<openid::TemporalBearerGuard, openid::error::Error> {
Ok(self
.client
.request_token_using_client_credentials(Some("openid"))
.request_token_using_client_credentials(self.scope.as_deref())
.await?
.into())
}
Expand Down
28 changes: 28 additions & 0 deletions server/src/profile/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,15 @@ pub struct EiOidcArguments {
default_value = "false"
)]
pub tls_insecure: bool,

/// OAuth scope(s) to request in the client_credentials token request.
/// Space-separated when multiple scopes are needed. If unset, no scope parameter is sent.
#[arg(
id = "ei_oidc_scope",
long = "ei-oidc-scope",
env = "EXPLOIT_INTELLIGENCE_OIDC_SCOPE"
)]
pub scope: Option<String>,
}

impl EiOidcArguments {
Expand All @@ -276,6 +285,7 @@ impl EiOidcArguments {
issuer_url,
refresh_before: self.refresh_before,
tls_insecure: self.tls_insecure,
scope: self.scope,
})
}
}
Expand Down Expand Up @@ -909,6 +919,7 @@ mod test {
issuer_url: None,
refresh_before: "30s".parse().unwrap(),
tls_insecure: false,
scope: None,
},
}
}
Expand Down Expand Up @@ -958,6 +969,7 @@ mod test {
issuer_url: None,
refresh_before: "30s".parse().unwrap(),
tls_insecure: false,
scope: None,
};
assert!(args.into_config().is_none());
}
Expand All @@ -970,12 +982,28 @@ mod test {
issuer_url: Some("https://idp.example.com".into()),
refresh_before: "60s".parse().unwrap(),
tls_insecure: true,
scope: None,
};
let config = args.into_config().expect("config should be Some");
assert_eq!(config.client_id, "client");
assert_eq!(config.client_secret, "secret");
assert_eq!(config.issuer_url, "https://idp.example.com");
assert!(config.tls_insecure);
assert!(config.scope.is_none());
}

#[test]
fn oidc_into_config_propagates_scope() {
let args = EiOidcArguments {
client_id: Some("client".into()),
client_secret: Some("secret".into()),
issuer_url: Some("https://idp.example.com".into()),
refresh_before: "60s".parse().unwrap(),
tls_insecure: false,
scope: Some("my-api/read my-api/write".into()),
};
let config = args.into_config().expect("config should be Some");
assert_eq!(config.scope.as_deref(), Some("my-api/read my-api/write"));
}

#[test_context(TrustifyContext)]
Expand Down