attestations: widen the beta #17692

woodruffw · 2024-07-13T14:53:14Z

This widens the attestations beta to include people with developer mode enabled, as well as those with HOMEBREW_DEVELOPER set in their environment.

~~Needs tests.~~

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?

This widens the beta to include people with developer mode enabled, as well as those with HOMEBREW_DEVELOPER set in their environment. Signed-off-by: William Woodruff <[email protected]>

Signed-off-by: William Woodruff <[email protected]>

Bo98 · 2024-07-13T15:27:42Z

What's the status of sigstore-ruby? Thought we were waiting to see how that went.

If we are wanting to force install gh for everyone then we probably want to at least skip doing so for older macOS as installing go is probably not exactly quick.

woodruffw · 2024-07-13T15:34:43Z

What's the status of sigstore-ruby? Thought we were waiting to see how that went.

It's still under active development here: https://github.com/sigstore/sigstore-ruby. I don't think it's ready for prime-time here yet, though.

If we are wanting to force install gh for everyone then we probably want to at least skip doing so for older macOS as installing go is probably not exactly quick.

I might be misunderstanding, but go should only be a build-time dependency here, right? That being said I agree that this shouldn't go GA yet, which is why it's limited to people who set developer flags with this change 🙂

Bo98 · 2024-07-13T15:36:41Z

I might be misunderstanding, but go should only be a build-time dependency here, right?

Yes, but we don't have gh bottles for older macOS (Big Sur and earlier) or, say, arm64 Linux.

MikeMcQuaid · 2024-07-13T15:39:01Z

Yes, but we don't have gh bottles for older macOS (Big Sur and earlier) or, say, arm64 Linux.

I think this should ideally just check if we have a valid bottle for this, then, and only auto-install if so.

woodruffw · 2024-07-13T15:39:15Z

Yes, but we don't have gh bottles for older macOS (Big Sur and earlier) or, say, arm64 Linux.

Ah, gotcha! Yeah, IMO we can carve those users out for the next round of expansion.

Signed-off-by: William Woodruff <[email protected]>

p-linnane

Awesome!

Bo98 · 2024-07-13T19:26:01Z

Ah, gotcha! Yeah, IMO we can carve those users out for the next round of expansion.

If you're wanting a quick way to do this, it would be worthwhile adding !OS.unsupported_configuration? && to the check.

shivammathur · 2024-07-13T21:37:14Z

@Bo98 @woodruffw
Enabling this for everyone using HOMEBREW_DEVELOPER is very wide.
Can this be disabled in CI environments as fixing it locally is easy, but it becomes a breaking change in CI.

p-linnane · 2024-07-13T21:59:16Z

@shivammathur Thanks for raising this. Should be resolved with #17713.

shivammathur · 2024-07-13T22:07:33Z

@p-linnane @MikeMcQuaid Thanks for resolving this quickly.

MikeMcQuaid · 2024-07-14T01:43:10Z

@shivammathur you're welcome!

woodruffw · 2024-07-14T02:45:46Z

@shivammathur to help us understand how to proceed here: is there a reason your action is using Homebrew with developer mode enabled? Most users shouldn't need to do this, especially when just using Homebrew to set up an action's dependencies.

