Codecov Report

Merging #3052 (3460165) into main (2e9d1d9) will decrease coverage by 0.09%.
The diff coverage is 0.00%.

❗ Current head 3460165 differs from pull request most recent head 4bcc9f5. Consider uploading reports for the commit 4bcc9f5 to get more accurate results

@@            Coverage Diff             @@
##             main    #3052      +/-   ##
==========================================
- Coverage   30.64%   30.55%   -0.09%     
==========================================
  Files         155      155              
  Lines        9791     9817      +26     
==========================================
  Hits         3000     3000              
- Misses       6341     6367      +26     
  Partials      450      450              

This looks good to me. Do you have any thoughts on how we can add some tests?

thanks for the review and sorry for a delayed reply @haydentherapper. I admit finding the automated testing of this feature somewhat hard because it requires rolling out some "test PKI" and also having (or at least mocking) the TSA server that uses certificates and does validation of the certificates in the clients' requests.

So far I have been testing this change manually - by starting the timestamp-server TSA server from https://github.com/sigstore/timestamp-authority with the --tls[*] parameters (a sample command below) and then running cosign with additional parameters from this PR and pointing to that TSA server. I made a testing script https://github.com/dmitris/cosign-keyless/blob/main/run-tls.sh for this scenario (mTLS connection cosign to TSA) - but I'm not aware of any public instance of a TSA server that would be using TLS params, so I left it as "replace this with your TSA server using TLS certificates".

https://github.com/sigstore/cosign/blob/main/internal/pkg/cosign/tsa/client/client.go currently doesn't have unit tests - the value of adding them for this change would probably be small without the "full cycle" test (using either the TSA server or mock).

Regarding the functional (end-to-end) tests - it should be possible to modify the return value of https://pkg.go.dev/github.com/sigstore/timestamp-authority/pkg/server#NewRestAPIServer (used for example in https://github.com/sigstore/cosign/blob/main/test/e2e_test.go#L755) to set the TLS* parameters (from https://pkg.go.dev/github.com/sigstore/timestamp-authority@v1.1.1/pkg/generated/restapi#Server.TLSCertificate) and therefore turn it into a "mTLS-enabled TSA server", and then use whatever is needed to pass the mTLS-related client parameters for the TSA server connection in the sign command.

I wonder if we could maybe merge the PR based on the manual testing - and create an issue to expand the e2e tests to cover this use case which either I myself or possibly someone else could implement 😄

@znewman01 Any thoughts, otherwise this is good to merge from my side.