feat: Adds a new raw file metadata storage for clients #347

kommendorkapten · 2022-07-29T12:17:26Z

This came up during a discussion related to new features for the sigstore TUF client, as there is a desire to make implementations in different languages sharing the target and metadata cache.

Release Notes: Possibility to store metadata files as raw JSON files to allow for interoperability with other TUF implementations.

Types of changes:

[ ] Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of the changes being introduced by the pull request:
This pull request introduces a new implementation of the client's LocalStore interface. This storage stores the metadata files are regular files in a provided directory. This would improve compatibility to share metadata cache between different language implementations.

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

joshuagl

Thanks, this LGTM.

It would be great to get a review from a maintainer who is more fluent in Go than I am.

client/filejsonstore/filejsonstore_test.go

asraa

The change looks good to me as a start, but I wonder a few things and would love feedback:

Do we need a lock mechanism, esp for SetMeta()? If there are more than one client accessing the store.

kommendorkapten · 2022-08-03T15:47:04Z

Yeah, let me add version that's safe for concurrent use!

kommendorkapten · 2022-08-04T06:17:39Z

I added a new wrapper, ConcurrentLocalStore which can be used to protect any LocalStore (e.g. the in memory one and the raw JSON one) for concurrent access.
This would let the client decide what use depending on their access pattern.

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

…ository side. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Changed package to client to align with leveldb storage. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Fixed spelling error found during review. Co-authored-by: Joshua Lock <jlock@vmware.com> Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

…ace. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

client/concurrentlocalstore.go

client/filejsonstore/filejsonstore.go

util/util.go

client/filejsonstore/filejsonstore.go

client/filejsonstore/filejsonstore_test.go

Co-authored-by: Ethan Lowman <53835328+ethan-lowman-dd@users.noreply.github.com> Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Moved IsMetaFile to new pkg, internal/fsutil Permission bits are validated when access the cache. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

kommendorkapten · 2022-08-10T12:12:23Z

Windows tests are failing, I will look into this tomorrow.

kommendorkapten · 2022-08-11T06:55:07Z

The failing tests are due to filesystem permission, which on Windows is very different from UNIX-like operating system.
I found this post on the topic: https://medium.com/@MichalPristas/go-and-file-perms-on-windows-3c944d55dd44
The summary is that in windows you can only create a file with either:

0666 (Read/writeable by all users)
0444 (Readable by all users)

And this seems to be correct according the source code: https://github.com/golang/go/blob/master/src/syscall/syscall_windows.go#L307
The provided permission is only used to determine if a file is intended to be create as read-only.
The same is valid for Mkdir, the permission mode bits are ignored on Windows: https://github.com/golang/go/blob/master/src/syscall/syscall_windows.go#L518
And for reference the Chmod call: https://github.com/golang/go/blob/master/src/syscall/syscall_windows.go#L646 (only the read-only flag is manipulated).

It seems that there are some ACL concept in Windows that we can rely on. But I'm not qualified to speak if the normal permission modes set (all-read or all-write) actually are system-wide or if the effective permission applies to the user only. I found this ACL implementation in go: https://github.com/hectane/go-acl
It haven't had any updates in three years, and I would not be comfortable relying on this, as I lack understanding on Windows filesystem access semantics.

As this feature of verifying the filesystem permission bits, is new for this implementation (LevelDB implementation does not care), nor is there any guidance for implementors of LocalStore to consider this, my suggestion is to skip file permissions for Windows.

Please advise on how to proceed.

client/filejsonstore/filejsonstore.go

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

kommendorkapten · 2022-08-23T14:21:41Z

@trishankatdatadog Of course! Created this: #360

internal/fsutil/perm.go

internal/fsutil/fsutil.go

internal/fsutil/perm_windows.go

internal/fsutil/perm.go

client/filejsonstore/filejsonstore.go

Signed-off-by: Ethan Lowman <ethan.lowman@datadoghq.com>

Co-authored-by: Ethan Lowman <53835328+ethan-lowman-dd@users.noreply.github.com> Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Mostly minor things like removing sentinel errors, wrapping errors from std library. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

kommendorkapten · 2022-08-24T06:31:55Z

Good feedback @ethan-lowman-dd, all your comments should be addressed now 👍

