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.

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

JavaLogFactory configuration should not override custom java LogManager configuration #710

Closed
jeremiahjstacey opened this issue Jun 17, 2022 Discussed in #709 · 7 comments · Fixed by #744
Closed

JavaLogFactory configuration should not override custom java LogManager configuration #710

jeremiahjstacey opened this issue Jun 17, 2022 Discussed in #709 · 7 comments · Fixed by #744

Comments

@jeremiahjstacey
Copy link
Collaborator

jeremiahjstacey commented Jun 17, 2022

Discussed in #709

Originally posted by chuckdumont June 17, 2022
Our application uses java.util.logging and handles logging configuration (setting handlers, log levels, etc.). When using esapi with JUL logging, the application's configured log levels are reset by esap. How can we configure esapi to use the java.util.logging APIs to log events, but not try to set it's own logging config?

Recommended Adjustment

If either supported configuration property for the LogManager class exists in the runtime, ESAPI will not apply configuration settings from the esapi-java-logging.properties file

Per the javadoc on the LogManager class, two existing properties may be used to centralize the log configuration:

In addition, the LogManager uses two optional system properties that allow more control over reading the initial configuration:

  • "java.util.logging.config.class"
  • "java.util.logging.config.file"

These two properties may be set via the Preferences API, or as command line property definitions to the "java" command, or as system property definitions passed to JNI_CreateJavaVM.

The ESAPI JavaLogFactory should not conflict with those settings, if they are defined.

