Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-64392

Upgrading workflow-cps to 2.85 or later leaks credentials in ui

      When viewing logs credentials are hidden as expected. But when in blueocean or viewing logs in the branch builds overview the "title" used to be "Shell Script (self time 3s)" but now includes the command but doesn't remove the secret when using withVault. 

      withCredentials will have a title of "echo ${TEXT} | wc" Good

      withVault will have a title of "echo cred123 | wc " Bad

      I don't know if this is an issue with workflow-cps or one of the hashicorp-vault plugins but it seem like there was a change in workflow-cps:2.85 that added the command to the title and now I can see credentials.

      Currently using
      hashicorp-vault-pipeline:1.3
      hashicorp-vault-plugin:3.6.1

          [JENKINS-64392] Upgrading workflow-cps to 2.85 or later leaks credentials in ui

          Carroll Chiou added a comment -

          So if the vault credential is appearing in Blue Ocean (and most likely pipeline steps), that would mean that workflow-cps is unable to find the credential variable within the list of sensitive environment variables.

          The change in workflow-cps 2.85+ is that it is using the new workflow-step-api API (2.23) for registering sensitive variables. It appears the hashicorp plugin is adding the sensitive variables via Context.env.

          Console masking would still work since the plugin is doing that on its own.

          In order for workflow-cps to properly identify a sensitive variable, it needs to be registered as a sensitive environment variable. This is done by adding the sensitive variables to an EnvironmentExpander and then merging it to the current environment variables.

          See the credential-binding plugin as an example here and here

          Carroll Chiou added a comment - So if the vault credential is appearing in Blue Ocean (and most likely pipeline steps), that would mean that workflow-cps is unable to find the credential variable within the list of sensitive environment variables. The change in workflow-cps 2.85+ is that it is using the new workflow-step-api API (2.23) for registering sensitive variables. It appears the hashicorp plugin is adding the sensitive variables via Context.env . Console masking would still work since the plugin is doing that on its own. In order for workflow-cps to properly identify a sensitive variable, it needs to be registered as a sensitive environment variable. This is done by adding the sensitive variables to an EnvironmentExpander and then merging it to the current environment variables. See the credential-binding plugin as an example here and here

          Adam added a comment -

          jetersen and tobilarscheid as top contributors to hashicorp-vault-plugin is this a known issue or something that you can help with? 

          Adam added a comment - jetersen  and tobilarscheid as top contributors to hashicorp-vault-plugin is this a known issue or something that you can help with? 

          Denys Digtiar added a comment -

          It looks like Groovy String interpolation might be at fault here, see String interpolation

          Denys Digtiar added a comment - It looks like Groovy String interpolation might be at fault here, see  String interpolation

          Hi adamphillips, my time with the vault plugin is long ago! Nevertheless I was the one originally implementing the log masking part, see this class: https://github.com/jenkinsci/hashicorp-vault-plugin/blob/master/src/main/java/com/datapipe/jenkins/vault/log/MaskingConsoleLogFilter.java

          Unfortunately I have not enough (developer) experience with blue ocean changes. I assume there must be a better, more general way of telling Jenkins that a certain string is a credential and should not be rendered anywhere in the UI. In hindsight my log filter feels pretty hand crafted and should definitely be replaced by some kind of jenkins build in functionality, if such exists.

          Tobias Larscheid added a comment - Hi adamphillips , my time with the vault plugin is long ago! Nevertheless I was the one originally implementing the log masking part, see this class: https://github.com/jenkinsci/hashicorp-vault-plugin/blob/master/src/main/java/com/datapipe/jenkins/vault/log/MaskingConsoleLogFilter.java Unfortunately I have not enough (developer) experience with blue ocean changes. I assume there must be a better, more general way of telling Jenkins that a certain string is a credential and should not be rendered anywhere in the UI. In hindsight my log filter feels pretty hand crafted and should definitely be replaced by some kind of jenkins build in functionality, if such exists.

          Jasen Minton added a comment -

          This bug is a regression in org.jenkinsci.plugins.workflow.cps.actions.ArgumentsActionImpl.java

          I think it's important to note that with this regression security sensitive content is now getting saved out to the $JENKINS_HOME/jobs files. This isn't a transitory UI leak that goes away with the server. It is the file based content that drives both the BlueOcean and standard UI's.

          Effectively, pipeline execution status is getting saved to XML job files and ArgumentsActionImpl is supposed to be doing the text masking before security sensitive content gets saved.

          In 2.84 during ArgumentsActionImpl.sanitizeObjectAndRecordMutation execution, it called isStringSafe and used a white list to determine whether or not to mask secrets. 

          In 2.85+ this is replaced with an attempt to match a list of variables from an EnvironmentExpander to mask. If no list is given, no variables are masked. Its not clear to me at all how the vault should have provided the list of variables to mask. However, the resulting code is that there is no list of sensitive variables to block and with the white list capability removed it doesn't appear possible to mask any content so all content is now getting saved to the XML files in $JENKINS_HOME/jobs that constitute what both UI's present later.

          A possible fix that worked for me is that if the list of sensitive variables is size zero, reinstate the white list filter. Here it is laid into a 2.87 version as an example: ArgumentsActionImpl.java

          I initially assumed this would be a vault bug, but I just didn't see how or where to supply the list of sensitive variables to the cps plugin to pick up and use in an EnvironmentExpander plus this API changed out from underneath it so the fallback shouldn't be a complete lack of masking that which used to be masked, which is the case 2.85+

          Jasen Minton added a comment - This bug is a regression in org.jenkinsci.plugins.workflow.cps.actions.ArgumentsActionImpl.java I think it's important to note that with this regression security sensitive content is now getting saved out to the $JENKINS_HOME/jobs files. This isn't a transitory UI leak that goes away with the server. It is the file based content that drives both the BlueOcean and standard UI's. Effectively, pipeline execution status is getting saved to XML job files and ArgumentsActionImpl is supposed to be doing the text masking before security sensitive content gets saved. In 2.84 during ArgumentsActionImpl.sanitizeObjectAndRecordMutation execution, it called isStringSafe and used a white list to determine whether or not to mask secrets.  In 2.85+ this is replaced with an attempt to match a list of variables from an EnvironmentExpander to mask. If no list is given, no variables are masked. Its not clear to me at all how the vault should have provided the list of variables to mask. However, the resulting code is that there is no list of sensitive variables to block and with the white list capability removed it doesn't appear possible to mask any content so all content is now getting saved to the XML files in $JENKINS_HOME/jobs that constitute what both UI's present later. A possible fix that worked for me is that if the list of sensitive variables is size zero, reinstate the white list filter. Here it is laid into a 2.87 version as an example:  ArgumentsActionImpl.java I initially assumed this would be a vault bug, but I just didn't see how or where to supply the list of sensitive variables to the cps plugin to pick up and use in an EnvironmentExpander plus this API changed out from underneath it so the fallback shouldn't be a complete lack of masking that which used to be masked, which is the case 2.85+

          Carroll Chiou added a comment - - edited

          So to clarify and expand my original post, what the hashicorp plugin does is add sensitive variables through Context.env. This is not the recommended way to add environment variables. The recommended way is to use the EnvironmentExpander.

          From a quick look, I think the vault can provide the list of sensitive variables by using the EnvironmentExpander in the same place that it adds the sensitive variables via Context.env. You can see the the credentials-binding plugin do that here and here. You should not need to change ArgumentsActionImpl.

          As for console masking, I think the same chunk of code can help. That way the plugin doesn't have to try and roll its own console filtering,.

          Carroll Chiou added a comment - - edited So to clarify and expand my original post , what the hashicorp plugin does is add sensitive variables through Context.env . This is not the recommended way to add environment variables. The recommended way is to use the EnvironmentExpander . From a quick look, I think the vault can provide the list of sensitive variables by using the EnvironmentExpander in the same place that it adds the sensitive variables via Context.env . You can see the the credentials-binding plugin do that here and here . You should not need to change ArgumentsActionImpl. As for console masking, I think the same chunk of code can help . That way the plugin doesn't have to try and roll its own console filtering,.

          Jasen Minton added a comment -

          The Context.env that you're referencing is not a StepContext. It's a public static final inner class named Context inherited from jenkins.tasks.SimpleBuildWrapper. The VaultBuildWrapper extends SimpleBuildWrapper which extends hudson.tasks.BuildWrapper. For VaultBuildWrapper to have access to org.jenkinsci.plugins.workflow.steps.EnvironmentExpander and a meaningful org.jenkinsci.plugins.workflow.steps.StepContext it would need to be managed and reimplemented as a org.jenkinsci.plugins.workflow.steps.Step subclass, unless I'm missing something and sorry if I am but I'm not seeing how to use Step and StepContext class based api from a BuildWrapper based subclass.

          As a type of BuildWrapper, there does exist API to manage the list of sensitive variable names. It's consistent and meaningful use seems to be optional however.

          I'm also very uneasy about this security sensitive information that was previously getting masked and not saved now getting saved to jobs XML files. If this is deemed a secure operation, please let me know the reference so I can pass it along, otherwise this really should be getting escalated as a greater security risk. Sorry if I'm being an alarmist but better err on the side of safety. 

          Jasen Minton added a comment - The Context.env that you're referencing is not a StepContext. It's a public static final inner class named Context inherited from jenkins.tasks.SimpleBuildWrapper. The VaultBuildWrapper extends SimpleBuildWrapper which extends hudson.tasks.BuildWrapper. For VaultBuildWrapper to have access to org.jenkinsci.plugins.workflow.steps.EnvironmentExpander and a meaningful org.jenkinsci.plugins.workflow.steps.StepContext it would need to be managed and reimplemented as a org.jenkinsci.plugins.workflow.steps.Step subclass, unless I'm missing something and sorry if I am but I'm not seeing how to use Step and StepContext class based api from a BuildWrapper based subclass. As a type of BuildWrapper, there does exist API to manage the list of sensitive variable names. It's consistent and meaningful use seems to be optional however. I'm also very uneasy about this security sensitive information that was previously getting masked and not saved now getting saved to jobs XML files. If this is deemed a secure operation, please let me know the reference so I can pass it along, otherwise this really should be getting escalated as a greater security risk. Sorry if I'm being an alarmist but better err on the side of safety. 

          Carroll Chiou added a comment - - edited

          Whoops, you're right, I missed that. Ideally this plugin should be updated to be used as step if we are using it in pipeline. I do see that you can use withCredentials to pass the vault secrets. withCredentials would be the recommended way to pass any secrets through pipeline. Does masking fail when using that method?

          Are there things that the withVault step does that withCredentials does not do? Ideally there should be no need for the plugin to try and add secrets on its own or even do its own console masking.

          I assume there must be a better, more general way of telling Jenkins that a certain string is a credential and should not be rendered anywhere in the UI.

          comment-403591
          That would be using the credentials-binding plugin.

          Carroll Chiou added a comment - - edited Whoops, you're right, I missed that. Ideally this plugin should be updated to be used as step if we are using it in pipeline. I do see that you can use withCredentials to pass the vault secrets. withCredentials would be the recommended way to pass any secrets through pipeline. Does masking fail when using that method? Are there things that the withVault step does that withCredentials does not do? Ideally there should be no need for the plugin to try and add secrets on its own or even do its own console masking. I assume there must be a better, more general way of telling Jenkins that a certain string is a credential and should not be rendered anywhere in the UI. comment-403591 That would be using the credentials-binding plugin.

            scddev Dietmar Scheidl
            adamphillips Adam
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

              Created:
              Updated:
              Resolved: