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

Add log messages #334

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add log messages #334

wants to merge 3 commits into from

Conversation

adombeck
Copy link
Contributor

Adds some log messages, see commit messages for details.

To 1. allow the user/admin to figure out why authentication is denied,
and 2. log failed login attempts, which is security critical (security
logging failure is in the OWASP top 10, see
https://owasp.org/Top10/A09_2021-Security_Logging_and_Monitoring_Failures).
To have more complete logs of failed login attempts.
Print the IsAuthenticated result for debugging.
@adombeck adombeck requested a review from a team as a code owner January 24, 2025 14:42
@adombeck adombeck requested review from didrocks and removed request for didrocks January 24, 2025 14:42
@adombeck
Copy link
Contributor Author

adombeck commented Jan 24, 2025

@didrocks @3v1n0 I would like to include this in the release, so I would be appreciate a quick review from one of you :)

@@ -634,6 +634,7 @@ func (b *Broker) handleIsAuthenticated(ctx context.Context, session *session, au
}

if !b.userNameIsAllowed(authInfo.UserInfo.Name) {
log.Errorf(context.Background(), "User %q is not in the list of allowed users", authInfo.UserInfo.Name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the only one that I'm unsure about... As IIRC we didn't want to give much information (neither in log form) whether someone was or not managed by the broker.

But I feel that's not a too big issue, given that normally users don't have access to the journal... That said, I'd leave the decision to @didrocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I mentioned in the commit message, it's considered a security risk to not log failed login attempts:

To 1. allow the user/admin to figure out why authentication is denied,
and 2. log failed login attempts, which is security critical (security
logging failure is in the OWASP top 10, see
https://owasp.org/Top10/A09_2021-Security_Logging_and_Monitoring_Failures).

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

Successfully merging this pull request may close these issues.

2 participants