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

[BUG] Relenv packaging shell scripts should use shellcheck to validate scripts #63517

Closed
9 tasks
dmurphy18 opened this issue Jan 19, 2023 · 4 comments
Closed
9 tasks
Assignees
Labels
Bug broken, incorrect, or confusing behavior pending-close

Comments

@dmurphy18
Copy link
Contributor

dmurphy18 commented Jan 19, 2023

Description
Need to add shellcheck to verify the validity of shell scripts to ensure that they are correct

For example: the current salt-call is using backticks around dirname

root@david-uarm64:/opt/saltstack/salt# cat salt-call
#!/bin/sh
"exec" "`dirname $(readlink -f $0)`/bin/python3" "$0" "$@"
# -*- coding: utf-8 -*-
import re
import sys
from salt.scripts import salt_call
if __name__ == '__main__':
    sys.argv[0] = re.sub(r'(-script\.pyw|\.exe)?$', '', sys.argv[0])
    sys.exit(salt_call())
root@david-uarm64:/opt/saltstack/salt# 

While I am an old UNIX hand and fine with backticks, modern practice is to use $() rather than backticks.
This is just one example of bad practice in scripts. Shellcheck is used on other repositories with VMware (other Salt repos) to ensure that shell scripts are correct and appropriate with basic security issues cleaned up.

Presumming /usr/bin should have symbolic links from /usr/bin into /opt/saltstack/salt

root@david-uarm64:/usr/bin# l salt-call 
lrwxrwxrwx 1 root root 29 Jan 19 16:54 salt-call -> /opt/saltstack/salt/salt-call

Please be as specific as possible and give set-up details.

  • on-prem machine
  • VM (Virtualbox, KVM, etc. please specify)
  • VM running on a cloud service, please be explicit and add details
  • container (Kubernetes, Docker, containerd, etc. please specify)
  • or a combination, please be explicit
  • jails if it is FreeBSD
  • classic packaging
  • onedir packaging
  • used bootstrap to install

Steps to Reproduce the behavior
(Include debug logs if possible and relevant)

Expected behavior
A clear and concise description of what you expected to happen.

Screenshots
If applicable, add screenshots to help explain your problem.

Versions Report

salt --versions-report (Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)
PASTE HERE

Additional context
Add any other context about the problem here.

@dmurphy18 dmurphy18 added Bug broken, incorrect, or confusing behavior needs-triage labels Jan 19, 2023
@dmurphy18 dmurphy18 added the Sulfur v3006.0 release code name and version label Jan 20, 2023
@whytewolf whytewolf self-assigned this Jan 20, 2023
@whytewolf
Copy link
Contributor

shellcheck results

"exec" "`dirname $(readlink -f $0)`/bin/python3" "$0" "$@"

should be

"exec" "$(dirname "$(readlink -f "$0")")/bin/python3" "$0" "$@"

The rest of the code is python which shellcheck blows up on.

looks like the relevant code that generates this is at https://github.com/saltstack/relative-environment-for-python/blob/b848c6f7cf7a36587935db349c42e89e0fb0e5b6/relenv/common.py#L48

@dmurphy18
Copy link
Contributor Author

Wondering about other shell scripts used in the pipelines.
Haven't got deep into the pipeline yet, but betting there are more apart from the salt-XXXXX scripts.

@s0undt3ch
Copy link
Member

This should be fixed in relenv saltstack/relenv@13a309f

@dwoz
Copy link
Contributor

dwoz commented Apr 4, 2023

This is fixed and we have tests in place to validate shebangs made by relenv.

@dwoz dwoz closed this as completed Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior pending-close
Projects
None yet
Development

No branches or pull requests

6 participants