Backchannel logout #70

Erik1000 · 2022-03-27T21:11:45Z

Resolves #68

ramosbugs · 2022-03-27T21:26:27Z

now that I merged #69, mind rebasing and force pushing (or merging main back into this branch)? that should make it easier to review

src/lib.rs

ramosbugs

Thanks for the PR!

As part of this feature, I think it's worth adding the backchannel_logout_supported and backchannel_logout_session_supported fields to ProviderMetadata (per Section 2.1). Since the contents of that struct are private and we only need to add new getters and setters, this change should be backward compatible.

Similarly, the spec defines backchannel_logout_uri and backchannel_logout_session_required for ClientMetadata.

Cargo.toml

ramosbugs · 2022-03-28T01:23:51Z

src/backchannel_logout.rs

+    where
+        D: serde::Deserializer<'de>,
+    {
+        let value: serde_json::Value = serde_json::Value::deserialize(deserializer)?;


If I'm reading the spec correctly, I think this struct should be parsed as a JWT rather than raw JSON, using similar code to IdToken:

OPs send a JWT similar to an ID Token to RPs called a Logout Token to request that they log out.

The code should also verify the JWT signature:

A Logout Token MUST be signed and MAY also be encrypted

I'd suggest structuring the code similarly to what this crate does for ID tokens:

LogoutTokenClaims struct similar to IdTokenClaims (implementing both Deserialize and Serialize so that both the RP and OP code paths are supported)

LogoutToken struct similar to IdToken that wraps JsonWebToken

LogoutTokenVerifier struct similar to IdTokenVerifier or UserInfoVerifier that contains a JwtClaimsVerifier and makes sure that a nonce is omitted. I think I would just add an optional nonce field (annotated with serde(skip_serializing) so that it doesn't get serialized by an OP) to LogoutTokenClaims have the verifier check that it's None. There are some other required verification steps in the spec, including for iat (see IdTokenVerifier::iat_verifier_fn). JwtClaimsVerifier will take care of iss and aud.

If I'm reading the spec correctly, I think this struct should be parsed as a JWT rather than raw JSON

You're totally right and it's because I planned on implementing this a bit differently than IdToken but guess I'll better keep the style of this crate then.

I'm not feeling so good implementing an extra LogoutTokenVerifier because it's basically copy-paste except for the nonce part. Maybe there's a way to be more generics about this?

hmm... I tried to move all of the shared JWT verification logic to JwtClaimsVerifier. The duplicated part here would mostly be the factory methods (e.g., new_public_client) and getters/setters I think. I'm not sure how to get around that extra boilerplate. There shouldn't be too much duplicated logic though, other than verifying iat, which is simple enough.

I think I'd start by implementing this verifier with all of the needed functionality, and then maybe it'll be clear whether there are any opportunities to factor out common code.

Alright then, I'll see what I can do tomorrow.

ramosbugs · 2022-03-28T01:25:03Z

src/backchannel_logout.rs

+        }
+
+        #[derive(Deserialize)]
+        struct Repr {


Following the suggestions above, I think the duplicate Repr and custom Deserialize implementation can probably be removed in favor of serde annotations where needed

ramosbugs · 2022-03-28T01:30:08Z

src/backchannel_logout.rs

+            jti: String,
+            #[serde(flatten)]
+            identifier: Identifier,
+            events: HashMap<String, serde_json::Value>,


Using types, we can have serde check the URL key for us:

#[derive(Clone, Debug, Deserialize, Serialize)] #[serde(deny_unknown_fields)] struct BackchannelLogoutEvent {} #[derive(Clone, Debug, Deserialize, Serialize)] struct LogoutTokenEvents { #[serde(rename = "http://schemas.openid.net/event/backchannel-logout")] backchannel_logout: BackchannelLogoutEvent, }

Yes, I thought about this and came to the conclusion that it's not a good idea if not done properly. But in order to to this right, I guess we need to completely implement the SET standard and have the different events typed up in something like an enum. Also, I think this

#[derive(Clone, Debug, Deserialize, Serialize)] #[serde(deny_unknown_fields)] // <--- would violate the spec struct BackchannelLogoutEvent {}

since the spec says about this (Section 2.4):

The corresponding member value MUST be a JSON object and SHOULD be the empty JSON object {}.

it only SHOULD not MUST be empty, so we can't deny unknown fields, can we?

I've only added the check here to ensure that we only parse tokens that are valid to the spec. I've even considered completely ignoring this field apart from making sure it complies to the spec.

Oops, you are correct! Let's remove deny_unknown_fields then. What if we leave the BackchannelLogoutEvent struct empty for now but use #[non_exhaustive] so that we can add fields in the future without a breaking change?

Wouldn't it make sense to use a enum for LogoutTokenEvents?

Enums seem appropriate when we need to distinguish between mutually-exclusive types. In this case, don't we know that it should always contain an event with the http://schemas.openid.net/event/backchannel-logout key? What other info do you envision needing to represent with LogoutTokenEvents?

If extensibility is the issue, we could add a generic flattened additional_claims field. Since there aren't any other flattened fields here, we won't need FilteredFlatten.

Similarly, we could make backchannel_logout a generic type instead of defining it as the empty BackchannelLogoutEvent struct. It depends how extensible we want to get and what future functionality users might want without needing to introduce breaking changes.

The thing with this is that it might include other "events" than http://schemas.openid.net/event/backchannel-logout
and as much as I'd like to have all of these typed in an enum or something, it's out of scope for this crate.

I think just exposing the HashMap like it is currently should be fine since I don't think anyone would actually care about this and even if someone does, they could just use serde_json::from_value to parse their structs.

What other info do you envision needing to represent with LogoutTokenEvents?

Forget that, I thought we could do something like this, but that ain't gonna work:

enum Event { #[serde(rename = "http://schemas.openid.net/event/backchannel-logout")] Backchannel {}, // other possible SET event #[serde(rename = "https://schemas.openid.net/secevent/risc/event-type/account-disabled")] AccountDisabled { reason: String } }

src/backchannel_logout.rs

src/lib.rs

src/backchannel_logout.rs

…pported` to `ProviderMeta`

codecov · 2022-03-31T16:46:57Z

Codecov Report

Merging #70 (126fa10) into main (9642f22) will decrease coverage by 0.35%.
The diff coverage is 60.52%.

@@            Coverage Diff             @@
##             main      #70      +/-   ##
==========================================
- Coverage   77.32%   76.96%   -0.36%     
==========================================
  Files          16       17       +1     
  Lines        3493     3569      +76     
==========================================
+ Hits         2701     2747      +46     
- Misses        792      822      +30

Impacted Files	Coverage Δ
src/discovery.rs	`88.76% <ø> (ø)`
src/lib.rs	`71.60% <ø> (ø)`
src/backchannel_logout.rs	`59.45% <59.45%> (ø)`
src/types.rs	`72.45% <100.00%> (+0.20%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9642f22...126fa10. Read the comment docs.

Erik1000 · 2022-04-01T11:48:52Z

Also, I think we should add support for the sid claim in the IdToken too (even tho this would be already possible with the additional claims support). The backchannel spec (Section 2.1) links to the front channel spec for this: https://openid.net/specs/openid-connect-frontchannel-1_0.html#OPLogout
It's basically an additional claim in the IdToken which would use the already existing SessionIdentifier type.

…dTokenClaims`

src/backchannel_logout.rs

Cargo.toml

src/backchannel_logout.rs

ramosbugs · 2022-04-01T20:49:36Z

src/backchannel_logout.rs

+    where
+        D: serde::Deserializer<'de>,
+    {
+        let value: serde_json::Value = serde_json::Value::deserialize(deserializer)?;


hmm... I tried to move all of the shared JWT verification logic to JwtClaimsVerifier. The duplicated part here would mostly be the factory methods (e.g., new_public_client) and getters/setters I think. I'm not sure how to get around that extra boilerplate. There shouldn't be too much duplicated logic though, other than verifying iat, which is simple enough.

I think I'd start by implementing this verifier with all of the needed functionality, and then maybe it'll be clear whether there are any opportunities to factor out common code.

ramosbugs · 2022-04-01T20:58:52Z

src/backchannel_logout.rs

+            jti: String,
+            #[serde(flatten)]
+            identifier: Identifier,
+            events: HashMap<String, serde_json::Value>,


Enums seem appropriate when we need to distinguish between mutually-exclusive types. In this case, don't we know that it should always contain an event with the http://schemas.openid.net/event/backchannel-logout key? What other info do you envision needing to represent with LogoutTokenEvents?

If extensibility is the issue, we could add a generic flattened additional_claims field. Since there aren't any other flattened fields here, we won't need FilteredFlatten.

Similarly, we could make backchannel_logout a generic type instead of defining it as the empty BackchannelLogoutEvent struct. It depends how extensible we want to get and what future functionality users might want without needing to introduce breaking changes.

ramosbugs · 2022-04-01T21:02:43Z

Also, I think we should add support for the sid claim in the IdToken too (even tho this would be already possible with the additional claims support). The backchannel spec (Section 2.1) links to the front channel spec for this: https://openid.net/specs/openid-connect-frontchannel-1_0.html#OPLogout It's basically an additional claim in the IdToken which would use the already existing SessionIdentifier type.

That's worth considering. Feel free to file a separate issue for tracking that. I think it'll be a breaking change though since some users may be relying on additional_claims already to parse that field, and with this change it would no longer be passed down. It might be simpler to provider helper types that users can pass via additional_claims to avoid breaking backward compatibility for the IdTokenClaims struct itself.

Erik1000 · 2022-04-01T21:28:06Z

That's worth considering. Feel free to file a separate issue for tracking that. I think it'll be a breaking change though since some users may be relying on additional_claims already to parse that field, and with this change it would no longer be passed down.

Yup, I thought exactly about that because I implemented that using an additional claim in one of my projects.

KevinNaidoo · 2022-08-29T18:25:22Z

Hi @Erik1000 / @ramosbugs,

I'd like to move this ahead, I'm currently implementing backchannel logout and have started adding functionality to the work done here. Any concerns around the approach or has development stopped because of other work?

Erik1000 · 2022-08-29T18:39:58Z

Hi @Erik1000 / @ramosbugs,

I'd like to move this ahead, I'm currently implementing backchannel logout and have started adding functionality to the work done here. Any concerns around the approach or has development stopped because of other work?

Hi, work here has stopped because the JWT implementation has some issues (including missing support for JWE and it requires some unnecessary code duplication). A friend of mine and me have started a JOSE implementation that tries to solve these problems with a better, more extensible, and powerful way including an rust only crypto backend (which solves #58). We’re aiming for our first stable release within the next two weeks (see minkan-chat/jose#30 and also look at the other branches). Current state is that JWS is working with a great api and we just want to implement an better way for payloads (needed because of detached content).
When we’re ready, back channel tokens can easily be parsed.

KevinNaidoo · 2022-08-30T20:40:10Z

Hi @Erik1000 / @ramosbugs,
I'd like to move this ahead, I'm currently implementing backchannel logout and have started adding functionality to the work done here. Any concerns around the approach or has development stopped because of other work?

Hi, work here has stopped because the JWT implementation has some issues (including missing support for JWE and it requires some unnecessary code duplication). A friend of mine and me have started a JOSE implementation that tries to solve these problems with a better, more extensible, and powerful way including an rust only crypto backend (which solves #58). We’re aiming for our first stable release within the next two weeks (see minkan-chat/jose#30 and also look at the other branches). Current state is that JWS is working with a great api and we just want to implement an better way for payloads (needed because of detached content). When we’re ready, back channel tokens can easily be parsed.

Thanks @Erik1000. I've made some progress with implementation of the logout token but will look forward to having a look at your JOSE implementation.

KevinNaidoo · 2022-08-30T21:05:04Z

Hi @Erik1000 / @ramosbugs,
I'd like to move this ahead, I'm currently implementing backchannel logout and have started adding functionality to the work done here. Any concerns around the approach or has development stopped because of other work?

Hi, work here has stopped because the JWT implementation has some issues (including missing support for JWE and it requires some unnecessary code duplication). A friend of mine and me have started a JOSE implementation that tries to solve these problems with a better, more extensible, and powerful way including an rust only crypto backend (which solves #58). We’re aiming for our first stable release within the next two weeks (see minkan-chat/jose#30 and also look at the other branches). Current state is that JWS is working with a great api and we just want to implement an better way for payloads (needed because of detached content). When we’re ready, back channel tokens can easily be parsed.

Thanks @Erik1000. I've made some progress with implementation of the logout token but will look forward to having a look at your JOSE implementation.

Ps., but I've seen exactly what you mean, and was why I wanted to implement the backchannel logout within the library, for JWE.

Erik1000 · 2022-08-30T21:10:19Z

Ps., but I've seen exactly what you mean, and was why I wanted to implement the backchannel logout within the library, for JWE.

@KevinNaidoo well, JWE is quite complex and isn't implemented by openidconnect (and it AFAIK violates the spec because of that?). We plan to implement it, but because JWSes are used most of the time (many people don't even know that JWTs can be encrypted, almost no JWT library supports it), we want to create a first release without JWS. The groundwork for JWE has been done on our end and we have some ideas how to implement it from an API perspective.

KevinNaidoo · 2022-08-30T21:28:47Z

Feels like I'm following in your footsteps... I've been asking the question of but what if the JWT is encrypted, but many people/libraries aren't seeming to be doing that. But it is complex, look forward to your API, makes sense to release without first. I've also decided to proceed without JWEs for now,

This comment was marked as outdated.

Sign in to view

Erik1000 force-pushed the backchannel-logout branch from d7f325e to 348f500 Compare March 27, 2022 21:14

Erik1000 mentioned this pull request Mar 27, 2022

Support parsing logout tokens #68

Open

Erik1000 added 4 commits March 27, 2022 23:41

add new SessionIdentifier type for sid claim

65807df

add feature flag for backchannel logout

3a10ef3

implement logout token deserialization

091b675

remove print from test case

9ab4304

Erik1000 force-pushed the backchannel-logout branch from d52976c to 9ab4304 Compare March 27, 2022 21:43

Erik1000 commented Mar 27, 2022

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

Erik1000 added 2 commits March 27, 2022 23:52

rename logout_token module and remove re-export

3785aa5

make backchannel_logout module public

d184068

ramosbugs reviewed Mar 28, 2022

View reviewed changes

Erik1000 added 2 commits March 31, 2022 18:41

add backchannel_logout_supported and `backchannel_logout_session_su…

f0c82e5

…pported` to `ProviderMeta`

move pub mod down to core

a377bd8

use JsonWebKeyId instead of String for jti claim

cde3bc2

Erik1000 added 3 commits April 1, 2022 14:50

rename Identifier to LogoutIdentifier

742f108

rename LogoutToken to LogoutTokenClaims to be consistence with `I…

94213f0

…dTokenClaims`

allow additional claims in LogoutTokenClaims

f9c62e8

Erik1000 commented Apr 1, 2022

View reviewed changes

src/backchannel_logout.rs Outdated Show resolved Hide resolved

implement serde serialization implementations for backchannel logout

351cda0

ramosbugs reviewed Apr 1, 2022

View reviewed changes

filter LogoutIdentifier instead of whole LogoutTokenClaims

126fa10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backchannel logout #70

Backchannel logout #70

Erik1000 commented Mar 27, 2022 •

edited by ramosbugs

This comment was marked as outdated.

ramosbugs commented Mar 27, 2022

ramosbugs left a comment

ramosbugs Mar 28, 2022

Erik1000 Mar 31, 2022

Erik1000 Apr 1, 2022

ramosbugs Apr 1, 2022

Erik1000 Apr 1, 2022

ramosbugs Mar 28, 2022

ramosbugs Mar 28, 2022

Erik1000 Mar 31, 2022

ramosbugs Mar 31, 2022

Erik1000 Apr 1, 2022

ramosbugs Apr 1, 2022

Erik1000 Apr 1, 2022

codecov bot commented Mar 31, 2022 •

edited

Erik1000 commented Apr 1, 2022

ramosbugs Apr 1, 2022

ramosbugs Apr 1, 2022

ramosbugs commented Apr 1, 2022

Erik1000 commented Apr 1, 2022

KevinNaidoo commented Aug 29, 2022

Erik1000 commented Aug 29, 2022

KevinNaidoo commented Aug 30, 2022

KevinNaidoo commented Aug 30, 2022

Erik1000 commented Aug 30, 2022

KevinNaidoo commented Aug 30, 2022 •

edited

Backchannel logout #70

Are you sure you want to change the base?

Backchannel logout #70

Conversation

Erik1000 commented Mar 27, 2022 • edited by ramosbugs

This comment was marked as outdated.

ramosbugs commented Mar 27, 2022

ramosbugs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 31, 2022 • edited

Codecov Report

Erik1000 commented Apr 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramosbugs commented Apr 1, 2022

Erik1000 commented Apr 1, 2022

KevinNaidoo commented Aug 29, 2022

Erik1000 commented Aug 29, 2022

KevinNaidoo commented Aug 30, 2022

KevinNaidoo commented Aug 30, 2022

Erik1000 commented Aug 30, 2022

KevinNaidoo commented Aug 30, 2022 • edited

Erik1000 commented Mar 27, 2022 •

edited by ramosbugs

codecov bot commented Mar 31, 2022 •

edited

KevinNaidoo commented Aug 30, 2022 •

edited