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

Enumerate Login and Authenticator Error Message Sources #1464

Closed
micahlee opened this issue Mar 31, 2020 · 15 comments
Closed

Enumerate Login and Authenticator Error Message Sources #1464

micahlee opened this issue Mar 31, 2020 · 15 comments
Assignees
Milestone

Comments

@micahlee
Copy link
Contributor

In order to make high-level authenticator failures more visible (#1377), we first need to enumerate and review the possible error messages though would be logged from the existing authenticators.

Once this list is available we can then do a security review of these messages to asses the risk of elevating this from DEBUG log level to WARN.

@micahlee
Copy link
Contributor Author

micahlee commented May 11, 2020

@orenbm, I made a first pass at this here:

Authentication_Error_Logging.pdf
(Source Diagram)

It doesn't necessarily give the reason some of these would occur, but should cover most of what would appear in the Conjur log if we changed the log level for the first error line (not the entire stack trace).

I would love to get your initial take and thoughts when you have chance!

CC: @andytinkham @shaharglazner, particularly regarding if there are classes of information you would like to see in this? Thanks!

@orenbm
Copy link
Member

orenbm commented May 12, 2020

I think that our log messages are not the problem, as we can look in the errors file and the debug logs file and see all the messages that are printed.

The problem is with log messages that print 3rd party errors, for example, this one.

What I'm afraid of is that even if we go over our current logs and verify nothing sensitive is written to the logs by default, a future developer will add some log (especially log a 3rd party error) and will add some sensitive data.

@micahlee
Copy link
Contributor Author

@orenbm, yes I agree with you. Andy requested that we try to better capture what kinds of errors are generated, and how they relate to the authenticator behavior. Which is what I was attempting to document with this.

It would be helpful to better understand what we consider confidential and a risk to non-repudiation in the context of access to the Conjur server and its logs. At the moment, I haven't seen evidence that increasing the log level for these messages (and the classes of information they contain) increases this risk.

Two observations regarding this:

  1. If you have access to the Conjur server to view the logs, I haven't seen an example where you also don't have access to increase the log level of Conjur to debug to view the information anyway.

  2. We already capture this top line of information in the audit logs, which for the DAP appliance also live in a file on disk that can be viewed with access to the Conjur server.

@orenbm
Copy link
Member

orenbm commented May 12, 2020

I also don't think there's a real concern here

@andytinkham
Copy link
Contributor

@micahlee - this map is very helpful. Thank you! While it seems there could be some leakage here - for example, trying different roles in authentication until you don't get an Err:RoleNotFound would allow disclosing the roles, I don't think any of these leak sensitive information. I also agree with your 2 observations - seems like there isn't any security issue with changing the log level to warn. Thank you again for making the message map!

@micahlee
Copy link
Contributor Author

Thanks for reviewing, @andytinkham!

@orenbm
Copy link
Member

orenbm commented May 27, 2020

@micahlee so are we good to go with the change from debug to warn?

Also - why use warn and not info?

here's the definition of Datadog:

- Info: useful information about normal application operations such as services starting or stopping.
- Warn: operations that an application can easily recover from but should be addressed soon. This could include using an out-of-date gem or retrying an operation.

the log message that you want to print states that the authentication had an error but the operation itself succeeded. The application (conjur) didn't fail on an operation (authentication). It finished the operation successfully by not authorizing the request.

Furthermore, the default log level is set to info for both production and appliance so the message will printed by default also if we log it in info.

wdyt?

@micahlee
Copy link
Contributor Author

so are we good to go with the change from debug to warn?

That is my understanding.

why use warn and not info?

I think there are arguments to go either way, but I think the Datadog definition would still indicate this should be a warning, because there was a failure preventing successful authentication. It's not an an error that causes the application (Conjur) to fail, but it does need to be addressed (e.g. correcting authenticator configuration, user policy, or submitted credentials). This doesn't strike me as similar information to "services starting and stopping."

@orenbm
Copy link
Member

orenbm commented May 27, 2020

then in that case why not use Error:

Error: errors that cause an operation to fail (e.g., missing data or files) but not the application. The issue should be resolved soon, but the application can log the exception and continue running.

@orenbm
Copy link
Member

orenbm commented May 27, 2020

We should define better the term "cause an operation to fail". Is the operation the authentication of the client or is the operation the authentication process by the server? the former failed whereas the latter succeeded.

@orenbm
Copy link
Member

orenbm commented May 27, 2020

@andytinkham do you have any stand on this?

@micahlee
Copy link
Contributor Author

then in that case why not use Error

I think there is a case that can be made for that. My opinion is that warn is most appropriate, but it's not a super strong opinion.

@orenbm
Copy link
Member

orenbm commented May 27, 2020

in this case i can go with anything other than debug as the main thing is that it will be logged by default. We will better define the types in this design

@orenbm
Copy link
Member

orenbm commented May 31, 2020

@micahlee what's the next step once this issue is closed? Does someone have an action item?

@micahlee
Copy link
Contributor Author

micahlee commented Jun 1, 2020

@orenbm, this has never been prioritized work, but led by developers out of last year's hackathon. Closing this issue unblocks the existing issue here: #1377, but it's still up to someone picking it back up on their own to proceed with it.

The larger effort on adjusting log levels may supersede this as well (although this answer in particular helps inform that project).

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

No branches or pull requests

3 participants