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.

Sign up for GitHub

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

Jump to bottom

_cli: Add --certificate-chain and support --rekor-url for verification #323

Merged
merged 11 commits into from
Nov 30, 2022
Merged

_cli: Add --certificate-chain and support --rekor-url for verification #323

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2596a2f
_cli: Add `--certificate-chain` and support `--rekor-url` for verific…
tetsuo-cpp Nov 29, 2022
88883b3
README: Update README with new flags
tetsuo-cpp Nov 29, 2022
6ef279e
README: Update usage
tetsuo-cpp Nov 29, 2022
6c2168b
README: Document the new `--certificate-chain` flag
tetsuo-cpp Nov 29, 2022
30ffa26
Update sigstore/_cli.py
tetsuo-cpp Nov 30, 2022
b3f3847
_cli: Amend `--certificate-chain` description
tetsuo-cpp Nov 30, 2022
ce3dc89
_cli: Move check for empty PEM file
tetsuo-cpp Nov 30, 2022
da25f55
_cli, _utils: Move split chain helper to utilities module
tetsuo-cpp Nov 30, 2022
6074cc2
_utils: appease the linter
woodruffw Nov 30, 2022
23e1a26
_utils: lintage
woodruffw Nov 30, 2022
598e86b
sigstore: more linting, use intrinsic list for type
woodruffw Nov 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ usage: sigstore sign [-h] [--identity-token TOKEN] [--oidc-client-id ID]
[--oidc-disable-ambient-providers] [--oidc-issuer URL]
[--no-default-files] [--signature FILE]
[--certificate FILE] [--rekor-bundle FILE] [--overwrite]
[--staging] [--rekor-url URL] [--fulcio-url URL]
[--ctfe FILE] [--rekor-root-pubkey FILE]
[--staging] [--rekor-url URL] [--rekor-root-pubkey FILE]
[--fulcio-url URL] [--ctfe FILE]
FILE [FILE ...]

positional arguments:
Expand Down Expand Up @@ -136,14 +136,14 @@ Sigstore instance options:
default production instances (default: False)
--rekor-url URL The Rekor instance to use (conflicts with --staging)
(default: https://rekor.sigstore.dev)
--fulcio-url URL The Fulcio instance to use (conflicts with --staging)
(default: https://fulcio.sigstore.dev)
--ctfe FILE A PEM-encoded public key for the CT log (conflicts
with --staging) (default: ctfe.pub (embedded))
--rekor-root-pubkey FILE
A PEM-encoded root public key for Rekor itself
(conflicts with --staging) (default: rekor.pub
(embedded))
--fulcio-url URL The Fulcio instance to use (conflicts with --staging)
(default: https://fulcio.sigstore.dev)
--ctfe FILE A PEM-encoded public key for the CT log (conflicts
with --staging) (default: ctfe.pub (embedded))
```
<!-- @end-sigstore-sign-help@ -->

Expand All @@ -152,9 +152,11 @@ Verifying:
<!-- @begin-sigstore-verify-help@ -->
```
usage: sigstore verify [-h] [--certificate FILE] [--signature FILE]
[--rekor-bundle FILE] [--cert-email EMAIL]
--cert-identity IDENTITY --cert-oidc-issuer URL
[--require-rekor-offline] [--staging] [--rekor-url URL]
[--rekor-bundle FILE] [--certificate-chain FILE]
[--cert-email EMAIL] --cert-identity IDENTITY
--cert-oidc-issuer URL [--require-rekor-offline]
[--staging] [--rekor-url URL]
[--rekor-root-pubkey FILE]
FILE [FILE ...]

positional arguments:
Expand All @@ -173,6 +175,12 @@ Verification inputs:
multiple inputs (default: None)

Extended verification options:
--certificate-chain FILE
Path to a list of CA certificates in PEM format which
will be needed when building the certificate chain for
the signing certificate; must start with the parent
intermediate CA certificate of the signing certificate
and end with the root certificate (default: None)
--cert-email EMAIL Deprecated; causes an error. Use --cert-identity
instead (default: None)
--cert-identity IDENTITY
Expand All @@ -190,6 +198,10 @@ Sigstore instance options:
default production instances (default: False)
--rekor-url URL The Rekor instance to use (conflicts with --staging)
(default: https://rekor.sigstore.dev)
--rekor-root-pubkey FILE
A PEM-encoded root public key for Rekor itself
(conflicts with --staging) (default: rekor.pub
(embedded))
```
<!-- @end-sigstore-verify-help@ -->

Expand Down
83 changes: 71 additions & 12 deletions sigstore/_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from importlib import resources
from pathlib import Path
from textwrap import dedent
from typing import Optional, TextIO, Union, cast
from typing import List, Optional, TextIO, Union, cast

from sigstore import __version__
from sigstore._internal.ctfe import CTKeyring
Expand Down Expand Up @@ -107,6 +107,13 @@ def _add_shared_instance_options(group: argparse._ArgumentGroup) -> None:
default=os.getenv("SIGSTORE_REKOR_URL", DEFAULT_REKOR_URL),
help="The Rekor instance to use (conflicts with --staging)",
)
group.add_argument(
"--rekor-root-pubkey",
metavar="FILE",
type=argparse.FileType("rb"),
help="A PEM-encoded root public key for Rekor itself (conflicts with --staging)",
default=os.getenv("SIGSTORE_REKOR_ROOT_PUBKEY", _Embedded("rekor.pub")),
)


def _add_shared_oidc_options(
Expand Down Expand Up @@ -229,13 +236,6 @@ def _parser() -> argparse.ArgumentParser:
help="A PEM-encoded public key for the CT log (conflicts with --staging)",
default=os.getenv("SIGSTORE_CTFE", _Embedded("ctfe.pub")),
)
instance_options.add_argument(
"--rekor-root-pubkey",
metavar="FILE",
type=argparse.FileType("rb"),
help="A PEM-encoded root public key for Rekor itself (conflicts with --staging)",
default=os.getenv("SIGSTORE_REKOR_ROOT_PUBKEY", _Embedded("rekor.pub")),
)

sign.add_argument(
"files",
Expand Down Expand Up @@ -275,6 +275,17 @@ def _parser() -> argparse.ArgumentParser:
)

verification_options = verify.add_argument_group("Extended verification options")
verification_options.add_argument(
"--certificate-chain",
metavar="FILE",
type=argparse.FileType("rb"),
help=(
"Path to a list of CA certificates in PEM format which will be needed when building "
"the certificate chain for the signing certificate; must start with the parent "
"intermediate CA certificate of the signing certificate and end with the root "
"certificate"
tetsuo-cpp marked this conversation as resolved.
Show resolved Hide resolved
),
)
verification_options.add_argument(
"--cert-email",
metavar="EMAIL",
Expand Down Expand Up @@ -536,10 +547,26 @@ def _verify(args: argparse.Namespace) -> None:
elif args.rekor_url == DEFAULT_REKOR_URL:
verifier = Verifier.production()
else:
# TODO: We need CLI flags that allow the user to figure the Fulcio cert chain
# for verification.
args._parser.error(
"Custom Rekor and Fulcio configuration for verification isn't fully supported yet!",
if not args.certificate_chain:
args._parser.error(
"Custom Rekor URL used without specifying --certificate-chain"
)

try:
certificate_chain = _split_certificate_chain(
args.certificate_chain.read_text()
)
except SplitCertificateChainError as error:
args._parser.error(f"Failed to split certificate chain: {error}")
tetsuo-cpp marked this conversation as resolved.
Show resolved Hide resolved

verifier = Verifier(
rekor=RekorClient(
url=args.rekor_url,
pubkey=args.rekor_root_pubkey.read(),
Copy link
Collaborator Author

@tetsuo-cpp tetsuo-cpp Nov 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder whether we should warn if a user supplies --rekor-root-pubkey but don't set the --rekor-url. Same with the new --certificate-chain. At the moment they just get ignored which might be confusing.

Same thing when we set --rekor-url but don't set --rekor-root-pubkey. It defaults to the rekor.pub that comes bundled with sigstore-python (which is almost certainly not what you want when you're using a custom Rekor) instead of warning.

Similar issues also exist in the signing code path.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it would be good to catch this and flag it as a warning at the minimum (or maybe even an error, since we expect it to fail anyways).

Copy link
Member

Choose a reason for hiding this comment

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

Let's track this with a follow-up issue.

# We don't use the CT keyring in verification so we can supply an empty keyring
ct_keyring=CTKeyring(),
),
fulcio_certificate_chain=certificate_chain,
)

for file, inputs in input_map.items():
Expand Down Expand Up @@ -692,3 +719,35 @@ def _get_identity_token(args: argparse.Namespace) -> Optional[str]:
issuer,
)
return token


class SplitCertificateChainError(Exception):
pass


def _split_certificate_chain(chain_pem: str) -> List[bytes]:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is pretty nasty. Not sure if there is a cleaner way to do it.

I believe that the load_pem...functions in cryptography load the first valid PEM entry and leave the rest so I don't think they can help us here.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this isn't ideal, but it looks like neither cryptography nor pyOpenSSL exposes a "load an entire chain from PEMs" API.

I think the corresponding OpenSSL API is SSL_CTX_get0_chain_certs, so this might be worth a patch to pyOpenSSL in the medium term. But for now, something like this approach is fine 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the OpenSSL APIs that accomplish this all work in the context of SSL contexts or connections, so they're probably not a great fit...

Copy link
Member

Choose a reason for hiding this comment

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

NB: I opened pyca/cryptography#7878 to add this API, so we'll be able to use it starting with cryptography 39 (assuming they accept it).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Awesome, thanks @woodruffw.

tetsuo-cpp marked this conversation as resolved.
Show resolved Hide resolved
PEM_BEGIN_CERTIFICATE = "-----BEGIN CERTIFICATE-----"

# Use the "begin certificate" marker as a delimiter to split the chain
certificate_chain = chain_pem.split(PEM_BEGIN_CERTIFICATE)

# Check for no certificates.
if not certificate_chain:
raise SplitCertificateChainError("empty PEM file")
tetsuo-cpp marked this conversation as resolved.
Show resolved Hide resolved

# The first entry in the list should be empty since we split by the "begin certificate" marker
# and there should be nothing before the first certificate
if certificate_chain[0]:
raise SplitCertificateChainError(
"encountered unrecognized content before first PEM entry"
)

# Remove the empty entry
certificate_chain = certificate_chain[1:]

# Add the delimiters back into each entry since this is required for valid PEM
certificate_chain = [
(PEM_BEGIN_CERTIFICATE + c).encode() for c in certificate_chain
]

return certificate_chain