(Similarly, is there a reason you're tracking master instead of our stable releases?)

I'd like to flip this on by default more in the coming days, so it'd be helpful to understand whether this needs to affect you yet or whether it's something that can be addressed per above 🙂

Bo98 · 2024-07-14T03:57:07Z

I agree that action probably shouldn't use HOMEBREW_DEVELOPER.

I'd like to flip this on by default more in the coming days

Even when ignoring the desire to avoid having to bootstrap gh laid out previously, we realistically can't expect every end user to set a GitHub token to use Homebrew. This is a fairly big blocker for wider rollout.

brew install has never required a GitHub token so to suddenly demand a token is a huge UX regression. Even the move to making GitHub tokens mandatory for HOMEBREW_DEVELOPER users is something I'm not even overly comfortable with, and is also a type of breaking change we typically hold for major/minor-version bumps. Its widespread use in CI (including test-bot, which force enables HOMEBREW_DEVELOPER) is proof of this.

I completely forgot that was a requirement when initially reviewing this.

shivammathur · 2024-07-14T04:39:01Z

@woodruffw

Yes, I will remove HOMEBREW_DEVELOPER in setup-php in the next release, it was added in past when the action used a dev-cmd command and also untapped a third party tap without uninstalling formulae from that tap. These use-cases are no longer there.

We sync brew to master instead of a release as that has been a better experience to sync the core tap in cases when updates in the core tap require latest changes from brew.

To @Bo98 point, yes, if this becomes a default, then anyone using brew in CI would have to provide a token, it would be great if the env flag to disable this is kept permanently so that we can recommend providing a token moving forward rather than it being a requirement.

woodruffw · 2024-07-14T12:52:33Z

Even when ignoring the desire to avoid having to bootstrap gh laid out previously, we realistically can't expect every end user to set a GitHub token to use Homebrew. This is a fairly big blocker for wider rollout.

I agree with this. To be clear: I'm not talking about widening the beta to GA in the coming days, I meant widening it back out to people with devcmdrun. My rationale for this is:

devcmdrun doesn't have stability guarantees (unless we've changed that) -- users who enable it should expect to get the Homebrew developer experience, which includes features that aren't ready for GA yet.
devcmdrun already enables dev-cmds that require GH auth. These are admittedly separate commands and not integrations into brew install, but IMO this establishes precedent for "be prepared to auth to the GH API if you enable developer mode." I agree however that making it required is a significant change, however, and that probably requires further accommodation.
- In particular, one thing I think we could do here is widen this back out to devcmdrun, but skip if the user doesn't have GH creds configured. That will avoid changing the requirement here while continuing to roll this out for testing.
All of this is happening on pre-tag master, where we should be making these kinds of changes to discover kinks before they can land in a release. For example, this temporary widening helped us discover that the glibc bottle was missing an attestation (because we enabled it on CI), and revealed a place in setup-homebrew where we weren't configuring the GH_TOKEN.

TL;DR my position is that we should widen this back to devcmdrun + CI users who explicitly have GH creds configured in the coming days, but not expect this approach to work for GA -- for that, we will absolutely need an alternative to requiring GH auth (and ideally, we will have sigstore-ruby ready to replace gh by then as well).

Edit: sorry, forgot to add my response to @shivammathur here as well:

Thank you for looking into this! Per above, I 100% agree that requiring GH auth cannot be a default for when we move this feature to GA.

MikeMcQuaid · 2024-07-14T12:55:47Z

widen this back out to devcmdrun, but skip if the user doesn't have GH creds configured

This seems like a good idea 👍🏻 Could also probably consider documenting the opt-out variable, too.

3. All of this is happening on pre-tag master, where we should be making these kinds of changes to discover kinks before they can land in a release

Agreed.

Bo98 · 2024-07-14T13:55:45Z

2. devcmdrun already enables dev-cmds that require GH auth. These are admittedly separate commands and not integrations into brew install, but IMO this establishes precedent for "be prepared to auth to the GH API if you enable developer mode."

I don't really agree with this. There's quite a large range of users with devcmdrun enabled that it's an oversimplification to assume they all have GitHub auth enabled.

