Use state param for original visited URL and support fixed OIDC callback URL #55
Conversation
k3a
force-pushed
the
k3a-cookieless-state
branch
from
f122d20
to
34fc892
Compare
…xed OIDC callback URL Use OIDC state parameter which is later returned during the auth flow to encode the original visited URL instead of using a cookie. Using a cookie is unnecessary in this case and even causes race conditions if multiple requests are made to a different OIDC-protected targets at the same time. Additionally, the previously-used cookie for this auth flow purpose was named the same as the session cookie, which could even effectively overwrite an existing valid session cookie. This commit also implements support for fixed callback URI/URLs as many public OIDC providers require admins to configure a fixed return/callback URLs in advance for every client to prevent abuse (e.g. by uploading a fake OIDC handler script on a different URI using some unprotected uploader and stealing tokens returned by the OIDC provider to this callback URL).
k3a
force-pushed
the
k3a-cookieless-state
branch
from
34fc892
to
8764ccf
Compare
| "github.com/ibm-cloud-security/app-identity-and-access-adapter/tests/fake" | ||
| ) | ||
|
|
||
| func TestCallbackURLForTarget(t *testing.T) { |
There was a problem hiding this comment.
Thanks for chacking this. Let's see what @ishangulhane or @TalAviel will say. I will soon start working on extending session store. Currently it stores all sessions in sync.Map in the adapter without any expiration (you can see it by searching for w.tokenCache). This needs to be extended in some plug-gable manner, through interface into more implementations. One of them could be Redis, the other one securecookie or something like that. This will have separate PR and/or issue.
|
@k3a @ishangulhane @TalAviel Any updates on this? Will these improvements make it to the master branch? |
|
I lost interest in this due to planned deprecation of Mixer in future Istio and their shift to webassembly (which is probably a good idea but still in early stages). Istio may be great in couple of years but I don't feel like I would be confident running it in production now. Breaking changes too often, new bugs introduced due to overall complexity and attempt to support wide range of use cases. I decided to use Traefik which makes it possible to apply auth "middleware" to specific routes only (instead of everything like we saw in this mixer adapter). Envoy supports ext_authz too and can be configured for individual routes but I don't like the Envoy config verbosity and Istio doesn't handle such use cases (yet). For now (and hopefully long enough) I am going Traefik + Cilium way. Of course, anyone is free to use or discard this pull request as they wish. |
This is a bigger commit extending the adapter greatly.
Use OIDC state parameter which is later returned during the auth flow
to encode the original visited URL instead of using a cookie. Using
a cookie is unnecessary in this case and even causes race conditions if
multiple requests are made to a different OIDC-protected targets
at the same time.
Additionally, the previously-used cookie for this auth flow purpose
was named the same as the session cookie, which could even
effectively overwrite an existing valid session cookie.
This commit also implements support for fixed callback URI/URLs
as many public OIDC providers require admins to configure
a fixed return/callback URLs in advance for every client
to prevent abuse (e.g. by uploading a fake OIDC handler script
on a different URI using some unprotected uploader and stealing
tokens returned by the OIDC provider to this callback URL). Fixes #52.
It is made backward-compatible so the default callback is relative
oidc/callbackas it was in the original implementation.Additionally it adds support for enforcing Secure attribute on the session
cookie (previously TODO). It is enabled using
--secure-cookiesflag.It is not enabled by default so that the adapter can be tested over HTTP
on test clusters. Helm template was updated to add support for this
secureCookiesHelm variable has to be set to true to enablethis flag in the Deployment.
Session cookie also contains Max-Age because support for Expires
is currently broken in Instio.
To increase security, session cookies also have SameSite=Lax.
This is browser default now but safer to explicitly say.
That prevents sending session cookie with cross-origin requests,
while still allowing cross-origin GET requests.
Some tests were slightly changed and some new tests were added.
Reviews are welcome. While I can eventually have my own fork,
I would like to get this merged upstream as long as there is nothing
conceptually or functionally wrong. I am ok to update the pull
request to fix the eventual problems.
Questions:
and the appidentityandaccessadapter-*.tgz recreated?
My image to play with
This pull request, together with my other pull requests here
is included in my k3a branch.
The latest image produced from that branch is:
kexik/app-identity-and-access-adapter:96ce1afso it is possible to play with that image or compile it yourself.
A bit of offtopic:
Next, this adapter needs more ways to handle session cookies.
Maybe session cookies should be configurable to use stateless
SecureCookie as well and/or other storage options like Redis.
If I understand it correctly, the session storage is what prevents HA.
But that is another topic to be discussed and addressed separately.