You are viewing a plain text version of this content. The canonical link for it is here.

Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/12/31 22:02:21 UTC

[GitHub] [kafka] C0urante opened a new pull request #9806: KAFKA-10895: Attempt to prevent JAAS config from being overwritten for basic auth filter

C0urante opened a new pull request #9806:
URL: https://github.com/apache/kafka/pull/9806


   [Jira](https://issues.apache.org/jira/browse/KAFKA-10895)
   
   # Problem
   
   If a connector, converter, etc. invokes [Configuration::setConfiguration](https://docs.oracle.com/javase/8/docs/api/javax/security/auth/login/Configuration.html#setConfiguration-javax.security.auth.login.Configuration-), it will cause the Connect basic auth filter to use that JAAS config instead of the one configured at startup with the `-Djava.security.auth.login.config` JVM property. This can cause requests to the worker to succeed initially but start to be rejected after the JVM's global JAAS config is altered.
   
   # Solution
   
   Cache the JVM's global JAAS configuration as soon as possible (in this case, as soon as the `BasicAuthSecurityRestExtension` class is loaded), and use that for all future authentication. It's still possible that a different JAAS config than the one that the JVM was configured to use on startup will be captured at this point, but the chances of that should be very slim since it would require a plugin class to install a new global JAAS config as soon as it was loaded. Even if that does happen, this should at the very least guarantee consistent behavior on the worker--any request that succeeds once will always succeed, as opposed to the current situation where requests can succeed for a little bit but then start to get inexplicably rejected after a little while when a plugin class chooses to install a new global JAAS config.
   
   # Testing
   
   Existing tests for the JAAS basic auth filter are modified to work with the new internal logic. The `testEmptyCredentialsFile` test is corrected to actually operate on an empty credentials file (instead of a non-existent credentials file, which it currently operates on). A new test is added to ensure that, even if the global JAAS config is overwritten, the basic auth filter will use the first one it could find.
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] Verify documentation (including upgrade notes)
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] gharris1727 commented on pull request #9806: KAFKA-10895: Attempt to prevent JAAS config from being overwritten for basic auth filter

Posted by GitBox <gi...@apache.org>.

gharris1727 commented on pull request #9806:
URL: https://github.com/apache/kafka/pull/9806#issuecomment-756853770


   > Presumably, if some client application is using the JVM-global JAAS config, it's because it wants to pick up on mutations to that config. ...in the case of the basic auth REST extension, we don't really want to do that, and so we shouldn't rely on that global mutable state in the first place.
   
   This is a very interesting way to look at this coordination, and I think is very helpful. I see that I'm falling into the trap of assuming that _every_ static shared resource is a bug waiting to happen, because i've only ever seen bugs result from them. This is surely confirmation bias, and i'm sure there's legitimate use-cases for this coordination that I just haven't seen because they aren't broken 😃. There is an argument to be made that this is just an isolation leak, and that such code is relying on a bug, but there's clearly a debate to be had from both sides.
   
   None of this discussion has anything to do with this fix directly, and I think it's a step in the right direction regardless. LGTM!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] gharris1727 commented on pull request #9806: KAFKA-10895: Attempt to prevent JAAS config from being overwritten for basic auth filter

Posted by GitBox <gi...@apache.org>.

gharris1727 commented on pull request #9806:
URL: https://github.com/apache/kafka/pull/9806#issuecomment-756853770


   > Presumably, if some client application is using the JVM-global JAAS config, it's because it wants to pick up on mutations to that config. ...in the case of the basic auth REST extension, we don't really want to do that, and so we shouldn't rely on that global mutable state in the first place.
   
   This is a very interesting way to look at this coordination, and I think is very helpful. I see that I'm falling into the trap of assuming that _every_ static shared resource is a bug waiting to happen, because i've only ever seen bugs result from them. This is surely confirmation bias, and i'm sure there's legitimate use-cases for this coordination that I just haven't seen because they aren't broken 😃. There is an argument to be made that this is just an isolation leak, and that such code is relying on a bug, but there's clearly a debate to be had from both sides.
   
   None of this discussion has anything to do with this fix directly, and I think it's a step in the right direction regardless. LGTM!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] C0urante commented on pull request #9806: KAFKA-10895: Attempt to prevent JAAS config from being overwritten for basic auth filter

Posted by GitBox <gi...@apache.org>.

C0urante commented on pull request #9806:
URL: https://github.com/apache/kafka/pull/9806#issuecomment-756841881

@gharris1727 thanks for taking a look. It's an interesting question about isolating plugins in that way... I have to wonder if it's possible to do that in a way that doesn't break as much as it fixes. Shared global JVM state isn't necessarily a bad thing and disallowing cross-plugin communication by mutating that state might cause issues in other cases than this one. Presumably, if some client application is using the JVM-global JAAS config, it's because it _wants_ to pick up on mutations to that config. In practice this doesn't seem to happen as often, but ruling that out completely may not be safe.

We allow for dynamically-constructed JAAS configs in other places to avoid using/mutating the global JAAS config; I think that approach might be the optimal way to address problems like this, at least when JAAS is involved. This goes back to the point about wanting to pick up mutations to the JVM-global JAAS config; in the case of the basic auth REST extension, we don't really want to do that, and so we shouldn't rely on that global mutable state in the first place. Would probably require a KIP to add dynamic JAAS configs to the basic auth extension though, so this placeholder bug fix is hopefully sufficient for until then.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] kkonstantine merged pull request #9806: KAFKA-10895: Attempt to prevent JAAS config from being overwritten for basic auth filter

Posted by GitBox <gi...@apache.org>.

kkonstantine merged pull request #9806:
URL: https://github.com/apache/kafka/pull/9806


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] C0urante commented on pull request #9806: KAFKA-10895: Attempt to prevent JAAS config from being overwritten for basic auth filter

Posted by GitBox <gi...@apache.org>.

C0urante commented on pull request #9806:
URL: https://github.com/apache/kafka/pull/9806#issuecomment-758124317


   Thanks Konstantine, I've resolved the merge conflicts.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] C0urante commented on pull request #9806: KAFKA-10895: Attempt to prevent JAAS config from being overwritten for basic auth filter

Posted by GitBox <gi...@apache.org>.

C0urante commented on pull request #9806:
URL: https://github.com/apache/kafka/pull/9806#issuecomment-756841881

For queries about this service, please contact Infrastructure at:
users@infra.apache.org