oidc icon indicating copy to clipboard operation
oidc copied to clipboard

feat: Request aware cookie handling

Open markylaing opened this issue 6 months ago • 4 comments

Hi :wave: firstly, thank you for this great library!

LXD is a hypervisor for system containers and VMs. It can be clustered to give a private cloud like experience.

When LXD is clustered, we have an issue wherein the authorization code flow fails if the host that receives the callback (receiver of /oidc/callback) is different from the host that begins the flow (receiver of /oidc/login; see https://github.com/canonical/lxd/issues/13644). This is because the cookie handler in the relying party has different hash and encryption keys for each cluster member. Thus, when the callback is executed, the relying party cannot decode the state or PKCE cookies and the callback fails.

As discussed in the linked issue, this can be mitigated via sticky sessions on an external load balancer. However, we are increasingly seeing LXD deployments placed behind DNS SRV records, where sticky sessions are not possible.

We would like to be able to derive cookie encryption keys from some shared secret, so that the hash and encryption keys are the same on all cluster members. To obfuscate the secret we would like to use a nonce value during derivation. Ideally this nonce would be unique per client.

The relying party accepts a CookieHandler, which in turn accepts static hash and block keys. In order to use unique keys per-client (or on rotation), a new relying party must be created for each cookie handler. This is not ideal, because creation of a new relying party re-triggers OIDC discovery and creates a new remote key set (which will retrieve JWKs when required).

This PR adds functionality to the CookieHandler to allow more dynamic handling of cookies, based on the incoming request. I have taken care not to break any existing APIs.

Please let me know what you think and what my next steps should be (e.g. testing)

Definition of Ready

  • [x] I am happy with the code
  • [x] Short description of the feature/issue is added in the pr description
  • [x] PR is linked to the corresponding user story
  • [ ] Acceptance criteria are met (not sure)
  • [ ] All open todos and follow ups are defined in a new ticket and justified (not sure)
  • [ ] Deviations from the acceptance criteria and design are agreed with the PO and documented. (not sure)
  • [x] No debug or dead code
  • [ ] My code has no repetitions (some slight repetition to avoid API breakage)
  • [ ] Critical parts are tested automatically (not sure)
  • [ ] Where possible E2E tests are implemented
  • [ ] Documentation/examples are up-to-date
  • [ ] All non-functional requirements are met (not sure)
  • [ ] Functionality of the acceptance criteria is checked manually on the dev system. (not sure)

markylaing avatar Jun 04 '25 11:06 markylaing

Code looks great. Perhaps you can get some testing in by using (or creating a variant of) testRelyingPartySession

Great stuff I'll look into doing that and ping you when I have something :)

markylaing avatar Jun 05 '25 12:06 markylaing

@muhlemmer hi :wave: I've added integration tests now. Tests all ok locally :)

markylaing avatar Jun 06 '25 08:06 markylaing

FYI I'll be away until the 17th of June and may not be able to respond or update this PR in the meantime. Apologies!

markylaing avatar Jun 06 '25 13:06 markylaing

@muhlemmer good morning :) I'm available to respond to any comments about this PR now. Looking forward to hearing your feedback. Thanks

markylaing avatar Jun 17 '25 08:06 markylaing

@muhlemmer is there anything else I can do to help get this over the line? Thanks

markylaing avatar Jun 24 '25 13:06 markylaing

Thanks @markylaing, Tim is on hols atm, he's back in a couple of days, then he'll have a look again!

elinashoko avatar Jul 10 '25 08:07 elinashoko

Thanks @markylaing, Tim is on hols atm, he's back in a couple of days, then he'll have a look again!

Great stuff thank you :pray:

markylaing avatar Jul 10 '25 08:07 markylaing

:tada: This PR is included in version 3.41.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

github-actions[bot] avatar Jul 16 '25 11:07 github-actions[bot]