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.

Sign up for GitHub

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
merged 1 commit into from
Dec 3, 2024
Merged

Fix security web authentication on windows #1480

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ...start/src/test/java/org/acme/security/openid/connect/web/authentication/CodeFlowTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,9 @@ private WebClient createWebClient() {
WebClient webClient = new WebClient();

webClient.setCssErrorHandler(new SilentCssErrorHandler());
// 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);
Comment on lines +159 to +161
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 •
edited
Loading

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


return webClient;
}
Expand Down
Loading