JavaLogFactory.readLoggerConfiguration Additions

  /*package*/ static void readLoggerConfiguration(LogManager logManager) {
        if (System.getProperties().keySet().stream().anyMatch(propKey -> "java.util.logging.config.class".equals(propKey) || "java.util.logging.config.file".equals(propKey))) {
            //LogManager has external configuration.  Do not load ESAPI defaults.
            return;
        }
@jeremiahjstacey jeremiahjstacey changed the title Update JavaLogFactory LogManager Configuration to be Optional JavaLogFactory configuration should not override custom java LogManager configuration Jun 19, 2022
@jeremiahjstacey
Copy link
Collaborator Author

Alternative implementation to what is listed in the description.
Same effect, but perhaps more readable.

  /*package*/ static void readLoggerConfiguration(LogManager logManager) {
        if (System.getProperty("java.util.logging.config.class") != null || System.getProperty("java.util.logging.config.file") != null) {
            //LogManager has external configuration.  Do not load ESAPI defaults.
            return;
        }

@chuckdumont
Copy link

Please also support a ESAPI.properties file setting to configure this behavior as well. This will allow applications to that don't currently use the above mentioned system properties to consume esapi without having to change the way that the way they configure logging. Thanks.

@kwwall
Copy link
Contributor

kwwall commented Jun 20, 2022

@chuckdumont wrote:

Please also support a ESAPI.properties file setting to configure this behavior as well. This will allow applications to that don't currently use the above mentioned system properties to consume esapi without having to change the way that the way they configure logging.

Um, no. We're not going to do that. We don't ever put non-ESAPI properties into ESAPI.properties or read System properties from there (because those properties are not accessible via System.getProperty()), so the only way to do this would be to introduce a new ESAPI property along the line of the proposed ESAPI.configureLoggingproperty that you suggested. But there are several problems with this. To retain backward compatibility with ESAPI, this would have to have a default value of 'true' and be treated as true if absent. So, the first complication means that that there is an expectation that this would do something with all the ESAPI loggers, not just with JUL, which means more code changes than just the simple one that @jeremiahjstacey outlined. Secondly, it makes it more difficult to document. But thirdly, is what do we do if we find (say) ESAPI.configureLogging=true but (say) the System property java.util.logging.config.file is also set to something? That has ambiguous meaning so in such cases we would have to through a ConfigurationException to try to explain what happened. And anytime we throw exceptions (especially that close to startup, which is when a ConfigurationException is most likely to happen), they inevitably lead to numerous questions StackOverflow and emails to us, etc. So, no. We are NOT going to do that.

Chances are if clients of ESAPI for both ESAPI logging and also using JUL with their own configuration file, they've already dealt with this by copying their JUL config file to esapi-java-logging.properties or my making that filename a symbolic (or hard) link to their JUL config file that was originally suggested to you. If so, they don't have to do anything; it should continue working for them. If they haven't done this, they have doing something even more radical like you are doing and altering the ESAPI classes. In the former case, they shouldn't have to do anything to change. In your case, your options it do copy / hard link / sym link trick or set the System property java.util.logging.config.file to the path of your JUL config file. The latter can be done either via a '-D' flag at runtime or could be set programmatically. And please don't tell me that this "unnecessarily complicates deployment". If anything, your "copying the implementation of JavaLogFactory and removing the call to readLoggerConfiguration and pointing ESAPI.logger to our modified implementation" is what complicates deployment. By comparison, this is simple, not to mention a standard JavaEE approach that Java developers have been using for decades.

So, I'm sorry, but I don't think adding a new property is the best for the rest of the ESAPI community. Instead, you can change your code or do one of the other alternatives that we have suggested.

@chuckdumont
Copy link

I think you're overstating the complexities of ESAPI supporting two configuration options. All you'd need to do is define, and document, which option takes precedence if they are both set (I would think the one in ESAPI.properties). Furthermore, if you didn't want to support this new option for the other loggers, just call it ESAPI.configureJULLogging instead and only support it for JUL.

With regard to logging configuration changes, it's not quite as easy for us as you make it sound. As previously mentioned, the feature using ESAPI that I work on is an optional feature that is installed separately from the main application. I can easily add a property to ESAPI.properties because it's bundled in our jar. But making changes to the way the app that we run on configures logging would need to be done by a different development team in a different organization within our company. They will naturally be resistant to making such a change and will want to know why we have to use a library that can't play nice with logging.

And finally, you seem to be confusing deployment with development. For us, deployment consists of dropping our jar into a folder of the main application. Requiring that symlinks be set up in addition to that is most definitely a complication. Any work that we can do on the development side to avoid such complications during deployment is worth while because the development work needs to be done only once while the deployment tasks need to be repeated by each customer that installs our feature.

@jeremiahjstacey
Copy link
Collaborator Author

We appreciate the feedback and input @chuckdumont. You have helped to identify an improvement we can make to the library that will expand the usability in the future.

As @kwwall stated previously, we will not be providing an ESAPI project level property in this scope.

We fully appreciate your use case, and I will admit that in the current baseline the only option is overriding the JavaLogFactory.

After the updates from this task are applied, the options available will be (in recommended order)

  1. Configure java LogManager environment properties as defined by the LogManager API
  2. Overwrite the esapi-java-logging.properties file with the desired logging configurations
  3. Specifically set the system property for LogManager in the custom-code solution to prevent ESAPI JavaLogFactory from overriding project configuration
  4. Create a custom JavaLogFactory class in client project baseline and update the ESAPI.properties configuration to use that reference. (previously the only option)

@kwwall @xeno6696 please let me know if this order should be modified. I may ask for your assistance in getting that into the right project documentation location.

@kwwall
Copy link
Contributor

kwwall commented Jun 21, 2022

@jeremiahjstacey - I just thought of one possible other option that @chuckdumont might be able to use, but I've not tested it and only glanced at the code, so I think this will work, but I'm not 100% certain so maybe you can comment on it.

But what I was thinking is perhaps @chuckdumont could configure ESAPI logging to use SLF4J, via setting

    ESAPI.Logger=org.owasp.esapi.logging.slf4j.Slf4JLogFactory

in his ESAPI.properties file and then add slf4j-jdk14.jar to his application dependencies and then use his current JUL configuration through the SLF4J interfaces.

AFAICT (and this is where I need your confirmation, Jeremiah), ESAPI does not try to do any configuration for SLF4J because we are not sure what library someone would be using with SLF4J (e.g., Logback, Log4J 2, JUL, etc.) So I think that means that ESAPI will not interfere with overwriting any JUL configuration that is present if JUL is the log library one selects for implementation with SLF4J (which I think is used via slf4-jdk14.jar). And I am pretty sure that that doesn't mean he would need to change his application code to use JUL through SLF4J but could continue to use JUL direction. That is, I think straight JUL would work fine mixed this way. I think it's worth a try anyway.

@jeremiahjstacey - BTW, I still think we should address this issue as you discussed here if if using JUL via SLF4J works for @chuckdumont.

@jeremiahjstacey
Copy link
Collaborator Author

@kwwall, from what I presently understand I believe your assessment is correct.
If the ESAPI configuration is set up to delegate to SLF4J, and SLF4J is configured to use a Java-logging backend then the configurations from the JavaLogFactory will not be applied to the runtime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants