Lugaresi / codesys2-mqtt-library

Backport of http://codesys-mqtt-library.sourceforge.net/
7 stars 8 forks source link

password limit #8

Open GVRHUB opened 1 month ago

GVRHUB commented 1 month ago

Hello, Is it possible to remove the limit on the password length? Best regard Glenn

huwylphi commented 1 month ago

From the spec, the password length should be limited to 65535 bytes. I can check the source code how this limit is handled and let you know.

huwylphi commented 1 month ago

It seems we currently defined a limit of 80 bytes for both user and password lengths. If you need this limit to be higher, I can try to see what I can do, but most probably not before next week.

GVRHUB commented 1 month ago

Can you increase the limit to 1000 bytes? Thank you in advance.

chillymattster commented 1 month ago

Hi, I see a potential problem here when the limit is generally increased to 1000 bytes. Regardless of how many of the 1000 bytes are actually used or needed, the compiler will always assign and reserve 1000 bytes statically. To my knowledge there is no dynamic memory allocation involved because dynamic memory allocation is risky in terms of deterministic behaviour. This means by just increasing the string length to 1000 bytes, there will almost 2kB of memory continously be blocked or used. In my opinion this seems to be a lot especially as Codesys 2 PLCs are not known for having large amounts of memory available. Such large username and password, especially as the lib doesn't support transport encryption, seem to be a very special and rare case. Therefore I doubt that increasing the two variables to 1000 bytes is a good idea.

Solution proposal: You can download and import the EXP-file in your Codesys 2 project and change the definition of the two variables according to your needs.

If I'm missing something or missunderstood the request, I'm happy to learn more about it 😄

huwylphi commented 1 month ago

Hi, I agree with @chillymattster that generally allocating such amount of memory might cause some issue for some PLC and since we provide the mqtt.exp export file, his proposition should work for such use case described by @GVRHUB.

However, I might have an other proposition that would allow the library to support up to 65535 password length (as defined by the MQTT 3.1.1 CONNECT payload Password field specifications 3.1.3.5):

chillymattster commented 1 month ago

Hi, Before spending time to implement a solution, I still would like to understand the use case of @GVRHUB for such a large username / password in combination with missing transport encryption. In my opinion there is no reasonable increase from a security perspective as unencrypted credentials are insecure by design anyway. So this must not be used for a WAN-connection at all and a basic level of security must be created by network separation and e.g. use of VLANs or VPN to prevent simple packet sniffing and credential disclosure.

However, from my point of view your idea, @huwylphi, is a possible solution on implementation level. I would like to add two points to your list:

huwylphi commented 1 month ago

Hi, thanks @chillymattster for these very good remarks!
So let's await on @GVRHUB to add some detail about his use case and especially the reason why he would need to setup such a long password?
If we will let the username/password length limited to 80 bytes, then we could document it in the readme.md file.