Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
todaywasawesome opened this issue Sep 8, 2022 · 8 comments · Fixed by #2322
Closed

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

todaywasawesome opened this issue Sep 8, 2022 · 8 comments · Fixed by #2322
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@todaywasawesome
Copy link

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.

@todaywasawesome todaywasawesome added the enhancement New feature or request label Sep 8, 2022
@znewman01 znewman01 added the good first issue Good for newcomers label Sep 8, 2022
@znewman01
Copy link
Contributor

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
Copy link
Contributor

asraa commented Sep 8, 2022

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
Copy link
Contributor

xmudrii commented Oct 3, 2022

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

@znewman01
Copy link
Contributor

@xmudrii: 👍

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

@xmudrii
Copy link
Contributor

xmudrii commented Oct 9, 2022

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:

  • create a linter based on golangci-lint requirements and guidelines
  • open source the said linter
  • integrate the linter with golangci-lint
    • A potential blocker here can be are golangci-lint folks willing to accept our linter. Otherwise, it doesn't seem too complicated to add a new linter, there's some work required, but it's not impossible
    • This might take some time to implement, integrate, and release, so I wouldn't block on this. If we agree to proceed with this, we can integrate linter later

(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
Copy link
Contributor

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
Copy link
Contributor

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 😄

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
Copy link
Contributor

xmudrii commented Oct 11, 2022

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. 🙈 I'll update #2322 to integrate forbidigo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants