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

Fix libsodium missing from GitHub Releases #1062

Merged
merged 3 commits into from Aug 12, 2021

Conversation

sehrope
Copy link
Contributor

@sehrope sehrope commented Aug 11, 2021

This PR fixes a security issue with wal-g that the binary releases published as GitHub Releases are built without libsodium support and silently ignore user requests to use libsodium encryption. The three commits in this PR:

  • Enable libsodium in the GitHub release builds.
  • Exit with failure if the user requests to use libsodium encryption but the application was not compiled with libsodium support.
  • Enables manual triggering of unit tests GitHub Action (not really related but was helpful for testing)

The security issue is that a user would expect that their backup files are encrypted but they're silently processed and uploaded as plaintext. The exit-with-failure piece checks if either WALG_LIBSODIUM_KEY or WALG_LIBSODIUM_KEY_PATH are specified and if the binary was not compiled with libsodium support, then it prints an error and immediately exits.

This will be a breaking change for anybody using the GitHub Release binaries who specify libsodium keys on their primary (performing the uploads) but do not include them in their replicas. Previously everything was handled as plaintext so the replicas would have been able to read the primary's plaintext backups. After including this fix, the primary will be uploading encrypted files but any replicas that are missing the libsodium keys will no longer be able to read them unless they also have the same environment variables specified for the libsodium keys (so users need to make sure all their servers are in sync for libsodium usage).

Longer term I think it'd be simpler if there was a single way to specify how to handle encryption. Right now it's a series of if-then-else logic that checks against a bunch of different environment variables, e.g. there's WALG_GPG_KEY_ID, WALG_PGP_KEY, WALG_LIBSODIUM_KEY... If more than one is defined than the one that is picked is based upon the if-then-else order.

A single environment variable, e.g. WALG_ENCRYPTION, could instead be used for both the encryption mechanism and where to look for key material. That way it's explicit and a user indicating a given mechanism knows for sure that's what shall be used, or the app could fail if it's not possible (e.g. no compiled support for the requested mechanism).

That'd be a separate, possibly breaking change, so just leaving it here for future discussion.

@sehrope sehrope requested a review from a team as a code owner August 11, 2021 17:54
Copy link
Member

@usernamedt usernamedt left a comment

Choose a reason for hiding this comment

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

Looks OK, but need to fix one small issue

internal/configure_crypter.go Outdated Show resolved Hide resolved
internal/configure_crypter.go Outdated Show resolved Hide resolved
…m support

Adds a check for the existence of libsodium keys in non-libsodium builds. If keys are detected then
exit with failure as the user likely wanted to encrypt all file activity but it is not possible as
the application was not compiled with support for libsodium.

Previously, wal-g would continue to operate as if no encryption was requested.
@usernamedt usernamedt merged commit cadf598 into wal-g:master Aug 12, 2021
53 checks passed
@sehrope
Copy link
Contributor Author

sehrope commented Aug 12, 2021

Thanks for the review and merging.

CVE-2021-38599 has been created for this as well: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-38599

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

2 participants