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

_verify: Check for URI SANs when verifying certificate emails #288

Merged
merged 8 commits into from
Nov 4, 2022

Conversation

tetsuo-cpp
Copy link
Contributor

No description provided.

@tetsuo-cpp
Copy link
Contributor Author

tetsuo-cpp commented Nov 3, 2022

Hmm, strange. I'm seeing this error.

============================================================ FAILURES ============================================================
_____________________________________________________ test_verifier_san_uri ______________________________________________________

signed_asset = <function signed_asset.<locals>._signed_asset at 0x102dc5670>

    @pytest.mark.online
    def test_verifier_san_uri(signed_asset):
        a_assets = signed_asset("c.txt")

        verifier = Verifier.staging()
>       assert verifier.verify(
            a_assets[0],
            a_assets[1],
            a_assets[2],
            expected_cert_email="https://github.com/sigstore/sigstore-python/.github/workflows/ci.yml@refs/pull/288/merge",
        )
E       assert CertificateVerificationFailure(success=False, reason='Failed to verify signing certificate', exception=X509StoreContextError([30, 0, 'authority and subject key identifier mismatch']))
E        +  where CertificateVerificationFailure(success=False, reason='Failed to verify signing certificate', exception=X509StoreContextError([30, 0, 'authority and subject key identifier mismatch'])) = <bound method Verifier.verify of <sigstore._verify.Verifier object at 0x102deb670>>(b'DO NOT MODIFY ME!\n\nthis is "c.txt", a sample input for sigstore-python\'s unit tests.\n\nDO NOT MODIFY ME!\n', b'-----BEGIN CERTIFICATE-----\nMIIDwjCCA0igAwIBAgIUWTX8JhuIl0ORzGW901qaNf9W5xcwCgYIKoZIzj0EAwMw\nNzEVMBMGA1UEChMMc2lnc...wYt+6G5EjTt491g\nPEd0AjEAqqyFS3tBoJ9pgoEb5GI8QvU6JlORPSXZMoPTqi0rWU97jExc4anXDc+g\nQ8c0ZbvM\n-----END CERTIFICATE-----', b'MGUCMFA7Llancvtw2vGFSPu7zZ2gMmnSvWF0goOLEoDrwpG5HwfjJRJBcH4k878MZKohGwIxAOS9JNAMXd7q2+LYb49j7tr6VfzC0utf10bOuADts8xw9TiYCB74EI8LSGpKHkFJow==', expected_cert_email='https://github.com/sigstore/sigstore-python/.github/workflows/ci.yml@refs/pull/288/merge')
E        +    where <bound method Verifier.verify of <sigstore._verify.Verifier object at 0x102deb670>> = <sigstore._verify.Verifier object at 0x102deb670>.verify

test/test_verify.py:100: AssertionError

But openssl verifies the certificate fine.

(env) tetsuo@Alexs-MacBook-Pro sigstore-python % openssl verify -CAfile sigstore/_store/fulcio.crt.pem -untrusted sigstore/_store/fulcio_intermediate.crt.pem -attime 1667452558 ./test/assets/c.txt.crt
./test/assets/c.txt.crt: OK

Edit: Ah, I was mixing up prod and staging. 🤦

a_assets[0],
a_assets[1],
a_assets[2],
expected_cert_email="william@yossarian.net",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@woodruffw Do you mind if I check this in a unit test? I can generate another certificate with my Trail of Bits email if you'd prefer that I don't expose this.

Copy link
Member

Choose a reason for hiding this comment

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

Nope, I don't mind. It's already checked in, anyways 🙂

Signed-off-by: Alex Cameron <asc@tetsuo.sh>
Signed-off-by: Alex Cameron <asc@tetsuo.sh>
@tetsuo-cpp tetsuo-cpp marked this pull request as ready for review November 3, 2022 07:53
@tetsuo-cpp tetsuo-cpp requested review from woodruffw and di November 3, 2022 07:53
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM structurally, although I think we'll need a resolution to #108 before we can merge this.

@woodruffw
Copy link
Member

I opened #289 for --cert-identity; we should base on top of that here.

woodruffw
woodruffw previously approved these changes Nov 3, 2022
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM! A small nit about testing, but otherwise good to go.

Signed-off-by: Alex Cameron <asc@tetsuo.sh>
Signed-off-by: Alex Cameron <asc@tetsuo.sh>
Signed-off-by: Alex Cameron <asc@tetsuo.sh>
Signed-off-by: Alex Cameron <asc@tetsuo.sh>
@tetsuo-cpp tetsuo-cpp enabled auto-merge (squash) November 4, 2022 04:30
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM!

@tetsuo-cpp tetsuo-cpp merged commit 2b26cf8 into main Nov 4, 2022
@tetsuo-cpp tetsuo-cpp deleted the alex/san-uri branch November 4, 2022 04:41
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