-
Notifications
You must be signed in to change notification settings - Fork 23
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
Support custom certificate configuration while interacting with central repository #234
Support custom certificate configuration while interacting with central repository #234
Conversation
fc8c2b4
to
cb0a6d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuing the review, but here are a few comments to start.
Thanks @prkalle for this PR.
cb0a6d2
to
d01c860
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if a user will understand they can use a hostname:port
form when we use the term hostname all the time? Would having --port
flag (which we would then join to the hostname before storing it in the config file)?
We can use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was really great @prkalle !
Two questions and some final in-line comments. Thanks!
- I'm not really sure, but do we need any special handling for a Proxy? Could there be a proxy in between the CLI and the central repo?
- The builder plugin uses
imgpkg
directly. Does it also need to respect the newcert
configuration? See: https://github.com/vmware-tanzu/tanzu-cli/blob/main/cmd/plugin/builder/imgpkg/imgpkg.go
Go by default handles the Proxy (User can set environment variables, HTTP_PROXY,HTTPS_PROXY,NO_PROXY). So we should be good. I did double check the imgpkg library, it does uses the DefaultTransport which handles the proxy.
This is a good point. I see currently, developer can use these plugins to test locally, where the local repository is instantiated without HTTPS port. The other usecase would be , Core CLI would use these plugin publish command to publish to other harbor repositories (which are currently signed with well known CA). I think we should respect the new cert configuration for these as well, but we can discuss before updating the same. |
aaf4ac3
to
06d71be
Compare
03daaba
to
def5b7a
Compare
Is it possible to generate the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all look good to me @prkalle .
As for the new co-existence tests failing, we may need to set the TANZU_CLI_PLUGIN_DISCOVERY_IMAGE_SIGNATURE_PUBLIC_KEY_PATH
as you did for the E2E tests.
@if [ ! -d $(ROOT_DIR)/hack/central-repo/registry-content ]; then \ | ||
(cd $(ROOT_DIR)/hack/central-repo && tar xjf registry-content.bz2 || true;) \ | ||
fi | ||
@docker run --rm -d -p 9876:5000 --name central \ | ||
@docker run --rm -d -p 9876:443 --name central \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking: But something to enhance in future if we decide to do so.
Generally, Convention is to use the 9443
port to run this type of server for testing. Basically 9443
port gives users the idea that the server is backed by https
and not http
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know of 8443 as substitute for 443(https port). Would like to know if there is any reference document for the 9443
port convention. I checked the IANA, but I couldn't get more information about the convention.
227fa95
to
7276474
Compare
Filed an issue :#249 for the same. Generating certs for test local central registry endpoint CA cert shouldn't be an issue. However co-sign keys generation and especially signing the local test central repository with cosign key would be tricky( since it would be time consuming, because currently we are using pregenerated registry contents with cosign signatures) but not impossible. [Update]: private keys (for both local test central repo cert private key and test central repo plugin inventory image co-sign key) would not be added to git repository until we work on the issue filed. |
7276474
to
15b4170
Compare
Signed-off-by: Prem Kumar Kalle <[email protected]>
… https port - Add cosign signatures to the local test repositories - Updated local test central repository to serve on HTTPS port using the self-signed certs - Add cosign binary to hack/tools/bin - Regenerated the test central repos with DB images signed using cosign generated key-pairs Signed-off-by: Prem Kumar Kalle <[email protected]>
- Add new tanzu config cert command and also added add/update/delete/list subcommands under cert commands. User can use this command to add custom certificate configuration and can be consumed by core CLI and plugins Signed-off-by: Prem Kumar Kalle <[email protected]>
…acting with central repository - Added functionality to use the custom certificate configuration(configured by the user using tanzu config cert command) while interacting with central repository endpoint having self-signed certs/expired certs Signed-off-by: Prem Kumar Kalle <[email protected]>
15b4170
to
9b84b0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment regarding the recent pipeline failure. Otherwise looks good.
Thanks for being flexible and fixing all the comments.
Signed-off-by: Prem Kumar Kalle <[email protected]>
- Starting the local OCI registry have dependency on tanzu CLI binary to add the test local central repository certs to the config before starting the local OCI registry. - Updated the co-existence tests to use the test local central repository certs Signed-off-by: Prem Kumar Kalle <[email protected]>
9b84b0d
to
83a45c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. 👏
What this PR does / why we need it
This PR adds support for custom certificate configuration for a given host.
Which issue(s) this PR fixes
Fixes #12, 133
Describe testing done for PR
tanzu plugin search
should show error that test central repo SSL cert is not signed by known authoritytanzu plugin search
should be successful and should show the available plugins and alsotanzu plugin install
should install the plugin successfully--skip-cert-verify
and--insecure
localhost:9876
Release note
Additional information
May have to raise another PR if checking in the test certs flags any security vulnerabilities.
Special notes for your reviewer