sigstore-python icon indicating copy to clipboard operation
sigstore-python copied to clipboard

Retrieve CTFE signing key via TUF

Open tetsuo-cpp opened this issue 3 years ago • 21 comments

At the moment, we've decided to check in the CTFE public key and use it to verify SCTs. The way this should really work is that we should check in the root key (root.json) and use it to download the CTFE key via TUF.

tetsuo-cpp avatar Apr 10 '22 15:04 tetsuo-cpp

Now that we're including an intermediate key, this should also include that as well.

di avatar Jun 03 '22 16:06 di

Hi, we are a small team (@wxjdsr, @vcharapa) that is working with professor Santiago (@SantiagoTorres) on fixing this issue.

wxjdsr avatar Sep 27 '22 22:09 wxjdsr

Cool! Please let me or one of the other maintainers know if there's something we can do to help.

woodruffw avatar Sep 27 '22 23:09 woodruffw

👋 python-tuf maintainer and sigstore/root-signing contributor here.

I posted a message on the #python Slack channel a few weeks back about some WIP changes I have to implement the certificate update workflow in python-sigstore. You can see my WIP branch here: https://github.com/joshuagl/sigstore-python/commits/joshuagl/tuf

Apologies for not posting about that work here, I didn't find this issue when I tried searching for something about the TUF workflow 🤦

It would be cool to collaborate on this, or have your work build on top of mine, @wxjdsr and @vcharapa?

joshuagl avatar Sep 28 '22 11:09 joshuagl

👋 python-tuf maintainer and sigstore/root-signing contributor here.

I posted a message on the #python Slack channel a few weeks back about some WIP changes I have to implement the certificate update workflow in python-sigstore. You can see my WIP branch here: https://github.com/joshuagl/sigstore-python/commits/joshuagl/tuf

Apologies for not posting about that work here, I didn't find this issue when I tried searching for something about the TUF workflow 🤦

It would be cool to collaborate on this, or have your work build on top of mine, @wxjdsr and @vcharapa?

Cool. I think we can collaborate on this.

wxjdsr avatar Sep 28 '22 21:09 wxjdsr

Great! Let me know how you want to proceed. Given timezone differences, if you want to build on my existing work (or even start afresh with that for inspiration) I'd be OK with that. Just want to make sure we aren't duplicating efforts, so please let me know how you would like to proceed :-)

joshuagl avatar Sep 30 '22 11:09 joshuagl

Sorry for late reply. We would like to build on your existing work. Currently we are working on fetching data from RemoteRoot for TUF. And I hope this does not conflict with your current work. XD

wxjdsr avatar Oct 02 '22 00:10 wxjdsr

Awesome, look forward to seeing your changes. I'd be more than willing to collaborate in any way that is useful for you and your team. Feel free to ping me on Sigstore Slack (I'm in the #python channel).

If you look at the branch I linked to above (https://github.com/joshuagl/sigstore-python/commits/joshuagl/tuf), you can see my WIP changes include initial retrieval of the TUF metadata and changes to access the certificates from a directory the TUF client fetches to, rather than accessing the bundled certificates using resourcelib (see https://github.com/joshuagl/sigstore-python/commit/e343366399bbe505b23c539cb2b94df2cb74c1c1).

What's next is adapting to the pending v5 root-signing changes in https://github.com/sigstore/root-signing/pull/430 where, instead of fetching a hard-coded set of targets, each of the certificates listed in the Targets metadata have additional metadata in the custom field that describes their usage.

For CTFE certificates we can query the metadata to retrieve only the targets in the which have custom.sigstore.usage == "CTFE". You can see how the Go Sigstore library is implementing this pattern in https://github.com/sigstore/sigstore/blob/fe89132b9369fc783254378ff8cfa68edfb64f72/pkg/fulcioroots/fulcioroots.go#L69 and https://github.com/sigstore/sigstore/blob/fe89132b9369fc783254378ff8cfa68edfb64f72/pkg/tuf/client.go#L373.

joshuagl avatar Oct 04 '22 09:10 joshuagl

each of the certificates listed in the Targets metadata have additional metadata in the custom field that describes their usage.

While this will work currently for all top-level targets, we're also adding targets to folders named by their usage, so all targets under ctfe/** will be for ctfe: this pattern will be stable in the future, and secure for any future delegations we add, so I recommend that pattern!

asraa avatar Oct 04 '22 13:10 asraa

Thanks for chiming in @asraa! To be clear, are we recommending targets are searched by delegation path not by the usage field in custom?

joshuagl avatar Oct 04 '22 14:10 joshuagl

To be clear, are we recommending targets are searched by delegation path not by the usage field in custom?

Searched by path (not necessarily delegation path): for top-level targets you can do either, but for other targets, we can't necessarily trust their usage field; we only trust that they were designated the correct delegation paths. (e.g. should a fulcio delegation be allowed to write to fulcio/**, but then adds a target there with custom usage of Rekor: that would be a no-go).

Technically for this root it's moot and the same since there are only top-level targets, but supporting based on paths would be stable going forward forever.

asraa avatar Oct 04 '22 14:10 asraa

Leaving a couple of notes:

  1. "target search" isn't really defined in the TUF spec (except for the specific case of searching with exact targetpath). As a result python-tuf does not currently officially support any other methods of target discovery. I think it can support the use cases you need and probably should, but the fact remains: there is no generic way to implement this for all TUF repositories -- this is going to require some explicit design decisions from the repository, and support from client libraries. I'm writing down some notes about this, will share shortly
  2. the client app needs to take care when handling targetpaths with dirs: by default python-tuf considers targetpaths as just identifiers that are translated to filenames (directory separator will be encoded). So the client app is responsible for creating local dirs and making sure it's safe to do so, if those are needed.

jku avatar Oct 07 '22 10:10 jku

TUF and target discovery

Speaking of this: the best way forward is probably to keep Sigstore TUF root with only a top-level targets, where we CAN do path filtering because TUF clients can expose the top-level targets list.

Then when we add delegations, we can view that as a new feature using one of the suggestions in the doc: I particularly like the well-known path location listing the targets.

asraa avatar Oct 07 '22 14:10 asraa

Just wanted to make sure I understand the conclusion reached here: when sigstore-python adds TUF support, should we discover the CT key(s) via their custom metadata, their filename, or some other mechanism?

woodruffw avatar Oct 20 '22 19:10 woodruffw

I understand the conclusion reached here: when sigstore-python adds TUF support, should we discover the CT key(s) via their custom metadata, their filename, or some other mechanism?

Since Target Discovery/endpoint to retrieve by usage is still "in flux", I think the simplest (and most secure) would be to rely on their filenames: IF we do update the target names, it will be at a point in time where we have this discovery piece laid out, and you could sub for file in ctfe_filenames: get_target(file) with for file in tuf.get_filenames(ctfe): get_target(file).

@joshuagl also has sigstore-python staged for our pre-submits when there's integration to make sure we won't break you before a TUF update.

asraa avatar Oct 20 '22 20:10 asraa

My WIP patches have the known cert filenames hard coded and just retrieve those after refreshing TUF metadata.

I can pick up work on those patches after KubeCon next week, or hand-off to another interested party. Let's assign this issue so we know who's working on it?

joshuagl avatar Oct 20 '22 20:10 joshuagl

@wxjdsr/@vcharapa/@SantiagoTorres should that be one of you? Haven't heard from you on this issue in a while.

di avatar Oct 24 '22 14:10 di

@wxjdsr/@vcharapa/@SantiagoTorres should that be one of you? Haven't heard from you on this issue in a while.

Yes, we are working on it.

wxjdsr avatar Oct 24 '22 16:10 wxjdsr

I've assigned this issue to @wxjdsr.

di avatar Oct 24 '22 17:10 di

Let me or @tetsuo-cpp know if there's any help we can provide!

woodruffw avatar Oct 24 '22 19:10 woodruffw

cc @wxjdsr @vcharapa any update here? We're getting close to a 1.0 release of sigstore-python, and this will probably be a necessary component.

We (ToB) are happy to help in the development if it's stuck, or you have limited bandwidth. Just let us know!

woodruffw avatar Dec 08 '22 17:12 woodruffw

For googleability, in older clients a missing key will manifest as a InvalidSctError with a stacktrace like:

$ python -m sigstore sign <file>
Go to the following link in a browser:

	https://oauth2.sigstore.dev/auth/auth?...
Enter verification code: <code>
Traceback (most recent call last):
  File "/usr/lib/python3.8/site-packages/sigstore/_internal/sct.py", line 303, in verify_sct
    ctfe_key.verify(
  File "/usr/lib/python3.8/site-packages/cryptography/hazmat/backends/openssl/ec.py", line 315, in verify
    _ecdsa_sig_verify(self._backend, self, signature, data)
  File "/usr/lib/python3.8/site-packages/cryptography/hazmat/backends/openssl/ec.py", line 122, in _ecdsa_sig_verify
    raise InvalidSignature
cryptography.exceptions.InvalidSignature

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/usr/lib/python3.8/runpy.py", line 194, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib/python3.8/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/usr/lib/python3.8/site-packages/sigstore/__main__.py", line 22, in <module>
    main()
  File "/usr/lib/python3.8/site-packages/sigstore/_cli.py", line 257, in main
    _sign(args)
  File "/usr/lib/python3.8/site-packages/sigstore/_cli.py", line 382, in _sign
    result = signer.sign(
  File "/usr/lib/python3.8/site-packages/sigstore/_sign.py", line 101, in sign
    verify_sct(
  File "/usr/lib/python3.8/site-packages/sigstore/_internal/sct.py", line 314, in verify_sct
    raise InvalidSctError from inval_sig
sigstore._internal.sct.InvalidSctError

and the solution is to upgrade to the latest version of the client with pip install -U sigstore.

di avatar Dec 08 '22 18:12 di

I plan to spend a bit of time on this this week: will update in a few days

jku avatar Dec 13 '22 13:12 jku

Hi, I'll be picking up the code that @wxjdsr sketched out. Should we submit a draft PR with their code or should I just finalize it and send it off in, say this weekend?

SantiagoTorres avatar Dec 13 '22 15:12 SantiagoTorres

@SantiagoTorres either works for us! A draft PR would help us make sure it meshes with our stabilization/1.0 plans, but whenever you feel it's ready.

(If you have limited bandwidth to finalize it, @jku or I could take it over from the draft as well.)

woodruffw avatar Dec 13 '22 15:12 woodruffw

A draft PR would help us make sure it meshes with our stabilization/1.0 plans

A bit more detail on this: this is included as a prerequisite for our 1.0 release which we're hoping to make before EOY.

di avatar Dec 13 '22 15:12 di

I'll leave some initial comments after just reading the code (I'm happy to work on this myself but I'm writing these down so A) it doesn't have to be me B) as reminder to myself):

  • there might a non-trivial rebase (unsure how much the resource handling has changed since august)
  • I think we should remove every feature that is an optimization or an extra feature from the first version. potential examples:
    • updating metadata only if it's expired is a good idea and if it was a python-tuf feature I would support using it. I'm not a fan of a workaround hack in the client. I filed an RFE on python-tuf: https://github.com/theupdateframework/python-tuf/issues/2225
    • bootstrap root verification is also good idea if implemented properly: current version does not look correct to me and does not seem required for v1. https://github.com/theupdateframework/python-tuf/issues/1168 is likely the good solution
    • all delegation handling should wait until the sigstore trust root design has a resolution
    • unsure if even the "download all targets" feature is necessary considering the code elsewhere still hardcodes the filenames. I understand the goal is to not do that but I'm just saying current sigstore functionality does not require "download all targets" (we could hard code like the rest of sigstore-python)
  • The resource handling is a little confusing -- _store is now both a module directory for Store code and a resource directory? Probably not a good idea
  • the code is clearly not yet finished: there are temporary files, undefined variables are being used
  • I disagree with "~/.sigstore/" as the cache path: sharing with other sigstore client implementations is possible but should be done very carefully. Left a longer comment in the design doc, but TL;DR: let's use application specific directory, preferably a runtime data directory for metadata (e.g. $HOME/.local/share/sigstore-python/metadata/ on linux)

jku avatar Dec 14 '22 14:12 jku

I'll leave some initial comments after just reading the code (I'm happy to work on this myself but I'm writing these down so A) it doesn't have to be me B) as reminder to myself):

Where is the code 😅? I don't see it on a PR.

Your comments all sound reasonable (caveat being that I haven't reviewed it myself) -- in particular I agree completely that the _store confusion needs to be resolved, and that we should absolutely use standard "runtime" state directories on relevant platforms rather than hardcoding ~/.sigstore.

Re: _store: we should probably put the TUF retrieval code under _tuf. _store should be limited to just static cryptographic materials, which after that PR should just be the TUF verification keys themselves.

woodruffw avatar Dec 14 '22 15:12 woodruffw

I think Jussi reviewed @joshuagl's WIP here: https://github.com/sigstore/sigstore-python/compare/main...joshuagl:sigstore-python:joshuagl/tuf

di avatar Dec 14 '22 15:12 di