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

sign, verify: Expose sign and verify as importable modules #383

Merged
merged 16 commits into from
Jan 7, 2023

Conversation

tetsuo-cpp
Copy link
Collaborator

Closes #4

Signed-off-by: Alex Cameron <asc@tetsuo.sh>
@tetsuo-cpp
Copy link
Collaborator Author

tetsuo-cpp commented Jan 5, 2023

Do we also want to somehow expose the web browser workflow and ambient credential detection? The signing side of the API is hard to use without these.

Either that or only expose verification initially.

I'm writing a simple example to test the verify API to get an idea for what this looks like for users.

@tetsuo-cpp
Copy link
Collaborator Author

import base64
from pathlib import Path

from sigstore.verify import Verifier, VerificationMaterials
from sigstore.verify.policy import Identity

artifact = Path("README.md")
cert = Path("README.md.crt")
signature = Path("README.md.sig")

with artifact.open("rb") as a, cert.open("r") as c, signature.open("rb") as s:
    materials = VerificationMaterials(
        input_=a,
        cert_pem=c.read(),
        signature=base64.b64decode(s.read()),
        offline_rekor_entry=None,
    )
    verifier = Verifier.production()
    result = verifier.verify(
        materials,
        Identity(
            identity="alex.cameron@trailofbits.com",
            issuer="https://accounts.google.com",
        ),
    )
    print(result)

@woodruffw
Copy link
Member

Do we also want to somehow expose the web browser workflow and ambient credential detection? The signing side of the API is hard to use without these.

Yeah, let's expose these as well. Let's make sure to expose the minimum surface necessary for each -- probably just the top-level functions/classes.

)

def to_bundle(self) -> RekorBundle:
def from_entry(cls, entry: RekorEntry) -> RekorBundle:
Copy link
Collaborator Author

@tetsuo-cpp tetsuo-cpp Jan 6, 2023

Choose a reason for hiding this comment

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

I moved this to_bundle method from the RekorEntry to a from_entry class method. This minimises the public API and means that it doesn't have to know about this bundle type.

Copy link
Member

Choose a reason for hiding this comment

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

That works for me. IMO it would also be acceptable to have the RekorBundle be part of the public API, since it's a well-defined public format already. But we can move forwards with this for now.

Signed-off-by: Alex Cameron <asc@tetsuo.sh>
Raises `AmbientCredentialError` if any detector fails internally (i.e.
detects a credential, but cannot retrieve it).
"""
from sigstore._internal.oidc.ambient import detect_gcp, detect_github
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to do this to avoid circular imports. I figured this would be better than further complicating the tree with more modules, but I can try fixing this another way if you'd like.

sigstore/oidc.py Show resolved Hide resolved
Signed-off-by: Alex Cameron <asc@tetsuo.sh>
@tetsuo-cpp
Copy link
Collaborator Author

The sign workflow looks reasonable too.

from pathlib import Path

from sigstore.sign import Signer
from sigstore.oidc import get_identity_token, Issuer

artifact = Path("README.md")
token = get_identity_token("sigstore", "", Issuer("https://oauth2.sigstore.dev/auth"))

with artifact.open("rb") as a:
    signer = Signer.production()
    result = signer.sign(input_=a, identity_token=token)
    print(result)

@tetsuo-cpp
Copy link
Collaborator Author

tetsuo-cpp commented Jan 6, 2023

One thing to note. The constructors for Signer takes FulcioClient and RekorClient and Verifier takes RekorClient. I'm assuming we probably don't want to expose these client types yet so I've left them under _internal. I don't think this is a big deal as most users are going to instantiate with one of the production or staging class methods.

The only issue is that pydoc still generates documentation for this constructor despite it not really being a usable part of the API.

@woodruffw woodruffw added the component:api Public APIs label Jan 6, 2023
@woodruffw woodruffw added this to the Stable release (1.0) milestone Jan 6, 2023
@woodruffw
Copy link
Member

Thanks @tetsuo-cpp! I'll review in a bit.

@woodruffw woodruffw mentioned this pull request Jan 6, 2023
23 tasks
@woodruffw
Copy link
Member

One thing to note. The constructors for Signer takes FulcioClient and RekorClient and Verifier takes RekorClient. I'm assuming we probably don't want to expose these client types yet so I've left them under _internal. I don't think this is a big deal as most users are going to instantiate with one of the production or staging class methods.

Yeah, I don't think we want to expose those directly. I'll see about maybe hiding the constructor in the docs.

Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@@ -122,9 +119,14 @@ def _set_default_verify_subparser(parser: argparse.ArgumentParser, name: str) ->
def _add_shared_instance_options(group: argparse._ArgumentGroup) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

N.B.: Conceptually these common options belong at the global sigstore level, rather than embedded into each subcommand. I've moved one of them in this PR (--staging) because I needed to fix its behavior with get-identity-token, but I'll move the rest in a follow-up.

@woodruffw
Copy link
Member

The sign workflow looks reasonable too.

from pathlib import Path

from sigstore.sign import Signer
from sigstore.oidc import get_identity_token, Issuer

artifact = Path("README.md")
token = get_identity_token("sigstore", "", Issuer("https://oauth2.sigstore.dev/auth"))

with artifact.open("rb") as a:
    signer = Signer.production()
    result = signer.sign(input_=a, identity_token=token)
    print(result)

NB, this is now:

from pathlib import Path

from sigstore.sign import Signer
from sigstore.oidc import Issuer

artifact = Path("README.md")
token = Issuer.production().identity_token()

with artifact.open("rb") as a:
    signer = Signer.production()
    result = signer.sign(input_=a, identity_token=token)
    print(result)

woodruffw
woodruffw previously approved these changes Jan 6, 2023
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, but I'll wait for @di to approve since I've been modifying things.

Makefile Outdated Show resolved Hide resolved
Signed-off-by: William Woodruff <william@trailofbits.com>
woodruffw
woodruffw previously approved these changes Jan 6, 2023
Signed-off-by: William Woodruff <william@trailofbits.com>
@tetsuo-cpp
Copy link
Collaborator Author

Thanks @woodruffw. Those changes to the get_identity_token API are looking really good.

I can't approve my own PR but your changes LGTM.

@woodruffw
Copy link
Member

I'll go ahead and merge then! 🤞

@woodruffw woodruffw merged commit 51b2279 into main Jan 7, 2023
@woodruffw woodruffw deleted the alex/public-api branch January 7, 2023 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:api Public APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Finalize importable sigstore API
2 participants