Signed-off-by: Ethan Lowman <ethan.lowman@datadoghq.com>

ethan-lowman-dd · 2022-08-24T17:49:53Z

@asraa or @joshuagl Could you please provide a followup review when you get the chance?

trishankatdatadog · 2022-08-24T18:15:17Z

Good feedback @ethan-lowman-dd, all your comments should be addressed now 👍

Nice work, thanks Ethan

joshuagl

Lovely work here @kommendorkapten and @ethan-lowman-dd , thank you!

trishankatdatadog · 2022-09-06T15:34:20Z

@kommendorkapten would you please rebase and resolve conflicts? thx!

kommendorkapten · 2022-09-12T11:29:30Z

Was off on PTO, so didn't see you message @trishankatdatadog until now. PR is updated now.

trishankatdatadog · 2022-09-12T17:23:49Z

Was off on PTO, so didn't see you message @trishankatdatadog until now. PR is updated now.

Thanks! Would you pls fix the linting errors?

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

kommendorkapten · 2022-09-12T17:38:59Z

Thanks! Would you pls fix the linting errors?

Sorry, was not aware of them, fixed now.

kommendorkapten changed the title ~~Adds a new raw file metadata storage for clients~~ Feat: Adds a new raw file metadata storage for clients Aug 1, 2022

kommendorkapten changed the title ~~Feat: Adds a new raw file metadata storage for clients~~ feat: Adds a new raw file metadata storage for clients Aug 1, 2022

kommendorkapten marked this pull request as ready for review August 2, 2022 14:27

joshuagl previously approved these changes Aug 3, 2022

View reviewed changes

client/filejsonstore/filejsonstore_test.go Outdated Show resolved Hide resolved

kommendorkapten dismissed joshuagl’s stale review via b7eb7bd August 3, 2022 11:43

asraa reviewed Aug 3, 2022

View reviewed changes

joshuagl requested a review from ethan-lowman-dd August 3, 2022 15:55

kommendorkapten and others added 9 commits August 4, 2022 08:21

Added first draft for a raw json local file storage.

dc9d46c

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Ignore emacs temporary files

c41bd2d

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

moved isMetaFile to util directory for reuse in other packages.

9fdf332

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Added unit tests and refactored code to match the local store for rep…

1acc7bf

…ository side. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Added test case for non json file in metadata directory.

c527e56

Changed package to client to align with leveldb storage. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Use os.MkdirAll when creating metadata cache.

b7dbd08

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

More consistent naming, added comments and a unit test.

2dc5d0e

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Added a localstore wrapper for concurrent access.

ad4974b

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Update client/filejsonstore/filejsonstore_test.go

61c7323

Fixed spelling error found during review. Co-authored-by: Joshua Lock <jlock@vmware.com> Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

kommendorkapten force-pushed the md_raw_json branch from 4dfab89 to 61c7323 Compare August 4, 2022 06:22

Added tests to make sure returned struct implements LocalStore interf…

4714ff1

…ace. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

ethan-lowman-dd reviewed Aug 9, 2022

View reviewed changes

kommendorkapten and others added 5 commits August 9, 2022 17:54

Update client/filejsonstore/filejsonstore.go

4048848

Co-authored-by: Ethan Lowman <53835328+ethan-lowman-dd@users.noreply.github.com> Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Update client/filejsonstore/filejsonstore_test.go

1d074f8

Co-authored-by: Ethan Lowman <53835328+ethan-lowman-dd@users.noreply.github.com> Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Update client/filejsonstore/filejsonstore.go

18f7ce7

Co-authored-by: Ethan Lowman <53835328+ethan-lowman-dd@users.noreply.github.com> Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Update client/filejsonstore/filejsonstore_test.go

800da60

Co-authored-by: Ethan Lowman <53835328+ethan-lowman-dd@users.noreply.github.com> Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Made FileJSONStore safe for concurrent access.

0627a27

Moved IsMetaFile to new pkg, internal/fsutil Permission bits are validated when access the cache. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

patflynn reviewed Aug 11, 2022

View reviewed changes

client/filejsonstore/filejsonstore.go Outdated Show resolved Hide resolved

Spelling error.

83c0eab

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

kommendorkapten mentioned this pull request Aug 23, 2022

Add support for verifying file and directory permission on Windows #360

