feat: Implement 'env' package to handle environment variables in cosign #2322

xmudrii · 2022-10-09T17:56:05Z

Summary

This PR implements a new env package to handle environment variables in cosign. This PR/implementation targets only COSIGN_ environment variables, but it can be extended to handle other environment variables as well. This implementation is based on hard mode described here: #2236 (comment)

The env package implements:

Variable type which is used to define an environment variable
VariableOpts type which is used to describe an environment variable
A slice with known environment variables and their descriptions (used for printing)
Getenv(Variable) string function which is a wrapper around os.Getenv for given Variable
LookupEnv(Variable) (string, bool) function which is a wrapper around os.LookupEnv for given Variable
PrintEnv() function which is used to print all known variables, their values, and descriptions

All os.Getenv/os.LookupEnv calls on COSIGN_ variables are replaced with env.Getenv and env.LookupEnv. There are some other os.Getenv calls on non-cosign variables, but those will be handled in a follow up PR (if this turns out to be an approach we want to follow). The purpose of this change is that we want to forbid using os.Getenv-related function and require using env package. That combined with Variable type ensures that each environment variable used by cosign must be registered in the env package.

This PR also modifies cosign env command with some additional features. First of all, the command is now using env.PrintEnv() to print the known cosign variables instead of searching Environ. However, we still search Environ for SIGSTORE_ and TUF_ variables. Additionally, there are two new flags:

--show-descriptions (default true) -- used to enable descriptions for known cosign variables
--show-sensitive-values (default false) -- by default, cosign env is going to show asterisk signs if variable is marked as sensitive. Turning this flag on will show values for sensitive variables

Example 1 (with descriptions and hidden sensitive values, default):

COSIGN_PASSWORD=test ./cosign env

# COSIGN_DOCKER_MEDIA_TYPES to be used with registries that do not support OCI media types
COSIGN_DOCKER_MEDIA_TYPES=""
# COSIGN_EXPERIMENTAL enables experimental cosign features
COSIGN_EXPERIMENTAL=""
# COSIGN_PASSWORD overrides password inputs with this value
COSIGN_PASSWORD="******"
# COSIGN_PKCS11_MODULE_PATH is PKCS11 module-path
COSIGN_PKCS11_MODULE_PATH=""
# COSIGN_PKCS11_PIN to be used if PKCS11 PIN is not provided
COSIGN_PKCS11_PIN=""
# COSIGN_REPOSITORY can be used to store signatures in an alternate location
COSIGN_REPOSITORY=""

Example 2 (with sensitive values):

COSIGN_PASSWORD=test ./cosign env --show-sensitive-values

# COSIGN_DOCKER_MEDIA_TYPES to be used with registries that do not support OCI media types
COSIGN_DOCKER_MEDIA_TYPES=""
# COSIGN_EXPERIMENTAL enables experimental cosign features
COSIGN_EXPERIMENTAL=""
# COSIGN_PASSWORD overrides password inputs with this value
COSIGN_PASSWORD="test"
# COSIGN_PKCS11_MODULE_PATH is PKCS11 module-path
COSIGN_PKCS11_MODULE_PATH=""
# COSIGN_PKCS11_PIN to be used if PKCS11 PIN is not provided
COSIGN_PKCS11_PIN=""
# COSIGN_REPOSITORY can be used to store signatures in an alternate location
COSIGN_REPOSITORY=""

This PR integrates forbidigo linter to forbid using os.Getenv and os.LookupEnv. We want to enforce the env package to be used so that we can register all environment variables. There are some legitimate use cases where we still use os.Getenv and os.LookupEnv, such as in tests or with non-cosign environment variables. I handled ignoring those legitimate use cases in two ways:

if the file/package has many os.Getenv/os.LookupEnv calls, ignore that file or package in .golangci.yml file
if the file/package has only one or a few os.Getenv/os.LookupEnv calls, use in-line //nolint:forbidigo comment

There are other environment variables such as SIGSTORE_ and TUF_ that are not handled, but this is something that can be discussed and handled later. I believe this PR should be a solid foundation for any further work on this topic.

