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.

Sign up for GitHub

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
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions e2e/testcases/private_cert_secret_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

// TODO: switch to HTTP instead of HTTPS, when supported by the local git server nginx proxy. That way we're not testing NoSSLVerify in the CACertSecretRef test.
repoSyncBackend.Spec.Git.Repo = repoSyncHTTPS
repoSyncBackend.Spec.Git.Auth = "none"
repoSyncBackend.Spec.Git.NoSSLVerify = true
repoSyncBackend.Spec.Git.SecretRef = nil
repoSyncBackend.Spec.Git.CACertSecretRef = nil
nt.RootRepos[configsync.RootSyncName].Add(nomostest.StructuredNSPath(backendNamespace, configsync.RepoSyncName), repoSyncBackend)
nt.RootRepos[configsync.RootSyncName].CommitAndPush("Update backend RepoSync use HTTPS with no auth")
nt.WaitForRepoSyncs()
err = nt.Validate(reconcilerName, v1.NSConfigManagementSystem, &appsv1.Deployment{},
nomostest.DeploymentMissingEnvVar(reconcilermanager.GitSync, "GIT_SSL_CAINFO"),
nomostest.DeploymentHasEnvVar(reconcilermanager.GitSync, "GIT_SYNC_REPO", repoSyncHTTPS),
nomostest.DeploymentHasEnvVar(reconcilermanager.GitSync, "GIT_SSL_NO_VERIFY", "true"))
if err != nil {
nt.T.Fatal(err)
}
}

func TestCACertSecretWatch(t *testing.T) {
Expand Down