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

dir: Use SHA256, not SHA1, to name the cache for a filtered remote #4716

Merged
merged 1 commit into from
Feb 8, 2022

Conversation

debarshiray
Copy link
Contributor

SHA1 hashes are considered weak these days. Some distributions have
static analysis tools to detect the use of such weak hashes, and they
get triggered by flatpak. While this particular use of SHA1 in flatpak
is likely not security sensitive, it's also easy to move to SHA256 to
avoid any debate.

Here, the SHA1 hash of a named remote's filter file is used to generate
the name of the directory where the refs from that remote are cached.
One can reasonably assume that the cache is frequently invalidated
because the list of refs on the remote changes all the time. Hence,
it's not big problem if it gets invalidated once more because of this
change.

SHA1 hashes are considered weak these days. Some distributions have
static analysis tools to detect the use of such weak hashes, and they
get triggered by flatpak. While this particular use of SHA1 in flatpak
is likely not security sensitive, it's also easy to move to SHA256 to
avoid any debate.

Here, the SHA1 hash of a named remote's filter file is used to generate
the name of the directory where the refs from that remote are cached.
One can reasonably assume that the cache is frequently invalidated
because the list of refs on the remote changes all the time. Hence,
it's not big problem if it gets invalidated once more because of this
change.
@debarshiray
Copy link
Contributor Author

The toolchain in Fedora 35, with GCC 11.2.1, errors out with:

tests/test-instance.c: In function ‘test_claim_per_app_temp_directory’:
tests/test-instance.c:225:3: error: ‘_GLNX_TEST_SCOPED_TEMP_DIR’ undeclared (first use in this function)
  225 |   _GLNX_TEST_SCOPED_TEMP_DIR;
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~
tests/test-instance.c:225:3: note: each undeclared identifier is reported only once for each function it appears in

Looking at the gcc -E output, I see that the #include libglnx/tests/libglnx-testlib.h has been truncated to:

G_DEFINE_AUTOPTR_CLEANUP_FUNC(_GLnxTestAutoError, _glnx_test_auto_error_cleanup);

Everything below that line is missing.

Let's see what the CI (on Ubuntu 18.04) has to say.

Copy link
Collaborator

@mwleeds mwleeds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@smcv smcv merged commit 5c59f1b into flatpak:main Feb 8, 2022
@debarshiray
Copy link
Contributor Author

Thanks for the reviews!

@debarshiray debarshiray deleted the wip/rishi/md5-sha1-deprecate branch February 9, 2022 12:31
@debarshiray
Copy link
Contributor Author

The toolchain in Fedora 35, with GCC 11.2.1, errors out with:

tests/test-instance.c: In function ‘test_claim_per_app_temp_directory’:
tests/test-instance.c:225:3: error: ‘_GLNX_TEST_SCOPED_TEMP_DIR’ undeclared (first use in this function)
  225 |   _GLNX_TEST_SCOPED_TEMP_DIR;
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~
tests/test-instance.c:225:3: note: each undeclared identifier is reported only once for each function it appears in

tl;dr: It was a false alarm.

Several hours of debugging into the depths of GCC's pre-processor made me realize that I still have an old copy of the libglnx sub-project under the top-level directory, in addition to the current copy inside the subprojects sub-directory, as a side-effect of fiddling with old branches. This was causing problems because the old copy was getting precedence due to the order in which the include paths (ie., the -I flags) were being passed to GCC.

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.

None yet

3 participants