search

eclipse-archived / codewind

The official repository of the Eclipse Codewind project
https://codewind.dev
Eclipse Public License 2.0
113 stars 45 forks source link

cwctl needs to persist docker registry secret creds in keystore for local #1306

Open maysunfaisal opened 4 years ago

maysunfaisal commented 4 years ago

Codewind version: OS:

Portal Driver: 0.10.0 IDE Driver: 0.11.0

Che version: IDE extension version: IDE version: Kubernetes cluster:

Description of the enhancement:

With https://github.com/eclipse/codewind/issues/665, we now have registry secrets API /api/v1/registrysecrets that takes url,username,password and creates a docker config on PFE at /root/.docker/config.json

On Kube, this was already being done since early Microclimate days to date for Codewind i.e.; registry secrets are at /root/.docker/config.json mounted from the Che secret for Codewind/Microclimate Kube Secret for Microclimate. But with /api/v1/registrysecrets, we're no longer relying on mounting secrets because the API does this for us(ie manually create /root/.docker/config.json, create Kube secret with encoded /root/.docker/config.json and patch the Service Account) and is an ideal flow for both Che and Remote cases on Kube. The API also has persistence for Kube whereby, if a Pod goes down or restarts, PFE entrypoint will check for a Kube secret and decode its data and re-create docker config at /root/.docker/config.json which is fine.

But on local, since we have no Kube Secrets, any data written to /root/.docker/config.json is lost if the PFE container goes down.

Proposed solution:

cwctl can wrap the PFE API calls for IDE.

For local, when cwctl calls POST /api/v1/registrysecrets, cwctl can persist the API request data ie url, username and password into the keystore since cwctl is doing this for connection credentials.

During PFE startup, cwctl can then search the keystore for these saved credentials and in turn call POST /api/v1/registrysecrets so the PFE container will have the docker config on startup.

For more info, the /api/v1/registrysecrets is documented in the open api schema.

makandre commented 4 years ago

Can cwctl also do a local docker login as well?

maysunfaisal commented 4 years ago

I guess the other thing to keep in mind would be, during PFE startup if there are projects on the volume; we should not kick off project create until the docker config is created. Since the docker volume may have appsody projects and will fail if project create is done before docker config is restored.

makandre commented 4 years ago

I had a chat with @sghung and he brought up a good point; this would not excuse us from doing local persist, but it may not be as bad without it in 0.7 for Appsody.

Assuming cwctl does a local docker login when user set the credentials, and it is persisted in the system keychain:

  1. when user create/binds an appsody project, cwctl runs appsody init, which would pull down the stack image from the private registry using the local creds
  2. PFE then would not need to pull down the image again, since it's talking to the host system docker daemon and the image is already available

But this can fail if:

  1. User shutdown Codewind
  2. They then delete the appsody stack image
  3. When Codewind starts again and it runs the appsody project, it would then need to pull the image again (and would fail w/o creds inside PFE)
jopit commented 4 years ago

I assume this should be assigned to portal...

jopit commented 4 years ago

We already have a solution working for PFE running on Kube, this issue is purely for the local scenario.

Do we want to expose/use the /api/v1/registrysecrets api, or do we want to somehow 'mount' the secrets into the container? Would mounting the secret be a security exposure?

If we use the registrysecrets api, does that open a timing window for builds etc.

Possible alternative, do the login for them, parse the docker config file to find the list of registries and make pfe aware of those? Probably not appropriate as it may not be possible to access the credentials.

Portal team to investigate and come back with a proposal.

Requiements

hhellyer commented 4 years ago

I've looked into this some more and come up with a proposed design:

The plan is to store docker credentials in the keychain but they could also be in a known location in the codewind docker volume, or in the ~/.codewind workspace. There's a question over whether storing these in the keychain actually buys us any security - the credentials will be stored in plain text inside the docker container in dockers own config file. That makes them visible to anyone with access to docker on the machine but this is only for single user docker Codewind which we expect to be on a users laptop or workstation.

The addition of a cwctl command to call api/v1/registrysecrets can be done first, this will allow the editor plugins to switch to using it first. The local persistence and startup changes can then be added to the new cwctl command and we shouldn't need any further changes to the UI plugins while that is added.

tobespc commented 4 years ago

@hhellyer looks good (and the design is ok as well)

hhellyer commented 4 years ago

@maysunfaisal @makandre - Could you take a look at this and see if the design I put above looks ok? If it is we'll want to move the editor plugins over to using cwctl to get/set/delete registries.

maysunfaisal commented 4 years ago

I wouldnt store docker credentials anywhere other than a keychain or in /root/.docker/config.json . I think the argument that it does not buys any security is not right. It is docker’s limitation that they’re storing credentials in plaintext/base64 but that doesn’t mean we should store them anywhere we would like. So I am against the idea of storing it in docker-volume or in ~/.codewind-workspace and open to discussion.

hhellyer commented 4 years ago

The changes to store the docker credentials in the local keychain are currently backed out as there is no key chain on jenkins. The work under https://github.com/eclipse/codewind/issues/1735 should provide an insecure keychain we can use in these cases and I will rebase https://github.com/eclipse/codewind-installer/pull/394 on those once they are delivered.

hhellyer commented 4 years ago

The changes to store the credentials in the keychain have been added back in. They have been integrated with the changes to provide a local, insecure keychain when running on a system with no keychain service. This is being used on Jenkins as the machines the builds run on there do not provide a keychain.

The credentials are persisted (or deleted) automatically when existing the cwctl registrysecret subcommands are invoked. (These were added in https://github.com/eclipse/codewind-installer/pull/387 ) The UI plugins should get this functionality for free if they are using those commands already instead of calling /api/v1/registrysecrets directly.

The only remaining item is to call docker login outside PFE when a new docker credential is added to make sure we are logged in before appsody init is run for appsody projects.

tetchel commented 4 years ago

So, if using one of our keychain'd features (local image registry, or logging into a remote cluster) on a keychain-less system, where is the data insecurely stored?

If we are storing the credentials insecurely (are they hashed? in plain text?) there should probably be some sort of warning step, so the user is aware of the risk.

What does docker do for docker login on keychainless systems when it writes to .docker/config.json ? do they have a warning?

edit: you have to explicitly enable insecure keyring mode, that's great :+1:

rwalle61 commented 4 years ago

@tetchel the data will be insecurely stored in a base64-encoded insecureKeychain.json, written to the ~/.codewind/config dir (same place as our connections.json)

Don't know what docker do

tetchel commented 4 years ago

I think docker does the same thing.

eharris369 commented 4 years ago

Eclipse IDE PR: https://github.com/eclipse/codewind-eclipse/pull/625

j-c-berger commented 4 years ago

Hi all, I'm touching base to see what ID's role needs to be for this issue. Let me know how we can help. Thanks!

@sishida, please assign me, thanks!

hhellyer commented 4 years ago

I had a chat with @j-c-berger about this. I'm going to suggest the changes below, @maysunfaisal and @makandre should probably take a look:

# Determining if you need to set an image registry.

You will need to set credentials for an image registry:
- If you are building a project with a dockerfile that pulls from a non-public image registry.
- If you are using codewind remotely to set at least one image registry that codewind can push built project images to. If you have multiple registries configured then you can select one of them to be the push registry

When you add an image registry Codewind will also log you into the image registry on your local machine as if you had run `docker login` locally. This is to support Appsody style projects.

@maysunfaisal @makandre - Currently cwctl only does a local docker login when setting a registry for a locakl connection. As I've realised talking to @j-c-berger about this that leaves us in the weird situation where for local codewind you don't need to do a local docker login but for remote connections you do. Should I raise a bug and move the docker login performed by cwctl so we do it regardless of connection type? I think this would be more consistent and it's easy to do but wanted to check your opinion as you work with Appsody more than me.

makandre commented 4 years ago

Even for remote codewind connection, we would need to do the local docker login, because the project creation is happening locally. That means appsody init is run locally, and it will need to pull image down from a possibly private registry

@j-c-berger is there a doc PR for this? I would like to review as well.

j-c-berger commented 4 years ago

@hhellyer, thanks for the chat and for posting the recap here along with your suggestions.

@makandre, I don't have a PR up yet. I'll assign you and Howard and as reviewers when I get it up. Thanks

hhellyer commented 4 years ago

@makandre - I've raised https://github.com/eclipse/codewind/issues/2582 to move the local docker login so we always run it and make sure the docs are in sync.

hhellyer commented 4 years ago

I've added the change to always do a local login to my existing PR https://github.com/eclipse/codewind-installer/pull/414 as that hasn't been merged yet.

tetchel commented 4 years ago

bug with this new functionality -> https://github.com/eclipse/codewind/issues/2588

hhellyer commented 4 years ago

The docs are updated and all the functionality is delivered (#2588 should be fixed) so I've moved this to verify.

codewind-bot commented 4 years ago

@maysunfaisal - this issue is now ready to be verified.

jopit commented 4 years ago

This feature still needs to be implemented by IntelliJ

j-c-berger commented 4 years ago

@hhellyer, can the docs label be removed from this issue? That is, is there anymore work for ID? Thanks!

hhellyer commented 4 years ago

@jopit - Will ID need to document anything IntelliJ specific for this, given that it's isn't implemented in IntelliJ yet?

micgibso commented 4 years ago

@jopit See Howard's question above ^^^, any ID reqd?

jopit commented 4 years ago

I don't think we need to doc anything IntelliJ specific for this.