Propose closing ESAPI issues 68, 198, and 231 as 'wontfix'
44 views
Skip to first unread message
Kevin W. Wall
Jun 13, 2019, 8:54:36 PM6/13/19
to esapi-pro...@owasp.org, esapi-pr...@owasp.org, Matt Seil, Jeremiah J. Stacey, Jeff Williams, Dave Wichers, Jim Manico
Please review these 3, somewhat related ESAPI GitHub issues:
I think that the recently added mechanism of setting the System property org.owasp.esapi.logSpecial.discard to true to suppress all log output from logSpecial() gets to the majority of the objections raised. It was only when Dave Wichers brought up the desire of suppressing the "noise" in the JUnit tests in a personal email that I was reminded that I had already implemented this for GitHub issue #385, which we closed via PR#451.
Fixing this architecturally once we get to ESAPI 3.x should be a goal, but I think it requires a substantial redesign of DefaultSecurityConfiguration and perhaps beyond that for ESAPI 2.x and that is not something that I really care to do. Right now, I am not convinced that it can be done in a manner that will not add security risks to ESAPI. (As I commented in some of these GitHub issues, I personally think that all the ESAPI reference implementation classes should be declared as 'final'.)
So because of that, I am thinking of adding the 'wontfix' label to each of these 3 GitHub issues, explaining why we won't fix them in ESAPI 2.x, and also making it clear how to suppress the logSpecial() output, which I think was really the main objection in all 3 of these issues amyway.
Fixing this architecturally once we get to ESAPI 3.x should be a goal, but I think it requires a substantial redesign of DefaultSecurityConfiguration and perhaps beyond that for ESAPI 2.x and that is not something that I really care to do. Right now, I am not convinced that it can be done in a manner that will not add security risks to ESAPI. (As I commented in some of these GitHub issues, I personally think that all the ESAPI reference implementation classes should be declared as 'final'.)
So because of that, I am thinking of adding the 'wontfix' label to each of these 3 GitHub issues, explaining why we won't fix them in ESAPI 2.x, and also making it clear how to suppress the logSpecial() output, which I think was really the main objection in all 3 of these issues amyway.
However, before doing so, I wanted to gather whatever feedback (objections, agreements, etc.) to this proposal of marking these 3 GitHub issues as 'wontfix'. So please let me hear from you ASAP. I'd like to hear back before the end of the month when we plan to do a formal ESAPI 2.2.0.0 release.
Thanks,
-kevin
--Blog: http://off-the-wall-security.blogspot.com/ | Twitter: @KevinWWall
NSA: All your crypto bit are belong to us.
NSA: All your crypto bit are belong to us.
Jeremiah Stacey
Jun 18, 2019, 5:41:57 AM6/18/19
to Kevin W. Wall, esapi-pro...@owasp.org, esapi-pr...@owasp.org, Matt Seil, Jeff Williams, Dave Wichers, Jim Manico
I am in full agreement with #231. If someone needs a different implementation then I think it's reasonable that they take responsibility for that implementation. When that happens, if someone designs a feasible solution that ends up being beneficial to the default implementation then it would be reasonable to consider an integration effort.
For 68/198 I'm on the fence. I understand that disabling logging
does quiet down the 'noise', but that noise is only noise until
you need it. There's no way to go back in time and replay an
event that happened that we have no logging for.
The only other thing I can think to suggest is a 'delayed logger'
implementation. Something that captures logs while no other
LogManager is available. When the actual log manager becomes
available the delayed logger should forward all captured log
messages to it, then be destroyed. I am pretty sure that is
something we can put together, but I'm not sure if it fully covers
the use cases we're concerned with.
I believe that the discard property fulfills the need to silence logSpecial output, and it is reasonable to move forward with that solution; however, I also think it may be worthwhile to investigate a 'delayed logger' (or other alternatives) as a future enhancement to reduce the potential for context loss during startup.
Thanks,
-J
Kevin W. Wall
Jun 18, 2019, 9:56:38 PM6/18/19
to Jeremiah Stacey, esapi-project-users, esapi-project-dev, Matt Seil, Jeff Williams, Dave Wichers, Jim Manico
My responses, inline, below.
-kevin
On Tue, Jun 18, 2019 at 5:41 AM Jeremiah Stacey
<jeremiah...@gmail.com> wrote:
>> On 6/13/2019 7:54 PM, Kevin W. Wall wrote:
>>
>> Please review these 3, somewhat related ESAPI GitHub issues:
>>
>> https://github.com/ESAPI/esapi-java-legacy/issues/68
>> https://github.com/ESAPI/esapi-java-legacy/issues/198
>> https://github.com/ESAPI/esapi-java-legacy/issues/231
>>
>> I think that the recently added mechanism of setting the System property
>> org.owasp.esapi.logSpecial.discard to true to suppress all log output from
>> logSpecial() gets to the majority of the objections raised. It was only when
>> Dave Wichers brought up the desire of suppressing the "noise" in the JUnit
>> tests in a personal email that I was reminded that I had already implemented
>> this for GitHub issue #385, which we closed via PR#451.
>>
>> Fixing this architecturally once we get to ESAPI 3.x should be a goal, but I
>> think it requires a substantial redesign of DefaultSecurityConfiguration and
>> perhaps beyond that for ESAPI 2.x and that is not something that I really
>> care to do. Right now, I am not convinced that it can be done in a manner
>> that will not add security risks to ESAPI. (As I commented in some of these
>> GitHub issues, I personally think that all the ESAPI reference implementation
>> classes should be declared as 'final'.)
>>
>> So because of that, I am thinking of adding the 'wontfix' label to each of
>> these 3 GitHub issues, explaining why we won't fix them in ESAPI 2.x, and
>> also making it clear how to suppress the logSpecial() output, which I think
>> was really the main objection in all 3 of these issues anyway.
>>
>> -kevin
>
> I am in full agreement with #231. If someone needs a different implementation
> then I think it's reasonable that they take responsibility for that
> implementation. When that happens, if someone designs a feasible solution
> that ends up being beneficial to the default implementation then it would be
> reasonable to consider an integration effort.
>
> For 68/198 I'm on the fence. I understand that disabling logging does quiet
> down the 'noise', but that noise is only noise until you need it. There's no
> way to go back in time and replay an event that happened that we have no
> logging for.
True, but 3 things here:
1) The logging that logSpecial() does is very limited and for the most part
really only debug logging anyway. It's basically used to aid ESAPI users
when they have placed their ESAPI.properties and validation.properties files
in some unusual place and ESAPI is having trouble locating them. But once you
have that working, unless you move those configuration files, the output of
logSpecial() really is "noise" that you can safely ignore.
2) Logging from logSpecial() is enabled by default so you have to deliberately
disable it to make it shut up.
3) Should one have the logSpecial() logging disabled and then some problem
arises, is it really that objectionable to restart your Java application
with the logSpecial() logging temporarily enabled again until you figure out
what went wrong?
> The only other thing I can think to suggest is a 'delayed logger'
> implementation. Something that captures logs while no other LogManager is
> available. When the actual log manager becomes available the delayed logger
> should forward all captured log messages to it, then be destroyed. I am
> pretty sure that is something we can put together, but I'm not sure if it
> fully covers the use cases we're concerned with.
Yes, some sort of delayed logger is a definite possibility and I would say that
it's even something that we should seriously think about when we eventually
redesign and tackle ESAPI 3.x. But for now, I would argue that it is not time
well spent, especially given that we are close to releasing 2.2.0.0.
Of course, it may be a lot easier in ESAPI 3.x where backward compatibility is
not required. (I figured we can name the base package org.owasp.esapi3 to more
or less guarantee that.) If backward compatibility is not required, perhaps the
prudent thing to do for ESAPI 3.x is to _only_ support SLF4J for ESAPI and then
people can use whatever it supports (logback, etc.). That would be a much
cleaner solution and we would not find ourselves in this catch-22 predicament
any longer. Back in ESAPI 1.x, supporting JUL and log4j likely made sense as it
did when ESAPI 2.0 was first released. But I'm not sure that is still a
requirement or even a good idea today.
> I believe that the discard property fulfills the need to silence logSpecial
> output, and it is reasonable to move forward with that solution; however, I
> also think it may be worthwhile to investigate a 'delayed logger' (or other
> alternatives) as a future enhancement to reduce the potential for context loss
> during startup.
Some other reasons that I think that these two GitHub issues (#68 and #198)
should be closed:
============
* Issue #68
============
The issue description states:
What steps will reproduce the problem?
1. call ESAPI.setLogFactory to set a LogFactory Programmatically.
2. call ESAPI.securityConfiguration()
What is the expected output? What do you see instead?
I would expect the output from the constructor of
DefaultSecurityConfiguration to log according to whatever configuration my
factory provides. Instead, it should log using the factory if it has
already been configured.
This is incorrect. There is no ESAPI.setLogFactory() method, nor even an
ESAPI.setLogger() method. There are 2 getter methods
public static Logger getLogger(Class clazz)
public static Logger getLogger(String moduleName)
but no setter method. (There may have been back when this original Google Code
issue was created, but it's a moot point now since there is not method in the
org.owasp.esapi.ESAPI class to set a LogFactory programatically. In fact, there
are no setter methods per se at all, so if where was a mechanism to alter
the logger programatically, and likely similar methods are long gone. (Because
most of ESAPI is implemented as singletons and the property values are all
therefore collected only upon the first call of getInstance(), such methods in
ESAPI would likely not operate as anticipated anyhow.)
And while there is a ESAPI.securityConfiguration() it returns the current
instance; it does not allow you to change a different instance with a different
behavior. (The 'ESAPI.override(SecurityConfiguration config)' method allows you
to do that, but the javadoc for that method has this not so prominent warning in
it:
This is meant to be used as a temporary means to alter the behavior of
the ESAPI and should *NEVER* be used in a production environment as it
will affect the behavior and configuration of the ESAPI *GLOBALLY*.
So, highly not recommended. It mostly was intended for a few JUNit tests that
were too cumbersome by extending
src/test/java/org/owasp/esapi/SecurityConfigurationWrapper.java
Furthermore, here are some additional reasons why I respectfully
disagree that these 2 GitHub issues
should remain open:
============
* Issue #198
============
This issue is titled
UnInitialized esapi logging assumes logging to
System.out/System.err - Make configurable/extensible
so it seems to be asking to make DefaultSecurityConfiguration extensible by
allowing logSpecial to be overridden, but let's take a deep look. Here is the
issue description:
Uninitialized logging calls logSpecial, which in turn calls System.out. This
behavior should be configurable with an option to turn console logging off.
So, I think you will agree that we've already made that possible via PR#451 and
setting org.owasp.esapi.logSpecial.discard to true.
That was the main request and it is already something that can be accomplished.
Furthermore, were it up to me, I would go back and declare the
DefaultSecurityConfiguration class as 'final'. In fact, I would do that with all
the ESAPI reference implementation classes. But doing so at this point would
potentially break people's code. (If I've learned one think through all the
years of developing various SDKs and APIs is that the best initial design is
always the one that is the MOST restrictive. You can always make things more
lenient, but you always risk breaking something when you move in the other
direction.)
So, unless someone speaks up as an ESAPI *user* and says "'issue #68' and/or
'issue #198' are really important to me and I can't simply live with suppressing
the output of logSpecial() altogether", I think I still am going to mark these
as 'wontfix' and close them. If they are really important, we can leave them
open and I can mark them for the next milestone release (presumably 2.3.0.0?).
So speak up now all you ESAPI users!
-kevin
On Tue, Jun 18, 2019 at 5:41 AM Jeremiah Stacey
<jeremiah...@gmail.com> wrote:
>> On 6/13/2019 7:54 PM, Kevin W. Wall wrote:
>>
>> Please review these 3, somewhat related ESAPI GitHub issues:
>>
>> https://github.com/ESAPI/esapi-java-legacy/issues/68
>> https://github.com/ESAPI/esapi-java-legacy/issues/198
>> https://github.com/ESAPI/esapi-java-legacy/issues/231
>>
>> I think that the recently added mechanism of setting the System property
>> org.owasp.esapi.logSpecial.discard to true to suppress all log output from
>> logSpecial() gets to the majority of the objections raised. It was only when
>> Dave Wichers brought up the desire of suppressing the "noise" in the JUnit
>> tests in a personal email that I was reminded that I had already implemented
>> this for GitHub issue #385, which we closed via PR#451.
>>
>> Fixing this architecturally once we get to ESAPI 3.x should be a goal, but I
>> think it requires a substantial redesign of DefaultSecurityConfiguration and
>> perhaps beyond that for ESAPI 2.x and that is not something that I really
>> care to do. Right now, I am not convinced that it can be done in a manner
>> that will not add security risks to ESAPI. (As I commented in some of these
>> GitHub issues, I personally think that all the ESAPI reference implementation
>> classes should be declared as 'final'.)
>>
>> So because of that, I am thinking of adding the 'wontfix' label to each of
>> these 3 GitHub issues, explaining why we won't fix them in ESAPI 2.x, and
>> also making it clear how to suppress the logSpecial() output, which I think
>>
>> However, before doing so, I wanted to gather whatever feedback (objections,
>> agreements, etc.) to this proposal of marking these 3 GitHub issues as
>> 'wontfix'. So please let me hear from you ASAP. I'd like to hear back before
>> the end of the month when we plan to do a formal ESAPI 2.2.0.0 release..
>> However, before doing so, I wanted to gather whatever feedback (objections,
>> agreements, etc.) to this proposal of marking these 3 GitHub issues as
>> 'wontfix'. So please let me hear from you ASAP. I'd like to hear back before
>>
>> -kevin
>
> I am in full agreement with #231. If someone needs a different implementation
> then I think it's reasonable that they take responsibility for that
> implementation. When that happens, if someone designs a feasible solution
> that ends up being beneficial to the default implementation then it would be
> reasonable to consider an integration effort.
>
> For 68/198 I'm on the fence. I understand that disabling logging does quiet
> down the 'noise', but that noise is only noise until you need it. There's no
> way to go back in time and replay an event that happened that we have no
> logging for.
1) The logging that logSpecial() does is very limited and for the most part
really only debug logging anyway. It's basically used to aid ESAPI users
when they have placed their ESAPI.properties and validation.properties files
in some unusual place and ESAPI is having trouble locating them. But once you
have that working, unless you move those configuration files, the output of
logSpecial() really is "noise" that you can safely ignore.
2) Logging from logSpecial() is enabled by default so you have to deliberately
disable it to make it shut up.
3) Should one have the logSpecial() logging disabled and then some problem
arises, is it really that objectionable to restart your Java application
with the logSpecial() logging temporarily enabled again until you figure out
what went wrong?
> The only other thing I can think to suggest is a 'delayed logger'
> implementation. Something that captures logs while no other LogManager is
> available. When the actual log manager becomes available the delayed logger
> should forward all captured log messages to it, then be destroyed. I am
> pretty sure that is something we can put together, but I'm not sure if it
> fully covers the use cases we're concerned with.
it's even something that we should seriously think about when we eventually
redesign and tackle ESAPI 3.x. But for now, I would argue that it is not time
well spent, especially given that we are close to releasing 2.2.0.0.
Of course, it may be a lot easier in ESAPI 3.x where backward compatibility is
not required. (I figured we can name the base package org.owasp.esapi3 to more
or less guarantee that.) If backward compatibility is not required, perhaps the
prudent thing to do for ESAPI 3.x is to _only_ support SLF4J for ESAPI and then
people can use whatever it supports (logback, etc.). That would be a much
cleaner solution and we would not find ourselves in this catch-22 predicament
any longer. Back in ESAPI 1.x, supporting JUL and log4j likely made sense as it
did when ESAPI 2.0 was first released. But I'm not sure that is still a
requirement or even a good idea today.
> I believe that the discard property fulfills the need to silence logSpecial
> output, and it is reasonable to move forward with that solution; however, I
> also think it may be worthwhile to investigate a 'delayed logger' (or other
> alternatives) as a future enhancement to reduce the potential for context loss
> during startup.
should be closed:
============
* Issue #68
============
The issue description states:
What steps will reproduce the problem?
1. call ESAPI.setLogFactory to set a LogFactory Programmatically.
2. call ESAPI.securityConfiguration()
What is the expected output? What do you see instead?
I would expect the output from the constructor of
DefaultSecurityConfiguration to log according to whatever configuration my
factory provides. Instead, it should log using the factory if it has
already been configured.
This is incorrect. There is no ESAPI.setLogFactory() method, nor even an
ESAPI.setLogger() method. There are 2 getter methods
public static Logger getLogger(Class clazz)
public static Logger getLogger(String moduleName)
but no setter method. (There may have been back when this original Google Code
issue was created, but it's a moot point now since there is not method in the
org.owasp.esapi.ESAPI class to set a LogFactory programatically. In fact, there
are no setter methods per se at all, so if where was a mechanism to alter
the logger programatically, and likely similar methods are long gone. (Because
most of ESAPI is implemented as singletons and the property values are all
therefore collected only upon the first call of getInstance(), such methods in
ESAPI would likely not operate as anticipated anyhow.)
And while there is a ESAPI.securityConfiguration() it returns the current
instance; it does not allow you to change a different instance with a different
behavior. (The 'ESAPI.override(SecurityConfiguration config)' method allows you
to do that, but the javadoc for that method has this not so prominent warning in
it:
This is meant to be used as a temporary means to alter the behavior of
the ESAPI and should *NEVER* be used in a production environment as it
will affect the behavior and configuration of the ESAPI *GLOBALLY*.
So, highly not recommended. It mostly was intended for a few JUNit tests that
were too cumbersome by extending
src/test/java/org/owasp/esapi/SecurityConfigurationWrapper.java
Furthermore, here are some additional reasons why I respectfully
disagree that these 2 GitHub issues
should remain open:
============
* Issue #198
============
This issue is titled
UnInitialized esapi logging assumes logging to
System.out/System.err - Make configurable/extensible
so it seems to be asking to make DefaultSecurityConfiguration extensible by
allowing logSpecial to be overridden, but let's take a deep look. Here is the
issue description:
Uninitialized logging calls logSpecial, which in turn calls System.out. This
behavior should be configurable with an option to turn console logging off.
So, I think you will agree that we've already made that possible via PR#451 and
setting org.owasp.esapi.logSpecial.discard to true.
That was the main request and it is already something that can be accomplished.
Furthermore, were it up to me, I would go back and declare the
DefaultSecurityConfiguration class as 'final'. In fact, I would do that with all
the ESAPI reference implementation classes. But doing so at this point would
potentially break people's code. (If I've learned one think through all the
years of developing various SDKs and APIs is that the best initial design is
always the one that is the MOST restrictive. You can always make things more
lenient, but you always risk breaking something when you move in the other
direction.)
So, unless someone speaks up as an ESAPI *user* and says "'issue #68' and/or
'issue #198' are really important to me and I can't simply live with suppressing
the output of logSpecial() altogether", I think I still am going to mark these
as 'wontfix' and close them. If they are really important, we can leave them
open and I can mark them for the next milestone release (presumably 2.3.0.0?).
So speak up now all you ESAPI users!
Reply all
Reply to author
Forward
0 new messages