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

signer: adopt upstream sigstore API change #578

Merged
merged 4 commits into from
May 2, 2023

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented May 1, 2023

Since sigstore 1.1.2 detect_credential requires an "audience" argument. This patch passes the internal (!) DEFAULT_AUDIENCE, which is also used by the related sigstore cli.

Using the non-public constant is only a temporary fix until I hear back from upstream devs.

The PR also adds a missing permission in a related GitHub workflow .

Sigstore tests need to be able to submit issues. They run on PR merge
(not on PR create) and thus inform us about a failure after the fact.

This commit adds the necessary permission.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@lukpueh lukpueh force-pushed the quickfix-sigstore-audience branch from 38332ce to 816efbc Compare May 1, 2023 12:42
Since sigstore 1.1.2 `detect_credential` requires an "audience"
argument. This patch passes the internal (!) DEFAULT_AUDIENCE, which is
also used by the related sigstore cli.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@lukpueh lukpueh force-pushed the quickfix-sigstore-audience branch from 816efbc to 85ce181 Compare May 1, 2023 12:48
@jku
Copy link
Collaborator

jku commented May 2, 2023

This is fixed in main now: sigstore/sigstore-python#641: the argument is removed in next release (2.0 unless we really want a point release fix).

So we should not add arguments to our calls :) Maybe the easy fix to get builds working is prevent sigstore 1.1.2 from being used in requirements?

@lukpueh
Copy link
Member Author

lukpueh commented May 2, 2023

So we should add arguments to our calls :) Maybe the easy fix to get builds working is prevent sigstore 1.1.2 from being used in requirements?

I'm fine either way. Given that our sigstore signer is highly experimental and uses other non-public API in sigstore-python too, I think, this fix is okay. Let's have our users of this (you) decide. ;)

If we don't merge here, I can PR the ci fixing commit of this branch separately.

@jku
Copy link
Collaborator

jku commented May 2, 2023

So we should add arguments to our calls :) Maybe the easy fix to get builds working is prevent sigstore 1.1.2 from being used in requirements?

I'm fine either way. Given that our sigstore signer is highly experimental and uses other non-public API in sigstore-python too, I think, this fix is okay. Let's have our users of this (you) decide. ;)

Oops, my brain left a "not" out of that original reply: I meant we should not add arguments to our calls because that will then break on next release

@lukpueh
Copy link
Member Author

lukpueh commented May 2, 2023

Oh! I didn't look at the fix. I assumed they made the argument optional. :D Yeah, then excluding 1.1.2 makes sense. I'll do it in this PR if you don't mind.

This reverts commit 85ce181.

The upstream change was a mistake. Reverting here and excluding
the faulty release in the next commit...

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
1.1.2 accidentally broke the API. The breaking change will be reverted
in the next release. On our side we:
- pin test requirement to latest working version 1.1.1
- exclude 1.1.2 in pyproject.toml

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Copy link
Collaborator

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, let's see what we get this time...

@jku jku merged commit 3753e16 into secure-systems-lab:main May 2, 2023
@MVrachev MVrachev mentioned this pull request Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants