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

StompSubProtocolHandler logs failed authentication with error stack trace #26026

Closed
icokk opened this issue Nov 3, 2020 · 6 comments
Closed
Assignees
Labels
in: messaging Issues in messaging modules (jms, messaging) status: backported An issue that has been backported to maintenance branches type: enhancement A general enhancement
Milestone

Comments

@icokk
Copy link

icokk commented Nov 3, 2020

StompSubProtocolHandler logs every exception, including failed authentication errors from spring-security-messaging, with error stack trace. This fills logs with garbage. It also allows simple DOS attacks by attempting unauthorized connection to websocket until the server disk is full.

Solution - allow authentication exceptions (for spring-security-messaging, it seems to be AccessDeniedException) to be returned to client as errors without logging anything.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 3, 2020
@rstoyanchev rstoyanchev self-assigned this Nov 5, 2020
@rstoyanchev rstoyanchev added in: messaging Issues in messaging modules (jms, messaging) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 5, 2020
@rstoyanchev rstoyanchev added this to the 5.3.1 milestone Nov 5, 2020
@rstoyanchev
Copy link
Contributor

We could refine the logging, I presume you mean here. Those are errors from sending the message through the inbound channel, which does not include actually handling them, so it's mostly the channel interceptors which likely include some logging themselves.

One option would be to log only a message without a stacktrace at ERROR but include the stacktrace at DEBUG. Would that work?

@icokk
Copy link
Author

icokk commented Nov 5, 2020

It would be an improvement, but the real solution would be to add a catch handler for failed authorization exceptions before the catch-all handler and inside only call handleError(session, ex, message); without logging anything.

For spring websocket security, the exception seems to be org.springframework.security.access.AccessDeniedException.

@icokk
Copy link
Author

icokk commented Nov 5, 2020

Oh, I forgot: if you don't want to add a refeernce to spring-security-core just to catch the exception, you could perheps detect it by full class name or add a configuration option for users to decide which errors to handle without logging.

@jhoeller
Copy link
Contributor

jhoeller commented Nov 5, 2020

We could only really make it configurable for custom exception classes, not refer to Spring Security exceptions there - not by reference and not by class name either.

@rstoyanchev a fine-tuning of our default logging, like the stacktrace only at debug level as you suggested, would be worth backporting to 5.2.x and 5.1.x as well.

@jhoeller jhoeller added the for: backport-to-5.2.x Marks an issue as a candidate for backport to 5.2.x label Nov 5, 2020
@spring-projects-issues spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.2.x Marks an issue as a candidate for backport to 5.2.x labels Nov 5, 2020
@jhoeller jhoeller added the for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x label Nov 5, 2020
@spring-projects-issues spring-projects-issues removed the for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x label Nov 5, 2020
rstoyanchev added a commit that referenced this issue Nov 5, 2020
Avoid a full stacktrace at ERROR level for a client message that could
not be sent to a MessageChannel.

See gh-26026
rstoyanchev added a commit that referenced this issue Nov 5, 2020
Avoid a full stacktrace at ERROR level for a client message that could
not be sent to a MessageChannel.

See gh-26026
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Nov 5, 2020

For 5.2 and 5.1, I've refined the logging along the lines I suggested earlier. For 5.3.1 I've taken it further by not logging at all at ERROR level for rejected CONNECT messages which are failures before the session is even established, likely authentication failures, and therefore handled a bit differently. In such cases, where errors can only come from interceptors, it could be reasonably expected that interceptors themselves should decide what is appropriate to log in case of a rejection.

Hopefully this works for you @icokk.

@icokk
Copy link
Author

icokk commented Nov 6, 2020

It will work, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: messaging Issues in messaging modules (jms, messaging) status: backported An issue that has been backported to maintenance branches type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants