Add note about Varnish and cookie-pass to the readme.

[mauritsvanrees]

Fixes https://github.com/collective/pas.plugins.oidc/issues/11

[coveralls]

Pull Request Test Coverage Report for Build 4004223000

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 58.639%

---

Totals
Change from base Build 4004169596:	0%
Covered Lines:	224
Relevant Lines:	382

---

💛 - Coveralls

[erral]

@mauritsvanrees we can even add this to the default configuration of plone.recipe.varnish right? It will not harm right?

[mauritsvanrees]

@mauritsvanrees we can even add this to the default configuration of plone.recipe.varnish right? It will not harm right?

I think that would (very slightly) drop performance for every user of this recipe, and only benefit the users of this plugin, as it is the only package that uses a cookie __ac_session.

But if we could change the recipe to treat any cookie starting with __ac this way, that may be good.

[mamico]

@mauritsvanrees @fredvd good catch! __ac_session is used also for state and should be checked against OAuth2 response (see https://github.com/collective/pas.plugins.oidc/blob/main/src/pas/plugins/oidc/browser/view.py#L190). I suppose that for a problem similar like this, the check was actually commented.

Alternatively, any suggestions for handling session information differently/better?

[fredvd]

Anything 'better' around the cookie handling could maybe be to manage the cookie setting client side using javascript and not depending on server side cookies. I've seen this also with consent state: you can do this 'server side' with forms that submit to the server and the server setting the state, or do everything client side. Drawback: graceful degradation. But I have no clue if this is feasible with the OIDC step and if it would bring any advantage.

Concerning cookie parsing in Varnish: there's some complex code now in the plone.recipe.varnish recipe that does the cookie sanitizing and it might impact performance with more cookie exceptions, but we haven't noticed anything special with 5-6 extra cookies in the whitelist. https://github.com/collective/plone.recipe.varnish/blob/266195e1c46d23af120a5f69dcad16dc8204635a/plone/recipe/varnish/templates/varnish6.vcl.jinja2#L145-L171

If performance would become a problem, the Varnish developers & community maintain a library of curate vmod's, varnish-modules: https://github.com/varnish/varnish-modules . The plone.recipe.varnish recipe can install those and the varnish-modules for 6.0 still have the cookie module (if you look at the master branch the cookie module is no longer there because it became part of the core varnish distribution in 6.4) We could decide to make the recipe dependent on varnish-modules and use the cookie vmod to do the cleaning/parsing.

But that's more for a discussion on the p.r.varnish repo and maybe the effort for that is better spend on upgrading the buildout recipe to use the latest Varnish 7.X. It does not seem that Varnish Software is going to release a new LTS version again soon, despite their 'promise' when switching to a half yearly time boxed release schedule. Not their priority in the current business model with Varnish Plus and apparently enough customers.

[mauritsvanrees]

@mauritsvanrees @fredvd good catch! __ac_session is used also for state and should be checked against OAuth2 response (see https://github.com/collective/pas.plugins.oidc/blob/main/src/pas/plugins/oidc/browser/view.py#L190). I suppose that for a problem similar like this, the check was actually commented.

Quick point: the commented line uses an assert statement, and this will be optimised away by the compiler, at least when compiling it to a .pyo file. So it would be better to use a condition and raise a ValueError (or similar) when the condition fails.

Alternatively, any suggestions for handling session information differently/better?

It seems fine and logical. You need some way to compare information from the browser, the oidc server, and Plone. Cookies seem fine for that. No immediate idea on an alternative.

Well, we could use standard Zope sessions. IIRC they use the ZopeId cookie to store a session id, and this is already in the cookie-whitelist of the recipe, so that should work. But then you need proper versions and configuration of tempstorage and related products that Plone 6 does not use by default anymore. Or add Products.mcdutils as the code points out, and store the sessions in a shared memcache server. So not something that really works out of the box. The current way, with all information stored in a cookie without server side sessions, should preferably always be available as fallback.