Use macro rather than shell expansion for string processing in spec file#8511
Use macro rather than shell expansion for string processing in spec file#8511alexey-tikhonov merged 2 commits intoSSSD:masterfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a security concern by replacing a shell expansion in the sssd.spec.in file with a built-in RPM macro. The change from %(echo "@PACKAGE_VERSION@" | sed 's/-/~/g') to %{gsub @PACKAGE_VERSION@ - ~} for version string processing is correct and effectively mitigates the risk of arbitrary code execution. This is a good improvement for security and maintainability. I have no further comments or suggestions.
Note: Security Review is unavailable for this PR.
|
Sorry, I thought getting rid of shell expansion in |
| %package common | ||
| Summary: Common files for the SSSD | ||
| License: GPL-3.0-or-later | ||
| %if "%{?samba_package_version}" != "" |
There was a problem hiding this comment.
Please forgive my ignorance - where is this macro defined?
There was a problem hiding this comment.
Please ignore. I got it.
There was a problem hiding this comment.
Ah, is this because Packit Service doesn't allow
%(rpm -q samba-devel --queryformat %{version})
anymore?
There was a problem hiding this comment.
Instead of wrapping every 'Requires:', let's better define samba_package_version to something in this case.
There was a problem hiding this comment.
Is it possible to re-define the same samba_package_version without introduction of new minimal_samba_version?
There was a problem hiding this comment.
IMO it should be possible to set it like this
%global samba_package_version %(rpm -q samba-devel --queryformat %{version})
%if "%{?samba_package_version}" != ""
%global samba_package_version 4.19
%endif
There was a problem hiding this comment.
That's what I meant.
With a small note there is likely a mistype in:
%if "%{?samba_package_version}" != ""
-- we need to check when it's equal.
There was a problem hiding this comment.
Is it possible to re-define the same
samba_package_versionwithout introduction of newminimal_samba_version?
AFAICT it's not a good practice, as it could have unintended side-effects, but it this case I think it should be fine. I've updated the commit.
thalman
left a comment
thalman
left a comment
There was a problem hiding this comment.
Thanks for the update, ACK
thalman
added
backport-to-sssd-2-9
backport-to-sssd-2-10
backport-to-sssd-2-11
backport-to-sssd-2-12
backport-to-sssd-2-9-4
Signed-off-by: Nikola Forró <nforro@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
Signed-off-by: Nikola Forró <nforro@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>
4 participants
We've hardened security in Packit Service and shell expansions in spec files are now rejected as they can be used to execute arbitrary code. There is no need to use shell expansion for string processing, there is an existing macro for this very purpose.