sigstore / cosign

Code signing and transparency for containers and binaries
Apache License 2.0
4.5k stars 547 forks source link

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

Closed todaywasawesome closed 2 years ago

todaywasawesome commented 2 years ago

Description

I'm trying to track down all the possible environment variables as I learn how to use cosign but they're not listed anywhere. It's not uncommon in other tools that use environment variables to have that command show all values set and unset.

So running cosign env would show something like:

COSIGN_PASSWORD=
...

This would make it a lot easier to learn how to use cosign.

znewman01 commented 2 years ago

This is a great idea!

There's two ways to do this:

  1. Easy mode: just grep the codebase for Getenv and similar calls, and make a new CLI command to print the values for a hardcoded list

  2. Hard mode: ban Getenv in the codebase (with a linter?), make callers go through a common pkg/env.go interface to access any environment variables, and have a hardcoded allowlist there. (Even better: parse all env vars for a given CLI command at the entry-point and pass them down through the call stack—IMHO Getenv in library code is a smell).

One wrinkle for both modes: I believe there are some called libraries whose behavior is affected by environment variables: SOFTHSM_*, GOOGLE_SERVICE_ACCOUNT_NAME, etc. We can try to do those best effort perhaps. For https://github.com/sigstore/sigstore we can do a similar trick as in cosign.

asraa commented 2 years ago

I believe there are some called libraries whose behavior is affected by environment variables: SOFTHSM_*, GOOGLE_SERVICE_ACCOUNT_NAME, etc. We can try to do those best effort perhaps. For https://github.com/sigstore/sigstore we can do a similar trick as in cosign.

Yes! The TUF client pkg exists in sigstore/sigstore, and the behavior there is also affected by env vars.

In the future, that will not be the case and all env vars will be at the top level in cosign. I have a TUF client redesign in the works, but it will likely not happen for the next 3 weeks.

xmudrii commented 2 years ago

Is this still available? I’d like to try my luck contributing to cosign and this seems like a nice starter. 🙂

znewman01 commented 2 years ago

@xmudrii: 👍

If you need guidance, let me know. My last comment shows a couple ways to get started.

xmudrii commented 2 years ago

I created #2322 which combines easy and hard modes with the difference that I didn't create a new CLI command, but used cosign env (as described in the ticket description).

ban Getenv in the codebase (with a linter?), make callers go through a common pkg/env.go interface to access any environment variables, and have a hardcoded allowlist there.

The PR should have accomplished everything but banning Getenv-related functions. I did some research on banning functions and it doesn't seem possible to ban functions natively with Go. Creating a linter is definitely an option and the workflow would be:

(Even better: parse all env vars for a given CLI command at the entry-point and pass them down through the call stack—IMHO Getenv in library code is a smell).

I agree that Getenv in the library code is a smell and should be avoided. I find this as an upgrade of work done in #2322. #2322 would still be useful in terms of keeping track of supported variables.

However, how doable is this? This requires some more significant changes to functions and their parameters. Is this something that is possible at this stage? Does cosign provide any backward compatibility promises that might block this work?

znewman01 commented 2 years ago

Hey, looks great on first skim @xmudrii! It's a holiday locally so I haven't had a time to really dig into your question; will get back to you soon.

znewman01 commented 2 years ago

I created #2322 which combines easy and hard modes with the difference that I didn't create a new CLI command, but used cosign env (as described in the ticket description).

Perfect, thank you!

[...] I did some research on banning functions and it doesn't seem possible to ban functions natively with Go. Creating a linter is definitely an option and the workflow would be:

  • create a linter based on golangci-lint requirements and guidelines
  • open source the said linter
  • integrate the linter with golangci-lint

I think it might be simpler than that. IIUC forbidigo, built into golanglint, makes this pretty easy. That only applies to our codebase, not upstream (e.g. github.com/sigstore/sigstore) but gets us 90% of the way there. Might even be straightforward to put that in #2322 (if you have a hard time, we won't block merging #2322).

(Even better: parse all env vars for a given CLI command at the entry-point and pass them down through the call stack—IMHO Getenv in library code is a smell).

[...]

However, how doable is this? This requires some more significant changes to functions and their parameters. Is this something that is possible at this stage? Does cosign provide any backward compatibility promises that might block this work?

Not necessary to do this for now :smile:

I think the above-mentioned work (#2322 + forbidigo) mostly solves this problem for users, and I'd be willing to close this issue after that. There are compatibility guarantees to worry about; there will be a Cosign v2 release at some point, after which things get a lot easier.

xmudrii commented 2 years ago

I think it might be simpler than that. IIUC forbidigo, built into golanglint, makes this pretty easy. That only applies to our codebase, not upstream (e.g. github.com/sigstore/sigstore) but gets us 90% of the way there.

I've looked at the list of linters and haven't seen this one. I guess I didn't pay enough attention. :see_no_evil: I'll update #2322 to integrate forbidigo.