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

Sign Bundle with a Timestamp Authority #1216

Merged
merged 26 commits into from
Nov 25, 2024
Merged

Sign Bundle with a Timestamp Authority #1216

merged 26 commits into from
Nov 25, 2024
+403 −8

Conversation

DarkaMaul
Copy link
Contributor

@DarkaMaul DarkaMaul commented Nov 15, 2024 •
edited
Loading

Final bit of #1182

Summary

This PR introduces the possibility to create a bundle with a timestamp signed by a TimestampAuthority.

Release Note

Added

  • If a Timestamp Authority URL has been provided in the SigningConfig, the bundle are now automatically generated with a signed timestamp.

/cc @woodruffw @facutuesca

DarkaMaul and others added 16 commits November 7, 2024 09:32
@DarkaMaul
Add tests for CertificateAuthority
c7de7db 
Signed-off-by: Alexis <[email protected]>
@DarkaMaul
Add root requirement for CA
ac0fa7e 
Signed-off-by: Alexis <[email protected]>
@DarkaMaul
Timestamp Verification
cc16b31 
Signed-off-by: Alexis <[email protected]>
@DarkaMaul
Add signature on Bundle and Envelope
9a1bbf4 
Signed-off-by: Alexis <[email protected]>
@DarkaMaul
Start to improve tests.
f686f53 
Signed-off-by: Alexis <[email protected]>
@DarkaMaul
Improve testing
86e4885 
Signed-off-by: Alexis <[email protected]>
@DarkaMaul
Improve tests
8d95387 
Signed-off-by: Alexis <[email protected]>
@DarkaMaul
Add CHANGELOG entry
9f9adf8 
Signed-off-by: Alexis <[email protected]>
@DarkaMaul
Fix test
303210f 
Signed-off-by: Alexis <[email protected]>
@DarkaMaul
Fix _verify_signed_timestamp return value.
7733a9b 
Signed-off-by: Alexis <[email protected]>
@DarkaMaul
Use a Signed Timestamp within a bundle
dc6545c 
Signed-off-by: Alexis <[email protected]>
@DarkaMaul
Refactor client timeout as constant
e0c10e5 
Signed-off-by: Alexis <[email protected]>
@DarkaMaul @facutuesca
Apply suggestions from code review
e0e5f6d 
Co-authored-by: Facundo Tuesca <[email protected]>
Signed-off-by: dm <[email protected]>
@DarkaMaul
Rename method to request_timestamp
b28a849 
Signed-off-by: Alexis <[email protected]>
@DarkaMaul
Merge branch 'main' into dm/timestamp-signing
1684b74 
# Conflicts:
#	CHANGELOG.md
#	sigstore/dsse/__init__.py
#	sigstore/verify/verifier.py
#	test/assets/tsa/bundle.txt.sigstore
#	test/unit/verify/test_verifier.py
@DarkaMaul
Use add_root_certificate
b4744dd 
Signed-off-by: Alexis <[email protected]>
woodruffw
woodruffw reviewed Nov 15, 2024
View reviewed changes
test/unit/conftest.py Outdated Show resolved Hide resolved
woodruffw
woodruffw reviewed Nov 15, 2024
View reviewed changes
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@jku
Copy link
Member

jku commented Nov 18, 2024

/gcbrun

@woodruffw
Copy link
Member

This looks like something bad on the Windows CI:

    Error configuring OpenSSL build:
        'perl' reported failure with exit code: 2
        Command failed: "perl" "./Configure" "--prefix=C:/Users/runneradmin/AppData/Local/Temp/pip-install-ldkkdr4i/rfc3161-client_a9d25b6f62a14f8aac4a34a83ab37a09/rust/target/release/build/openssl-sys-b380da04be89fe31/out/openssl-build/install" "--openssldir=SYS$MANAGER:[OPENSSL]" "no-shared" "no-ssl3" "no-tests" "no-comp" "no-zlib" "no-zlib-dynamic" "--libdir=lib" "no-md2" "no-rc5" "no-weak-ssl-ciphers" "no-camellia" "no-idea" "no-seed" "no-capieng" "no-asm" "VC-WIN64A"

@jku
Copy link
Member

jku commented Nov 18, 2024 •
edited
Loading

This looks like something bad on the Windows CI

I think this might require some of the same build config tricks that rfc3161-client CI has... alternatively maybe wait for a rfc3161-client release and avoid installing from git

@woodruffw
Copy link
Member

This looks like something bad on the Windows CI

I think this might require some of the same build config tricks that rfc3161-client CI has... alternatively maybe wait for a rfc3161-client release and avoid installing from git

Yeah, I think we can prep another release. CC @DarkaMaul

@jku
Copy link
Member

jku commented Nov 20, 2024

/gcbrun

@woodruffw
Copy link
Member

/gcbrun

woodruffw
woodruffw reviewed Nov 25, 2024
View reviewed changes
.github/workflows/ci.yml
make test TEST_ARGS="-m timestamp_authority -rs" | tee output
! grep -q "skipping test that requires a Timestamp Authority" output || (echo "ERROR: Found skip message" && exit 1)
env:
SIGSTORE_TIMESTAMP: "v1.2.3"
Copy link
Member

Choose a reason for hiding this comment

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

I don't love that we pull down a binary here, and hardcode the version. It'd be great if we could get this into Dependabot somehow.

(Not a blocker, flagging as a follow-up.)

woodruffw
woodruffw reviewed Nov 25, 2024
View reviewed changes
sigstore/sign.py
fulcio: FulcioClient,
rekor: RekorClient,
trusted_root: TrustedRoot,
tsa_clients: List[TimestampAuthorityClient] | None = None,
Copy link
Member

Choose a reason for hiding this comment

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

Flagging: it's not ideal that this parameter list continues to grow; I think we could probably slice it down substantially by passing the entire ClientTrustConfig and doing the instantiations internally.

That'd be good for a follow-on refactor PR here.

woodruffw
woodruffw approved these changes Nov 25, 2024
View reviewed changes
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, nice work @DarkaMaul!

I left some non-blocking comments; the one about refactoring SigningContext's ctor in particular would be good for a follow-up PR.

@woodruffw woodruffw added component:signing Core signing functionality component:api Public APIs labels Nov 25, 2024
@woodruffw
Copy link
Member

This also needs a CHANGELOG entry, but I'm going to merge as-is and do a follow-up for that.

@woodruffw woodruffw enabled auto-merge (squash) November 25, 2024 17:09
@woodruffw
Copy link
Member

/gcbrun

@woodruffw woodruffw merged commit 383797a into sigstore:main Nov 25, 2024
23 checks passed
@woodruffw woodruffw deleted the dm/timestamp-signing branch November 25, 2024 17:14
woodruffw added a commit that referenced this pull request Nov 25, 2024
@woodruffw
CHANGELOG: record #1216
21ad848 
Signed-off-by: William Woodruff <[email protected]>
@DarkaMaul DarkaMaul mentioned this pull request Nov 26, 2024
Open
woodruffw added a commit that referenced this pull request Nov 26, 2024
@woodruffw
CHANGELOG: record #1216 (#1224)
3caf36f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woodruffw woodruffw woodruffw approved these changes

Assignees
No one assigned
Labels
component:api Public APIs component:signing Core signing functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DarkaMaul @jku @woodruffw