search

edgelesssys / contrast

Deploy and manage confidential containers on Kubernetes
https://docs.edgeless.systems/contrast
GNU Affero General Public License v3.0
161 stars 6 forks source link

generate: check for existing public key in manifest #650

Open davidweisse opened 5 days ago

davidweisse commented 5 days ago

If you call generate twice with the same seed share owner public key, the public key would just have been appended to the list instead of checking if the public key is already present in the manifest. Now, calling generate twice will result in the same manifest.

katexochen commented 5 days ago

I'm not sure about this. If I generate twice like this:

contrast generate --seedshare-owner-key=alice.pem deploy.yml
contrast generate --seedshare-owner-key=bob.pem deploy.yml

Would I really expect both alice and bob to be in the manifest? I think it might be less surprising if we cleared out the keys and then added what's given as flag.

We should match the behavior of workload owner keys (or update it, too).

davidweisse commented 4 days ago

I'm not sure about this. If I generate twice like this:

contrast generate --seedshare-owner-key=alice.pem deploy.yml
contrast generate --seedshare-owner-key=bob.pem deploy.yml

Would I really expect both alice and bob to be in the manifest? I think it might be less surprising if we cleared out the keys and then added what's given as flag.

This is the current behavior for workload owner keys, as far as I can see.

Freax13 commented 1 day ago

I'm not sure about this. If I generate twice like this:

contrast generate --seedshare-owner-key=alice.pem deploy.yml
contrast generate --seedshare-owner-key=bob.pem deploy.yml

Would I really expect both alice and bob to be in the manifest? I think it might be less surprising if we cleared out the keys and then added what's given as flag.

We could rename the flag to something like --add-seedshare-owner-key to make it more explicit to the user that we only add keys and don't clear them.

katexochen commented 1 day ago

We could rename the flag to something like --add-seedshare-owner-key to make it more explicit to the user that we only add keys and don't clear them.

I say that's a good solution, wdyt @burgerdev ?

burgerdev commented 17 hours ago

Calling the flags --add-*-key should manage expectations, I agree, so if there's no concrete use-case for idempotency of generate runs I won't object. When we get to actually implement seed sharing, I wonder what the UX would look like for many keys, though - maybe users would rather modify the manifest directly?