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

Security enhancements to API #14733

Merged
merged 2 commits into from Feb 22, 2023
Merged

Security enhancements to API #14733

merged 2 commits into from Feb 22, 2023

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Feb 20, 2023

  • Use signed API endpoint and verify signature using included public certificate.
  • Add source checksums to JSON and use them in for source downloads.
    • Note a race condition exists for formulae.brew.sh/api/cask-source where it can technically be updated after the JSON was last read/cached. This problem does not exist for endpoints with the git head in their URL (i.e. raw GitHub). Given formulae.brew.sh is a "last resort", this is not too big of an issue.

@BrewTestBot
Copy link
Member

Review period will end on 2023-02-21 at 18:50:49 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 20, 2023
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.

Great work here so far @Bo98, really pleased to see this coming along!

Library/Homebrew/api.rb Outdated Show resolved Hide resolved
Library/Homebrew/api.rb Outdated Show resolved Hide resolved
Library/Homebrew/api.rb Outdated Show resolved Hide resolved
Library/Homebrew/api.rb Outdated Show resolved Hide resolved
Library/Homebrew/api.rb Outdated Show resolved Hide resolved
Library/Homebrew/api.rb Outdated Show resolved Hide resolved
Library/Homebrew/api.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula_installer.rb Show resolved Hide resolved
@BrewTestBot BrewTestBot added waiting for feedback Merging is blocked until sufficient time has passed for review and removed waiting for feedback Merging is blocked until sufficient time has passed for review labels Feb 21, 2023
@BrewTestBot BrewTestBot added waiting for feedback Merging is blocked until sufficient time has passed for review and removed waiting for feedback Merging is blocked until sufficient time has passed for review labels Feb 21, 2023
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.

Great work again @Bo98.

I'm fine for this to be merged first and cask/formula unification to come after if you can apply the TODO. Everything else can be non-blocking and addressed later.

Library/Homebrew/api.rb Outdated Show resolved Hide resolved
Library/Homebrew/api.rb Show resolved Hide resolved
Library/Homebrew/api.rb Outdated Show resolved Hide resolved
Library/Homebrew/api/cask.rb Show resolved Hide resolved
Library/Homebrew/formula_installer.rb Show resolved Hide resolved
Library/Homebrew/formula_installer.rb Show resolved Hide resolved
Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

I agree that the resource method feels a bit weird, here, but think it's worth moving forward with a TODO comment for now

Library/Homebrew/api.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula.rb Show resolved Hide resolved
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 21, 2023
@BrewTestBot
Copy link
Member

Review period ended.

Comment on lines 353 to 358
def ruby_source_checksum
return JSON.parse(@source)["ruby_source_checksum"] if loaded_from_api

{
"sha256" => Digest::SHA256.file(sourcefile_path).hexdigest,
}
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather not have to parse the source twice here.

Suggested change
def ruby_source_checksum
return JSON.parse(@source)["ruby_source_checksum"] if loaded_from_api
{
"sha256" => Digest::SHA256.file(sourcefile_path).hexdigest,
}
end
def ruby_source_checksum
@ruby_source_checksum ||= {
"sha256" => Digest::SHA256.file(sourcefile_path).hexdigest,
}
end

You could do this instead and just pass it as another parameter to the constructor when it's loaded from the API though the list of values being passed to the constructor just keeps growing.

Copy link
Member Author

Choose a reason for hiding this comment

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

The amount of parameters we pass to the constructor is a mess but it probably is the better option for now.

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 to me. If you apply the few suggestions here as-is I reckon just ship it.

Library/Homebrew/api.rb Outdated Show resolved Hide resolved
Library/Homebrew/api.rb Outdated Show resolved Hide resolved
Library/Homebrew/formulary.rb Show resolved Hide resolved
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.

👍🏻 🚢

@Bo98 Bo98 merged commit 336c2c7 into Homebrew:master Feb 22, 2023
@Bo98 Bo98 deleted the api-security branch February 22, 2023 23:10
Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

Thanks again for this!

@MikeMcQuaid
Copy link
Member

Great work here, thanks again @Bo98!

taoky added a commit to ustclug/ustcmirror-images that referenced this pull request Feb 24, 2023
@github-actions github-actions bot added the outdated PR was locked due to age label Mar 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants