-
Notifications
You must be signed in to change notification settings - Fork 29
NOISSUE - Disconnect client on Failed Authz Publish #101
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Arvindh <arvindh91@gmail.com>
pkg/session/stream.go
Outdated
| if err = authorize(ctx, pkt, h); err != nil { | ||
| if pub, ok := pkt.(*packets.PublishPacket); ok { | ||
| switch pub.Qos { | ||
| case 0: |
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.
In this approach, If authn and authz fails and if QoS is greater than 0 , then MQTT Client will keep on retrying.
Best approach is disconnect the MQTT Client if authn or authz fails
Signed-off-by: Arvindh <arvindh91@gmail.com>
pkg/session/stream.go
Outdated
| if _, ok := pkt.(*packets.PublishPacket); ok { | ||
| pkt = packets.NewControlPacket(packets.Disconnect).(*packets.DisconnectPacket) |
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.
First, can we go with just pkt = packets.NewControlPacket(packets.Disconnect)?
Second, is this OK according to the MQTT 3.1.1 spec? I mean, to send a disconnect right after publish and with no any other info (I guess this info is missing since, unlike 5.1, 3.1.1 does not have a disconnect reason code).
Finally, can we replace this with only:
if err = authorize(ctx, pkt, h); err != nil {
if _, ok := pkt.(*packets.PublishPacket); ok {
// In case of unauthorized access, send disconnect.
pkt = packets.NewControlPacket(packets.Disconnect)
}
}since we have
// Send to another.
if err := pkt.Write(w); err != nil {
errs <- wrap(ctx, err, dir)
return
}a few lines outside of this switch-case?
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.
// Send to another.
if err := pkt.Write(w); err != nil {
errs <- wrap(ctx, err, dir)
return
}
This code block write back to the server, not to the client whose authz is incorrect
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.
First, can we go with just pkt = packets.NewControlPacket(packets.Disconnect)?
Yes we can go like this
Second, is this OK according to the MQTT 3.1.1 spec?
Yes as per MQTT 3.1.1 server can disconnect at any time when authorization fails
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.
This is OK from the client's perspective, but what about the broker? The broker will never know there was a publish, and will also not gracefully handle client disconnection. For the broker, it would be like if the client's conn is left hanging (reset by peer, I guess), and - what's next? What happens if the client tries to reconnect?
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.
What about the broker?
This need to be checked, This need to verify and test it
I just followed the logic used in subscription authz failure
https://github.com/absmach/mgate/blob/main/pkg/session/stream.go#L81-L87
What happens if the client tries to reconnect?
We could not prevent the client trying again with same wrong password.
May be we block in ingress if any client tries with same password.
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.
@arvindh123 We need to address this and move on with this PR.
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.
@arvindh123 Any news on this?
Signed-off-by: Arvindh <arvindh91@gmail.com>
Pull request title should be
MF-XXX - descriptionorNOISSUE - descriptionwhere XXX is ID of issue that this PR relate to.Please review the CONTRIBUTING.md file for detailed contributing guidelines.
What does this do?
Which issue(s) does this PR fix/relate to?
Put here
Resolves #XXXto auto-close the issue that your PR fixes (if such)List any changes that modify/break current functionality
Have you included tests for your changes?
Did you document any new/modified functionality?
Notes