akuity / kargo

Application lifecycle orchestration
https://kargo.akuity.io/
Apache License 2.0
1.8k stars 148 forks source link

Add MTLS support #1808

Open obeyler opened 7 months ago

obeyler commented 7 months ago

Checklist

Proposed Feature

On Create Credentials, we should have the opportunity to specify a client certificate ( CRT,KEY, CA) to be able to connect on repository protected by MTLS.

Motivation

In our case both our self hosted Gitlab and Harbor registry (docker image and helm) are protected by MTLS and login/password

Suggested Implementation

krancour commented 7 months ago

@obeyler even though this doesn't seem like a lot of work, before going too far down this rabbit hole, I am wondering if this problem isn't best addressed with a service mesh.

jessesuen commented 7 months ago

I think in their use case, mTLS is being used a second layer of client authentication to that specific server. So even if the kargo pods were meshed with gitlab/harbor servers, the user still may only give the client certificates to select, trusted clients, and not everything on the mesh.

Does anyone know if it's possible for the client certificates used in mTLS to be dropped into well-known operating system paths (e.g. /etc/ssl/certs) to be picked up by applications like curl, wget, golang's HTTP SDK? Similar to how you can drop in root certificates at the OS level? If this is possible, I wonder if kargo itself could be volumeMounted with the client certs and automatically pick them up without an explicit feature needed to be developed.

krancour commented 7 months ago

I think in their use case, mTLS is being used a second layer of client authentication to that specific server.

Right. But it's the fact of it being a second factor rather than the only factor, that raised for me the possibility that the requirement possibly could be met with a mesh. It's what prompted the request for more details.

I can equally see the possibility that a mesh solves the problem here or that it doesn't.

the user still may only give the client certificates to select, trusted clients, and not everything on the mesh.

It's been a while since I've gone deep on any meshes, but I think routing rules can constrain a service to permitting ingress only from specific services or namespaces rather than simply permitting ingress from any service in the mesh.

Does anyone know if it's possible for the client certificates used in mTLS to be dropped into well-known operating system paths (e.g. /etc/ssl/certs) to be picked up by applications like curl, wget, golang's HTTP SDK? Similar to how you can drop in root certificates at the OS level? If this is possible, I wonder if kargo itself could be volumeMounted with the client certs and automatically pick them up without an explicit feature needed to be developed.

If a mesh doesn't meet the need, this may not either (if it's even possible, that is) because this would be roughly equivalent to using a mesh insofar as all the outbound traffic would use a single client cert as opposed to selecting a specific client cert from a credential/Secret.

obeyler commented 7 months ago

I think mesh service is a little bit oversized for this and implies a lot of other constraint. I hope that as argocd did it for git or helm repository for might be easy to be inspire image

obeyler commented 7 months ago
apiVersion: v1
kind: Secret
type: Opaque
metadata:
  name: kargo-demo-repo
  namespace: kargo-demo
  labels:
    kargo.akuity.io/cred-type: git
stringData:
  repoURL: ${GITOPS_REPO_URL}
  username: ${GITHUB_USERNAME}
  password: ${GITHUB_PAT}
  tlsClientCertData: ##optional value
  tlsClientCertKey ## optional value

can be possible yaml to allow git or an docker registry protected behind a MTLS, ( same as argocd does)

krancour commented 7 months ago

@obeyler thanks. I don't see this as being a problem. I just wanted to be sure that the notion of a service mesh wasn't being completely overlooked in the event that it was a viable option.

raphaelappsolute commented 7 months ago

+1 to increase priority

ManuHeel commented 6 months ago

+1 same issue here

juleshry commented 6 months ago

+1

m3astwood commented 6 months ago

Another +1 interested

timbtimbtimb commented 6 months ago

+1

krancour commented 6 months ago

It is clear to us that this is a popular feature request and we are factoring that into our planning. To spare the team from some noise, further +1s aren't going to move the needle any more than it's already been moved.

obeyler commented 2 months ago

Hi, I'm feel sad to see that this issue is now on low priority even if it's looks popular. If we plan to propose a PR to solve it can we expect to be merged ?

krancour commented 2 months ago

@obeyler priorities are relative things. Right now, we've basically moved anything to low if it's something we plan to do post-GA.

Contributions are always welcome and do tend to accelerate things, but do always require review, of course, which can still be time consuming. I cannot make a guarantee that maintainers will be able to prioritize reviewing this feature pre-GA any more than we could prioritize working on it.

Again, with all things being relative, if the feature is thorough, documented, and implemented well, it is faster to get such a thing merged than if maintainers have to invest a lot of time advocating for revisions. tl;dr the answer to "can we expect to be merged" depends entirely on what the contributor puts into it.