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

[WIP] Add test for GIT_SSL_CAINFO env var removal #315

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

karlkfi
Copy link
Contributor

@karlkfi karlkfi commented Nov 22, 2022

This confirms that the new SSA used to update the reconciler Deployment correctly deletes the GIT_SSL_CAINFO env var (as long as no one else has modified it).

Change-Id: Ib46708cc2cc7d8b92b694e3bea92e66ab4d0b737
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from karlkfi by writing /assign @karlkfi in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@karlkfi karlkfi requested review from sdowell and removed request for mikebz November 22, 2022 23:50
@karlkfi
Copy link
Contributor Author

karlkfi commented Nov 22, 2022

Gonna wait to merge this until @sdowell adds support for http access to git. That way we won't pollute the CACertSecretRef test with needing to use NoSSLVerify (which has its own test).

@@ -280,6 +280,25 @@ func TestCACertSecretRefV1Beta1(t *testing.T) {
if err != nil {
nt.T.Fatal(err)
}

// Set RepoSync to use HTTPS with no auth and SSL verification disabled.
// This validates that the GIT_SSL_CAINFO env var can be removed.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe line 266 should already be verifying this. It unsets caCertSecretRef and then checks that the env var has been removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically you're correct, but it also doesn't sync. I wanted one where it actually worked.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like you could add a DeploymentMissingEnvVar predicate to line 279 then, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough.

That said, I don't understand how 278 works without the CACertSecretRef. The SecretRef is a private key to authenticate the user, right? How does it validate the server cert without a CA file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does git@ scheme not validate the server cert by default??

Copy link
Contributor

Choose a reason for hiding this comment

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

caCertSecretRef is only used for verifying the authenticity of the server certificate over HTTPS. SSH is a different protocol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does SSH not validate the server cert? I thought it did.

Or is the git server configured to use a known CA when using SSH?

Copy link
Contributor

@sdowell sdowell Nov 23, 2022

Choose a reason for hiding this comment

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

Certificate authorities aren't involved with SSH protocol. They are used to sign certificates used in HTTPS. Authenticity over SSH is established using the public/private key pair

This is at least typically the case, to my understanding. There may be some exceptions where certificates are used with SSH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the connection setup phase, the SSH server authenticates itself to the client by providing its public key. This allows the SSH client to verify that it is actually communicating with the correct SSH server (instead of an attacker that could be posing as the server).

Weird... How can you trust that the SSH server is who they say they are if you get the public key from them instead of a trusted third party? In TLS, the server gives you its public key, but it has to have been signed by a CA you trust. It sounds like maybe SSH (or at least git?) doesn't have that layer of security by default, which is... really surprising.

@sdowell
Copy link
Contributor

sdowell commented Nov 23, 2022

@karlkfi This should add support for HTTP, although I'm still not sure whether it's needed #316

@mikebz
Copy link
Contributor

mikebz commented Apr 17, 2024

curious if this is still WIP or if it's even relevant. If it is consider closing the PR and keeping the private branch or converting to draft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants