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

Lock TUF client during target loading operations #1992

Closed
wants to merge 1 commit into from

Conversation

puerco
Copy link
Member

@puerco puerco commented Jun 14, 2022

Summary

While #1953 resolved access to the TUF cache when running cosign in parallel, cosign now panics when running concurrent operations as threads will try to write read data to the TUF client at the same time.

Here's a sample of the crash:

Successfully verified SCT...
Successfully verified SCT...
Successfully verified SCT...
fatal error: concurrent map writes

goroutine 881 [running]:
runtime.throw({0x278fcc7?, 0xc000d5b290?})
	/usr/lib/golang/src/runtime/panic.go:992 +0x71 fp=0xc000b34680 sp=0xc000b34650 pc=0x43a271
runtime.mapassign_faststr(0x2330a00?, 0xc000f1a000?, {0x277a539, 0xc})
	/usr/lib/golang/src/runtime/map_faststr.go:212 +0x39c fp=0xc000b346e8 sp=0xc000b34680 pc=0x4151dc
github.com/theupdateframework/go-tuf/client.(*Client).getLocalMeta(0xc000202000)
	/home/urbano/go/pkg/mod/github.com/theupdateframework/go-tuf@v0.3.0/client/client.go:457 +0x518 fp=0xc000b347e0 sp=0xc000b346e8 pc=0xbf8698
github.com/theupdateframework/go-tuf/client.(*Client).loadLocalSnapshot(0xc000202000)
	/home/urbano/go/pkg/mod/github.com/theupdateframework/go-tuf@v0.3.0/client/delegations.go:65 +0x33 fp=0xc000b34878 sp=0xc000b347e0 pc=0xbfc5f3
github.com/theupdateframework/go-tuf/client.(*Client).getTargetFileMeta(0xc000202000, {0xc000a97921, 0x8})

This PR embeds a mutex to the TUF struct to allow it to lock itself when loading data using the tuf client. This resolves the above panic. Now, operations to reload the targets from the cache into the client now queue properly.

cc: @asraa @znewman01 @mnm678

Signed-off-by: Adolfo García Veytia (Puerco) puerco@chainguard.dev

Ticket Link

Follow-up to: #1953

Release Note

Fixed a bug where `cosign` would panic when trying to reload TUF client data concurrently

This commit adds adds a mutex to the tuf client to lock itself when
loading data. This resolves a panic where multiple operations would
cause cosign to crash when signing in parallel.

Signed-off-by: Adolfo García Veytia (Puerco) <puerco@chainguard.dev>
@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2022

Codecov Report

Merging #1992 (5c0abb0) into main (b01a173) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1992      +/-   ##
==========================================
+ Coverage   28.69%   28.73%   +0.03%     
==========================================
  Files         133      133              
  Lines        8092     8096       +4     
==========================================
+ Hits         2322     2326       +4     
  Misses       5463     5463              
  Partials      307      307              
Impacted Files Coverage Δ
pkg/cosign/tuf/client.go 64.42% <100.00%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b01a173...5c0abb0. Read the comment docs.

@imjasonh
Copy link
Member

imjasonh commented Jun 14, 2022

This package was moved to pkg/tuf in sigstore/sigstore and will be removed from cosign soon. Can you make this change there instead?

See #1866

@puerco
Copy link
Member Author

puerco commented Jun 14, 2022

Thanks @imjasonh I've ported the change here: sigstore/sigstore#503

Would you mind ensuring that #1866 pulls that commit if it merges before? 🙏

Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thank you so much!

@dlorenc
Copy link
Member

dlorenc commented Jun 14, 2022

Closing this one in favor of sigstore/sigstore#503

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.

5 participants