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

feat: Add Filesystem based remote store to support airgap. #397

Merged
merged 6 commits into from
Sep 30, 2022

Conversation

vaikas
Copy link
Contributor

@vaikas vaikas commented Sep 24, 2022

Signed-off-by: Ville Aikas vaikas@chainguard.dev

Please fill in the fields below to submit a pull request. The more information that is provided, the better.

Fixes #
Release Notes:

  • Add file_store that implements RemoteStore backed by https://pkg.go.dev/io/fs. This is handy if you want to run airgap and bring your own repo.

Types of changes:

  • [ x ] New feature (non-breaking change which adds functionality)

Description of the changes being introduced by the pull request:

Add Filesystem based remote store that the client can use. Motivation is to support airgapped environments which can't make network calls. The interface is golang io/fs.FS so that it can be backed by anything that supports the interface instead of only relying on disk.

I may have missed something, but I can't find a way to use it for airgap environments as is, since initializing requires network calls.

Please verify and check that the pull request fulfills the following requirements:

  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@vaikas vaikas changed the title Add Filesystem based remote store to support airgap. feat: Add Filesystem based remote store to support airgap. Sep 24, 2022
vaikas added a commit to vaikas/cosign that referenced this pull request Sep 24, 2022
@asraa asraa self-requested a review September 26, 2022 11:07
Copy link
Contributor

@znewman01 znewman01 left a comment

Choose a reason for hiding this comment

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

You've pointed out a few things you need to fix, otherwise looking good

client/file_store_test.go Outdated Show resolved Hide resolved
@vaikas
Copy link
Contributor Author

vaikas commented Sep 26, 2022

@znewman01 @asraa please take particular look at this commit. Based on this here:
https://github.com/theupdateframework/go-tuf/blob/master/client/client.go#L843

I believe returning an error is what should be happening, so modified things accordingly.

@vaikas vaikas marked this pull request as ready for review September 26, 2022 19:39
@vaikas
Copy link
Contributor Author

vaikas commented Sep 26, 2022

Ho hum:
golang/go#51442

--- FAIL: TestBadDirs (1.51s)
    testing.go:1090: TempDir RemoveAll cleanup: remove C:\Users\RUNNER~1\AppData\Local\Temp\TestBadDirs1564658038\001\repository-that-isfile: The process cannot access the file because it is being used by another process.
FAIL
coverage: 83.3% of statements

client/client_test.go Show resolved Hide resolved
client/client_test.go Outdated Show resolved Hide resolved
client/file_store_test.go Show resolved Hide resolved
vaikas added a commit to vaikas/cosign that referenced this pull request Sep 27, 2022
@vaikas
Copy link
Contributor Author

vaikas commented Sep 27, 2022

lol, pasted to wrong issue, sorry, my bad:

FWIW, this has been wired in through sigstore here:
sigstore/sigstore#715
And
cosign here:
https://github.com/vaikas/cosign/tree/air-gap

And finally in scaffolding I use a local filesystem based custom TUF root here:
sigstore/scaffolding#382

https://github.com/sigstore/scaffolding/actions/runs/3139401394/jobs/5099778521
If you look at the 'Untar the repository from the fetched secret' section, and it's using a cosign built from here:
https://github.com/vaikas/cosign/tree/air-gap

@joshuagl
Copy link
Member

golang/go#51442

The linked issue implies this is fixed in go 1.19, but that's the version in use on the failing tests? 😕

Should we implement the suggested workaround in golang/go#51442 (comment)

There is also a workaround that you can apply for your own specific tests independent of the Go version: you can defer (or t.Cleanup) your own function that iterates os.RemoveAll on the directory returned by TempDir until it is successfully removed.

@vaikas
Copy link
Contributor Author

vaikas commented Sep 28, 2022

Thanks @joshuagl I added a loop through the files we create.

@JustinCappos
Copy link
Member

How will metadata expiration be handled in the air gapped case? Is timestamp expiration a problem? Do you plan to just generate very long lifetimes for metadata files?

@trishankatdatadog
Copy link
Member

How will metadata expiration be handled in the air gapped case? Is timestamp expiration a problem? Do you plan to just generate very long lifetimes for metadata files?

See the same issue and some ideas about it here

znewman01
znewman01 previously approved these changes Sep 29, 2022
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
znewman01
znewman01 previously approved these changes Sep 29, 2022
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
Copy link
Member

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

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

This is a cool feature, thanks for working on it @vaikas !

dabde9f makes me a little sad, but I can understand why you made that choice. I'll file an issue to try and resolve this in future.

@joshuagl joshuagl merged commit 3890c1e into theupdateframework:master Sep 30, 2022
@vaikas
Copy link
Contributor Author

vaikas commented Sep 30, 2022

Thank you @joshuagl!!!

@vaikas vaikas deleted the air-gap branch September 30, 2022 14:54
vaikas added a commit to vaikas/cosign that referenced this pull request Oct 1, 2022
znewman01 pushed a commit to znewman01/go-tuf that referenced this pull request May 22, 2023
…framework#397)

* Add Filesystem based remote store to support airgap.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* Try with T.Cleanup too. Wonder if this goes to 11 at some point?

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* close the file.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* t.Cleanup vs. defer.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* omit one initializing test for windows.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
Co-authored-by: Joshua Lock <jlock@vmware.com>
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.

6 participants