Don't allow inheriting session unless username matches about server

[thedevop]

Hi @snej, currently the authentication is called before inheritClientSession:

server/server.go

Lines 443 to 457 in 5966c7f

	if !s.hooks.OnConnectAuthenticate(cl, pk) { // [MQTT-3.1.4-2]
	err := s.SendConnack(cl, packets.ErrBadUsernameOrPassword, false, nil)
	if err != nil {
	return fmt.Errorf("invalid connection send ack: %w", err)
	}
	return packets.ErrBadUsernameOrPassword
	}
	atomic.AddInt64(&s.Info.ClientsConnected, 1)
	defer atomic.AddInt64(&s.Info.ClientsConnected, -1)
	s.hooks.OnSessionEstablish(cl, pk)
	sessionPresent := s.inheritClientSession(pk, cl)

Although with static username, it is reasonable to assume username should remain the same to inherit a clientID, but I have seen use-case where username is dynamic, like using JWT.

Given MQTT spec (section 5.4.2) leaves what should be checked (username, IP, etc) to the authentication mechanisms, perhaps implementing custom OnConnectAuthenticate is the better route if the requirement is to associate username with clientID?

[snej]

currently the authentication is called before inheritClientSession

Yes, but this issue isn't related to client authentication, rather to channel authorization.

For example, if I log in with my valid username and password, but use a session ID that matches your last session, then I will end up with all of your channel subscriptions, whether or not I was supposed to be able to access those channels.

perhaps implementing custom OnConnectAuthenticate is the better route if the requirement is to associate username with clientID?

That seems reasonable. The Auth Ledger hook should be updated to do this, then, since I assume that's what most developers will use.

The check in the hook would probably look something like this?

func (h *myAuthHook) OnConnectAuthenticate(cl *mochi.Client, pk packets.Packet) bool {
	existing, _ := h.server.Clients.Get(pk.Connect.ClientIdentifier)
	if existing != nil && !slices.Equal(existing.Properties.Username, cl.Properties.Username) {
		return false
	}
	...
}

This does have the problem that the error sent to the client will be ErrBadUsernameOrPassword, which isn't accurate; probably ErrClientIdentifierNotValid would be better. Is there any way to customize the error returned to the client?

[thedevop]

Yes, but this issue isn't related to client authentication, rather to channel authorization.

For example, if I log in with my valid username and password, but use a session ID that matches your last session, then I will end up with all of your channel subscriptions, whether or not I was supposed to be able to access those channels.

Just to ensure we're on the same page, session ID means the client ID the client used to connect? If so, then the use-case you described for the complete authentication should includes clientID/username/password. I assume that username/password should always be associated with a clientID (not only on inheriting a session)

MQTTv5 reason code

Value	Hex	Reason Code name	Description
133	0x85	Client Identifier not valid	The Client Identifier is a valid string but is not allowed by the Server.
135	0x87	Not authorized	The Client is not authorized to connect.

MQTTv3 connect return code

Value	Return Code Response	Description
2	0x02 Connection Refused, identifier rejected	The Client identifier is correct UTF-8 but not allowed by the Server
5	0x05 Connection Refused, not authorized	The Client is not authorized to connect

perhaps implementing custom OnConnectAuthenticate is the better route if the requirement is to associate username with clientID?

That seems reasonable. The Auth Ledger hook should be updated to do this, then, since I assume that's what most developers will use.

There are 2 general use-cases of mochi-mqtt:

1. embedded MQTT server, generally do not use auth ledger and already use custom authentications.
2. packaged solution, as mochi indicated previously, the auth ledger was meant to be a simple hook. However, more and more people are using this with different needs, perhaps it's time to revisit and redesign this?

The check in the hook would probably look something like this?

func (h *myAuthHook) OnConnectAuthenticate(cl *mochi.Client, pk packets.Packet) bool {
	existing, _ := h.server.Clients.Get(pk.Connect.ClientIdentifier)
	if existing != nil && !slices.Equal(existing.Properties.Username, cl.Properties.Username) {
		return false
	}
	...
}

This does have the problem that the error sent to the client will be ErrBadUsernameOrPassword, which isn't accurate; probably ErrClientIdentifierNotValid would be better. Is there any way to customize the error returned to the client?

Yes, currently OnConnectAuthenticate only returns a bool. We can either change the hardcoded return code to something generic like packets.ErrNotAuthorized:

server/server.go

Lines 443 to 450 in 5966c7f

	if !s.hooks.OnConnectAuthenticate(cl, pk) { // [MQTT-3.1.4-2]
	err := s.SendConnack(cl, packets.ErrBadUsernameOrPassword, false, nil)
	if err != nil {
	return fmt.Errorf("invalid connection send ack: %w", err)
	}
	return packets.ErrBadUsernameOrPassword
	}

or change the signature of OnConnectAuthenticate. Thoughts @mochi-co?

[werbenhu]

@snej Consider a scenario where I want all devices to log in using a single username and password, but each device is identified by a different ClientID. I think the association between username and clientID should be managed and allocated by the users themselves through hooks for better suitability.

[snej]

I'm fine having the Server struct not enforce matching usernames; I'll close this PR. But it would be good if the OnConnectAuthenticate hook could specify a different error code -- otherwise a client accidentally using someone else's client ID would be told its login is incorrect, which could be very confusing. I'll create a new PR for that.