openidconnect-rs icon indicating copy to clipboard operation
openidconnect-rs copied to clipboard

Backchannel logout

Open Erik1000 opened this issue 2 years ago • 12 comments

Resolves #68

Erik1000 avatar Mar 27 '22 21:03 Erik1000

This branch contains the commits from #69

Erik1000 avatar Mar 27 '22 21:03 Erik1000

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

ramosbugs avatar Mar 27 '22 21:03 ramosbugs

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%) :arrow_up:

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.

codecov[bot] avatar Mar 31 '22 16:03 codecov[bot]

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.

Erik1000 avatar Apr 01 '22 11:04 Erik1000

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.

ramosbugs avatar Apr 01 '22 21:04 ramosbugs

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.

Erik1000 avatar Apr 01 '22 21:04 Erik1000

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?

KevinNaidoo avatar Aug 29 '22 18:08 KevinNaidoo

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 https://github.com/minkan-chat/jose/issues/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.

Erik1000 avatar Aug 29 '22 18:08 Erik1000

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 avatar Aug 30 '22 20:08 KevinNaidoo

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.

KevinNaidoo avatar Aug 30 '22 21:08 KevinNaidoo

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.

Erik1000 avatar Aug 30 '22 21:08 Erik1000

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,

KevinNaidoo avatar Aug 30 '22 21:08 KevinNaidoo