identity: add WebSSO authentication - #3941

should not malformed entries return an error or at least a warning? I think it's reasonable to consider missing and/or expired as a cache miss, but a malformed entries could be a result of something wrong somewhere else in the code.

I intentionally made the cache best-effort here. A malformed or stale local entry should not make authentication fail when we can delete it and continue through the normal browser flow; the next successful login repairs the entry. The cache package also has no logging facility, and returning a decoding error would make an optional cache less reliable than having no cache.

I agree that malformed data is distinct from an ordinary miss, though, and I can clarify that behavior in the docs. If the preferred contract is to expose backend or decoding errors, I can change Load to return an error, but my inclination is still delete-and-miss for malformed data and to reserve errors for failures that prevent recovery.

[...] A malformed or stale local entry should not make authentication fail when we can delete it and continue through the normal browser flow; the next successful login repairs the entry. [...]

This has convinced me, so I'm okay keeping as-is. But let's keep the thread open in case anyone has objections.

As I can see, we don't have any concrete implementation of this interface throughout the PR (regardless the tests).

Is this being addressed in a follow-up or is it intended to be optional and implemented by the client?

So the python client uses the keyring package to store the cache in the system key chain. It's ultimately implemented by the end user client. But it would require each client to implement this. So I'm not sure how to best address this. There could be a default implementation in gophercloud? I'd be happy to do it as a follow up.

As a SDK point of view, we want to make the end users' life easier, so IMO it's worth having at least one basic implementation while delivering the cache benefits (it improves overall performance and avoid round tripping to keystone).

If the other people agree, I think we could do this in a follow-up PR yes 🙂.

The interface is already consumed by WebSSO in this PR: before opening the browser, it loads and validates a cached unscoped token; if Keystone rejects that token, it deletes the entry; after a successful browser flow, it persists the newly captured token. What is not included here is a concrete persistent-storage backend.

The original version of this integration put the system-keyring implementation in gophercloud/utils, alongside the higher-level clientconfig wiring. As the rest of this work evolved, the core clouds.ParseV3 follow-up made that Utils layer unnecessary. I did not move the keyring backend into base Gophercloud because doing so would add go-keyring and its platform-specific transitive dependencies to the core module for every user. Instead, the provider integration currently supplies the implementation in internal/keyringcache and passes it through clouds.WithTokenCache.

I am not opposed to putting a concrete implementation here. If the maintainers are comfortable with those dependencies in the base Gophercloud module, I would be happy to move the keyring implementation into this PR or a small follow-up. I mainly wanted to avoid making that dependency decision implicitly.

For context, the complete dependency plan is:

• #3939, context-aware reauthentication, is merged and is the common prerequisite.
• #3984 adds the Identity v3 provider-construction path used by the higher-level parser.
• This PR adds WebSSO and the generic cache contract. WebSSO is its first consumer.
• #3942 adds OIDC client-credentials auth. Assuming the PR we're commenting on now merges first, I will update this OIDC PR to make it use the same cache contract.
• #3940 adds OAuth2 mTLS support.
• The unposted feat/federated-auth-clouds follow-up adds clouds.ParseV3, registers those auth methods, and replaces the remaining gophercloud/utils clientconfig role. This might be a little confusing to look at since it is currently my own kind of ... integration branch that I use for deploying, but once a couple more of these merge I will have some follow-ups. I wish GitHub would let me do stacked PRs from my fork. :(
• garm-provider-openstack#30 is the merged Gophercloud v2 baseline. Its integration follow-up consumes clouds.ParseV3 and supplies the OS-keyring backend shown above.

Keeping the cache contract generic lets WebSSO now and OIDC later share the key format and token envelope, while leaving storage selection outside the core module unless the maintainers prefer otherwise.

Again, I am happy to add an implementation here, I was actually not happy to leave it without one either, but it felt like ya'll might balk at the added dependencies, so say the word and I'll put it in here.

Although the cache is a "should have" considering the performance improvements, it isn't a hard dependency and we can do it in a follow-up PR if we want, so I'll not push this too long.

Can we at least document that users need to implement this and provide one reliable source?

I will try to thoroughly review the PRs in the correct order. This seems a perfect use case for stacked PRs 🙂

Yeah I wish they worked from a fork. If I had write access to this repo I could push them all to branches here and then created stacked PRs, but they don't work from a fork to the source repo (or fork to fork). :(

Again, I would PREFER to add the cache implementation here NOW, the blocker for me was that I thought it would not be ideal to add client dependencies here on the keyring deps... but I am totally happy to just DO IT, if you give me a green light on that (and would rather just do it than put in temporary docs).

Let's land this PR and let me pull it down in some local work and then stack some more PRs.

Yeah, I mean the real issue is that I also am waiting for about three of these to land so that I can make some follow ups that tie them all together better, I literally have some branches just sitting waiting to be able to sanely make them a PR without the ability to stack :(

Looking at the upstream implementation, the callback is opened to CSRF and @cardoe tried to mitigate this with some validation in a helper function.

For security reasons, wdyt of having something similar here? We're only checking method, content-type, and body size.

It's worth noting that the CSRF is happening locally so it's not really a big danger. But I am trying to mitigate it going forward.

Good point. The listener is restricted to localhost or a loopback address, accepts only a form POST on the callback path, limits the body size, and validates the submitted token with Keystone before using it. That keeps it from being a remotely exposed arbitrary-token endpoint, but it does not bind the callback to the browser flow that opened it. A malicious local process, or a web page able to submit a form to the loopback listener, could race the legitimate callback and cause a denial of service or, with a valid token, an authentication-confusion result.

I agree that a per-flow unpredictable nonce is worthwhile defense in depth here. I can add the equivalent binding and focused tests in this PR. To be honest, I thought I remembered already doing that, but it might have been in one of my experimental branches. I'll just go ahead and handle that now.

One correction after checking this while implementing it: a per-flow nonce cannot be added to the origin, because Keystone requires that value to match a trusted_dashboard entry verbatim. The current keystoneauth change documents the same limitation.

I added the equivalent request-shape hardening in 43bc756: the callback now requires top-level navigation Fetch Metadata, rejects direct navigation, and rejects mismatched Origin or Referer when those headers are available. Focused tests cover each case. This narrows what can reach token handling, but it cannot cryptographically bind the callback to the flow, so the remaining login-CSRF limitation is documented.

So this is where our previously discussed lack of a keystoneauth equivalent comes back to bite us. Does this file belong here? Typically we only modules corresponding to APIs in openstack/<service>/<version> packages.

Would it be possible to have this (and it's test) in e.g. openstack/auth instead.

Yep, or just auth/ which #4008 introduced (not sure whether that needs to move or not)

I definitely feel like this doesn't belong here. I had a look for some of our more unusual existing auth mechanisms and found openstack/identity/v3/oauth1/doc.go. I note that that uses the more standard AuthenticateV3 function from openstack/client.go. I'm wondering if this PR ought to hang off of the pluggable auth PR (which I'm yet to review also). I need to think about this one...

Later: Maybe if we called this "Create" we could deal with this for now?

It's layering on more c***, but at least it's consistent c***?

I'm just going to refactor completely on #4008