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 custom CA validation for Git over HTTPS #283

Merged
merged 1 commit into from
Feb 8, 2021

Conversation

phillebaba
Copy link
Member

@phillebaba phillebaba commented Feb 7, 2021

Adds the option to pass a caFile value in the credential secret to allow the use of self signed certificates. This feature will only work with the libgit2 https transport implementation as it has a certificate callback which allows for custom x509 certificate validation when the system is not able to. All other implementations will return an error if a user attempts to set a value for caFile in the secret.

@phillebaba
Copy link
Member Author

Need to add docs describing how to use this feature and its limitations.

@stefanprodan
Copy link
Member

@phillebaba this is different from what I proposed in the issue, like HelmRepositories, the certs files are keys in the auth secret. Why introduce a new field in the API?

@phillebaba
Copy link
Member Author

Sorry I must have misunderstood you, I will change it to read all the data from the same secret.

@stefanprodan
Copy link
Member

I don't see where are we loading the clientCert and clientKey. See https://github.com/fluxcd/image-reflector-controller/blob/main/controllers/imagerepository_controller.go#L281 for an example.

@phillebaba
Copy link
Member Author

This only implements CA validation so that you can use self signed certs. It does not implement client certificate authentication.

@@ -57,26 +61,49 @@ func (s *BasicAuth) Method(secret corev1.Secret) (*common.Auth, error) {
if d, ok := secret.Data["password"]; ok {
password = string(d)
}
if username == "" || password == "" {
return nil, fmt.Errorf("invalid '%s' secret data: required fields 'username' and 'password'", secret.Name)
if username != "" && password != "" {
Copy link
Member Author

Choose a reason for hiding this comment

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

This behavior change is needed as it should be possible to have cert validation on repositories with no authentication. This will change libgit2 behavior to not throw an error if username and password are missing if a secret is specified.

@stefanprodan
Copy link
Member

@phillebaba this doesn’t fixes the issue, it addresses only the CA, please change the description. Thanks

@phillebaba
Copy link
Member Author

Sure I will change it so it does not close the issue, as it only solved half of the issue.

Has anyone requested client certificate authentication or is just a feature that is needed to match fluxv1 functionality. I do not think we will be able to implement the second half of the feature any time soon judging by this issue.
libgit2/libgit2#4204 (comment)

@phillebaba phillebaba changed the title Add custom certificate validation Add custom CA validation for GitRepositories. Feb 7, 2021
@stefanprodan stefanprodan changed the title Add custom CA validation for GitRepositories. Add custom CA validation for Git over HTTPS Feb 7, 2021
pkg/git/v1/transport.go Outdated Show resolved Hide resolved
pkg/git/v1/transport.go Outdated Show resolved Hide resolved
@phillebaba phillebaba force-pushed the feature/self-signed-certs branch 2 times, most recently from 8abbc99 to 8a38d10 Compare February 7, 2021 16:21
@phillebaba phillebaba force-pushed the feature/self-signed-certs branch 2 times, most recently from 3b124f0 to 27dad1d Compare February 7, 2021 16:37
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.

This is looking good, couple of tiny nits from me that you should be used to by now 😬 💯

docs/spec/v1beta1/gitrepositories.md Outdated Show resolved Hide resolved
docs/spec/v1beta1/gitrepositories.md Outdated Show resolved Hide resolved
docs/spec/v1beta1/gitrepositories.md Outdated Show resolved Hide resolved
pkg/git/common/common.go Show resolved Hide resolved
Signed-off-by: Philip Laine <philip.laine@gmail.com>
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 @phillebaba 🥇

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.

Thank you @phillebaba 🥇

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

Successfully merging this pull request may close these issues.

3 participants