-
Notifications
You must be signed in to change notification settings - Fork 123
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
Log authentication and login errors in info #1657
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for following up on this @orenbm!
I left a couple of small comments. This will probably also require an issue/PR for the appliance tests, correct?
CHANGELOG.md
Outdated
- Print login and authentication error to the log in INFO level | ||
([cyberark/conjur#1377](https://github.com/cyberark/conjur/issues/1377)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The guidance for changelog entries was recently updated. So these should now end with punctuation and not put the link in parens. For example:
- Print login and authentication error to the log in INFO level | |
([cyberark/conjur#1377](https://github.com/cyberark/conjur/issues/1377)) | |
- Print login and authentication error to the log in INFO level. | |
[cyberark/conjur#1377](https://github.com/cyberark/conjur/issues/1377) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, didn't know that.
validate_account_exists | ||
validate_role_is_defined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like these caused some rspec failures that will require test updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another great enhancement :)
@@ -128,23 +128,24 @@ def k8s_inject_client_cert | |||
private | |||
|
|||
def handle_login_error(err) | |||
logger.debug("Login Error: #{err.inspect}") | |||
logger.info("Login Error: #{err.inspect}") | |||
err.backtrace.each do |line| | |||
logger.debug(line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we print info level only for the first issue in the trace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we print just the one line that customers will look for and can help them. This line prints the stack trace which is more for developers and thus is printed at DEBUG level.
@@ -128,23 +128,24 @@ def k8s_inject_client_cert | |||
private | |||
|
|||
def handle_login_error(err) | |||
logger.debug("Login Error: #{err.inspect}") | |||
logger.info("Login Error: #{err.inspect}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is confusing to have a login error
in INFO level. maybe it will be better to write login failed
as this is the information you want to give.
Unless it is really an error and not a valid part of the business logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not an error but customers (and SEs) already know to look for these messages so i don't want to change that.
raise Unauthorized | ||
else | ||
raise err | ||
end | ||
end | ||
|
||
def handle_authentication_error(err) | ||
logger.debug("Authentication Error: #{err.inspect}") | ||
logger.info("Authentication Error: #{err.inspect}") | ||
err.backtrace.each do |line| | ||
logger.debug(line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this is repeating itself, shouldn't we refactor this into a method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
validate_webservice_exists | ||
validate_user_is_defined | ||
validate_user_has_access | ||
validate_role_has_access |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if moving from user to role does not create confusion for the future devs
3dd3c80
to
8256c34
Compare
Yes @micahlee, I already created it and linked to it from the PR description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Please see you don't have conflicts with #1656
Once an authentication or login request fails, we write the reason to the log. We currently log it in DEBUG mode which is hard to filter out. As this info is necessary, and customers would like to get it easily, we would like to print it in INFO level, which is the default one.
Previously we didn't verify that the user exists in authentication requests with the default authenticator. However, verifying this can lead to enhanced log supportability as in case a user doesn't exist it is better to log a `RoleNotFound` message rather than an `InvalidCredentials` message. Note: this message will not be returned in the response and will only be written to the log.
e53788c
to
5fb231d
Compare
@micahlee can you re-review please? |
Code Climate has analyzed commit 5fb231d and detected 4 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 86.1% (0.0% change). View more on Code Climate. |
Connected to #1377, #1655
What does this PR do?
Once an authentication or login request fails, we write the reason
to the log. We currently log it in DEBUG mode which is hard to filter
out. As this info is necessary, and customers would like to get it easily,
we would like to print it in INFO level, which is the default one.
Checklists
Change log
Test coverage
We can't test this feature in Conjur as the log level in the tests are at DEBUG. However, this PR in the appliance will test it.
Documentation