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 Git test coverage for supported algorithms #708

Merged
merged 2 commits into from
May 9, 2022
Merged

Conversation

pjbgf
Copy link
Member

@pjbgf pjbgf commented May 6, 2022

Provides test coverage for the supported algorithms for go-git and libgit2 (Managed Transport):

  • Authentication Key Types

    • rsa
    • ecdsa P256
    • ecdsa P384
    • ecdsa P521
    • ed25519
  • Key Exchange Algoritms:

    • diffie-hellman-group14-sha1
    • diffie-hellman-group14-sha256
    • curve25519-sha256
    • ecdh-sha2-nistp256
    • ecdh-sha2-nistp384
    • ecdh-sha2-nistp521
    • curve25519-sha256@libssh.org
  • HostKey Algoritms:

    • ssh-rsa
    • rsa-sha2-256
    • rsa-sha2-512
    • ecdsa-sha2-nistp256
    • ecdsa-sha2-nistp384
    • ecdsa-sha2-nistp521
    • ssh-ed25519

These tests will provide Flux the guarantees that the algorithms above are supported. When our upstream dependencies (e.g. libgit2, go crypto) withdraw such support, we should be able to identify and warn users accordingly.

xref: fluxcd/flux2#2593

@pjbgf pjbgf added the area/git Git related issues and pull requests label May 6, 2022
@pjbgf pjbgf added this to the GA milestone May 6, 2022
Copy link
Member

@aryan9600 aryan9600 left a comment

Choose a reason for hiding this comment

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

Really awesome stuff! 💯

pkg/git/gogit/checkout_test.go Outdated Show resolved Hide resolved
pkg/git/options.go Show resolved Hide resolved
@pjbgf pjbgf force-pushed the algo-tests branch 2 times, most recently from f1c8dd3 to 90c8be6 Compare May 6, 2022 14:12
Enables the setting of HostKey algorithms to be used from
a client perspective. This implementation supports go-git
and libgit2 when in ManagedTransport.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @pjbgf

}

// lastly, ensure that if the authentication was not
// successful, an error is returned.
Copy link
Contributor

@darkowlzz darkowlzz May 9, 2022

Choose a reason for hiding this comment

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

Maybe optional:

Instead of a separate section at the end here, this can be consolidated within the above subtests with a few more fields in the test cases struct, maybe also following the pattern we follow in a lot of our other tests where we run beforeFunc just before the function that we are testing and apply any configurations accordingly.

In this case, I think we can update the tests struct to something like:

	tests := []struct {
                name string
		keyType ssh.KeyPairType
                beforeFunc func(publicKey []byte)
                wantErr bool
	}

name can be brought back to describe the case, especially when we have a failure case.

beforeFunc can be called before checkout and used to set authorizedPublicKey, similar to other usage of beforeFunc, for example this.

This will make it easier to add more different test cases.

if v, ok := os.LookupEnv("EXPERIMENTAL_GIT_TRANSPORT"); ok {
return strings.ToLower(v) == "true" || v == "1"
}
return false
}

// SetEnabled allows for the managed transport state to be changed on demand.
// This is mainly used to enable testing.
Copy link
Contributor

@darkowlzz darkowlzz May 9, 2022

Choose a reason for hiding this comment

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

If this is just for testing, I wonder if it'd be better to just write a helper functions that sets and unsets the env var instead of introducing conditionals in the main code that'll only be used for testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a fair point, I removed the SetEnabled and just set the environment variable for the time being.

I haven't unset the environment variable as this has an ever lasting side effect. Once enabled, unmanaged transport is overwritten by the managed transport, so unsetting the variable may lead to an in-between state.

I am working towards having managed transport enabled by default soon (maybe next minor), so this will be flipped around.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Would be good to leave a comment about why it's not being unset at present. Usually we unset all the env vars set in tests.

@pjbgf pjbgf force-pushed the algo-tests branch 2 times, most recently from f8bd8be to 6b64fd3 Compare May 9, 2022 10:08
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

LGTM 🍍

pkg/git/gogit/checkout_test.go Show resolved Hide resolved
pkg/git/options.go Outdated Show resolved Hide resolved
pkg/git/options.go Outdated Show resolved Hide resolved
Assures support for:
- Authentication Key Types
  - rsa
  - ecdsa P256
  - ecdsa P384
  - ecdsa P521
  - ed25519
- Key Exchange Algoritms:
  - diffie-hellman-group14-sha1
  - diffie-hellman-group14-sha256
  - curve25519-sha256
  - ecdh-sha2-nistp256
  - ecdh-sha2-nistp384
  - ecdh-sha2-nistp521
  - curve25519-sha256@libssh.org
- HostKey Algoritms:
  - ssh-rsa
  - rsa-sha2-256
  - rsa-sha2-512
  - ecdsa-sha2-nistp256
  - ecdsa-sha2-nistp384
  - ecdsa-sha2-nistp521
  - ssh-ed25519

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
@pjbgf pjbgf merged commit 43235df into fluxcd:main May 9, 2022
@pjbgf pjbgf deleted the algo-tests branch May 9, 2022 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/git Git related issues and pull requests
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants