Avoid exposing secure data in the cluster configuration UI

[tjjjwxzq]

GoCD Server Version: 22.3.0 Environment: DigitalOcean Droplet, Docker 20.10.21 on Ubuntu 22.04

My team is using the docker elastic agent plugin with GoCD server configured to point to a remote docker daemon running on another VM, with the docker daemon socket secured with TLS. We're also trying to use custom agent images stored on a private DigitalOcean Container Registry. We've configured the cluster profile with the appropriate CA and client cert and keys, as well as the necessary private registry configurations, and functionally everything works fine.

However, the following are exposed in the cluster configuration drop down section, as well as the edit form, in plaintext:

• docker_client_key
• private_registry_username

docker_client_key should clearly be set as secure data. I wonder if changing the last variable during metadata construction from false to true on this line would be enough to fix this?

Technically, we don't expect the private_registry_username to be secure, so the plugin is technically unproblematic here. However, due to DigitalOcean's (imho) poor design for how to authenticate to their container registry with docker, one has to supply a DigitalOcean API token (which is a secret value) to both the username and password fields in docker credentials. I don't expect this plugin to have to change to make up for DigitalOcean's poor design, but we thought we could circumvent the issue of sensitive info showing up in the UI by choosing to use a Docker configuration file instead of specifying custom credentials directly in the GoCD admin UI.

Unfortunately, even when we choose to use a Docker configuration file, the field private_registry_username (as well as private_registry_password, but that's appropriately masked, so not actually a problem) still shows up under the cluster configuration drop down section as you can see below, which seems like odd behaviour to me.

I tried looking around the code a bit, but don't really have enough context to figure out how the plugin actually configures which fields to show in the UI here. I suppose the hacky workaround for our unfortunate DigitalOcean use-case is to set the username metadata to secure so it would always be masked even if it shows up, but that wouldn't really be implementing correct behaviour. I wonder if there's a way to not display those fields if a user chooses to use a docker configuration file instead of custom credentials?

[chadlwilson]

👋 @tjjjwxzq !

docker_client_key should clearly be set as secure data. I wonder if changing the last variable during metadata construction from false to true on this line would be enough to fix this?

Yeah, changing the metadata line like you suggest would change things to secure variables. Gosh that does seem like mistake during the transition to the v5 Elastic Agent API in gocd/kubernetes-elastic-agents#101 .

I don't expect this plugin to have to change to make up for DigitalOcean's poor design

I'm not against changing the private_registry_username to secure as well, personally.

We can make both changes, just not sure on its effect on existing configurations, and whether the server will automagically encrypt the values when loading from the plugin, or whether it will break backwards compat, and require folks to re-enter the values (due to inability to decrypt a value it expects to be encrypted but is not). If I change it and produce an experimental build, would you be willing to help test how it handles plugin upgrade/downgrade with existing config?

I tried looking around the code a bit, but don't really have enough context to figure out how the plugin actually configures which fields to show in the UI here. I suppose the hacky workaround for our unfortunate DigitalOcean use-case is to set the username metadata to secure so it would always be masked even if it shows up, but that wouldn't really be implementing correct behaviour. I wonder if there's a way to not display those fields if a user chooses to use a docker configuration file instead of custom credentials?

As for what the UI shows/displays, it's based on these somewhat strange Angular 1 templates like the below bit. I don't have the background of why it was implemented this way though (to continue to display the username/pass when using a credentials file, and to re-use the same values), might require some digging through gocd/kubernetes-elastic-agents#71 to see if there was any particular reason. There are some (limited?) docs on how this works at https://plugin-api.gocd.org/current/elastic-agents/#the-settings-view-object

https://github.com/gocd-contrib/docker-elastic-agents-plugin/blob/8e97ea9d4cb4d2f62424f507889f2714acbe16d6/src/main/resources/plugin-settings.template.html#L110-L135

[tjjjwxzq]

👋 @chadlwilson and thanks for the quick response! If you're okay with securing even private_registry_username, I'd be happy for a quick fix and definitely willing and able to test it out.

Re: the UI code, yeah I saw those Angular templates, but they seem to only exist for the configuration forms but not the plaint view of the cluster configuration page? So I thought the logic might be hidden somewhere in how the plugin interfaces with the core GoCD code but after poking around back and forth for a limited time to no avail I gave up 😅 I did see gocd/kubernetes-elastic-agents#71 and tried to figure out if there was any relevant logic there, but it seems that the files have changed quite a bit since gocd/kubernetes-elastic-agents#71 was merged some years ago (which I'm guessing now that it's due to API upgrade changes in gocd/kubernetes-elastic-agents#101 and maybe elsewhere) and couldn't sleuth anything out yet.

[chadlwilson]

Will leave this open until confirmed.

If you're okay with securing even private_registry_username, I'd be happy for a quick fix and definitely willing and able to test it out.

There's an experimental build at https://github.com/gocd-contrib/docker-elastic-agents-plugin/releases/tag/v3.2.1-310-exp you can give a go with.

Re: the UI code, yeah I saw those Angular templates, but they seem to only exist for the configuration forms but not the plaint view of the cluster configuration page?

Ahh OK, yeah that one possibly just displays all configuration values. Is what you are seeing in the username with with use of the docker configuration file some some "stale" values from when the username/pass were configured directly and can just be cleared or set to dud values before switching to the config file?

I can't really see a loop back from reading the config file to those values appearing in the UI, so I'd have thought there might be a workaround there by clearing the old values before switching to config file?

https://github.com/gocd-contrib/docker-elastic-agents-plugin/blob/8e97ea9d4cb4d2f62424f507889f2714acbe16d6/src/main/java/cd/go/contrib/elasticagents/docker/DockerClientFactory.java#L59-L71

[tjjjwxzq]

Thanks! Appreciate the quick merge and I'll update here when I manage to test it out.

And yes, it seems like you're right about the private_registry_username and password actually being dud values from previously configuring custom credentials. If you deliberately clear them out before switching to the docker config file option they show up as (Not specified). Probably should have checked this behaviour earlier. In the meantime, I'll update the installation documentation to document this unobvious behaviour explicitly (plus enhance the docs generally). Appreciate your review 🙏

[chadlwilson]

FWIW, if you are playing around locally you can build your own plugin version with ./gradlew assemble check, it'll be in build/libs ready to drop into a test server. Using Java 17 will match how the "official" builds are produced.

[chadlwilson]

And yes, it seems like you're right about the private_registry_username and password actually being dud values from previously configuring custom credentials. If you deliberately clear them out before switching to the docker config file option they show up as (Not specified). Probably should have checked this behaviour earlier. In the meantime, I'll update the installation documentation to document this unobvious behaviour explicitly (plus enhance the docs generally).

Cool, thanks for checking. Given this, do you think we are better to change the username back to "non-secure" and in the Digital Ocean kind of case prefer use of the config file, or do you see value in the with the change in your gocd/kubernetes-elastic-agents#221 applying to both vars for ease of use?

Ignoring possibly backwards compat issues for now, the downside of making the username "secure" is possible inconsistency with other plugins, and making it a bit harder to answer questions like "I am trying to clean up credentials on my registry - which username/cred is being actively used by the gocd docker agents plugin?".

[tjjjwxzq]

Given this, do you think we are better to change the username back to "non-secure"

I do believe so, since to me treating the username as non-secure is technically correct behaviour, and setting it to secure has the downsides you mention. Plus, since there is a relatively easy workaround for the DigitalOcean case (and the behaviour is now documented), there's no real reason to keep the username as secure (FWIW, I did open a feature request with them to allow a more secure way of authenticating with their private registry, but I don't expect them to pick that up any time soon)

What I think can be done to implement "more correct" behaviour in the plugin, however, would be to add logic to automatically clear out the private registry username and password values stored in plugin settings if the user chooses to use docker configuration instead of custom credentials. I believe this could be achieved by modifying the relevant getters and/or setters in PluginSettings.java. And if that doesn't work, as a backup approach it should be possible to throw an error in the cluster profile validator if the user chooses to use a docker config file but didn't clear their custom credentials, so that we manually enforce that behaviour on users.

https://github.com/gocd-contrib/docker-elastic-agents-plugin/blob/8e97ea9d4cb4d2f62424f507889f2714acbe16d6/src/main/java/cd/go/contrib/elasticagents/docker/PluginSettings.java#L196-L210

https://github.com/gocd-contrib/docker-elastic-agents-plugin/blob/8e97ea9d4cb4d2f62424f507889f2714acbe16d6/src/main/java/cd/go/contrib/elasticagents/docker/executors/ClusterProfileValidateRequestExecutor.java#L39-L68

Anyway, I'll try and test out the experimental build first, see if backwards compat is a non-issue then open a PR to revert the change to secure private_registry_username for now. As for the latter enhancements, I'll see if I can make them when time permits.

[tjjjwxzq]

Can confirm that there shouldn't be any backwards compatibility issues with securing new variables. Tested: Upgrading plugin from v3.2.0-283 to the experimental v3.2.1-310 GoCD Server Version: 22.3.0 Environment: DigitalOcean Droplet, Docker 20.10.21 on Ubuntu 22.04

Behaviour: When stopping GoCD server, any configuration fields in cruise-config.xml that are encrypted remain encrypted. When starting GoCD server again, regardless of whether one changes the plugin version or not, at some point in the server's start up process all encrypted values (at least those in the cluster profile config that I noted) in cruise-config.xml are decrypted and visible as plaintext in the file. However, the fields marked secure are still masked correctly in the UI. So when one upgrades to 3.2.1-310, docker_client_key, private_registry_username, and private_registry_password are all correctly masked, even if they remain unencrypted in cruise-config.xml (but I did catch a slight UI bug where the values were initially unmasked on the first page load when directly navigating to the elastic agent configuration page after the GoCD server loading screen, but they were masked on a reload). The values in cruise-config.xml only seem to be re-encrypted when one explicitly saves any one cluster profile configuration. The behaviour is similar when downgrading the plugin (ie. functionally works fine, just with the cruise-config.xml decryption-reencryption gotcha)

I suppose the fact that cruise-config.xml could be harbouring plaintext secrets on the filesystem for quite a while (until one does an explicit save) is somewhat problematic, but that seems to be core GoCD behaviour rather than this plugin. I suspect GoCD just generally decrypts all values stored in an <encryptedValue> tag in cruise-config.xml on start-up, and re-encrypts when any configuration change is made (can confirm that any configuration change triggers this, not just changes to elastic agent configuration)

Anyway, I'll revert the change to secure private_registry_username

[chadlwilson]

Thanks! I did a quick sanity check just now as it turns out and it seemed to work OK.

I suspect GoCD just generally decrypts all values stored in an <encryptedValue> tag in cruise-config.xml on start-up, and re-encrypts when any configuration change is made (can confirm that any configuration change triggers this, not just changes to elastic agent configuration)

Yeah, it does do something like that. Good to verify it at least supports this kind of change though.

But yeah, it does expose the server to slightly horrifying edge case bugs such as https://github.com/gocd/gocd/issues/13067 and https://groups.google.com/g/go-cd/c/p1uBISisDx0/m/TVSP1v42AgAJ - aside from requiring a re-save of config to re-encrypt.

[chadlwilson]

There's a more official build released at https://github.com/gocd-contrib/docker-elastic-agents-plugin/releases/tag/v3.2.1-316

If you get around to the other changes you suggest some time, that'd be great - but a bonus :-)