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

Known issue: KeyError: 'sct' #105

Closed
di opened this issue Jun 1, 2022 · 11 comments
Closed

Known issue: KeyError: 'sct' #105

di opened this issue Jun 1, 2022 · 11 comments
Labels
bug Something isn't working upstream Requires upstream work or coordination

Comments

@di
Copy link
Member

di commented Jun 1, 2022

Description

When signing, the following exception is displayed:

Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/sigstore/_internal/fulcio/client.py", line 233, in post
    sct = FulcioSignedCertificateTimestamp(resp.headers["SCT"])
  File "/usr/local/lib/python3.10/site-packages/requests/structures.py", line 54, in __getitem__
    return self._store[key.lower()][1]
KeyError: 'sct'

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

Traceback (most recent call last):
  File "/usr/local/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/local/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/usr/local/lib/python3.10/site-packages/sigstore/__main__.py", line 22, in <module>
    main()
  File "/usr/local/lib/python3.10/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/usr/local/lib/python3.10/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.10/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/local/lib/python3.10/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/sigstore/_cli.py", line 119, in _sign
    result = sign(
  File "/usr/local/lib/python3.10/site-packages/sigstore/_sign.py", line 102, in sign
    certificate_response = fulcio.signing_cert.post(certificate_request, identity_token)
  File "/usr/local/lib/python3.10/site-packages/sigstore/_internal/fulcio/client.py", line 236, in post
    raise FulcioClientError from exc
sigstore._internal.fulcio.client.FulcioClientError

Version

This affects all currently published versions of sigstore.

Fix

This is blocked on a release that includes #84.

@di di added the bug Something isn't working label Jun 1, 2022
@di di pinned this issue Jun 1, 2022
@woodruffw woodruffw added the upstream Requires upstream work or coordination label Jun 1, 2022
@di di mentioned this issue Jun 2, 2022
@di
Copy link
Member Author

di commented Jun 2, 2022

For context, the actual issue is that we're blocked on a new release of cryptography that has the changes we need for #84.

Thinking a bit about workarounds for this:

  1. We could provide a forked cryptography release under a new project name, and use this as our subdependency instead. This is undesirable for a number of reasons, including having to mimic their build process, and just generally not being great to introduce forks, especially for what should be a trusted subdependency.
  2. We could update our cryptography to point at the git repo where the changes that we needed have landed. This would prevent us from publishing to PyPI, but we could advise users to temporarily install from our git repo to acquire a working version. Once the cryptography release is made, we remove the dependency on the repo and make a release. This is not great because all sigstore releases remain broken, but does give users a way to install something that works, and allows us to merge the pending PRs.
  3. We could do nothing, and wait until the cryptography release is made (ETA: some time in July). This is undesireable because it leaves a lot of PRs hanging around, and doesn't provide us with a way to get users a working version.

I'm leaning towards #2, but want to hear what @woodruffw and @tetsuo-cpp think.

@woodruffw
Copy link
Member

#2 sounds reasonable to me. The only caveat that I'll flag is that cryptography's build requires a local Rust toolchain, so users who attempt to install from our git repo may run into build errors (until they install Rust). I don't think that's a problem given our other alternatives, but we'll probably want to document it.

@di
Copy link
Member Author

di commented Jun 2, 2022

Perhaps another workaround would be to temporarily try and bring some of the functionality we introduced to cryptography here? (I'm not familiar enough with the changes we made there to say whether this is actually possible).

@woodruffw
Copy link
Member

Perhaps another workaround would be to temporarily try and bring some of the functionality we introduced to cryptography here? (I'm not familiar enough with the changes we made there to say whether this is actually possible).

Unfortunately, I don't think that'll be possible 😞 -- the majority of the changes we made were in Rust, since (AFAIK) cryptography doesn't provide public X.509 APIs for manipulating the extension list. I think the closest thing we could do is try to rebuild the X.509 certificate entirely using the CertificateBuilder API, while omitting the extensions. I can give that a try today.

@woodruffw
Copy link
Member

I think the closest thing we could do is try to rebuild the X.509 certificate entirely using the CertificateBuilder API, while omitting the extensions. I can give that a try today.

Tried this, but no luck -- the CertificateBuilder APIs want a new keypair to generate the signature with, rather than allowing me to fill in a pre-existing public key and signature. They've made their APIs a little too misuse resistant 🙂.

@woodruffw
Copy link
Member

(One last thing I'll try is just YOLOing the extension out of the certificate, modifying it in place. That might cause an error, but we'll see.)

@woodruffw
Copy link
Member

woodruffw commented Jun 2, 2022

Oh, another option: we could temporarily depend on pyasn1 and manually do the filtering in Python ourselves. I can also look into that. That's a much better solution than hacking through cryptography's internals.

@woodruffw
Copy link
Member

woodruffw commented Jun 2, 2022

Oh, another option: we could temporarily depend on pyasn1 and manually do the filtering in Python ourselves. I can also look into that. That's a much better solution than hacking through cryptography's internals.

This would work, but it still requires new attributes on the SCT that aren't available yet (and are added via Rust). So I think we're out of luck here, unless we want to refactor the current certificate request code to manually extract and re-parse the SCTs using a new FulcioEmbeddedSCT (or similar) model.

Edit: I just found ExtensionType.public_bytes(), which will give us the DER encoded structure of the SCT list extension itself. So I might be able to use that.

@woodruffw
Copy link
Member

Closable after #84 and #80 land, now that we have a workaround.

@woodruffw
Copy link
Member

#84 and #80 are in, just needs a release.

@woodruffw
Copy link
Member

#111.

@woodruffw woodruffw unpinned this issue Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working upstream Requires upstream work or coordination
Projects
None yet
Development

No branches or pull requests

2 participants