Property values are truncated

fdcg commented 4 years ago

If a property value in a devon.properties file contains certain special characters (e.g. } or !} then the property value is truncated there and thus incorrectly transferred to the environment. This is at least the case with Windows 10.

hohwille commented 4 years ago

@fdcg thanks for reporting this. Is there a real use-case behind this or is this just a theoretical observation? If there is a real use-case please give an example or explanation so we can prioritize this issue.

fdcg commented 4 years ago

Yes, there is a real use case. Each member of my team has their own account on a central infrastructure for a Maven repo, etc. I want to create centralized settings for the team that use fixed environment variables where credentials are required. These environment variables must be set by the developer in the private devon.properties. For example, if the password contains the special characters mentioned above, it will be truncated.

markusschuh commented 3 years ago

Indeed the current environment-project.bat replaces } with %% even in cases, where this is not needed. I'll provide a PR to fix this.

Beyond this detail - which is easy to fix - your use case IMHO raises some more open questions. This is somehow also related to #415 :

Just from a practical point of view use of clear-text passwords within config files is always quite a challenge, since some characters - valid for passwords - will anyway need some quoting in common config files. Imagine the following were - even when quite unlikely - a user's password. It will be hard to enable the user, to define this within a properties file - even in unix and outside of devonfw-ide:

~${PATH}"''"

Also from view point of security sensitive details like a user password should be protected by additional controls.

In practice this is of course more simpler to state than to realise. So what is possible with devonfw-ide out of the box - as far as I can say?

If the password were only used for access with maven to a repo, protected by basic authentication, the Maven --encrypt-password method can and should be used within devonfw-ide. The needed master password is seeded automatically during setup of devonfw-ide and the per default installed ${DEVON_IDE_HOME}/conf/.m2/repository contains helpful guidance, how to protect the password such against the simpler spy out methods.

But the user's settings.xml is only installed from any ide-settings repository if it does not exist yet, if I correctly understand the current devonfw-ide implementation of devon ide update settings. So if the settings.xml inside the project related ide-settings repo is later updated, the user will need to remove his/her own copy first - and after the update add the password via mvn --encrypt-password again, won't he/she?

In this case, it might indeed help - though I have never verified, it works - if the settings.xml contains a line

<password>${USER_PASSWORD}</password>

and the user has a

export   USER_PASSWORD="{_output_of_mvn_encrypt_password_}"

within the user's devon.properties. Indeed this specific case can't work currently under Windows, since the environment-project.bat replaces the "}" unconditionally with a "%%". This code line should only be executed, if a related "${" exists.

I can imagine some more use cases - beyond mvn - where some password protected repository access should be handled by a project-provided script. This script should be able to consume the individual team members password - at least when write access is needed. If read access is enough use of a shared password of a service account might be sufficient.

But I'd say, those use cases must be handled individually. Adding a definition like

  export MY_PASSWORD=_some_complex_set_of_any_char_

to user's own devon.properties and scripts reusing this be adding the password - read from environment - to some command line call - is not the way, how those use cases should be resolved. IMHO the sensitive character of user's password deserves some more specific solutions - even when this means more effort to realise.

hohwille commented 3 years ago

Recomendation for passwords is to never put them into devon.properties what will expose them as environment variables and is a security weakness. Instead use password encrpytion. For maven the default settings repository provides a template for settings.xml that every developer will get in conf/.m2/settings.xml and allows him to take over control. This already comes with the documentation hints to setup credentials for maven: https://github.com/devonfw/ide-settings/blob/master/devon/conf/.m2/settings.xml#L8

@markusschuh your PR will still be a valuable improvement. According to my CMD hacking experience this is quite tricky to solve.

markusschuh commented 3 years ago

I have created the PR. This slows down variable read from properties a bit, since it calls FINDSTR.EXE for each variable.

I have verified the change with a devon.properties (using "\r\n" line endings) that contains the following variable definitions:

FOO=${bar}                           
FOO="${bar}"                         
FOO={bar}                            
FOO="{bar}"

After a set DEVON_IDE_TRACE=yes a call of devon | findstr /c:"call set " results in

 call set "FOO=%bar%"
 call set "FOO=%bar%"
 call set "FOO={bar}"
 call set "FOO={bar}"

Without this change the two latter resulted in

``´ call set "FOO={bar%" call set "FOO={bar%"



The "!" could also be made a supported character in value - but this were a bit more complex change, since it would need the `setlocal EnableDelayedExpansion` - which is the reason, the "!" is removed currently - to only be called, when really necessary.

hohwille commented 3 years ago

Just to document the status of this issue:

we had done some improvements to more common cases with #430 and that was already released
reading properties sounds like a trivial thing for a reasonable programming language but is a complex thing for windows batch scripting
we have an open PR #487 that could improve this. However, this is reducing performance significantly and that matters as multiple devon.properties files have to be read every time you invoke a devon command.
I still see this as an exotic edge-case and do not really see the need for this change
There is no feedback anymore, neither on my comment about password best practices nor on the PR and the performance impact.
After all, you can simply use bash (e.g. via git-bash) for everything and ignore ugly windows command world and their problems.
So I labeled as waiting-for-feedback and unless someone shows up here, there will be no further progress. We have more important issues to work on. However, feel free to make progress or suggestions at any time so we may be able to proceed here.

fdcg commented 3 years ago

Currently, we no longer store the passwords in devon.properties. So we don't need full support for every special character in properties. But support for closing } in settings like foo={bar} is still needed.

markusschuh commented 2 years ago

If windows CMD is considered ugly anyway - why then care about some more slow-down of CMD execution? The time execution test results inside #456 had been: <291/ 376 / 675 / 907 / 982> milliseconds with the feature vs <85 / 599 / 617 / 640 / 752> milliseconds without. Why should that be "twice as much with the feature active than without?" For me this more looks like an increase of 20%. (when I use the mean of both series)

But if I remember some of my ide optimization tests in 2020 correctly, the massive slowness problem of devon ide in CMD (wich exists with or without the #495 feature) can be reduced drastically by other means, which would make the #456 irrelevant:

There is no real need to do any handling of properties files directly under CMD. bash is called in any devon run, so lets use the handling of properties file in bash as well for CMD. This just needs a bit more scripting in bash so it can return the variables in a format, that CMD can read.

devonfw / ide

Property values are truncated #456