Closed

ethan-lowman-dd reviewed Aug 23, 2022

View reviewed changes

Clarify permissions check naming and comment, add tests

39f4018

Signed-off-by: Ethan Lowman <ethan.lowman@datadoghq.com>

ethan-lowman-dd force-pushed the md_raw_json branch from be33028 to 39f4018 Compare August 23, 2022 22:12

kommendorkapten and others added 2 commits August 24, 2022 07:40

Update internal/fsutil/fsutil.go

01bbd76

Co-authored-by: Ethan Lowman <53835328+ethan-lowman-dd@users.noreply.github.com> Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

Updates based on feeback.

19ef1ac

Mostly minor things like removing sentinel errors, wrapping errors from std library. Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

ethan-lowman-dd added 3 commits August 24, 2022 12:43

Use fs.ErrNotExist instead of os.ErrNotExist as recommended by Go docs

db6ad66

Signed-off-by: Ethan Lowman <ethan.lowman@datadoghq.com>

Clean up remaining unnecessary usage of filepath.FromSlash

ff675c6

Signed-off-by: Ethan Lowman <ethan.lowman@datadoghq.com>

Add missing error checks in tests

9ca46f4

Signed-off-by: Ethan Lowman <ethan.lowman@datadoghq.com>

ethan-lowman-dd previously approved these changes Aug 24, 2022

View reviewed changes

joshuagl previously approved these changes Sep 6, 2022

View reviewed changes

Merge branch 'master' into md_raw_json

86e5085

Merge branch 'master' into md_raw_json

94cf072

Make sure to test that returned err is nil in the tests.

210b417

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>

kommendorkapten dismissed stale reviews from joshuagl and ethan-lowman-dd via 210b417 September 12, 2022 17:38

trishankatdatadog requested review from joshuagl and ethan-lowman-dd September 12, 2022 17:53

ethan-lowman-dd approved these changes Sep 12, 2022

View reviewed changes

trishankatdatadog approved these changes Sep 13, 2022

View reviewed changes

trishankatdatadog merged commit 7097fd8 into theupdateframework:master Sep 13, 2022

kommendorkapten deleted the md_raw_json branch September 14, 2022 05:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adds a new raw file metadata storage for clients #347

feat: Adds a new raw file metadata storage for clients #347

kommendorkapten commented Jul 29, 2022

joshuagl left a comment

asraa left a comment

kommendorkapten commented Aug 3, 2022

kommendorkapten commented Aug 4, 2022 •

edited

Loading

kommendorkapten commented Aug 10, 2022

kommendorkapten commented Aug 11, 2022 •

edited

Loading

kommendorkapten commented Aug 23, 2022 •

edited

Loading

kommendorkapten commented Aug 24, 2022

ethan-lowman-dd commented Aug 24, 2022

trishankatdatadog commented Aug 24, 2022

joshuagl left a comment

trishankatdatadog commented Sep 6, 2022

kommendorkapten commented Sep 12, 2022

trishankatdatadog commented Sep 12, 2022

kommendorkapten commented Sep 12, 2022

feat: Adds a new raw file metadata storage for clients #347

feat: Adds a new raw file metadata storage for clients #347

Conversation

kommendorkapten commented Jul 29, 2022

joshuagl left a comment

Choose a reason for hiding this comment

asraa left a comment

Choose a reason for hiding this comment

kommendorkapten commented Aug 3, 2022

kommendorkapten commented Aug 4, 2022 • edited Loading

kommendorkapten commented Aug 10, 2022

kommendorkapten commented Aug 11, 2022 • edited Loading

kommendorkapten commented Aug 23, 2022 • edited Loading

kommendorkapten commented Aug 24, 2022

ethan-lowman-dd commented Aug 24, 2022

trishankatdatadog commented Aug 24, 2022

joshuagl left a comment

Choose a reason for hiding this comment

trishankatdatadog commented Sep 6, 2022

kommendorkapten commented Sep 12, 2022

trishankatdatadog commented Sep 12, 2022

kommendorkapten commented Sep 12, 2022

kommendorkapten commented Aug 4, 2022 •

edited

Loading

kommendorkapten commented Aug 11, 2022 •

edited

Loading

kommendorkapten commented Aug 23, 2022 •

edited

Loading