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] UsernameCredentials.isUsernameSecret #205

Merged
merged 9 commits into from
Jun 2, 2021
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
</scm>

<properties>
<revision>2.4.2</revision>
<revision>2.5</revision>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just proposing a version bump. Do we really need to keep the micro version? Can revert if desired.

<changelist>-SNAPSHOT</changelist>
<jenkins.version>2.222.4</jenkins.version>
<java.level>8</java.level>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,11 @@ Result name(@NonNull Credentials credentials, @NonNull Class<?> clazz) {
if (nameWith != null) {
try {
CredentialsNameProvider nameProvider = nameWith.value().newInstance();
return new Result(nameProvider.getName(credentials), nameWith.priority());
String name = nameProvider.getName(credentials);
if (!name.isEmpty()) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it possible to just return credentials.getDescription() and if that happens to be the empty string, still behave reasonably (choose a different provider).

LOGGER.fine(() -> "named `" + name + "` from " + nameProvider);
return new Result(name, nameWith.priority());
}
} catch (ClassCastException | InstantiationException | IllegalAccessException e) {
// ignore
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ class NameProvider extends CredentialsNameProvider<StandardCredentials> {
@Override
public String getName(@NonNull StandardCredentials c) {
String description = Util.fixEmptyAndTrim(c.getDescription());
// TODO perhaps omit id (return description) if it is just a UUID
return c.getId() + (description != null ? " (" + description + ")" : "");
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ class NameProvider extends CredentialsNameProvider<StandardUsernameCredentials>
@NonNull
@Override
public String getName(@NonNull StandardUsernameCredentials c) {
if (c.isUsernameSecret()) {
return c.getDescription();
}
String description = Util.fixEmptyAndTrim(c.getDescription());
return c.getUsername() + (description != null ? " (" + description + ")" : "");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ class NameProvider extends CredentialsNameProvider<StandardUsernameCredentials>
@NonNull
@Override
public String getName(@NonNull StandardUsernameCredentials c) {
if (c.isUsernameSecret()) {
return c.getDescription();
}
String description = Util.fixEmptyAndTrim(c.getDescription());
return c.getUsername() + "/******" + (description != null ? " (" + description + ")" : "");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,15 @@ public interface UsernameCredentials extends Credentials {
@NonNull
String getUsername();

/**
* Whether {@link #getUsername} should be considered a secret for purposes of behaviors like masking in build logs.
* @return true by default
* @since 2.4
*/
default boolean isUsernameSecret() {
return true;
}

/**
* Our name provider.
*
Expand All @@ -57,7 +66,7 @@ class NameProvider extends CredentialsNameProvider<UsernameCredentials> {
@NonNull
@Override
public String getName(@NonNull UsernameCredentials credentials) {
return credentials.getUsername();
return credentials.isUsernameSecret() ? "" : credentials.getUsername();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class NameProvider extends CredentialsNameProvider<UsernamePasswordCredentials>
@NonNull
@Override
public String getName(@NonNull UsernamePasswordCredentials credentials) {
return credentials.getUsername() + "/******";
return credentials.isUsernameSecret() ? "" : credentials.getUsername() + "/******";
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,12 @@
import com.cloudbees.plugins.credentials.common.StandardUsernamePasswordCredentials;
import edu.umd.cs.findbugs.annotations.CheckForNull;
import edu.umd.cs.findbugs.annotations.NonNull;
import edu.umd.cs.findbugs.annotations.Nullable;
import hudson.Extension;
import hudson.Util;
import hudson.util.Secret;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.DataBoundSetter;

/**
* Concrete implementation of {@link StandardUsernamePasswordCredentials}.
Expand All @@ -53,6 +55,9 @@ public class UsernamePasswordCredentialsImpl extends BaseStandardCredentials imp
@NonNull
private final Secret password;

@Nullable
private Boolean usernameSecret = false;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default false in the Java constructor which would be used only from tests or scripts.


/**
* Constructor.
*
Expand All @@ -72,6 +77,13 @@ public UsernamePasswordCredentialsImpl(@CheckForNull CredentialsScope scope,
this.password = Secret.fromString(password);
}

private Object readResolve() {
if (usernameSecret == null) {
usernameSecret = true;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default true for deserialized credentials, either from settings predating the upgrade, or REST uploads as XML.

}
return this;
}

/**
* {@inheritDoc}
*/
Expand All @@ -88,6 +100,16 @@ public String getUsername() {
return username;
}

@Override
public boolean isUsernameSecret() {
return usernameSecret;
}

@DataBoundSetter
public void setUsernameSecret(boolean usernameSecret) {
this.usernameSecret = usernameSecret;
}

/**
* {@inheritDoc}
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@
<f:entry title="${%Username}" field="username">
<f:textbox/>
</f:entry>
<f:entry field="usernameSecret">
<f:checkbox title="${%Treat username as secret}"/>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default false for new credentials created from the GUI.

</f:entry>
<f:entry title="${%Password}" field="password">
<f:password/>
</f:entry>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<div>
Historically, not only passwords but usernames were masked in the build log.
Since these can interfere with diagnostics,
and cause unrelated occurrences of a common word to be masked,
you may choose to leave usernames unmasked if they are not sensitive.
Note that regardless of this setting, the username will be displayed to anyone permitted to reconfigure the credentials.
</div>
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ public void restCRUDSmokes() throws Exception {
+ " <id>smokey-id</id>\n"
+ " <description>created from xml</description>\n"
+ " <username>example-com-deployer</username>\n"
+ " <usernameSecret>false</usernameSecret>\n"
+ " <password>super-secret</password>\n"
+ "</com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsImpl>");
assertThat(con.getResponseCode(), is(200));
Expand All @@ -193,6 +194,7 @@ public void restCRUDSmokes() throws Exception {
+ " <description>created from xml</description>\n"
+ " <username>example-com-deployer</username>\n"
+ " <password><secret-redacted/></password>\n"
+ " <usernameSecret>false</usernameSecret>\n"
+ "</com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsImpl>").ignoreWhitespace().ignoreComments());

// update credentials
Expand All @@ -202,6 +204,7 @@ public void restCRUDSmokes() throws Exception {
+ " <id>smokey-id</id>\n"
+ " <description>updated by xml</description>\n"
+ " <username>example-org-deployer</username>\n"
+ " <usernameSecret>false</usernameSecret>\n"
+ " <password>super-duper-secret</password>\n"
+ "</com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsImpl>");
assertThat(con.getResponseCode(), is(200));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ public Secret getPassword() {
return password;
}

@Override
public boolean isUsernameSecret() {
return false;
}

@Extension
public static class DescriptorImpl extends CredentialsDescriptor {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,27 @@
package com.cloudbees.plugins.credentials.impl;

import com.cloudbees.plugins.credentials.CredentialsNameProvider;
import java.util.logging.Level;
import org.junit.Test;
import static org.junit.Assert.*;
import org.junit.Rule;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.LoggerRule;

public class UsernamePasswordCredentialsImplTest {

@Rule public JenkinsRule r = new JenkinsRule(); // needed for Secret.fromString to work
@Rule public LoggerRule logging = new LoggerRule().record(CredentialsNameProvider.class, Level.FINE);

@Test public void displayName() {
assertEquals("bob/****** (Bob’s laptop)", CredentialsNameProvider.name(new UsernamePasswordCredentialsImpl(null, "abc123", "Bob’s laptop", "bob", "s3cr3t")));
UsernamePasswordCredentialsImpl creds = new UsernamePasswordCredentialsImpl(null, "abc123", "Bob’s laptop", "bob", "s3cr3t");
assertEquals("bob/****** (Bob’s laptop)", CredentialsNameProvider.name(creds));
creds.setUsernameSecret(true);
assertEquals("Bob’s laptop", CredentialsNameProvider.name(creds));
creds = new UsernamePasswordCredentialsImpl(null, "abc123", null, "bob", "s3cr3t");
assertEquals("bob/******", CredentialsNameProvider.name(creds));
creds.setUsernameSecret(true);
assertEquals("abc123", CredentialsNameProvider.name(creds));
}

}