13.5% of our analytics are from users with devcmdrun. This is a very significant number.
(For comparison 1.6% have HOMEBREW_DEVELOPER and just 0.04% have both - that 0.04% being what I'd attribute to true developers/contributors.)

one thing I think we could do here is widen this back out to devcmdrun, but skip if the user doesn't have GH creds configured

This could work, though might suggest adjusting the messaging and/or handling for the invalid credentials case to potentially be non-fatal for this subset of users.

woodruffw · 2024-07-14T14:04:50Z

Hmm, that's a much higher number than I realized. Given that, yeah, I can see how developer mode is not a strong precedent for expecting GH creds to be present.

This could work, though might suggest adjusting the messaging and/or handling for the invalid credentials case to potentially be non-fatal for this subset of users.

Yes, agreed -- I think it makes sense to make it a non-fatal warning for this phase.

Take 2 of #17692 but with: - provide and document `HOMEBREW_NO_VERIFY_ATTESTATIONS` - don't try to run unless there's a GitHub token set - don't try to run unless `gh` is installed - don't try to run in CI While we're here: - split out a `Homebrew::EnvConfig.devcmdrun?` helper method - default `HOMEBREW_GITHUB_API_TOKEN` to `$GITHUB_TOKEN` if unset. - add some missing `Homebrew::EnvConfig.github_api_token` presence checks

Take 2 of #17692 but with: - provide and document `HOMEBREW_NO_VERIFY_ATTESTATIONS` - don't try to run unless there's GitHub credentials - don't try to run unless `gh` is installed - don't try to run in CI While we're here: - split out a `Homebrew::EnvConfig.devcmdrun?` helper method - add some missing `Homebrew::EnvConfig.github_api_token` presence checks

Take 2 of Homebrew#17692 but with: - provide and document `HOMEBREW_NO_VERIFY_ATTESTATIONS` - don't try to run unless there's GitHub credentials - don't try to run unless `gh` is installed - don't try to run in CI While we're here: - split out a `Homebrew::EnvConfig.devcmdrun?` helper method - add some missing `Homebrew::EnvConfig.github_api_token` presence checks

attestations: widen the beta

570cf65

This widens the beta to include people with developer mode enabled, as well as those with HOMEBREW_DEVELOPER set in their environment. Signed-off-by: William Woodruff <[email protected]>

woodruffw added the enhancement label Jul 13, 2024

woodruffw self-assigned this Jul 13, 2024

workflows/tests: add some tokens

6d8067d

Signed-off-by: William Woodruff <[email protected]>

MikeMcQuaid approved these changes Jul 13, 2024

View reviewed changes

workflows/tests: drop my changes

c7d5e02

Signed-off-by: William Woodruff <[email protected]>

woodruffw marked this pull request as ready for review July 13, 2024 18:05

Merge branch 'master' into ww/attestations-for-dev

255e750

woodruffw mentioned this pull request Jul 13, 2024

Integrating attestation verification into brew install (and brew verify) #17019

Open

4 tasks

woodruffw requested review from Bo98, p-linnane and MikeMcQuaid July 13, 2024 18:17

p-linnane approved these changes Jul 13, 2024

View reviewed changes

woodruffw merged commit 6a5bcb3 into master Jul 13, 2024
37 checks passed

woodruffw deleted the ww/attestations-for-dev branch July 13, 2024 19:09

shivammathur mentioned this pull request Jul 13, 2024

All Mac runners failing: No such file or directory shivammathur/setup-php#857

Closed

MikeMcQuaid mentioned this pull request Jul 14, 2024

Widen attestation verification rollout #17716

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

attestations: widen the beta #17692

attestations: widen the beta #17692

woodruffw commented Jul 13, 2024 •

edited

Loading

Bo98 commented Jul 13, 2024 •

edited

Loading

woodruffw commented Jul 13, 2024

Bo98 commented Jul 13, 2024 •

edited

Loading

MikeMcQuaid commented Jul 13, 2024

woodruffw commented Jul 13, 2024

p-linnane left a comment

Bo98 commented Jul 13, 2024 •

edited

Loading

shivammathur commented Jul 13, 2024

p-linnane commented Jul 13, 2024

shivammathur commented Jul 13, 2024

MikeMcQuaid commented Jul 14, 2024

woodruffw commented Jul 14, 2024

Bo98 commented Jul 14, 2024 •

edited

Loading

shivammathur commented Jul 14, 2024

woodruffw commented Jul 14, 2024 •

edited

Loading

MikeMcQuaid commented Jul 14, 2024

Bo98 commented Jul 14, 2024 •

edited

Loading

woodruffw commented Jul 14, 2024

attestations: widen the beta #17692

attestations: widen the beta #17692

Conversation

woodruffw commented Jul 13, 2024 • edited Loading

Bo98 commented Jul 13, 2024 • edited Loading

woodruffw commented Jul 13, 2024

Bo98 commented Jul 13, 2024 • edited Loading

MikeMcQuaid commented Jul 13, 2024

woodruffw commented Jul 13, 2024

p-linnane left a comment

Choose a reason for hiding this comment

Bo98 commented Jul 13, 2024 • edited Loading

shivammathur commented Jul 13, 2024

p-linnane commented Jul 13, 2024

shivammathur commented Jul 13, 2024

MikeMcQuaid commented Jul 14, 2024

woodruffw commented Jul 14, 2024

Bo98 commented Jul 14, 2024 • edited Loading

shivammathur commented Jul 14, 2024

woodruffw commented Jul 14, 2024 • edited Loading

MikeMcQuaid commented Jul 14, 2024

Bo98 commented Jul 14, 2024 • edited Loading

woodruffw commented Jul 14, 2024

woodruffw commented Jul 13, 2024 •

edited

Loading

Bo98 commented Jul 13, 2024 •

edited

Loading

Bo98 commented Jul 13, 2024 •

edited

Loading

Bo98 commented Jul 13, 2024 •

edited

Loading

Bo98 commented Jul 14, 2024 •

edited

Loading

woodruffw commented Jul 14, 2024 •

edited

Loading

Bo98 commented Jul 14, 2024 •

edited

Loading