Fixes #2236

Release Note

Implement env package used to register and handle environment variables used by cosign
cosign env now prints all registered COSIGN_ environment variables
Add --show-descriptions (default true) flag to cosign env used to show description for each registered COSIGN_ environment variable
Add --show-sensitive-values (default false) flag to cosign env used to hidh values for COSIGN_ environment variables marked as sensitive

Documentation

I'm not sure, but I guess mostly like yes.

xmudrii · 2022-10-09T17:57:26Z

cc @znewman01 PTAL ^^ 🙂

codecov-commenter · 2022-10-09T22:47:44Z

Codecov Report

Merging #2322 (3bf16dc) into main (797033c) will increase coverage by 0.26%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##             main    #2322      +/-   ##
==========================================
+ Coverage   29.85%   30.11%   +0.26%     
==========================================
  Files         134      136       +2     
  Lines        8287     8344      +57     
==========================================
+ Hits         2474     2513      +39     
- Misses       5482     5498      +16     
- Partials      331      333       +2

Impacted Files	Coverage Δ
cmd/cosign/cli/options/env.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/options/experimental.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/env.go	`43.75% <9.09%> (-16.25%)`	⬇️
pkg/cosign/env/env.go	`90.90% <90.90%> (ø)`
cmd/cosign/cli/fulcio/fulcioverifier/ctl/verify.go	`41.73% <100.00%> (ø)`
cmd/cosign/cli/generate/generate_key_pair.go	`8.75% <100.00%> (ø)`
cmd/cosign/cli/importkeypair/import_key_pair.go	`14.58% <100.00%> (ø)`
...ernal/pkg/cosign/fulcio/fulcioroots/fulcioroots.go	`57.77% <100.00%> (ø)`
pkg/blob/load.go	`70.27% <100.00%> (ø)`
pkg/cosign/tlog.go	`38.43% <100.00%> (ø)`
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

xmudrii · 2022-10-10T14:06:03Z

I have an issue with running make docgen:

go run -tags pivkey,pkcs11key,cgo ./cmd/help/
# pkg-config --cflags  -- libpcsclite
Package libpcsclite was not found in the pkg-config search path.
Perhaps you should add the directory containing `libpcsclite.pc'
to the PKG_CONFIG_PATH environment variable
No package 'libpcsclite' found
pkg-config: exit status 1
make: *** [Makefile:176: docgen] Error 2

Any idea how can I fix this? I'm running Ubuntu 22.04.

cpanato · 2022-10-10T20:36:22Z

I have an issue with running make docgen:

go run -tags pivkey,pkcs11key,cgo ./cmd/help/
# pkg-config --cflags  -- libpcsclite
Package libpcsclite was not found in the pkg-config search path.
Perhaps you should add the directory containing `libpcsclite.pc'
to the PKG_CONFIG_PATH environment variable
No package 'libpcsclite' found
pkg-config: exit status 1
make: *** [Makefile:176: docgen] Error 2

Any idea how can I fix this? I'm running Ubuntu 22.04.

you need to install the libpcsclite-dev - apt install libpcsclite-dev

znewman01

Great idea to redact sensitive information by default 👍

pkg/cosign/env/env.go

xmudrii · 2022-10-14T19:34:32Z

@znewman01 I should have addressed all your comments. I'm trying to configure forbidigo and after that the PR should be ready.

xmudrii · 2022-10-14T19:45:10Z

@znewman01 I added forbidigo to golangci-lint. There are some places where we still use os.Getenv with non-cosign environment variables. I handled that in two ways:

if the file/package has many os.Getenv/LookupEnv calls, ignore that file or package to .golangci.yml file
if the file/package has only one or few os.Getenv/LookupEnv calls, use in-line //nolint:forbidigo comment

Please take a look at this and let me know if this approach is okay or if I should change anything.

znewman01

EDIT: that was the wrong PR, sorry!

znewman01

This is a really great first contribution to Cosign, thank you so much!

Some changes requested, mostly minor.

.golangci.yml

cmd/cosign/cli/fulcio/fulcioverifier/ctl/verify.go

pkg/cosign/env/env.go

pkg/cosign/env/env_test.go

pkg/cosign/tlog.go

pkg/cosign/env/env_test.go

pkg/cosign/tlog.go

pkg/cosign/git/github/github.go

pkg/blob/load.go

pkg/cosign/env/env.go

xmudrii · 2022-10-14T20:56:36Z

@znewman01 Thank you so much for the review! I agree with all those comments. It’s already late for me (I’m in the CEST time zone), so I’ll address those comments beginning of the next week if that’s okay. 🙂

znewman01 · 2022-10-14T20:59:23Z

@znewman01 Thank you so much for the review! I agree with all those comments. It’s already late for me (I’m in the CEST time zone), so I’ll address those comments beginning of the next week if that’s okay. 🙂

No rush! Have a great weekend

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

xmudrii · 2022-10-17T18:18:56Z

@znewman01 I think that I should have addressed all comments. Please let me know if I forgot something or if you have any additional comments. I tried to document all variables as best as I could, but I'm not sure have I got all descriptions and expectations correctly, so please take a look at that.

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

cmd/cosign/cli/env_test.go

cmd/cosign/cli/env.go

znewman01

So close! This is looking great. Just a couple small things

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

xmudrii · 2022-10-17T20:08:17Z

@znewman01 I addressed your comments, PTAL when you have some time. 🙂

znewman01 · 2022-10-17T20:10:20Z

Small lint issue, once you fix that I think we can merge!

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

xmudrii · 2022-10-17T20:23:50Z

@znewman01 Fixed. 🙂

znewman01 · 2022-10-17T21:09:08Z

Nice, if tests pass this should merge automatically!

Thank you so much! 🙏 Great first PR, I wish everybody sent one like this 😄

xmudrii · 2022-10-17T21:43:30Z

@znewman01 I'm so happy to see this PR merged! Thanks a lot for going through this with me, I really appreciate it!

These were removed in sigstore#2322, which made it an accidentally-semver-breaking change. This unbreaks semver. These should be removed at the next major version release or sooner if we decide keeping semver is not important. Signed-off-by: Zachary Newman <zjn@chainguard.dev>

These were removed in #2322, which made it an accidentally-semver-breaking change. This unbreaks semver. These should be removed at the next major version release or sooner if we decide keeping semver is not important. Signed-off-by: Zachary Newman <zjn@chainguard.dev> Signed-off-by: Zachary Newman <zjn@chainguard.dev>

xmudrii changed the title ~~Implement 'env' package to handle environment variables in cosign~~ feat: Implement 'env' package to handle environment variables in cosign Oct 9, 2022

xmudrii mentioned this pull request Oct 9, 2022

feat: Cosign env should show all possible environment variables. #2236

Closed

znewman01 reviewed Oct 11, 2022

View reviewed changes

pkg/cosign/env/env.go Show resolved Hide resolved

pkg/cosign/env/env.go Outdated Show resolved Hide resolved

pkg/cosign/env/env.go Show resolved Hide resolved

xmudrii force-pushed the env branch from 89d8b00 to 0a46b61 Compare October 14, 2022 19:29

xmudrii requested a review from znewman01 October 14, 2022 19:45

znewman01 reviewed Oct 14, 2022

View reviewed changes

znewman01 suggested changes Oct 14, 2022

View reviewed changes

xmudrii added 9 commits October 17, 2022 18:34

Implement 'env' package to handle environment variables

6c3b12c

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

Make 'cosign env' use the env package

9951b61

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

Make packages use env instead os.Getenv/LookupEnv

f8be9af

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

Fix oci unit test

dd91e55

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

Fix E2E tests

7bd0da8

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

Address review comments

b6e2438

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

Update docgen

feef7e2

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

Integrate forbidigo linter for os.Getenv/LookupEnv

f3d1aa8

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

Document all environment variables used by cosign

3b98d00

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

xmudrii force-pushed the env branch from 3bf16dc to 0769177 Compare October 17, 2022 18:15

