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

add 'cosign sign' command-line parameters for mTLS #3052

Merged
merged 7 commits into from
Jul 20, 2023

Conversation

dmitris
Copy link
Contributor

@dmitris dmitris commented Jun 13, 2023

Summary

Add command-line parameters for key/cert/cacert used for the connection to the TSA server. Initially this is done only for the cosign sign command - the other will be added in a follow-up PR.

Fixes #3006.

Release Note

  • New features and improvements, including behavioural changes, UI changes and CLI changes
    ** Additional command-line options for cosign command for mTLS connection to the TSA server

Documentation

sigstore/docs#192

@dmitris dmitris force-pushed the mtls-gh branch 2 times, most recently from e06f505 to 48cd2e4 Compare June 13, 2023 14:21
@codecov
Copy link

codecov bot commented Jun 13, 2023

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              
Impacted Files Coverage Δ
cmd/cosign/cli/options/sign.go 0.00% <0.00%> (ø)
cmd/cosign/cli/sign.go 0.00% <0.00%> (ø)
cmd/cosign/cli/sign/sign.go 14.25% <0.00%> (-0.28%) ⬇️

Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

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

@dmitris
Copy link
Contributor Author

dmitris commented Jul 10, 2023

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/[email protected]/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 😄

@dmitris dmitris force-pushed the mtls-gh branch 3 times, most recently from 82b6509 to 39528b3 Compare July 14, 2023 23:24
@dmitris dmitris force-pushed the mtls-gh branch 4 times, most recently from 70aeec8 to 85aa387 Compare July 15, 2023 00:03
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

Great work on the tests, thank you for adding this!

test/e2e_tsa_mtls.sh Outdated Show resolved Hide resolved
test/e2e_tsa_mtls.sh Outdated Show resolved Hide resolved
Add command-line parameters for key/cert/cacert used for
the connection to the TSA server.

Fixes sigstore#3006

Signed-off-by: Dmitry S <[email protected]>
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

Really appreciate all of your work on this!

@haydentherapper
Copy link
Contributor

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

@znewman01 znewman01 merged commit f633221 into sigstore:main Jul 20, 2023
@github-actions github-actions bot added this to the v2.1.1 milestone Jul 20, 2023
@dmitris dmitris deleted the mtls-gh branch July 20, 2023 17:27
@cpanato cpanato modified the milestones: v2.1.1, v2.2.0 Jul 28, 2023
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.

add support for mTLS connection to the TSA (timestamp server)
5 participants