Conversation
This will help limit the number of times we need to query GitHub for the attestation in CI.
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Makes sense to me! Does make me wonder whether this caching could/should be in Homebrew/brew, though?
That's where we're making this change! |
I cannot read today 🙈 apologies! |
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Confirmed this still looks good now I know what repository I'm in today 🙃
|
|
||
| sig { params(pathname: Pathname, scrub: T::Boolean).returns(T::Boolean) } | ||
| def stale_attestation?(pathname, scrub) | ||
| scrub || prune?(pathname, ATTESTATION_CLEANUP_DAYS) |
There was a problem hiding this comment.
Might be nice if this could be cleaned up based on the state of the cached bottles? Not a blocker, very much nice-to-have.
Co-authored-by: Carlo Cabrera <github@carlo.cab>
|
|
||
| if cached_attestation.exist? | ||
| begin | ||
| return JSON.parse(cached_attestation.read) |
There was a problem hiding this comment.
Just to make sure I understand: the intended behavior here is to treat a cached attestation as implicitly previously verified, right?
I think we could go two ways with this: either assume cached == verified (which in turn means an attacker who can modify the cache/insert a dummy cache member can bypass verification, although such an attacker is already pretty powerful), or perform verification again on the cached attestation (so skip the download, but do the rest of the gh attestation verify).
The first seems fine to me, but the second is perhaps preferable.
There was a problem hiding this comment.
The first seems fine to me, but the second is perhaps preferable.
Agreed 👍🏻
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
woodruffw
added
in progress
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
3 participants
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?This will help limit the number of times we need to query GitHub for the
attestation in CI.