-
Notifications
You must be signed in to change notification settings - Fork 108
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
Enhance configuration of Keycloak valid redirects list in the canned deployment #3503
Merged
webbnh
merged 5 commits into
distributed-system-analysis:main
from
webbnh:keycloak_redirects
Aug 4, 2023
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
c67bd5e
Pick some lint
webbnh 91456c9
Improve Keycloak valid redirects list
webbnh beeb9f0
Enhance the canned server's cert and Keycloak configuration
webbnh 376ff63
IPv6 fixup
webbnh b567f88
Add IPv6 'localhost' (::1) to the Keycloak valid redirects and the ce…
webbnh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dang: so
"localhost 10.0.0.104 <IPv6> dbutenho-thinkpadp1gen4i.bos.csb dbutenho-thinkpadp1gen4i.bos.csb dbutenho-thinkpadp1gen4i.bos.csb"
(that's my local router IPv4 since I'm not on VPN, and the csb FQDNs aren't routable, so the IPv6 is the only one I actually ought to avoid revealing on GitHub 😆 )But, seriously, using
hostname -f
when you're usinghostname -A
seems pointless as the former is "the" FQDN while the latter is all FQDNs and therefore will always be a superset. (And, since I don't think any of our systems have separate FQDNs across NICs, probably still always a duplicate.)Also, I'm not sure the redirect will take an undelimited IPv6 address, so your
https://<IPv6>:8443
probably won't work and might generate an error in Keycloak. I suspect it'd need to behttps://[<IPv6>]:8443
... all of which makes this IP stuff a lot more complicated than it might seem at first.I just think you're potentially asking for trouble here, and I'm not sure your result is really going to help anyone anyway since we're not using this in a real deployment anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree...but, I'm pretty sure that I one point I found myself in a situation where the output of
-f
was not in the list from-A
, so I included it out of an abundance of caution. (I assume that I was inside a container at the time.... 🦐)However, if your goal is to avoid duplication, I think you're out of luck -- I just got this:
🤣
intlab-002
has two NICs, but the second one doesn't seem to have a FQDN (i.e., the reverse-lookup on its IPv4 address fails), buthostname -A
there reports a duplication of then002
name (in addition to theintlab-002
name), too.I may indeed be asking for trouble, since I don't seem to have a system with IPv6 configured that I can test on. However, the result helps me -- running with the browser on my laptop and the canned server on my VM in the IntLab, the Keycloak redirect has to be something other than
localhost
. So, the question is, how to get that "something", and I was hoping thathostname
could provide the answer...but there is no good way to choose among (or even recognize) what it offers. 🙁I just tried a manual test...and it did! 🥴
So, I guess it just does a dumb-ish pattern match on the backside.
But, you're absolutely right, the IPv6 addresses should be wrapped in square-brackets. 😝
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, they are now wrapped. Assuming the tests pass, I don't suppose you'd be willing to try the script on your machine? (My machines don't speak IPv6 outside of containers, and my attempt to run
jenkins/runlocal
usingjenkins/run
built the RPM but then couldn't find it to build the container.... 🍤)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait; so where did it get a "hostname" like that? Even inside a container, I'm seeing
host.containers.internal
-- which is stupid, but arguably less stupid than yours. 🤔Interesting. I know the BOS lab used to have a high-speed internal subnet; and presumably the "second" proxy NIC would be on the internal net, which I don't think had DNS. RDU2 may well do something similar.
I really don't understand the logic of the CSB configuration. They have DHCP addresses, but apparently no DDNS names, and
hostname
generates only non-routable blather. Then again, VMs and containers generally aren't any better.(Then again, on the staging VM,
hostname -f
gives "rhel9-pbenchstaging" whilehostname -A
gives the actual routable DDNS DHCP FQDN, so that's something.)Leaving the obvious question of which form, if either, would match on a redirect.
I pulled your branch and did a
runlocal
. I verified that the Keycloak has the (delimited) IPv6 address. (Actually, it's got both the link-local address and the routable address ... and while my first thought was that the link-local was silly as it's not routable off the local subnet, well, my laptop curiously enough is on its own local subnet...) Anyway, I can open a web page on either delimited IPv6 address copied from the redirects list, and the page loads and I can log in!And, interestingly, while the CSB
hostname
isn't actually routable, I'd never had occasion to notice before that it actually works as a "localhost" alias on my laptop, sohttps://dbutenho-thinkpadp1gen4i.bos.csb:8443/dashboard/
loads (and logs in) just fine!The upshot here is that every one of the redirect URLs you're programming on my laptop actually work to launch the dashboard and to log in: even the ridiculously silly ones. 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addendum: on a whim, I also tried the local dashboard dev-mode to exercise the
http://localhost:3000
redirect, although there was no reason to think it wouldn't still work. It does... but, feeling in an IPv6-ish mood, I tried connecting to IPv6 localhost[::]
, and that's not mapped in your cert (or probably in the allowed redirects), so it didn't work. Hmm. But the link-local IPv6 (PBENCH_SERVER=https://\[fd10:22:16:1::10b9\]:8443 npm run dev
) worked just fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No clue...I'm just reporting what I'm seeing. 🙃
😆
I expect that you're correct.
Yeah, and it's not just CSB. My home network has similarly awkward hostnames (which are not, generally, as far as I can tell, routable or even usable except in rare circumstances).
Hah! You found it!! That's a case of the return from
-f
not being in the list (of one...) returned by-A
....Well, the redirect is provided by the client (to wit, the Dashboard) and checked by the OIDC server and then invoked. So, if the provided value doesn't match the valid-redirect list, it doesn't really matter whether it is a valid URL format. 🙃 And, if it does match the list, but isn't a valid URL, then that's a problem for the client...so, I'm not sure that Keycloak has any motivation for enforcing anything other than fairly simple pattern matching.
Yay! 🎉 Thanks for checking that!!
I'm glad it's good for something.... 😉
Hurrah! (Thanks again for checking!!)
OK...if you're willing to test it again, I'll see if I can close that gap.... 🤔 But see my comment below.