Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-44860] Allow usernames to be left unmasked in build log #131

Merged
merged 11 commits into from Jun 2, 2021

Conversation

jglick
Copy link
Member

@jglick jglick commented May 3, 2021

JENKINS-44860: Allows usernames to left unmasked in the build log. Continues #50 (and thus #44).

Note that this functionality is available only in Scripted syntax, or at least when explicitly using the withCredentials step. In Declarative using the credentials syntax in an environment block, https://github.com/jenkinsci/pipeline-model-definition-plugin/blob/15978cd172a8d34e2cd6bf8a6bfeaa5514d44dad/pipeline-model-definition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy#L463-L475 calls https://github.com/jenkinsci/pipeline-model-definition-plugin/blob/15978cd172a8d34e2cd6bf8a6bfeaa5514d44dad/pipeline-model-definition/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/credentials/impl/UsernamePasswordHandler.java#L61-L68 which will continue to mask the username. Adding a third binding in which it is unmasked would do no good, since it would still be considered secret due to the second binding. Changing the second binding to set showUsername to true would potentially be a security vulnerability, in case a user expected the username from some credentials to be sensitive: merely upgrading the credentials-binding plugin would quietly begin printing the username in new builds. The problem is that Declarative tries to autodetect the right bindings to use based on the credentials type, which is convenient but forecloses the possibility of (compatibly) adding alternative behaviors: there is no place in the syntax for selecting a binding or a binding option.

Stovoy and others added 8 commits July 14, 2018 00:40
- `set +x` will avoid any console output, so the tests didn't actually
  test anything, as no credentials was logged masked or not.
  Same goes for `@echo off` for Windows
To avoid findbugs to fail the build. Used the same value as used in the
other serializable classes.
@jglick jglick changed the title show username JENKINS 44860 [JENKINS-44860] Allow usernames to be left unmasked in build log May 3, 2021
@jglick jglick requested a review from Wadeck May 4, 2021 14:49
@bitwiseman
Copy link

Note that this functionality is available only in Scripted syntax, or at least when explicitly using the withCredentials step.

This adds showUserName as a property of the credential. Why would this not be something that could be made to work in Declarative?

@jglick
Copy link
Member Author

jglick commented May 4, 2021

This adds showUserName as a property of the credential. Why would this not be something that could be made to work in Declarative?

Because the Declarative syntax is simply

environment {
    MYCREDS = credentials('the-credentials-id')
}

You do not get a choice of which binding to use, much less its specific options.

@bitwiseman
Copy link

This adds showUserName as a property of the credential. Why would this not be something that could be made to work in Declarative?

Because the Declarative syntax is simply

environment {
    MYCREDS = credentials('the-credentials-id')
}

You do not get a choice of which binding to use, much less its specific options.

Could we make a setting on the credential itself to control whether the name is exposed by default, hidden by default, or not allowed? This seems like a behavior the admins who control the credentials should have some control over.

@daniel-beck
Copy link
Member

Could we make a setting on the credential itself

I think I remember talking about this with @jeffret-b a while back, IIRC we agreed that this is where it should be as need to mask might depend on the kind of username it is.

@jglick
Copy link
Member Author

jglick commented May 5, 2021

Could become a field in UsernamePasswordCredentialsImpl if we wish to transfer control from the Pipeline author to the credentials creator. Or we can just declare that usernames are unconditionally public once you upgrade the plugin, as in #44/#50, though it scares me some. Note that currently the username is saved and round-tripped in plain-text as a String, not a Secret, and is also shown in the job config dropdown via StandardUsernamePasswordCredentials.NameProvider. Happy to prototype alternate behaviors if there is any consensus.

@jeffret-b
Copy link
Contributor

Yes, we did discuss adding some properties on a credential to control things like this -- visibility and other usage behaviors. It does transfer control from the pipeline author to the credential creator, but it makes sense that the owner / creator of the credential should be able to control things like this. It's been quite a while since I looked into things with this, though.

@jglick
Copy link
Member Author

jglick commented May 6, 2021

Could we make a setting on the credential itself

Check #132.

@jglick jglick merged commit 8cf5a0e into jenkinsci:master Jun 2, 2021
@jglick jglick deleted the show-username-JENKINS-44860 branch June 2, 2021 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants