Skip to content
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

Enforce JWT expiration for subscriptions #4166

Merged
merged 16 commits into from
Nov 13, 2023
Merged

Enforce JWT expiration for subscriptions #4166

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
9c5c1a2
Enfore JWT expiration for subscriptions
garypen Nov 8, 2023
2bd87c5
after discussion with Ben
garypen Nov 8, 2023
66bf9e0
implement code review suggestion to simplify processing
garypen Nov 8, 2023
519534d
Tidy up the implementation so only do JWT check for subs and deferred
garypen Nov 8, 2023
08a7b50
add a changeset
garypen Nov 8, 2023
803931a
Merge branch 'dev' into garypen/3947-jwt-sub-expire
garypen Nov 8, 2023
3df2501
Remove the extremely unlikely expect() from DateTime creation
garypen Nov 8, 2023
58a0254
lint
garypen Nov 8, 2023
d30a606
code review comments
garypen Nov 10, 2023
ead759e
Move the subscription check to the request
garypen Nov 10, 2023
9e33cc7
Cleanup the timeout/sleep logic in the select loop
garypen Nov 10, 2023
56e2b4a
use Duration::ZERO
garypen Nov 10, 2023
52a35ae
Clean up FIXME and get Duration logic right
garypen Nov 10, 2023
9d0a00e
Merge branch 'dev' into garypen/3947-jwt-sub-expire
garypen Nov 10, 2023
f1d7b66
Merge branch 'dev' into garypen/3947-jwt-sub-expire
garypen Nov 13, 2023
bcd0bda
move expire calculation from supergraph_service to authentication mod
garypen Nov 13, 2023
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
5 changes: 5 additions & 0 deletions .changesets/fix_garypen_3947_jwt_sub_expire.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
### Enfore JWT expiration for subscriptions ([Issue #3947](https://github.com/apollographql/router/issues/3947))

If a JWT expires whilst a subscription is executing, the subscription should be terminated. This also applies to deferred responses.

By [@garypen](https://github.com/garypen) in https://github.com/apollographql/router/pull/4166
62 changes: 62 additions & 0 deletions Cargo.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,21 @@ version = "0.2.16"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "0942ffc6dcaadf03badf6e6a2d0228460359d5e34b57ccdc720b7382dfbd5ec5"

[[package]]
name = "android-tzdata"
version = "0.1.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "e999941b234f3131b00bc13c22d06e8c5ff726d1b6318ac7eb276997bbb4fef0"

[[package]]
name = "android_system_properties"
version = "0.1.5"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "819e7219dbd41043ac279b19830f2efc897156490d7fd6ea916720117ee66311"
dependencies = [
"libc",
]

[[package]]
name = "anes"
version = "0.1.6"
Expand Down Expand Up @@ -303,6 +318,7 @@ dependencies = [
"brotli",
"buildstructor",
"bytes",
"chrono",
"ci_info",
"clap 4.4.6",
"console-subscriber",
Expand Down Expand Up @@ -1451,6 +1467,20 @@ version = "1.0.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "baf1de4339761588bc0619e3cbc0120ee582ebb74b53b4efbf79117bd2da40fd"

[[package]]
name = "chrono"
version = "0.4.31"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "7f2c685bad3eb3d45a01354cedb7d5faa66194d1d58ba6e267a8de788f79db38"
dependencies = [
"android-tzdata",
"iana-time-zone",
"js-sys",
"num-traits",
"wasm-bindgen",
"windows-targets 0.48.5",
]

[[package]]
name = "ci_info"
version = "0.14.13"
Expand Down Expand Up @@ -3414,6 +3444,29 @@ dependencies = [
"tokio-io-timeout",
]

[[package]]
name = "iana-time-zone"
version = "0.1.58"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "8326b86b6cff230b97d0d312a6c40a60726df3332e721f72a1b035f451663b20"
dependencies = [
"android_system_properties",
"core-foundation-sys",
"iana-time-zone-haiku",
"js-sys",
"wasm-bindgen",
"windows-core",
]

[[package]]
name = "iana-time-zone-haiku"
version = "0.1.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "f31827a206f56af32e590ba56d5d2d085f558508192593743f16b2306495269f"
dependencies = [
"cc",
]

[[package]]
name = "idna"
version = "0.4.0"
Expand Down Expand Up @@ -7674,6 +7727,15 @@ version = "0.4.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f"

[[package]]
name = "windows-core"
version = "0.51.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "f1f8cf84f35d2db49a46868f947758c7a1138116f7fac3bc844f43ade1292e64"
dependencies = [
"windows-targets 0.48.5",
]

[[package]]
name = "windows-sys"
version = "0.45.0"
Expand Down
1 change: 1 addition & 0 deletions apollo-router/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ base64 = "0.21.4"
bloomfilter = "1.0.12"
buildstructor = "0.5.4"
bytes = "1.5.0"
chrono = "0.4.31"
garypen marked this conversation as resolved.
Show resolved Hide resolved
clap = { version = "4.4.6", default-features = false, features = [
"env",
"derive",
Expand Down
88 changes: 73 additions & 15 deletions apollo-router/src/services/execution_service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ use crate::json_ext::Object;
use crate::json_ext::Path;
use crate::json_ext::PathElement;
use crate::json_ext::ValueExt;
use crate::plugins::authentication::APOLLO_AUTHENTICATION_JWT_CLAIMS;
use crate::plugins::subscription::Subscription;
use crate::plugins::subscription::SubscriptionConfig;
use crate::plugins::subscription::APOLLO_SUBSCRIPTION_PLUGIN;
Expand Down Expand Up @@ -117,6 +118,10 @@ impl ExecutionService {
.query_plan
.is_deferred(operation_name.as_deref(), &variables);
let is_subscription = req.query_plan.is_subscription(operation_name.as_deref());
let mut claims = None;
if is_subscription || is_deferred {
claims = context.get(APOLLO_AUTHENTICATION_JWT_CLAIMS)?
}
let (tx_close_signal, subscription_handle) = if is_subscription {
let (tx_close_signal, rx_close_signal) = broadcast::channel(1);
(
Expand Down Expand Up @@ -174,21 +179,74 @@ impl ExecutionService {

let execution_span = Span::current();

let stream = stream
.filter_map(move |response: Response| {
ready(execution_span.in_scope(|| {
Self::process_graphql_response(
&query,
operation_name.as_deref(),
&variables,
is_deferred,
&schema,
&mut nullified_paths,
response,
)
}))
})
.boxed();
let stream =
stream
.map(move |mut response: Response| {
// Enforce JWT expiry for deferred and subscription responses
Copy link
Contributor

Choose a reason for hiding this comment

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

could it be enforced on the request side, before executing the subscription?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes good catch ! I think you can move your logic right here gary to not try to fetch data from subgraphs if your jwt is expired

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've split the logic for enforcement and now enforce subscription expiration in the request as @bnjjj suggested. I've left the defer enforcement in the response. We could remove that I suppose, but it's probably better to have it than not.

if is_deferred || is_subscription {
let ts_opt = claims
.as_ref()
.and_then(|x: &Value| {
if !x.is_object() {
tracing::error!("JWT claims should be an object");
return None;
}
let claims = x.as_object().expect("claims should be an object");
let exp = claims.get("exp")?;
if !exp.is_number() {
tracing::error!("JWT 'exp' (expiry) claim should be a number");
return None;
}
exp.as_i64()
})
.and_then(|seconds_since_epoch| {
let dt = chrono::DateTime::from_timestamp(seconds_since_epoch, 0);
garypen marked this conversation as resolved.
Show resolved Hide resolved
if dt.is_none() {
tracing::error!("JWT 'exp' (expiry) claim could not be converted to a DateTime");
}
dt
});
if let Some(ts) = ts_opt {
let now = chrono::Utc::now();
if ts < now {
tracing::debug!("token has expired, shut down the subscription");
if is_deferred {
response = Response::builder()
.has_next(false)
.error(Error::builder()
.message("deferred response closed because the JWT has expired",)
.extension_code("DEFERRED_RESPONSE_JWT_EXPIRED")
.build())
.build()
}
if is_subscription {
response = Response::builder()
.subscribed(false)
.error(Error::builder()
.message("subscription closed because the JWT has expired",)
.extension_code("SUBSCRIPTION_JWT_EXPIRED")
.build())
.build()
}
}
}
}
response
})
.filter_map(move |response: Response| {
ready(execution_span.in_scope(|| {
Self::process_graphql_response(
&query,
operation_name.as_deref(),
&variables,
is_deferred,
&schema,
&mut nullified_paths,
response,
)
}))
})
.boxed();

Ok(ExecutionResponse::new_from_response(
http::Response::new(stream as _),
Expand Down