8000 Avoiding nip.io for stability by ahus1 · Pull Request #38613 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Avoiding nip.io for stability #38613

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

Jump to bottom
Merged
merged 1 commit into from
Apr 8, 2025
Merged

Avoiding nip.io for stability #38613

merged 1 commit into from
Apr 8, 2025

Conversation

ahus1
Copy link
Contributor
@ahus1 ahus1 commented Apr 2, 2025

Closes #38104

@ahus1 ahus1 self-assigned this Apr 2, 2025
@ahus1
Avoiding nip.io for stability
4a9cf5d 
Closes keycloak#38104

Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
@ahus1 ahus1 force-pushed the is-38104-avoid-nip-io-for-stability branch from 67fca54 to 4a9cf5d Compare April 2, 2025 14:45
ahus1
ahus1 commented Apr 2, 2025
View reviewed changes
...ration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/AbstractSamlTest.java
@@ -73,6 +74,7 @@ public abstract class AbstractSamlTest extends AbstractAuthTest {

static {
try {
CryptoIntegration.init(Thread.currentThread().getContextClassLoader());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case you are wondering: This was necessary to run SamlReverseProxyTest isolated from the CLI, as then the crypto integration wasn't initialized yet. This call doesn't do any harm if it isn't initialized yet.

@ahus1 ahus1 marked this pull request as ready for review April 2, 2025 16:05
@ahus1 ahus1 requested review from a team as code owners April 2, 2025 16:05
@ahus1
Copy link
Contributor Author
ahus1 commented Apr 2, 2025

@vaceksimon / @mhajas - this is a PR which avoids nip.io in the reported places. Please review when you have the time and merge. Thanks!

mhajas
mhajas reviewed Apr 3, 2025
View reviewed changes
Copy link
Contributor
@mhajas mhajas left a comment

Choose a reason for hiding this comment

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

I am not sure we can do this. The purpose of nip.io usage is to simulate usage of a real domain in browser. Cookies for 0.0.0.0 may be handled differently, like it is possible secure cookies are sent also to insecure context when 0.0.0.0 is used.

@ahus1
Copy link
Contributor Author
ahus1 commented Apr 3, 2025

For the cookie provider test, 0.0.0.0 is not considered a secure context as long as no https is used, so IMHO it is equivalent to .nip.io and it is not considered a localhost address. I see this regularly when opening that URL in the browse locally and the APIs to copy to the clipboard are regularly not working.

For the reverse proxy it doesn't matter as there are both http and https test. And using 0.0.0.0 didn't work here, as the client will be connecting via the public address, and then the tests will fail.

@ahus1
Copy link
Contributor Author
ahus1 commented Apr 3, 2025

@jonkoops - AFAIK you have been involved in the discussion about secure vs. non-secure contexts. Maybe you can help to assess this change.

@jonkoops
Copy link
Contributor
jonkoops commented Apr 4, 2025 •
edited
Loading

0.0.0.0 is not considered a secure context (see specification), so I agree with @ahus1's assessment that this is likely equivalent. We only need to be careful with tests that might do cross-origin requests (need to be on different domains), as those might result in a different result for setting cookies, but I see no such changes here at this time.

Note that 127.0.0.1 (previously proxy.kc.127.0.0.1.nip.io) is considered a secure context, but I am not quite sure if the context for the reverse proxy was ever used securely anywhere that impacts user-agents.

mhajas
mhajas approved these changes Apr 8, 2025
View reviewed changes
Copy link
Contributor
@mhajas mhajas left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@mhajas mhajas merged commit 9eb336a into keycloak:main Apr 8, 2025
75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhajas mhajas mhajas approved these changes

Assignees

@ahus1 ahus1

Labels
team/cloud-native team/core-clients team/core-iam
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Temporary failure in name resolution with nip.io
3 participants
@ahus1 @jonkoops @mhajas
0