From ccac214f3343677e26ea908f77fc1afd30760362 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 6 Jun 2023 12:24:10 -0400 Subject: [PATCH 1/9] workflows: debug staging-tests Signed-off-by: William Woodruff --- .github/workflows/staging-tests.yml | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/.github/workflows/staging-tests.yml b/.github/workflows/staging-tests.yml index ab63c073..232f519a 100644 --- a/.github/workflows/staging-tests.yml +++ b/.github/workflows/staging-tests.yml @@ -11,6 +11,7 @@ on: push: branches: - main + - ww/debug-staging schedule: - cron: '0 */8 * * *' @@ -42,11 +43,11 @@ jobs: # Our signing target is not important here, so we just sign # the README in the repository. - ./staging-env/bin/python -m sigstore --staging sign README.md + ./staging-env/bin/python -m sigstore --verbose --staging sign README.md # Verification also requires a different Rekor instance, so we # also test it. - ./staging-env/bin/python -m sigstore --staging verify identity \ + ./staging-env/bin/python -m sigstore --verbose --staging verify identity \ --cert-oidc-issuer https://token.actions.githubusercontent.com \ --cert-identity ${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/.github/workflows/staging-tests.yml@${GITHUB_REF} \ README.md @@ -70,12 +71,12 @@ jobs: ${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/$GITHUB_RUN_ID EOF - - name: open an issue if the staging tests fail - if: failure() - uses: peter-evans/create-issue-from-file@433e51abf769039ee20ba1293a088ca19d573b7f # v4.0.1 - with: - title: "[CI] Integration failure: staging instance" - # created in the previous step - content-filepath: /tmp/staging-instance-issue.md - labels: bug,component:cicd,component:tests - assignees: woodruffw,di,tetsuo-cpp + # - name: open an issue if the staging tests fail + # if: failure() + # uses: peter-evans/create-issue-from-file@433e51abf769039ee20ba1293a088ca19d573b7f # v4.0.1 + # with: + # title: "[CI] Integration failure: staging instance" + # # created in the previous step + # content-filepath: /tmp/staging-instance-issue.md + # labels: bug,component:cicd,component:tests + # assignees: woodruffw,di,tetsuo-cpp From 401c8ee3d9a954ff469a2555bc156046bf344990 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 6 Jun 2023 12:31:13 -0400 Subject: [PATCH 2/9] DEBUG: oidc: disable iat verification Signed-off-by: William Woodruff --- sigstore/oidc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sigstore/oidc.py b/sigstore/oidc.py index dd8964dc..ef6fb5a1 100644 --- a/sigstore/oidc.py +++ b/sigstore/oidc.py @@ -96,7 +96,7 @@ def __init__(self, raw_token: str) -> None: options={ "verify_signature": False, "verify_aud": True, - "verify_iat": True, + # "verify_iat": True, "verify_exp": True, "require": ["aud", "iat", "exp", "iss"], }, From 971bcb9b0420d8b3c9d977376f30ce2e57676cf7 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 6 Jun 2023 12:37:45 -0400 Subject: [PATCH 3/9] sigstore, test: ratchet subject handling Signed-off-by: William Woodruff --- sigstore/oidc.py | 7 +++++-- test/unit/test_oidc.py | 16 +++++++++++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/sigstore/oidc.py b/sigstore/oidc.py index ef6fb5a1..adec8da4 100644 --- a/sigstore/oidc.py +++ b/sigstore/oidc.py @@ -96,9 +96,12 @@ def __init__(self, raw_token: str) -> None: options={ "verify_signature": False, "verify_aud": True, - # "verify_iat": True, + "verify_iat": True, "verify_exp": True, - "require": ["aud", "iat", "exp", "iss"], + # These claims are required by OpenID Connect, so + # we can strongly enforce their presence. + # See: https://openid.net/specs/openid-connect-basic-1_0.html#IDToken + "require": ["aud", "sub", "iat", "exp", "iss"], }, audience=DEFAULT_AUDIENCE, ) diff --git a/test/unit/test_oidc.py b/test/unit/test_oidc.py index f3d9b4b1..d6a1a8a0 100644 --- a/test/unit/test_oidc.py +++ b/test/unit/test_oidc.py @@ -31,6 +31,7 @@ def test_missing_iss(self, dummy_jwt): jwt = dummy_jwt( { "aud": "sigstore", + "sub": "fakesubject", "iat": now, "nbf": now, "exp": now + 600, @@ -46,6 +47,7 @@ def test_missing_aud(self, dummy_jwt): now = int(datetime.datetime.now().timestamp()) jwt = dummy_jwt( { + "sub": "fakesubject", "iat": now, "nbf": now, "exp": now + 600, @@ -64,6 +66,7 @@ def test_invalid_aud(self, dummy_jwt, aud): jwt = dummy_jwt( { "aud": aud, + "sub": "fakesubject", "iat": now, "nbf": now, "exp": now + 600, @@ -81,6 +84,7 @@ def test_missing_iat(self, dummy_jwt): jwt = dummy_jwt( { "aud": "sigstore", + "sub": "fakesubject", "nbf": now, "exp": now + 600, "iss": "fake-issuer", @@ -98,6 +102,7 @@ def test_invalid_iat(self, dummy_jwt, iat): jwt = dummy_jwt( { "aud": "sigstore", + "sub": "fakesubject", "iat": iat, "nbf": now, "exp": now + 600, @@ -115,6 +120,7 @@ def test_missing_nbf_ok(self, dummy_jwt): jwt = dummy_jwt( { "aud": "sigstore", + "sub": "fakesubject", "iat": now, "exp": now + 600, "iss": "fake-issuer", @@ -129,6 +135,7 @@ def test_invalid_nbf(self, dummy_jwt): jwt = dummy_jwt( { "aud": "sigstore", + "sub": "fakesubject", "iat": now, "nbf": now + 600, "exp": now + 601, @@ -147,6 +154,7 @@ def test_missing_exp(self, dummy_jwt): jwt = dummy_jwt( { "aud": "sigstore", + "sub": "fakesubject", "iat": now, "nbf": now, "iss": "fake-issuer", @@ -163,6 +171,7 @@ def test_invalid_exp(self, dummy_jwt): jwt = dummy_jwt( { "aud": "sigstore", + "sub": "fakesubject", "iat": now - 600, "nbf": now - 300, "exp": now - 1, @@ -175,12 +184,15 @@ def test_invalid_exp(self, dummy_jwt): ): oidc.IdentityToken(jwt) - @pytest.mark.parametrize("iss", oidc._KNOWN_OIDC_ISSUERS.keys()) + @pytest.mark.parametrize( + "iss", [k for k, v in oidc._KNOWN_OIDC_ISSUERS.items() if v != "sub"] + ) def test_missing_identity_claim(self, dummy_jwt, iss): now = int(datetime.datetime.now().timestamp()) jwt = dummy_jwt( { "aud": "sigstore", + "sub": "fakesubject", "iat": now, "nbf": now, "exp": now + 600, @@ -200,6 +212,7 @@ def test_invalid_federated_claims(self, dummy_jwt, fed): jwt = dummy_jwt( { "aud": "sigstore", + "sub": "fakesubject", "iat": now, "nbf": now, "exp": now + 600, @@ -240,6 +253,7 @@ def test_ok(self, dummy_jwt, iss, identity_claim, identity_value, fed_iss): jwt = dummy_jwt( { "aud": "sigstore", + "sub": "fakesubject", "iat": now, "nbf": now, "exp": now + 600, From c57eeeea0cbdc6b591f538f155884b55e0124912 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 6 Jun 2023 12:39:28 -0400 Subject: [PATCH 4/9] test: lint Signed-off-by: William Woodruff --- test/unit/test_oidc.py | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/test_oidc.py b/test/unit/test_oidc.py index d6a1a8a0..216934b6 100644 --- a/test/unit/test_oidc.py +++ b/test/unit/test_oidc.py @@ -120,7 +120,6 @@ def test_missing_nbf_ok(self, dummy_jwt): jwt = dummy_jwt( { "aud": "sigstore", - "sub": "fakesubject", "iat": now, "exp": now + 600, "iss": "fake-issuer", From dfda0d14cf7d42417292e0cf0cae07d808adf1c0 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 6 Jun 2023 12:59:50 -0400 Subject: [PATCH 5/9] oidc: add a little leeway Signed-off-by: William Woodruff --- sigstore/oidc.py | 1 + 1 file changed, 1 insertion(+) diff --git a/sigstore/oidc.py b/sigstore/oidc.py index adec8da4..317649f3 100644 --- a/sigstore/oidc.py +++ b/sigstore/oidc.py @@ -104,6 +104,7 @@ def __init__(self, raw_token: str) -> None: "require": ["aud", "sub", "iat", "exp", "iss"], }, audience=DEFAULT_AUDIENCE, + leeway=5, ) except Exception as exc: raise IdentityError( From f3beab7e93a08e915954a66cec2a36920e6c67dc Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 6 Jun 2023 13:03:13 -0400 Subject: [PATCH 6/9] test: fixup OIDC test Signed-off-by: William Woodruff --- test/unit/test_oidc.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/unit/test_oidc.py b/test/unit/test_oidc.py index 216934b6..4ac7cc3f 100644 --- a/test/unit/test_oidc.py +++ b/test/unit/test_oidc.py @@ -173,7 +173,8 @@ def test_invalid_exp(self, dummy_jwt): "sub": "fakesubject", "iat": now - 600, "nbf": now - 300, - "exp": now - 1, + # NOTE: 6 seconds due to +/- 5 second flutter. + "exp": now - 6, "iss": "fake-issuer", } ) From 35b2660e777844f94ac81918607cf4616db9e065 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 6 Jun 2023 13:08:25 -0400 Subject: [PATCH 7/9] test: fixup signing tests Signed-off-by: William Woodruff --- test/unit/test_sign.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/unit/test_sign.py b/test/unit/test_sign.py index a4bea319..a030a495 100644 --- a/test/unit/test_sign.py +++ b/test/unit/test_sign.py @@ -45,8 +45,9 @@ def test_sign_rekor_entry_consistent(id_config): payload = io.BytesIO(secrets.token_bytes(32)) with ctx.signer(identity) as signer: - expected_entry = signer.sign(payload, identity).log_entry - actual_entry = signer._rekor.log.entries.get(log_index=expected_entry.log_index) + expected_entry = signer.sign(payload).log_entry + + actual_entry = ctx._rekor.log.entries.get(log_index=expected_entry.log_index) assert expected_entry.uuid == actual_entry.uuid assert expected_entry.body == actual_entry.body @@ -71,7 +72,7 @@ def test_sct_verify_keyring_lookup_error(id_config, monkeypatch): InvalidSCTError, ) as excinfo: with ctx.signer(identity) as signer: - signer.sign(payload, identity) + signer.sign(payload) # The exception subclass is the one we expect. assert isinstance(excinfo.value, InvalidSCTKeyError) @@ -109,7 +110,7 @@ def test_identity_proof_claim_lookup(id_config, monkeypatch): with ctx.signer(identity) as signer: expected_entry = signer.sign(payload).log_entry - actual_entry = signer._rekor.log.entries.get(log_index=expected_entry.log_index) + actual_entry = ctx._rekor.log.entries.get(log_index=expected_entry.log_index) assert expected_entry.uuid == actual_entry.uuid assert expected_entry.body == actual_entry.body From 408ca6a5dccfeaa2dc57cb65e54ec5e98e9d94f8 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 6 Jun 2023 13:10:07 -0400 Subject: [PATCH 8/9] oidc: document leeway Signed-off-by: William Woodruff --- sigstore/oidc.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sigstore/oidc.py b/sigstore/oidc.py index 317649f3..6c2b53b7 100644 --- a/sigstore/oidc.py +++ b/sigstore/oidc.py @@ -104,6 +104,9 @@ def __init__(self, raw_token: str) -> None: "require": ["aud", "sub", "iat", "exp", "iss"], }, audience=DEFAULT_AUDIENCE, + # NOTE: This leeway shouldn't be strictly necessary, but is + # included to preempt any (small) skew between the host + # and the originating IdP. leeway=5, ) except Exception as exc: From 1574a27be30f9ad4c071a72f742ab482512e9a01 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 6 Jun 2023 13:11:11 -0400 Subject: [PATCH 9/9] workflows/staging-tests: undo debug changes Signed-off-by: William Woodruff --- .github/workflows/staging-tests.yml | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/.github/workflows/staging-tests.yml b/.github/workflows/staging-tests.yml index 232f519a..749f5610 100644 --- a/.github/workflows/staging-tests.yml +++ b/.github/workflows/staging-tests.yml @@ -11,7 +11,6 @@ on: push: branches: - main - - ww/debug-staging schedule: - cron: '0 */8 * * *' @@ -71,12 +70,12 @@ jobs: ${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/$GITHUB_RUN_ID EOF - # - name: open an issue if the staging tests fail - # if: failure() - # uses: peter-evans/create-issue-from-file@433e51abf769039ee20ba1293a088ca19d573b7f # v4.0.1 - # with: - # title: "[CI] Integration failure: staging instance" - # # created in the previous step - # content-filepath: /tmp/staging-instance-issue.md - # labels: bug,component:cicd,component:tests - # assignees: woodruffw,di,tetsuo-cpp + - name: open an issue if the staging tests fail + if: failure() + uses: peter-evans/create-issue-from-file@433e51abf769039ee20ba1293a088ca19d573b7f # v4.0.1 + with: + title: "[CI] Integration failure: staging instance" + # created in the previous step + content-filepath: /tmp/staging-instance-issue.md + labels: bug,component:cicd,component:tests + assignees: woodruffw,di,tetsuo-cpp