Created attachment 1133 [details] server erro upon saving IPSec settings with secret When Ipsec definition is updated with secret containing some of the characters below, upon save IPFire reports "Internal server error". The definition for which the secret was saved then gest randomly mixed up settings some values are lost, most values shown in different fields. Settings in Advanced sections are mostly lost, option fields and values mixed up too. Different secret must be used and whole IPSec settings manually restored I was able to replicate this, using e.g. follwoing secret "/?,wH,tIo^N#XP^U" (withouyt quotation marks) and then again with just "/?," characters used for secret.
Created attachment 1134 [details] setings lost and miced up in advanced section of IPSec
Hello Jan, yes, it is indeed not possible to have comma in the PSK since it is being stored in a CSV-style format which does not handle this well. Please don't use a comma as a workaround for the moment. I will leave this ticket open and hopefully will soon have some time for a fix. There is currently a large pile of tickets that require our attention first.
(In reply to Michael Tremer from comment #2) Hallo Michael, yes, this is a minor issue and I worked around it using different secret. I was using some random passwords from a password generator and when tried first one, I actually got message that the sevcret contained some charactars that are not allowed... so tried the second one that has crashed it. Since this was doen as last update to IPSEc connection I was setting up where started with a simple secret initially, it was a bit of surprise. Just an inconvenience to restore everything back I guess, no major problem. Thanks for looking into this, I appreciate your prompt verification of this. Cheers, Jan
I have updated the IPSec wiki page to clarify that a comma and a single quotation mark are invalid characters for the PSK. The vpnmain.cgi code already flags up a single quotation mark as an invalid character. I will pick this bug up to update the cgi code to also flag up a comma as invalid.
If someone has time for this, we could simply encode the PSK in base64. That will allow any kind of characters without taking care about the character encoding and any special characters after the base64 encode. To be backwards-compatible, we can simply use the original value if it cannot be base64 decoded.
I am willing to have a look at base64 encoding but that would take me longer than my original idea as I would need to better understand the vpnmain.cgi code and where the changes would need to be implemented. I also have a lot of other bugs on my list that I need to close off first, so if someone else wants to pick it up I am fine with that but if it is still here when I have gone through all my other bugs, I will have a go at it.
Just posting here that I stumbled across this issue today. Posted up in the forum, FWIW. Having now found this, I will continue to workaround by changing the PSK. https://community.ipfire.org/t/comma-in-ipsec-psk-causes-problems/11651
I have now got some time to start working on this issue. I have read up on base64 encoding/decoding and understand it enough to work on this. However, I have come across a problem. @Michael wrote in comment 5 > To be backwards-compatible, we can simply use the original value if it cannot > be base64 decoded. However there is no way to easily identify if an original string has been base64 encoded if the string only uses upper and lower case letters and digits and it has an integral number of 6 bit groups. So a user could have had an original PSK of reiyy558sJJU which when base64 encoded becomes cmVpeXk1NThzSkpV However you can't identify that the user did not have a PSK string of cmVpeXk1NThzSkpV originally. The only thing I could do is check if a PSK string has characters that are not valid in a base64 string, in which case it must have been a PSK that was not base64 encoded. How do I handle this. Maybe there is a simple solution that due to my lack of knowledge about base64 encoding/decoding, I am missing. Open to suggestions.
Interesting thing I have found is that base64 encode/decode is already used in the vpnmain.cgi for the CA and the certificate. In terms of the backwards compatibility for PSK's that were historically not base64 encoded I have had the idea that I can add a $cgiparams{'KEY'} entry for if base64 encoding is used. All previous values would have no entry in that key but all PSK's created would have the key with an entry such as YES. Then base64 encoding and decoding would only be done for PSK's where this additional BASE64 KEY value was set as YES. All others would have the PSK taken as it is. That seems a reasonable way around the issue I highlighted yesterday. I will start working on that basis.
*** Bug 11725 has been marked as a duplicate of this bug. ***
I have been able to successfully store the PSK in the config file as a Base64 encoded string. That means there are no problems with commas or any other type of character. When storing the PSK into the ipsec.secrets file the PSK is decoded back from Base64. I have tested the above with my laptop using PSK and was able to successfully use a comma and all sorts of other characters. The only limitation that still stays in place is that single quotation marks can not be used in the PSK. The single quotation mark is used in the ipsec.secrets file to delineate the bounds of the PSK. I have also added a variable for new PSK's which flags them as base64 encoded. So old valid PSK's stay working without the base64 encoding. If the old PSL connection is edited (even with no change) then the PSK will be updated to base64 encoded. This will be transparent to the user. I tested a non-base64 encoded PSK and the connection worked. Editing the connection and converting the PSK to base64 encoded still worked with the same PSK used on the client. I will submit a patch for this so it can be reviewed to see if there are any coding style improvements that can be made.
Bit more work to do. Have found that if I disable the connection and then re-enable it that the ipsec-secrets PSK value is base64 encoded which should not occur. It looks like when it is disabled that it loses track on whether it is Base64 encoded or a raw PSK.
Have now got it working correctly when disabled/enabled, edited or done from fresh. It also works fine with existing PSK's that are not base64 encoded. They stay as they are if just disabled/enabled but if you edit it and press the Save button, even if nothing is changed, then the PSK in the config file will be base64 encoded. The user will not notice that effect. The same PSK will still be used in the client. Later today or tomorrow I will submit the patch for the fix for review in the developers mailing list.
Patch set has been submitted. https://lists.ipfire.org/hyperkitty/list/development@lists.ipfire.org/thread/A5272AVHRSA4EPLZSSKFNQIBZOZ274J7/ https://patchwork.ipfire.org/project/ipfire/list/?series=4361