Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dollar sign escape #185

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FrenkenFlores
Copy link
Contributor

IPMI commands get executed in a separate shell where the combination "status" has its own behavior. In our case,
Popen starts a new shell, for example by running /bin/bash, and when the BMC password contains the combination
above it gets replaced to "bin/bash" (in this case the combination holds the last executed command as a string).
I have added a method to Session class that will escape the dollar sign. Such behavior is expected with other signs
(like combination "^_") but I didn't find it critical.

@coveralls
Copy link

coveralls commented Jan 7, 2025

Coverage Status

coverage: 69.842% (+0.009%) from 69.833%
when pulling a4859d1 on FrenkenFlores:dollar_sign_escape
into f0df31e on kontron:master.

@hthiery
Copy link
Contributor

hthiery commented Jan 7, 2025

could you please meld the commits into one for a better review. you should also add testcase(s) for issues/enhancements that are not yet covered by a testcase.

thanks

@FrenkenFlores
Copy link
Contributor Author

FrenkenFlores commented Jan 7, 2025

I have checked the Codacy Static Code Analysis reports. It has conflict with using hard coded password and Popen function from the subprocess library. But I wanted to emulate the same behavior as Ipmitool class. Can we ignore the reports and merge it?

@hthiery
Copy link
Contributor

hthiery commented Jan 7, 2025

I have checked the Codacy Static Code Analysis reports. It have conflict with using hard coded password and the Popen function from the subprocess library. But I wanted to emulate the same behavior as Ipmitool class. Can we ignore the reports and merge it?

we can configure codacy to ignore tests, e.g. https://docs.codacy.com/repositories-configure/codacy-configuration-file/#ignore-files

IPMI commands get executed in a separate shell where the combination "status" has its own behavior. In our case,
Popen starts a new shell, for example by running /bin/bash, and when the BMC password contains the combination
above it gets replaced to "bin/bash" (in this case the combination holds the last executed command as a string).
I have added a method to Session class that will escape the dollar sign and test function. Such behavior is expected with other signs
(like combination "^_") but I didn't find it critical for them.

Signed-off-by: Evloev Sayfuddin <[email protected]>
@FrenkenFlores
Copy link
Contributor Author

I have checked the Codacy Static Code Analysis reports. It have conflict with using hard coded password and the Popen function from the subprocess library. But I wanted to emulate the same behavior as Ipmitool class. Can we ignore the reports and merge it?

we can configure codacy to ignore tests, e.g. https://docs.codacy.com/repositories-configure/codacy-configuration-file/#ignore-files

I can't configure codacy by myself, I will wait for you to skip it.

@FrenkenFlores
Copy link
Contributor Author

@hthiery Hi!) Is there any update on this PR?

@hthiery
Copy link
Contributor

hthiery commented Jan 13, 2025

@hthiery Hi!) Is there any update on this PR?

I just see that calling ipmitool with popen and Shell=True is an issue .

Using e.g.: ./ipmitool.py -vv -I ipmitool -o interface_type=lan -H 10.0.114.137 -U '";$(xterm);"' -P admin bmc info can execute something that is embedded in user or password.

So I think we should change Shell=True to Shell=False

@FrenkenFlores
Copy link
Contributor Author

Ok. I will look into it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants