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

bin/brew: tighten check in export_homebrew_env_file #18095

Merged
merged 1 commit into from
Aug 20, 2024
Merged

Conversation

carlocab
Copy link
Member

@carlocab carlocab commented Aug 20, 2024

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

The current glob check will accept lines like

HOMEBREW_FOO=bar BAD_ENV_VAR=baz

and happily export them, but we don't want that.

Let's tighten up the check to reject lines like the above.

The current glob check will accept lines like

  HOMEBREW_FOO=bar BAD_ENV_VAR=baz

and happily export them, but we don't want that.

Let's tighten up the check to reject lines like the above.
@@ -108,14 +108,14 @@ HOMEBREW_LIBRARY="${HOMEBREW_REPOSITORY}/Library"

# Load Homebrew's variable configuration files from disk.
export_homebrew_env_file() {
local env_file
local env_file="${1}"
local homebrew_env_regex="^HOMEBREW_[A-Z_]+=[^=]*$"
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally we should be able to reject spaces after the =, but we have a lot of env variables that take multiple values that are space-separated.

Personally I'd be in favour of deprecating separating these with spaces and delimit them with , or ; instead.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather we did't deprecate for this. An alternative approach would be to "build" the line separately by extracting first the HOMEBREW_[A-Z_]+, then the =, then the rest of the line without = or HOMEBREW_ in it.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks good! Happy to merge as-is and iterate.

@carlocab carlocab merged commit 75ab63b into master Aug 20, 2024
24 checks passed
@carlocab carlocab deleted the secure-env-files branch August 20, 2024 09:49
@ctaintor
Copy link
Contributor

ctaintor commented Aug 21, 2024

this has broken our organization's usage of setting a custom HOMEBREW_ARTIFACT_DOMAIN. We have to override HOMEBREW_DOCKER_REGISTRY_BASIC_AUTH_TOKEN to be anonymous:anonymous which, when base64 encoded, has the line end in =.

Can this PR be reverted? The effect of this means that the auth token isn't set, which then means that Homebrew will add --header Authorization:\ Bearer\ QQ== which will then cause our mirror to return 401

HOMEBREW_DOCKER_REGISTRY_BASIC_AUTH_TOKEN=YW5vbnltb3VzOmFub255bW91cw==

@ctaintor
Copy link
Contributor

Opened #18110

@ctaintor
Copy link
Contributor

just an idea how you might want to do what you were doing

#!/bin/bash

# Define the input and output dotfiles
input_dotfile="/etc/homebrew/brew.env"
output_dotfile="sanitized.env"
before_vars="" #initializing the variable so it doesn't show as "new"

pattern="^HOMEBREW_[A-Z_]*="

before_vars=$(compgen -v)

source "$input_dotfile"

after_vars=$(compgen -v)

# Get the difference: variables that are in after_vars but not in before_vars
new_vars=$(comm -13 <(echo "$before_vars" | sort) <(echo "$after_vars" | sort))

# Filter and save only the variables matching the pattern
>| $output_dotfile
for var in $new_vars; do
    value=$(eval echo \$$var)
    if [[ "$var=$value" =~ $pattern ]]; then
        echo "$var=$value" >> "$output_dotfile"
    fi
done

echo "Filtered variables have been saved to $output_dotfile"

@MikeMcQuaid
Copy link
Member

@ctaintor Could you try and open a pull request? This document should help and we're happy to walk you through anything else.

Thanks!

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.

3 participants