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 S3 use_ssl and autocreate default values #1948

Closed
4 of 7 tasks
aentwist opened this issue Mar 21, 2023 · 2 comments
Closed
4 of 7 tasks

Fix S3 use_ssl and autocreate default values #1948

aentwist opened this issue Mar 21, 2023 · 2 comments
Labels
1. To develop bug feature: auto config (environment variables) Auto configuring via environment variables needs review Needs confirmation this is still happening or relevant

Comments

@aentwist
Copy link
Contributor

aentwist commented Mar 21, 2023

Description

From the PHP Manual for getenv, we have

getenv(string $varname, bool $local_only = false): string|false

Returns the value of the environment variable varname, or false if the environment variable varname does not exist.

Now check this out

$use_ssl = getenv('OBJECTSTORE_S3_SSL');

'use_ssl' => (strtolower($use_ssl) === 'false' || $use_ssl == false) ? false : true,

Value not set -> getenv returns false -> use_ssl gets false -> ternary goes to false

This is a great example of why to not allow poorly written code into a codebase. It obfuscates things.

Should probably be

// if defined and value is "false" then set to false
!(getenv('OBJECTSTORE_S3_SSL') && strtolower(getenv('OBJECTSTORE_S3_SSL')) === 'false')
// demorgans law
// if not defined or doesn't have value "false" then set to true
!getenv('OBJECTSTORE_S3_SSL') || strtolower(getenv('OBJECTSTORE_S3_SSL')) !== 'false'

There are several other suspicious variables. Using a ternary operator for a boolean expression is redundant. Not relying on truthy falsy values in favor of explicit checks is also somewhat suspicious here.

'autocreate' => (strtolower($autocreate) === 'false' || $autocreate == false) ? false : true,
'use_ssl' => (strtolower($use_ssl) === 'false' || $use_ssl == false) ? false : true,
// required for some non Amazon S3 implementations
'use_path_style' => $use_path == true && strtolower($use_path) !== 'false',
// required for older protocol versions
'legacy_auth' => $use_legacyauth == true && strtolower($use_legacyauth) !== 'false'

'autocreate' => $autocreate == true && strtolower($autocreate) !== 'false',


Requirements

  • build docker image
  • set only OBJECTSTORE_S3_BUCKET (use defaults)
  • exec in
  • examine config/config.php and check default values against README
    • autocreate is true
    • use_ssl is true
    • use_path_style is false
    • legacy_auth is false
  • set only OBJECTSTORE_SWIFT_URL
    • autocreate is false
@aentwist aentwist changed the title The default values of use_ssl and other config are incorrect The default values of use_ssl and potentially other config are incorrect Mar 21, 2023
@aentwist aentwist changed the title The default values of use_ssl and potentially other config are incorrect The default values of S3 use_ssl and autocreate are incorrect Mar 21, 2023
@aentwist aentwist changed the title The default values of S3 use_ssl and autocreate are incorrect Fix S3 use_ssl and autocreate default values Mar 26, 2023
@aentwist
Copy link
Contributor Author

will pick this up if my other PRs are resolved

@J0WI
Copy link
Contributor

J0WI commented Oct 24, 2024

#2309

@J0WI J0WI closed this as completed Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. To develop bug feature: auto config (environment variables) Auto configuring via environment variables needs review Needs confirmation this is still happening or relevant
Projects
None yet
Development

No branches or pull requests

3 participants