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

Triple audit logging fix (#1995) #1996

Merged
merged 1 commit into from
Aug 12, 2022
Merged

Triple audit logging fix (#1995) #1996

merged 1 commit into from
Aug 12, 2022

Conversation

jchipmunk
Copy link
Contributor

Revert some changes introduced by #1563 to correct work with log4j.

Signed-off-by: Andrey Pustovetov [email protected]

Description

[Describe what this change achieves]

  • Category: Bug fix
  • Why these changes are required?
    When I execute GET request to .opendistro_security index, I see three audit messages in the logs instead of one.

Issues Resolved

#1995

Testing

I manually checked the changes by doing GET request to the index and checking the logs.

Check List

  • New functionality includes testing
  • New functionality has been documented
  • [ X] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Revert some changes introduced by #1563 to correct work with log4j.

Signed-off-by: Andrey Pustovetov <[email protected]>
@jchipmunk jchipmunk requested a review from a team August 5, 2022 10:21
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading and rereading this code I'm not sure what was causing the triple logging which has been fixed with this PR, could you provide more details about what wasn't working before?

}
return true;
}

private boolean isLogLevelEnabled(Logger logger, Level level) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for removing these extraneous functions

@jchipmunk
Copy link
Contributor Author

jchipmunk commented Aug 8, 2022

Hi, @peternied

Reading and rereading this code I'm not sure what was causing the triple logging which has been fixed with this PR, could you provide more details about what wasn't working before?

The bug is the incorrect usage of switch-case construction in Java:

private void logAtLevel(Logger logger, Level level, String msg) {
    switch(level.toString()) {
        case "TRACE": logger.trace(msg);
        case "DEBUG": logger.debug(msg);
        case "INFO": logger.info(msg);
        case "WARN": logger.warn(msg);
        case "ERROR": logger.error(msg);
    }
}

This code does not contain break/return keywords to stop switching so the message with INFO logging level will be logged three times:

logger.info(msg);
logger.warn(msg);
logger.error(msg);

@peternied
Copy link
Member

@jchipmunk 🤣 Thank you for that gem, you have made my day.

@peternied
Copy link
Member

Looks like CI got stuck when a new OpenSearch 2.2.0 build was being created, re-running the failed CI jobs

@peternied peternied added the backport 2.x backport to 2.x branch label Aug 12, 2022
@peternied peternied merged commit 68f5624 into opensearch-project:main Aug 12, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 12, 2022
Revert some changes introduced by #1563 to correct work with log4j.

Signed-off-by: Andrey Pustovetov <[email protected]>
(cherry picked from commit 68f5624)
@jchipmunk
Copy link
Contributor Author

@peternied, thanks for reviewing the PR

Is it possible to backport the changes to the branch 1.3?

@peternied peternied added the backport 1.3 backport to 1.3 branch label Aug 12, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 12, 2022
Revert some changes introduced by #1563 to correct work with log4j.

Signed-off-by: Andrey Pustovetov <[email protected]>
(cherry picked from commit 68f5624)
@peternied
Copy link
Member

@jchipmunk I've created the backport #2026, we will continue the discussion over there.

peternied pushed a commit that referenced this pull request Aug 12, 2022
Revert some changes introduced by #1563 to correct work with log4j.

Signed-off-by: Andrey Pustovetov <[email protected]>
(cherry picked from commit 68f5624)

Co-authored-by: Andrey Pustovetov <[email protected]>
@jchipmunk jchipmunk deleted the triple_audit_logging branch September 15, 2022 07:03
stephen-crawford pushed a commit to stephen-crawford/security that referenced this pull request Nov 10, 2022
…t#1996)

Revert some changes introduced by opensearch-project#1563 to correct work with log4j.

Signed-off-by: Andrey Pustovetov <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023
…t#1996) (opensearch-project#2024)

Revert some changes introduced by opensearch-project#1563 to correct work with log4j.

Signed-off-by: Andrey Pustovetov <[email protected]>
(cherry picked from commit 68f5624)

Co-authored-by: Andrey Pustovetov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.3 backport to 1.3 branch backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants