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

git: refactor authentication, checkout and verification #462

Merged
merged 15 commits into from
Oct 28, 2021
Merged

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented Oct 23, 2021

This PR refactors the git package, so that the interface and generics no longer depend on implementation specifics. Commit metadata, (PGP) commit verification, and authentication options have been standardized into various structs and methods.

User-facing Changes

Commit reference without branch

In the previous iteration it was not possible to define a Git commit reference without defining a branch. This limitation has been removed. However, using a branch if the information is available is beneficial for the go-git Git implementation, as this allows for a shallow clone. For libgit2, the branch is no longer taken into account.

If just a Git commit reference is defined, the Artifact revision becomes HEAD/<commit>

Fixes #407
Fixes #315

@hiddeco hiddeco force-pushed the git-auth-opts branch 10 times, most recently from f7d7c9a to e5a548f Compare October 24, 2021 09:34
@hiddeco hiddeco changed the title git: refactor AuthStrategy into AuthOptions git: refactor authentication, checkout and verification Oct 24, 2021
@hiddeco hiddeco marked this pull request as ready for review October 24, 2021 09:38
@hiddeco hiddeco added area/git Git related issues and pull requests enhancement New feature or request labels Oct 24, 2021
@hiddeco hiddeco force-pushed the git-auth-opts branch 2 times, most recently from a3bff1c to d9a8004 Compare October 24, 2021 10:10
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 @hiddeco 🏅

PS. I suggested some docs improvements but it's non blocking.

if short := strings.SplitAfterN(c.Reference, "/", 3); len(short) == 3 {
return fmt.Sprintf("%s/%s", short[2], c.Hash)
}
return fmt.Sprintf("HEAD/%s", c.Hash)
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to document the revision format in docs/docs/spec/v1beta1/common.md for each source type and give some examples for Git.

hiddeco and others added 13 commits October 27, 2021 00:43
This commit moves the previous `AuthStrategy` wiring to a more generic
`AuthOptions`, breaking free from implementation specific details in
the `git` package.

Signed-off-by: Hidde Beydals <hello@hidde.co>
Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit refactors the previous `Commit` interface into a
standardised `Commit` struct. This object contains sufficient
information for referencing, observating and (PGP) verification.

- `libgit2` commit checkout does now return `HEAD/<SHA1>` as
  the branch is not taken into account.
- `git2go` objects are now properly `Free`d everywhere
- `Verify` logic is tested.

Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit changes the `gogit` behavior for commit checkouts,
now allowing one to reference to just a commit while omitting any
branch reference. Doing this creates an Artifact with a
`HEAD/<commit>` revision.

If both a `branch` and `commit` are defined, the commit is expected
to exist within the branch. This results in a more efficient clone
of just the target branch, and also makes this change backwards
compatible.

Fixes #407
Fixes #315

Signed-off-by: Hidde Beydals <hello@hidde.co>
Main requirement for this is the image-automation-controller
depending on being able to get a working auth configuration.

Once the package is moved, we should add push logic to it,
so that the controller is able to use that instead.

Signed-off-by: Hidde Beydals <hello@hidde.co>
Adds more test cases for Validate() and an error for unknown transport.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
Adds tests for git.CheckoutStrategy to check if both the git
implementations work with all the authentication methods.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
As we properly nest errors.

Signed-off-by: Hidde Beydals <hello@hidde.co>
Signed-off-by: Hidde Beydals <hello@hidde.co>
This changes the logic of `credentialsCallback` so that it takes the
`allowedTypes` passed on by `git2go` into account. Reason for this
change is because this prepares it to work with `v33`, but also
because it can provide better guidance when `libgit2` has been
compiled with a different configuration, which e.g. doesn't
allow for "in-memory SSH keys".

Because `AuthOptions#Identity` now gets validated by the callback
and go-git does its own validaiton, the check has been removed
from `Validate` (and now does a simple check if the fields are set).

Signed-off-by: Hidde Beydals <hello@hidde.co>
Adds tests for git.CheckoutStrategy to check if both the git
implementations follow the same SemVer tag selection rules.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
For ssh, Host field is required in AuthOptions.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
New gittestserver fixes the issue with custom branch in an
initialized repo.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
darkowlzz and others added 2 commits October 27, 2021 16:59
Update GitRepositoryReconciler to use a nil authOpts unless it's
configured.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
Signed-off-by: Hidde Beydals <hello@hidde.co>
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.

💯

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 enhancement New feature or request
Projects
None yet
3 participants