xmudrii requested a review from znewman01 October 17, 2022 18:19

Address review comments - part 2

31a6fa2

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

xmudrii force-pushed the env branch from 0769177 to 31a6fa2 Compare October 17, 2022 18:21

znewman01 reviewed Oct 17, 2022

View reviewed changes

cmd/cosign/cli/env_test.go Outdated Show resolved Hide resolved

cmd/cosign/cli/env_test.go Show resolved Hide resolved

cmd/cosign/cli/env_test.go Show resolved Hide resolved

cmd/cosign/cli/env.go Outdated Show resolved Hide resolved

znewman01 suggested changes Oct 17, 2022

View reviewed changes

xmudrii added 2 commits October 17, 2022 21:53

Fix issue with tests conflicting with OS envs

5c8b0b0

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

Address review comments - part 3

a7c0abf

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

xmudrii requested a review from znewman01 October 17, 2022 20:07

znewman01 previously approved these changes Oct 17, 2022

View reviewed changes

Fix gocritic lint errors

252c362

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

xmudrii dismissed znewman01’s stale review via 252c362 October 17, 2022 20:23

xmudrii requested a review from znewman01 October 17, 2022 20:23

znewman01 enabled auto-merge (squash) October 17, 2022 21:08

znewman01 approved these changes Oct 17, 2022

View reviewed changes

znewman01 merged commit 285459e into sigstore:main Oct 17, 2022

github-actions bot added this to the v1.14.0 milestone Oct 17, 2022

xmudrii deleted the env branch October 17, 2022 21:42

cpanato modified the milestones: v1.13.1, v1.14.0 Oct 18, 2022

asraa mentioned this pull request Oct 20, 2022

verify: remove SIGSTORE_TRUST_REKOR_API_PUBLIC_KEY test env var for using a key from rekor's API #2362

Merged

znewman01 mentioned this pull request Oct 20, 2022

Replace accidentally-removed public constants #2370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement 'env' package to handle environment variables in cosign #2322

feat: Implement 'env' package to handle environment variables in cosign #2322

xmudrii commented Oct 9, 2022 •

edited

xmudrii commented Oct 9, 2022

codecov-commenter commented Oct 9, 2022 •

edited

xmudrii commented Oct 10, 2022

cpanato commented Oct 10, 2022

znewman01 left a comment

xmudrii commented Oct 14, 2022

xmudrii commented Oct 14, 2022

znewman01 left a comment •

edited

znewman01 left a comment

xmudrii commented Oct 14, 2022

znewman01 commented Oct 14, 2022

xmudrii commented Oct 17, 2022

znewman01 left a comment

xmudrii commented Oct 17, 2022

znewman01 commented Oct 17, 2022

xmudrii commented Oct 17, 2022

znewman01 commented Oct 17, 2022

xmudrii commented Oct 17, 2022

feat: Implement 'env' package to handle environment variables in cosign #2322

feat: Implement 'env' package to handle environment variables in cosign #2322

Conversation

xmudrii commented Oct 9, 2022 • edited

Summary

Release Note

Documentation

xmudrii commented Oct 9, 2022

codecov-commenter commented Oct 9, 2022 • edited

Codecov Report

xmudrii commented Oct 10, 2022

cpanato commented Oct 10, 2022

znewman01 left a comment

Choose a reason for hiding this comment

xmudrii commented Oct 14, 2022

xmudrii commented Oct 14, 2022

znewman01 left a comment • edited

Choose a reason for hiding this comment

znewman01 left a comment

Choose a reason for hiding this comment

xmudrii commented Oct 14, 2022

znewman01 commented Oct 14, 2022

xmudrii commented Oct 17, 2022

znewman01 left a comment

Choose a reason for hiding this comment

xmudrii commented Oct 17, 2022

znewman01 commented Oct 17, 2022

xmudrii commented Oct 17, 2022

znewman01 commented Oct 17, 2022

xmudrii commented Oct 17, 2022

xmudrii commented Oct 9, 2022 •

edited

codecov-commenter commented Oct 9, 2022 •

edited

znewman01 left a comment •

edited