Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Store publicKey in secret. #2916

Closed
wants to merge 1 commit into from
Closed

Store publicKey in secret. #2916

wants to merge 1 commit into from

Conversation

alaa
Copy link
Contributor

@alaa alaa commented Mar 16, 2020

Storing SSH PublicKey along with the Identity (Private Key) in the secret object in order to make it easier to query, instead of using fluxctl identity command or checking the logs of the fluxd container.

This can be useful while integrating the Flux installation with automation tools.

For example, Terrafrom installs Flux using Helm Provider and then use the Kubernetes provider to get the public key from the secret and store that in Github using github provider.

I understand the PublicKey is not actually a secret, and it could be stored in a ConfigMap for example. But maybe it is more convenient to keep both the Public and Identity keys at the same place.

@hiddeco
Copy link
Member

hiddeco commented Mar 26, 2020

Playing the devil's advocate here, but can you elaborate on why just the private key is not sufficient when the public key can be derived from this (using e.g. openssl rsa -in private.key -pubout -out public.key for RSA keys).

@alaa
Copy link
Contributor Author

alaa commented Mar 26, 2020

Sure, we can derive the public key from the private key. The problem is when we are automating the flux installation with Terraform. The TLS provider can only read certain private key formats. So if we wish to keep the default key format that Flux generates (#2911) we won't be able to use the official Terraform TLS provider (https://www.terraform.io/docs/providers/tls/index.html) in a nice and clean way.

We will have to do hacks around it to get it, and that includes copying the private key to execute the openssl command or use external command fluxctl identity which introduces an external dependency for the flux module.

But if we could just have the public key written along the private key then it will integrate very nicely with the automation system. like this one: (https://www.terraform.io/docs/providers/kubernetes/d/secret.html)

@hiddeco hiddeco requested a review from squaremo March 26, 2020 10:31
@squaremo
Copy link
Member

This will cover the case in which flux creates the secret; but, flux does not have exclusive access to the secret, and any other process (or more likely, person) can come and patch the resource. It may not matter for your use case, but having them both implies that they always correspond to each other, and sooner or later it will be taken for granted that's the case. So I worry that the secret and public keys will easily fall out of corresponding, and that will break assumptions made elsewhere.

I can see why this is a tidy solution to what you're trying to do, and similar situations. Is there some kind of failsafe that can be built in, to e.g., invalidate the public key entry if the secret key changes?

@alaa
Copy link
Contributor Author

alaa commented Mar 30, 2020

I see your point, I think there is no strong guarantees in Kubernetes secrets "in general" which correlates changes consistently, nor there is immutability.

So anyone can patch a Secret of ConfigMap and make a breaking change. However, it is the client responsibility to manage this if it regenerates the key.

Actually, I could think of a similar use-case where public key is needed to connect with external system. k8s cert manager.

https://cert-manager.io/docs/configuration/ca/

@squaremo
Copy link
Member

OK, I am in favour of this change, but I need to have a think about how to put caveats around it.

In the meantime, rebasing might fix the test failure (and needs doing in any case -- should be straight-forward).

@kingdonb
Copy link
Member

@alaa Are you still using Flux v1 / interested in merging this feature anymore?

I see the upstream fork has been deleted

@alaa
Copy link
Contributor Author

alaa commented Jan 28, 2021

@kingdonb Yes, I am still using v1 and would love to have this change in place. but will be migrating to v2 soon.
if you are in favor of this change I can try to fix this or make a new PR.

@kingdonb
Copy link
Member

A new PR would be fine, if you can't adjust this one (I don't know what happens to PR when upstream fork is deleted)

I'm glad to hear you are moving to Flux v2. If there is a way to get this merged, I will make sure it gets reviewed and that the maintainers see it. As long as it doesn't change any defaults or behavior, I think it would be possible, but I will definitely be trying to get all such chart changes together in one batch so that we don't have to do more than one chart release.

@alaa
Copy link
Contributor Author

alaa commented Jan 28, 2021

Because the fork upstream was deleted, I am cloning this PR into #3411 and closing this one.

@alaa alaa closed this Jan 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants