Potential race condition when using Ledger.Update

[sgebbie]

While reviewing the code for Ledger.AuthOK in order to understand how to configure authentication, a visual inspection of the locking of Ledger.Update stood out as a potential race condition:

• https://github.com/mochi-mqtt/server/blob/main/hooks/auth/ledger.go#L138

In particular,

// Update updates the internal values of the ledger.                                                                    
func (l *Ledger) Update(ln *Ledger) {                                                                                   
    l.Lock()                                                                                                            
    defer l.Unlock()                                                                                                    
    l.Auth = ln.Auth                                                                                                    
    l.ACL = ln.ACL                                                                                                      
}

takes care to lock and release the mutex. However, neither Ledger.AuthOK nor Ledger.ACLOk participate in the locking.

While I have not reproduced a bug, reviewing the code it would seem that a race condition on reading l.Auth or l.ACL might occur, if the ledge is updated concurrently.

Additionally, related but separately, l.Users is not carried over by l.Update.

Potential Remediation

1. Extend l.Update to additionally perform l.Users = ln.Users
2. Protect the access to the authentication configuration:

• Either: add calls to l.Lock(); defer l.Unlock() to l.AuthOK() and l.ACLOk(),
• Or: use an atomic.Value and follow the "configuration update pattern"

Additional considerations:

1. given that l.AuthOK() and l.ACLOk might be contended under load, it might be worth using an sync.RWMutex and then only holding reader locks in the authentication check routines.

Coordination

If someone, who is more familiar with the code base, can confirm that the above problem should be addressed, I can prepare a PR to submit for review.

Thanks,
Stewart.

[werbenhu]

The scenarios in which l.Update() is called are likely not very frequent, and using lock would waste a lot of performance. I think using atomic.Value is a better choice. @sgebbie , it would be great if you could submit a PR.