refactor: break --insecure into separate flags

[joonas]

Description

Attempting to break up the --insecure global flag into four separate flags:

• --plain-http
• --insecure-skip-tls-verify
• --skip-signature-validation

Related Issue

Fixes #2860

Relates to #

Checklist before merging

• Test, docs, adr added or updated as needed
• Contributor Guide Steps followed

[netlify]

✅ Deploy Preview for zarf-docs canceled.

Name	Link
🔨 Latest commit	868c7c0
🔍 Latest deploy log	https://app.netlify.com/sites/zarf-docs/deploys/66e2fce0e2bc7d0008ec16d7

[AustinAbro321]

Great question, unfortunately the insecure flag is pretty overloaded. I think it'll be better to just create two more flags to replace it's behavior

For this block, add a flag called --insecure-no-shasum to just the zarf package pull command zarf/src/pkg/packager/sources/url.go

For this block, add a flag called --insecure-no-signature-validation to the zarf package command zarf/src/pkg/packager/sources/validate.go

You can add these new fields to the types.ZarfPackageOptions struct

[AustinAbro321]

Great start, left some feedback. Could you also replace the uses of --insecure in the test suite. Leave one use of --insecure for each new flag so we can test that the flag still works though deprecated.

[joonas]

@AustinAbro321 appreciate all the feedback, I tried to make sure I addressed it diligently, but please let me know if I missed the mark somewhere.

I haven't yet had a chance to revisit the use of --insecure in the test suite, but on a cursory look across the tests that contain the flag I noticed there's probably a combination of the above flag changes should be applied.

My plan for how I was going to address the tests is as follows:

1. Where --insecure is being replaced with --insecure-skip-tls-verify and --plain-http, replace the --insecure flag with both of those in each case
2. Where --insecure is being replaced with --insecure-no-signature-validation, apply that across the impacted examples
3. Where --insecure is being replaced with --insecure-no-shasum, apply that across the impacted examples

Does that roughly line up with what you had in mind?

[phillebaba]

Some comments about this change. First of all we should stop adding things to the whole lang package as it is something that should be removed. Second of all I don't see the point of adding an "insecure" prefix to all of these flags. I am fine with --insecure-skip-tls-verify and --plain-http as they are common in other projects. The other two flags should be called something else or not exist at all. I suggest renaming --insecure-no-signature-validation to --skip-signature-validation.

I'm confused on the point of being able to disable digest verification while pulling? If the digests do not match something has gone very wrong. It also result in confusion for users in the future who assume that a package pulled has the expected digest.

[AustinAbro321]

Yeah that looks like a solid strategy for changing the flag in tests @joonas

Good point @phillebaba, I doubt anyone is relying on the insecure feature for zarf package pull and if they are they shouldn't be. I also like --skip-signature-validation better

[joonas]

I believe I've now addressed all of test cases and instances of where the flags were used 🙂

[phillebaba]

Looks good I have a small thing that we need to verify if it is an issue or not before merging.

[codecov]

Codecov Report

Attention: Patch coverage is 11.47541% with 54 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/cmd/package.go	0.00%	20 Missing ⚠️
src/pkg/packager/sources/oci.go	0.00%	9 Missing ⚠️
src/pkg/packager/creator/normal.go	0.00%	8 Missing ⚠️
src/pkg/packager/sources/tarball.go	22.22%	5 Missing and 2 partials ⚠️
src/cmd/root.go	0.00%	6 Missing ⚠️
src/internal/packager/images/common.go	0.00%	1 Missing and 1 partial ⚠️
src/cmd/initialize.go	0.00%	1 Missing ⚠️
src/internal/packager/helm/repo.go	0.00%	1 Missing ⚠️

Files with missing lines	Coverage Δ
src/cmd/common/viper.go	0.00% <ø> (ø)
src/internal/packager/helm/chart.go	14.41% <100.00%> (ø)
src/pkg/packager/sources/url.go	51.35% <100.00%> (ø)
src/pkg/packager/sources/validate.go	43.90% <ø> (+1.04%)	⬆️
src/pkg/zoci/common.go	62.50% <100.00%> (ø)
src/cmd/initialize.go	0.00% <0.00%> (ø)
src/internal/packager/helm/repo.go	28.08% <0.00%> (ø)
src/internal/packager/images/common.go	41.17% <0.00%> (ø)
src/cmd/root.go	0.00% <0.00%> (ø)
src/pkg/packager/sources/tarball.go	41.80% <22.22%> (+0.96%)	⬆️
... and 3 more

[phillebaba]

@joonas I have verified that setting a pre run function in a sub command disables the root pre run. Which would explain why the tests are currently failing.

[joonas]

@joonas I have verified that setting a pre run function in a sub command disables the root pre run. Which would explain why the tests are currently failing.

Ah, it looks like to enable this behavior, the EnableTraverseRunHooks variable introduced in spf13/cobra#2044 needs to be set to true.

Does that seem like a reasonable change to make, or would you rather have the logic for setting pkgConfig.PkgOpts.SkipSignatureValidation to true if config.CommonOptions.Insecure is set to true copied to each sub command under the package command where it might be used?

I'm fine to go either way if changing the global flag in Cobra seems like too much of a behavior change to resolve this.

[phillebaba]

Looks like my comments about the pre run functions did not make it with my review. Or I cant find it anymore.

In it I shared that I was unsure about enabling EnableTraverseRunHooks due to it being a global variable. My first thought when finding the same issue that you did a while back was to enable it. However because Zarf embeds other commands it makes me unsure if this would cause unexpected behavior in these tools.

In the end we probably want to get away from the shared package options struct which is currently in use. My other suggestion would be to put it in the root pre run function. Thinking about it now I would prefer the code duplication over that option.

[joonas]

@AustinAbro321 @phillebaba I finally had a chance to go back and fix the one e2e test that was failing due to the help text changes.

I've also re-generated the command docs pages to incorporate the flag changes and I went ahead and rebased on top of the latest changes (as of Friday evening)

This should hopefully be all that's needed to have the test suite now pass with flying colors 🙂

[AustinAbro321]

It's close, requested a few more changes. Thank you for taking this on.

[AustinAbro321]

LGTM! Will need @phillebaba to approve as well

[phillebaba]

LGTM