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

fix encoding of RFC3161 timestamps #118

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

bdehamer
Copy link
Contributor

@bdehamer bdehamer commented Jan 5, 2024

Updates the d.txt.good.sigstore and d.txt.tsa-timestamp-error.sigstore bundles with new RFC3161 timestamp values.

The original timestamps each included an encoding quirk (constructed OCTET STRINGs vs. pure, DER-encoded OCTET STRINGs) that didn't quite match the structure of the timestamps generated by the timestamp-authority service. The updated timestamp values included here fix the encoding issue and look more the timestamps you'd receive from TSA (take a look at sigstore/sigstore-js#912 if you wanna see the specifics of the encoding change).

The timestamp in d.txt.good.sigstore has a signing time that falls within the validity period of the bundle's Fulcio-issued signing certificate:

cat test/assets/d.txt.good.sigstore | \
  jq -r '.verificationMaterial.timestampVerificationData.rfc3161Timestamps[0].signedTimestamp' | \
  base64 -d | \
  openssl ts -reply -in /dev/stdin -text

Using configuration from /opt/homebrew/etc/openssl@3/openssl.cnf
Status info:
Status: Granted.
Status description: unspecified
Failure info: unspecified

TST info:
Version: 1
Policy OID: 1.3.6.1.4.1.57264.2
Hash Algorithm: sha256
Message data:
    0000 - f3 71 7e cc e4 61 e6 00-ac da d8 fe 1d eb d8 e3   .q~..a..........
    0010 - 25 1c a9 4b d7 be 98 c6-8a 75 88 c4 53 d2 a2 d2   %..K.....u..S...
Serial number: 0x-21524111
Time stamp: Feb  1 00:00:00 2023 GMT
Accuracy: 0x01 seconds, unspecified millis, unspecified micros
Ordering: no
Nonce: 0x499602D2
TSA: unspecified
Extensions:

The timestamp in d.txt.tsa-timestamp-error.sigstore has a signing time that falls OUTSIDE the validity period of the bundle's Fulcio-issued signing certificate:

cat test/assets/d.txt.tsa-timestamp-error.sigstore | \ 
  jq -r '.verificationMaterial.timestampVerificationData.rfc3161Timestamps[0].signedTimestamp' | \
  base64 -d | \
  openssl ts -reply -in /dev/stdin -text

Using configuration from /opt/homebrew/etc/openssl@3/openssl.cnf
Status info:
Status: Granted.
Status description: unspecified
Failure info: unspecified

TST info:
Version: 1
Policy OID: 1.3.6.1.4.1.57264.2
Hash Algorithm: sha256
Message data:
    0000 - 65 fe 77 25 a7 76 f4 cd-4b 7e cf 6a 37 0b c7 42   e.w%.v..K~.j7..B
    0010 - 1e 51 23 13 2d 94 0f 33-3d 7d 55 73 95 2f 51 e7   .Q#.-..3=}Us./Q.
Serial number: 0x-21524111
Time stamp: Feb  2 00:00:00 2023 GMT
Accuracy: 0x01 seconds, unspecified millis, unspecified micros
Ordering: no
Nonce: 0x499602D2
TSA: unspecified
Extensions:

Other than changes to the signedTimestamp values, the bundles are unchanged.

I tested these changes against both sigstore-js and sigstore-go and got the expected results.

Signed-off-by: Brian DeHamer <bdehamer@github.com>
@haydentherapper
Copy link
Collaborator

Just to check, there isn’t a bug in the Sigstore implementation of the TSA, correct? Have we confirmed that the DER encoding matches the RFC?

@bdehamer
Copy link
Contributor Author

bdehamer commented Jan 5, 2024

Just to check, there isn’t a bug in the Sigstore implementation of the TSA, correct? Have we confirmed that the DER encoding matches the RFC?

@haydentherapper, no, the Sigstore TSA implementation is fine. I've done a fairly meticulous comparison of the TSA-generated timestamps to the spec at this point and things look good (though there was one very minor issue I did uncover).

I'm pretty sure that the encoding issue I'm addressing here isn't even significant as I was able to verify the previous timestamps successfully with both openssl and the timestamp-cli. It was just bugging me that it didn't match exactly the TSA-issued timestamps.

Copy link
Member

@steiza steiza left a comment

Choose a reason for hiding this comment

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

Nice find!

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.

Thanks @bdehamer!

Any thoughts on the value of a backstop/negative test here? I'm assuming clients that support TSA timestamps should reject non-RFC 3161 encodings, but it looks like there was malleability here?

@woodruffw woodruffw merged commit 0d14f2d into sigstore:main Jan 5, 2024
3 checks passed
@bdehamer bdehamer deleted the bdehamer/timestamp-update branch January 5, 2024 15:25
@haydentherapper
Copy link
Collaborator

@bdehamer Thanks, I'll go update the TSA service now (dependabot didn't pick up the change since we're using commits)

@woodruffw woodruffw mentioned this pull request Jan 5, 2024
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.

4 participants