Followups from #435

[imjasonh]

Followups from #435

• remove panic in fulcioroots.Get()
• add doc comments for un-doc-commented exported methods (or unexport them if appropriate)
• drop dependency on GCS and just make regular HTTP calls directly Drop tuf client dependency on GCS client library cosign#1967
• bump version: 3 in root.json (Move fulcioroots and tuf packages from cosign #435 (comment))

/assign @haydentherapper

[znewman01]

Quoting from
From sigstore/cosign#1865:

I don't think I'd mind as a prospective consumer of the package that its behavior was dependent on an env var, especially if that was named like SIGSTORE_ and not COSIGN_.

I would TBH, I think overall burying config options deep in some automagic code that's tough to modify as a caller without setting an env var is bad for testability etc. Further, if I'm using sigstore as a client but I want my own TUF state it'd possible that I wouldn't want to respect that variable. Maybe if there was a Sigstore-TUF client struct that had this configurable, and it had a default constructor that pulled from an environment variable?

And while I'm being cranky, I already think this is misbehaved because it doesn't respect XDG_STATE_HOME too.

So I'm gonna add to your TODO list :)

[imjasonh]

SGTM, I can update my PRs so that sigstore/sigstore's fulcioroots.Get doesn't consult the env var, and only cosign's usage of it does, with comments indicating the difference, and that callers should prefer sigstore's.

[znewman01]

Also fine to stick one that does consult the env var in sigstore/sigstore as long as it's not the only option. I just don't want to force that on callers.

[imjasonh]

Also fine to stick one that does consult the env var in sigstore/sigstore as long as it's not the only option. I just don't want to force that on callers.

If more callers want to adopt the env var version, we can move that into sigstore/sigstore too, as a separate opt-in process. For now I think you've convinced me that the env var behavior is only documented for cosign, and so should stay there unless/until someone else wants it.

After some of this lands I want to move more things into internal packages in cosign, to discourage/prevent their use outside of cosign and encourage folks to depend on sigstore/sigstore

[haydentherapper]

Are we concerned with breaking existing clients that may rely on this now? Hopefully folks are using TUF and not relying on the environment variables, but those were added for testing purposes. There's one for each of the TUF targets (CT log, Rekor, Fulcio), so we should probably add support for passing in the env var to a constructor (or maybe adding it to the context?) in a later PR.

[znewman01]

Hmm...I was gonna say "it's not documented anywhere" but that's not true. I dunno, I guess I struggle to think of a situation that this would break...cosign will continue to work, and callers of cosign will probably continue to work too if we handle the env var in cosign itself. 🤷

[haydentherapper]

Sorry, I didn’t mention context - the PR in Sigstore/Sigstore removed the environment variable. I just wanted to confirm we plan to continue supporting the variable by adding a constructor or something.

[imjasonh]

I agree with Zack, callers of cosign methods will continue to get the same behavior with the env var, for better or worse. If they move to the s/s equivalent (as we recommend) they won't, but I doubt they've intended to get that behavior anyway. If they need it, they can stay on the cosign version until the s/s one gets an equivalent.

[haydentherapper]

Sounds good, let’s add this as a TODO on this issue to track this.

[imjasonh]

With #435 merged, our thoughts now turn to all the TODOs we'd left for a future date 😆

The env var issue was resolved by keeping that behavior in cosign, and the GCS dependency was snipped out before moving to sigstore.

Does anybody fancy some godoc writing? 🙏

It sounds like we also need to update the root.json file now in sigstore/sigstore, but I'm not sure I understand the implications of that well enough to do it myself.

[haydentherapper]

Bumping the root doesn't need to happen right now, it just saves one or two network calls to fetch an extra root.