add filestatus command #545

endorama · 2019-10-08T19:06:14Z

Closes #460

Add filestatus command, reporting if the file is in encrypted or unencrypted state.

I reused ensureNoMetadata logic, thus the command would return 0 when the file is not encrypted and 203 when the file is encrypted (respecting the codes.FileAlreadyEncrypted error code)

It does not output the error message as returned by ensureNoMetadata, preferring a simpler output: "File is encrypted" or "File is unencrypted".

As I'm not so fond on sops internals, I'm sure there are multiple things to review, I'll gladly update the code to reflect any suggestion.

cmd/sops/main.go

endorama · 2019-10-08T19:07:01Z

cmd/sops/main.go

+					return common.NewExitError("File is encrypted", codes.FileAlreadyEncrypted)
+				}
+
+				fmt.Println("File is unencrypted")


Is fmt the proper way to output such an informative message?

Looks fine to me. I'm not sure whether we should use a machine-parseable and extensible format (probably JSON), though. It would let us put more status information there in the future, e.g. whether the MAC is okay.

Would something like {"encrypted": true|false} be ok?

Looks good to me.

cmd/sops/main.go

autrilla

Looks pretty good :) just a few comments.

cmd/sops/main.go

autrilla · 2019-10-08T19:38:00Z

cmd/sops/main.go

+					return common.NewExitError(fmt.Sprintf("Error unmarshalling file: %s", err), codes.CouldNotReadInputFile)
+				}
+				if err := ensureNoMetadata(opts, branches[0]); err != nil {
+					return common.NewExitError("File is encrypted", codes.FileAlreadyEncrypted)


Hmmm, I'm wondering whether we should return an error when the file is not encrypted, or the other way around. What was your reasoning for choosing this over the alternative?

Mainly because there was an error code for "file is already encrypted" and not the other way round. So I would expect users to be already used to such an error.

IMO none is an error, and using JSON output like you requested in a comment above everything makes more sense and this error can probably be removed.

Yeah, I agree. Let's make none of them be an error for now.

cmd/sops/subcommand/filestatus/filestatus.go

codecov-io · 2019-10-10T22:42:35Z

Codecov Report

Merging #545 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #545   +/-   ##
========================================
  Coverage    36.46%   36.46%           
========================================
  Files           20       20           
  Lines         2863     2863           
========================================
  Hits          1044     1044           
  Misses        1725     1725           
  Partials        94       94

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 4b5b7ae...0fba7e1. Read the comment docs.

cmd/sops/subcommand/filestatus/filestatus.go

autrilla · 2019-10-11T16:09:40Z

cmd/sops/subcommand/filestatus/filestatus.go

+	// OutputStore       sops.Store
+	InputPath string
+	// KeyServices       []keyservice.KeyServiceClient
+	// KeyGroups         []sops.KeyGroup
+	// GroupThreshold    int


Remove the commented lines since they're not used

Removed in commit 80686ab

cmd/sops/subcommand/filestatus/filestatus.go

autrilla · 2019-10-11T16:16:18Z

cmd/sops/subcommand/filestatus/filestatus.go

+
+// Status rapresent file status
+type Status struct {
+	Encrypted bool


// Encrypted represents whether the file provided is encrypted by SOPS

Resolved in 80686ab

jvehent · 2019-10-11T19:03:07Z

cmd/sops/subcommand/filestatus/filestatus.go

+func checkMetadata(branch sops.TreeBranch) bool {
+	for _, b := range branch {
+		if b.Key == "sops" {
+			return true


isn't that a bit weak as a test? I'd suggest at least verifying that a mac is set and one key exists under the various sections.

It's indeed a bit weak. It's what we do elsewhere as an easy test to check whether the file is encrypted, but maybe as you say we should have a stricter test here.

May I propose to go with this version for consistency, and work on a more complex test (that we can discuss more) on a separate one? I'm willing to work on that.
We could also extend a bit the scope and extract the check so both places can use the same code.

cmd/sops/subcommand/filestatus/filestatus.go

kowalczykp · 2020-03-15T09:50:00Z

Any update on that? It would be a really nice feature

mazdack · 2020-08-03T09:27:25Z

any update? would be really nice to have this command

autrilla · 2020-08-03T13:32:23Z

No updates. The review comments haven't been addressed in this PR, plus there's merge conflicts. We'd be happy to review a PR that builds on this one an handles the review comments.

endorama · 2020-08-10T14:41:58Z

@autrilla May you let me know your opinion on #545 (comment)?

I was waiting on a reply there, other concerns should have been addressed as requested.

autrilla · 2020-08-10T15:19:22Z

@autrilla May you let me know your opinion on #545 (comment)?

It wouldn't take a lot of work to have a slightly better check by doing what jvehent suggests, e.g. ensuring there's a mac field. Bonus points for reusing this better check in the place where we already check if a file is encrypted :)

codecov-commenter · 2020-08-11T10:08:21Z

Codecov Report

Merging #545 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #545   +/-   ##
========================================
  Coverage    36.44%   36.44%           
========================================
  Files           22       22           
  Lines         3205     3205           
========================================
  Hits          1168     1168           
  Misses        1918     1918           
  Partials       119      119

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 4bd640e...ff0888f. Read the comment docs.

endorama · 2020-08-11T10:58:04Z

@autrilla I resolved conflicts and rebased onto latest develop so this should be now safe to merge again.

I started working on how to improve the check and I found out that using an implementation of EncryptedFileLoader.LoadEncryptedFile(in []byte) (Tree, error) on a non-encrypted file results in a MetadataNotFound error. Would this be ok as a deeper test? It feels "right" from my point of view. Is it enough or once metadata are found should it perform some other check?

autrilla · 2020-09-02T17:13:55Z

@autrilla I resolved conflicts and rebased onto latest develop so this should be now safe to merge again.

I started working on how to improve the check and I found out that using an implementation of EncryptedFileLoader.LoadEncryptedFile(in []byte) (Tree, error) on a non-encrypted file results in a MetadataNotFound error. Would this be ok as a deeper test? It feels "right" from my point of view. Is it enough or once metadata are found should it perform some other check?

Trying to load the file sounds good to me.

onedr0p · 2020-11-19T16:27:51Z

@autrilla any status here, or are you waiting on the author to rebase this PR again? It seems like he addressed the issues before but now there's conflicts again. What's the hold up?

onedr0p · 2021-01-19T16:15:35Z

Bump ❤️

autrilla · 2021-01-19T16:18:16Z

Not all the issues are addressed. In particular, I asked for a better test that the file is encrypted. It's also missing documentation and tests, and as you mention, there's conflicts.

endorama · 2021-03-19T23:30:47Z

Hello,

I have redone the implementation.
The check is performed in the function cfs and:

tries to load the encrypted file
if it fails, report as not encrypted
if encryption works, checks for version and mac keys to be present

filestatus subcommand now output JSON as requested ({"encrypted": true|false}).

I rebased onto master, so I'm not sure why it's still complaining about the conflict.

@autrilla is this implementation ok? I also added some tests for it. The cfs function is private in the filestatus package, let me know where is the most appropriate place to put it into so it can be reused in other areas as well.

Thanks and sorry for the long wait!

Pehesi97 · 2023-05-01T14:17:08Z

How is the progress in this PR? Do you need any help?

aDingil · 2023-05-12T05:04:05Z

@ajvb

ggordan · 2023-06-05T10:52:33Z

Hi @ajvb @autrilla @jvehent, I see that roughly a year ago the milestone for this issue was changed to v3.8.0.

Would that suggest that this PR is "complete" meaning that when v3.8.0 is released, this will be included, or are there still things that need to be addressed for the maintainers to be happy to merge this? If the answer is no, can we get a indication of what is needed, I'd be more than happy to help out.

Subsequently, is there a rough estimate on when v3.8.0 is expected to land?

Thanks,
Gordan

felixfontein · 2023-12-17T15:29:01Z

@endorama would you mind rebasing another time and signing off your commits? Would be great if we could finally get this merged :)

davinkevin · 2024-01-21T18:26:20Z

Can't wait to use it!

Signed-off-by: Edoardo Tenani <[email protected]>

endorama · 2024-06-23T14:16:19Z

Hello everybody and sorry for the long wait! I'd love to see this finally being merge and I have some time to dedicate to it.

Rebasing the old work was quite complex, so given the overall addition in this PR was small I re-implemented it on top of the latest master branch. I kept the implementation the same with the same tests.

(The second force push is due to the DCO, which I didn't sign the first time)

Waiting on the review ❤

felixfontein · 2024-06-23T15:06:34Z

I did some extensive tests, and it looks good! The method of detecting encrypted files is pretty good; obviously you can fake it with fake values, like the follwing YAML file:

sops:
    lastmodified: "2020-01-01T00:00:00Z"
    mac: x
    pgp:
        - created_at: "2020-01-01T00:00:00Z"
          enc: |-
            x
          fp: x
    version: x

(and similar variants for other storages), but I think that's acceptable, and it also seems to agree to what all reviewers so far asked for.

If nobody objects (especially from @getsops/maintainers side), I propose to merge this soon!

sabre1041

LGTM

felixfontein · 2024-06-24T05:31:35Z

@endorama thanks a lot for contributing this!

Also thanks a lot to everyone who reviewed this and commented on this over all these years! I'm glad this finally made it :)

endorama commented Oct 8, 2019

View reviewed changes

cmd/sops/main.go Outdated Show resolved Hide resolved

endorama commented Oct 8, 2019

View reviewed changes

cmd/sops/main.go Outdated Show resolved Hide resolved

autrilla reviewed Oct 8, 2019

View reviewed changes

endorama commented Oct 10, 2019

View reviewed changes

cmd/sops/subcommand/filestatus/filestatus.go Outdated Show resolved Hide resolved

autrilla reviewed Oct 11, 2019

View reviewed changes

cmd/sops/subcommand/filestatus/filestatus.go Outdated Show resolved Hide resolved

autrilla reviewed Oct 11, 2019

View reviewed changes

cmd/sops/subcommand/filestatus/filestatus.go Outdated Show resolved Hide resolved

autrilla reviewed Oct 11, 2019

View reviewed changes

jvehent reviewed Oct 11, 2019

View reviewed changes

ajvb reviewed Oct 11, 2019

View reviewed changes

cmd/sops/subcommand/filestatus/filestatus.go Show resolved Hide resolved

endorama force-pushed the add-filestatus-command branch 2 times, most recently from 0fba7e1 to 80686ab Compare October 18, 2019 16:08

autrilla mentioned this pull request Nov 14, 2019

Q: How to prevent unencrypted files from being committed #571

Open

endorama force-pushed the add-filestatus-command branch 2 times, most recently from 3568c88 to ff0888f Compare August 11, 2020 10:02

endorama force-pushed the add-filestatus-command branch from ff0888f to d89029a Compare March 19, 2021 23:26

ajvb self-requested a review March 24, 2021 20:58

ajvb added this to the v3.7.3 milestone Mar 3, 2022

ajvb modified the milestones: v3.7.3, v3.8.0 Apr 4, 2022

thewilli mentioned this pull request Jun 2, 2022

Allow sops updatekeys to work on a directory tree #1071

Open

onedr0p mentioned this pull request Nov 22, 2022

false positive? onedr0p/sops-pre-commit#24

Open

cesarjorgemartinez mentioned this pull request Sep 8, 2023

implement a sops --verify #437

Open

felixfontein mentioned this pull request Jun 16, 2024

vars plugin: make valid_extensions configurable ansible-collections/community.sops#186

Merged

endorama force-pushed the add-filestatus-command branch from e681ee7 to 0f71653 Compare June 23, 2024 14:07

endorama added 2 commits June 23, 2024 16:08

add filestatus subcommand

b8e65f9

Signed-off-by: Edoardo Tenani <[email protected]>

hook subcommand in main

59e2ece

Signed-off-by: Edoardo Tenani <[email protected]>

endorama force-pushed the add-filestatus-command branch from 0f71653 to 59e2ece Compare June 23, 2024 14:09

felixfontein approved these changes Jun 23, 2024

View reviewed changes

sabre1041 approved these changes Jun 24, 2024

View reviewed changes

felixfontein merged commit 15d30ed into getsops:main Jun 24, 2024
9 checks passed

add filestatus command #545

add filestatus command #545

Conversation

endorama commented Oct 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autrilla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endorama Oct 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 10, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endorama Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

kowalczykp commented Mar 15, 2020

mazdack commented Aug 3, 2020

autrilla commented Aug 3, 2020

endorama commented Aug 10, 2020

autrilla commented Aug 10, 2020 • edited Loading

codecov-commenter commented Aug 11, 2020

Codecov Report

endorama commented Aug 11, 2020 • edited Loading

autrilla commented Sep 2, 2020

onedr0p commented Nov 19, 2020 • edited Loading

onedr0p commented Jan 19, 2021 • edited Loading

autrilla commented Jan 19, 2021

endorama commented Mar 19, 2021 • edited Loading

Pehesi97 commented May 1, 2023

aDingil commented May 12, 2023 • edited Loading

ggordan commented Jun 5, 2023

felixfontein commented Dec 17, 2023

davinkevin commented Jan 21, 2024

endorama commented Jun 23, 2024

felixfontein commented Jun 23, 2024

sabre1041 left a comment

Choose a reason for hiding this comment

felixfontein commented Jun 24, 2024

endorama Oct 10, 2019 •

edited

Loading

codecov-io commented Oct 10, 2019 •

edited

Loading

endorama Oct 28, 2019 •

edited

Loading

autrilla commented Aug 10, 2020 •

edited

Loading

endorama commented Aug 11, 2020 •

edited

Loading

onedr0p commented Nov 19, 2020 •

edited

Loading

onedr0p commented Jan 19, 2021 •

edited

Loading

endorama commented Mar 19, 2021 •

edited

Loading

aDingil commented May 12, 2023 •

edited

Loading