search

securityclippy / magicstorage

storage backends for certmagic
7 stars 8 forks source link

Locking implementation does not appear to be atomic #3

Open mholt opened 5 years ago

mholt commented 5 years ago

Thanks for making this implementation for CertMagic!

It came up in the Gophers Slack today that this implementation may not be atomic; specifically the existence check and the creation are racey between multiple instances: https://github.com/securityclippy/magicstorage/blob/0c0f14ceed9fbbde73feb831d1fb49748f45f7ad/s3storage.go#L246-L254

I'm not sure that S3 provides atomic operations at all; however, maybe some use of DynamoDB could be made to provide proper locking?

I was pointed to Terraform, which uses S3 for storage - I have not vetted its implementation but just wanted to include the link for reference, in case it's helpful: https://github.com/hashicorp/terraform/tree/master/backend/remote-state/s3

OrkhanAlikhanov commented 14 hours ago

IfMatch and If-None-Match headers can be used to create/update lockfile atomically. Most servers like MinIO, GCS, Azure, AWS S3, IBM COS etc. support it. Seaweedfs does not seem to support it yet.

mholt commented 12 hours ago

@OrkhanAlikhanov Ah, that's interesting, and worth exploring!