Repository navigation
fix: cutting down on the memory footprint for import - #41196
Conversation
There was a problem hiding this comment.
@shawkins Thanks for the PR, nice work! Just went briefly over the code and put one comment.
It'd be also good to have some results on the improved memory footprint (or at least verify it really helped :)). Would you have a chance to try it with some profiler, or do you want me to test it?
There was a problem hiding this comment.
Byte limit is one thing, but maybe adding a recursion barrier for the replaceProperties() might also be good. Setting a limit on how far the recursion can go. Just to be sure. WDYT?
The second call for replaceProperties catches the StackOverflow exception, so at least add the check there if we don't provide the byte limit?
Do we have some possible estimation on how big byte limit would be reasonable ?
There was a problem hiding this comment.
but maybe adding a recursion barrier for the replaceProperties() might also be good. Setting a limit on how far the recursion can go. Just to be sure. WDYT?
It is better to have a depth rather than just catch the overflow yes, but the risk to the overall vm isn't that bad since it will be limited to a specific stack.
The risk from not supporting a byte limit is that ${ naturally appearing unclosed in the doc will cause the remaining bytes to be read into memory.
If we don't think this is likely to occur, then it may not be worth worrying about as our logic in general isn't protecting against memory attacks in realm files / uploads.
Do we have some possible estimation on how big byte limit would be reasonable ?
In theory it shouldn't be that large. Most env keys won't be that large. However you can certainly create something with multiple large keys and a large default value that takes up quite a few characters.
|
Moved to draft to address the close method. Also it would be better to use a Reader, rather than an InputStream, but that further complicates the implementation. |
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.MultipleTabsLoginTest#loginActionWithoutExecutionKeycloak CI - Forms IT (firefox) |
fcb412e to
fd5b85b
Compare
closes: keycloak#40875 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
There was a problem hiding this comment.
@shawkins Great job! LGTM
We should have a good test coverage for the import and possible replacements. Moreover, I did some experiments to verify the functionality - everything seems fine.
I see that the same StringPropertyReplacer is used in various projects, so it creates some sort of convention for the property replacement in strings.
Projects:
- JBoss Narayana JTA
- Infinispan (cc: @ryanemerson @pruivo)
- WildFly Elytron
It might be beneficial for these projects (and us) to also have unified approach as we could also get more eyes on this and get more reviews.
@shawkins Just a consideration. WDYT?
I'd like to see also approval from the @keycloak/core team.
+1. It seems Infinispan would benefit from these changes \cc @tristantarrant |
It's certianly possible to shoot for a common utility, but there are a few additional considerations:
|
Infinispan only uses this when parsing configuration files which generally tend to be fairly small (<10KB), so the benefits would be quite low. |
|
Thanks for all these comments. So we know Keycloak will benefit from this, but other projects might not, so it might not worth the effort to try to include it to these projects.
@shawkins Do we? AFAIK we only support property replacement based on the used property resolver as it's usually ENV vars, possibly system props, or some simple custom ones, right?. I would not support inlining additional JSON in a JSON file as it might introduce some other issues as we could provide users/customer more freedom in this area and not sure if we(maintanance) or they need it so much. If they need to do some more advanced things, they can do it on their own. Or have you encountered some request for such a feature? Or can you clarify it a little bit? Thanks! |
|
Can we get someone from @keycloak/core to review it? (or @keycloak/sre when you might have some experience in this matter?) Thanks! |
The previous/current logic happens before any structural parsing - it doesn't matter what the String content is - xml, json, yaml, etc. it just looks for ${...}. So for example in a json file you could have: And in an environment variable set foo= This is powerful and users may not be aware that it can be done. If we don't want to allow this, then I will create a follow up issue to do string replacement after the json parsing. This would probably look something like the rules for openshift templates: https://docs.redhat.com/en/documentation/openshift_container_platform/4.9/html/images/using-templates - it additionally has special handling for ${{...}} replacements. |
Ahh, that's true.
I'm not very comfortable supporting this as it raises a risk of unintentional changes, as possible injection attacks can be done - as the realm setting is dependent on the environment (machine). I know in order to execute that kind of attack, the attacker needs to have access to the machine itself, so some more harmful stuff can be done, but it'd be good to lower the number of possibilities :)
But I believe there are some folks who already use this. I'm on top of the fence right now - allow or not to allow, that's the que... We should consider the effort behind the analysis and needed work for this. So it'd need some analysis on how attacker can leverage that functionality, how harmful it could get, decide whether it worths the effort to limit devs with possible tiny benefits,... Maybe it'd be good to limit it by default, but having the possibility to "unblock" the limitation and do it on your own risk. But still, not sure if it worths the effort and bring some more benefits (at least it could strengthen our secure by default principles). @shawkins What's your opinion on this? |
My guess is that it's an undocumented / unintended consequence of the original implementation, but it is entirely possible that someone figured out that it was possible and did something clever like have composable realm imports - where common elements could be shared via environment variables. Supporting such a scheme post parsing is simply not possible because the original file will not be structurally valid, so if we allow that style of replacement we'll have to keep around this existing code path in some form. Moved to a discussion: #41611 |
Changes the property replacement logic to support streaming. The corner case is an unclosed ${ - there is a TODO to address that with a limit on how large the key contents can be.
After reviewing more of the logic this is the only simple fix we can do. Any other changes to the import logic will require two passes over the stream - which gets messy / expensive for multiple realms in a file, or from doing the import over the admin api.
closes: #40875