Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix security web authentication on windows #1480

Merged

Conversation

jedla97
Copy link
Contributor

@jedla97 jedla97 commented Dec 3, 2024

We (QE) found out that on our windows machine the security-openid-connect-web-authentication-quickstart one test failing. After looking into it I see that for some reason the HtmlUnit WebClient caching the request/response. So setting the max size of cache to 0. Maybe this can happen on other machines then win, it's probably depends on setting of machine and jvm

Also can tis be backported to 3.15 branch

Check list

Your pull request:

  • targets the development branch
  • uses the 999-SNAPSHOT version of Quarkus
  • has tests (mvn clean test)
  • works in native (mvn clean package -Pnative)
  • has integration/native tests (mvn clean verify -Pnative)
  • makes sure the associated guide must not be updated
  • links the guide update pull request (if needed)
  • updates or creates the README.md file (with build and run instructions)
  • for new quickstart, is located in the directory component-quickstart
  • for new quickstart, is added to the root pom.xml and README.md

Copy link

quarkus-bot bot commented Dec 3, 2024

Status for workflow Pull Request Build - development

This is the status report for running Pull Request Build - development on commit 13aba8e.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@fedinskiy
Copy link
Contributor

@rsvoboda could you take a look?

@rsvoboda
Copy link
Member

rsvoboda commented Dec 3, 2024

@jedla do we need the same change also in security-openid-connect-multi-tenancy-quickstart/src/test/java/org/acme/quickstart/oidc/CodeFlowTest.java?

@jedla97
Copy link
Contributor Author

jedla97 commented Dec 3, 2024

@jedla do we need the same change also in security-openid-connect-multi-tenancy-quickstart/src/test/java/org/acme/quickstart/oidc/CodeFlowTest.java?

Looking at the test and they don't call the same endpoint multiple time in same test so I believe it's not needed

@rsvoboda rsvoboda merged commit 538f9a6 into quarkusio:development Dec 3, 2024
2 checks passed
@rsvoboda
Copy link
Member

rsvoboda commented Dec 3, 2024

@jedla97 please prepare backport PR for 3.15 branch

@jedla97 jedla97 deleted the fix-sec-web-auth-on-windows branch December 3, 2024 14:13
Comment on lines +159 to +161
// Setting the cache size to zero as webClient by default can store cached request,
// which causing the `testTokenTimeoutLogout` fail as the session cookie is not changed
webClient.getCache().setMaxSize(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit concerned by that. Because if the response is cached by testing and the behavior is not what we expect, I would expect some browsers to also cache the page.

Should we fix the headers on the server side instead to make sure the page is not cached? I haven't looked at the quickstart and if the fix would be in the quickstart or in Quarkus.

Note that I haven't looked at the details (I'm in the train), I just wanted to log this concern so I don't forget about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jedla97 will be looking to this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I was looking at it.

TL; DR; It's the htmlunit logic how they store cache, but something weird is happening as the response headers are different when sending request with webclient and sending it using browser.

Founding:

I found out that this is caused by difference in last-modified header. On Windows the header is the time of creation/last modification of file. On Linux the header have timestamp close to time of the request (Date header).

I was able to get these headers only when running test (hmtlUnit WebClient, restAssured given()). When running curl or browser request, I dind't get the last-modified header.

Why whas this cached and causing the test failiure.
Debuging it I see that the isCacheableContent method don't store automaticaly when the no-store cache header is used. In this case it get's the this is cacheble because the index.html has not been modified for long.

Afaik I spot that this using 2.36.0 version which was released in 2019. I tried to update it to latest version 4.7.0. They seems change this part of logic. And now they use this logic isWithinCacheWindows.

When testing this I was able to reproduce the same issue which I have on Windows on Linux.

The response setting 2 values which can modify behavioral of the cache cache-control=public, immutable, max-age=86400 and last-modified=Thu, 5 Dec 2024 16:42:30 GMT. The htmlUnit now cache the response because of the max-age, so to be not cached the value should be 0 or use no-cache value insted.

Interesting that both of these headers are not part of the response in Chrome and Firefox.

Also tested it with 3.2.12 to check how it work back then and I see the same headers applied (same for resteasy classic).

Btw if you want to check just header this quickstart is not needed as the same happening with simple quarkus-rest + index.html resource.

Also if you want I can update html unit to latest version

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, so my actual problem is that a page protected by authentication should probably not be cached. So I wonder if we should add a no cache header when a page is protected with something like:

quarkus.http.auth.permission.authenticated.paths=/*
quarkus.http.auth.permission.authenticated.policy=authenticated

Now maybe it's not our responsibility to do it automatically, I'm just a bit concerned that it could cause some issues. Which are demonstrated by this failing test.

@michalvavrik @sberyozkin @cescoffier any opinion on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(re-ping) @sberyozkin @cescoffier @michalvavrik any opinion on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, so my actual problem is that a page protected by authentication should probably not be cached. So I wonder if we should add a no cache header when a page is protected with something like:

quarkus.http.auth.permission.authenticated.paths=/*
quarkus.http.auth.permission.authenticated.policy=authenticated

Now maybe it's not our responsibility to do it automatically, I'm just a bit concerned that it could cause some issues. Which are demonstrated by this failing test.

We don't know HTTP response content, it can be perfectly fine to cache response. As for whether it is protected resource or not, depending on the service, protected resources can be all of them. Can you really make any decision regarding caching based on authentication or not? Because it has no relation to the nature of the content. I think it is a mistake to cache many 4xx or 5xx HTTP statuses. I'd expect that API Gateway should configure when to cache and if it is configured to cache 401 than it's likely a mistake.

Now, I think most of the time it is right to add Cache-Control: no-cache for responses we know that ended with 401/403 as it can easily change, for example if you have reported that some users can't access a stuff, you add him a permission and you don't want to tell him wait for "xyz" (or force reload with no cache). but I also think it should be configurable.

I'm bit short on time, can you maybe open issue and we can discuss it there? I'll definitely forget about this.

Copy link
Member

@michalvavrik michalvavrik Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, I was talking about the client side. if you are worried that this private content is cached before that like in AWS CloudFront (and served to a different person?), then I think it is a misconfiguration. I think you need to check with someone else, I am not sure how companies configure that and I can be easily wrong. Thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quarkusio/quarkus#45187 created to have the discussion about this topic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants