Skip to content

Commit

Permalink
[SECURITY-2291] Unprotected Storage of Credentials
Browse files Browse the repository at this point in the history 
Signed-off-by: Olivier Lamy <olamy@apache.org>
  • Loading branch information
@olamy
olamy committed Feb 10, 2022
1 parent 7be6927 commit 70b7689
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 31 deletions.
59 changes: 39 additions & 20 deletions src/main/java/jenkins/plugins/publish_over_ssh/BapSshHostConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@

import java.io.IOException;
import java.util.ArrayList;
import java.util.Objects;
import java.util.Properties;

import hudson.util.Secret;
import jenkins.model.Jenkins;
import jenkins.plugins.publish_over.*;
import jenkins.plugins.publish_over_ssh.descriptor.BapSshHostConfigurationDescriptor;
Expand Down Expand Up @@ -70,7 +72,10 @@ public class BapSshHostConfiguration extends BPHostConfiguration<BapSshClient, B
private String proxyHost;
private int proxyPort;
private String proxyUser;

@Deprecated
private String proxyPassword;
private Secret secretProxyPassword;

public BapSshHostConfiguration() {
// use this constructor instead of the default w/o parameters because there is some
Expand Down Expand Up @@ -183,7 +188,13 @@ public void setEncryptedPassword(final String encryptedPassword) {

public String getProxyUser() { return proxyUser; }

public String getProxyPassword() {return proxyPassword; }
public String getProxyPassword() {
return Secret.toString(this.secretProxyPassword);
}

public Secret getSecretProxyPassword() {
return this.secretProxyPassword;
}

@DataBoundSetter
public void setProxyType(String proxyType) {
Expand All @@ -207,7 +218,21 @@ public void setProxyUser(String proxyUser) {

@DataBoundSetter
public void setProxyPassword(String proxyPassword) {
this.proxyPassword = proxyPassword;
this.secretProxyPassword = Secret.fromString(proxyPassword);
}

@DataBoundSetter
public void setSecretProxyPassword(Secret secretProxyPassword) {
this.secretProxyPassword = secretProxyPassword;
}

@Override
public Object readResolve() {
if(StringUtils.isNotEmpty(proxyPassword)) {
this.secretProxyPassword = Secret.fromString(this.proxyPassword);
this.proxyPassword = null;
}
return super.readResolve();
}

public boolean isEffectiveDisableExec() {
Expand Down Expand Up @@ -251,13 +276,7 @@ public BapSshClient createClient(final BPBuildInfo buildInfo, final boolean conn
}
if (connectSftp)
setupSftp(bapClient);
} catch (IOException e) {
bapClient.disconnectQuietly();
throw new BapPublisherException(Messages.exception_failedToCreateClient(e.getLocalizedMessage()), e);
} catch (JSchException e) {
bapClient.disconnectQuietly();
throw new BapPublisherException(Messages.exception_failedToCreateClient(e.getLocalizedMessage()), e);
} catch (BapPublisherException e) {
} catch (IOException | JSchException | BapPublisherException e) {
bapClient.disconnectQuietly();
throw new BapPublisherException(Messages.exception_failedToCreateClient(e.getLocalizedMessage()), e);
}
Expand All @@ -284,7 +303,7 @@ static String[] getHosts(String target, String jumpHosts) {
}
}
hosts.add(target);
return hosts.toArray(new String[hosts.size()]);
return hosts.toArray(new String[0]);
}
}

Expand Down Expand Up @@ -395,24 +414,24 @@ private Session createSession(final BPBuildInfo buildInfo, final JSch ssh, Strin
if (StringUtils.isNotEmpty(proxyType) && StringUtils.isNotEmpty(proxyHost)) {
if (StringUtils.equals(HTTP_PROXY_TYPE, proxyType)) {
ProxyHTTP proxyHTTP = new ProxyHTTP(proxyHost, proxyPort);
if (StringUtils.isNotEmpty(proxyUser) && StringUtils.isNotEmpty(proxyPassword)) {
proxyHTTP.setUserPasswd(proxyUser, proxyPassword);
if (StringUtils.isNotEmpty(proxyUser) && StringUtils.isNotEmpty(secretProxyPassword.getPlainText())) {
proxyHTTP.setUserPasswd(proxyUser, secretProxyPassword.getPlainText());
} else {
proxyHTTP.setUserPasswd(null, null);
}
session.setProxy(proxyHTTP);
} else if (StringUtils.equals(SOCKS_4_PROXY_TYPE, proxyType)) {
ProxySOCKS4 proxySocks4 = new ProxySOCKS4(proxyHost, proxyPort);
if (StringUtils.isNotEmpty(proxyUser) && StringUtils.isNotEmpty(proxyPassword)) {
proxySocks4.setUserPasswd(proxyUser, proxyPassword);
if (StringUtils.isNotEmpty(proxyUser) && StringUtils.isNotEmpty(secretProxyPassword.getPlainText())) {
proxySocks4.setUserPasswd(proxyUser, secretProxyPassword.getPlainText());
} else {
proxySocks4.setUserPasswd(null, null);
}
session.setProxy(proxySocks4);
} else if (StringUtils.equals(SOCKS_5_PROXY_TYPE, proxyType)) {
ProxySOCKS5 proxySocks5 = new ProxySOCKS5(proxyHost, proxyPort);
if (StringUtils.isNotEmpty(proxyUser) && StringUtils.isNotEmpty(proxyPassword)) {
proxySocks5.setUserPasswd(proxyUser, proxyPassword);
if (StringUtils.isNotEmpty(proxyUser) && StringUtils.isNotEmpty(secretProxyPassword.getPlainText())) {
proxySocks5.setUserPasswd(proxyUser, secretProxyPassword.getPlainText());
} else {
proxySocks5.setUserPasswd(null, null);
}
Expand All @@ -435,7 +454,7 @@ protected JSch createJSch() {
}

public BapSshHostConfigurationDescriptor getDescriptor() {
return Jenkins.getInstance().getDescriptorByType(BapSshHostConfigurationDescriptor.class);
return Jenkins.get().getDescriptorByType(BapSshHostConfigurationDescriptor.class);
}

protected EqualsBuilder addToEquals(final EqualsBuilder builder, final BapSshHostConfiguration that) {
Expand All @@ -449,7 +468,7 @@ protected EqualsBuilder addToEquals(final EqualsBuilder builder, final BapSshHos
.append(proxyHost, that.proxyHost)
.append(proxyPort, that.proxyPort)
.append(proxyUser, that.proxyUser)
.append(proxyPassword, that.proxyPassword);
.append(secretProxyPassword, that.secretProxyPassword);
}

@Override
Expand All @@ -464,7 +483,7 @@ protected HashCodeBuilder addToHashCode(final HashCodeBuilder builder) {
.append(proxyHost)
.append(proxyPort)
.append(proxyUser)
.append(proxyPassword);
.append(secretProxyPassword.getPlainText());
}

@Override
Expand All @@ -479,7 +498,7 @@ protected ToStringBuilder addToToString(final ToStringBuilder builder) {
.append("proxyHost", proxyHost)
.append("proxyPort", proxyPort)
.append("proxyUser", proxyUser)
.append("proxyPassword", proxyPassword);
.append("proxyPassword", "xxxxxxx");
}

@Override
Expand Down
1 change: 0 additions & 1 deletion ...n/java/jenkins/plugins/publish_over_ssh/descriptor/BapSshHostConfigurationDescriptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,5 +99,4 @@ public FormValidation doTestConnection(final StaplerRequest request, final Stapl
public jenkins.plugins.publish_over.view_defaults.HostConfiguration.Messages getCommonFieldNames() {
return new jenkins.plugins.publish_over.view_defaults.HostConfiguration.Messages();
}

}
18 changes: 9 additions & 9 deletions ...ain/java/jenkins/plugins/publish_over_ssh/descriptor/BapSshPublisherPluginDescriptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@
import java.util.Comparator;
import java.util.List;

import hudson.Extension;
import hudson.ExtensionPoint;
import hudson.init.InitMilestone;
import hudson.init.Initializer;
import hudson.util.Secret;
import jenkins.plugins.publish_over.BPHostConfiguration;
import org.apache.commons.lang.StringUtils;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.StaplerResponse;

Expand Down Expand Up @@ -66,7 +73,7 @@ public class BapSshPublisherPluginDescriptor extends BuildStepDescriptor<Publish
/** null - prevent complaints from xstream */
@SuppressFBWarnings(value = "URF_UNREAD_FIELD")
private transient Class hostConfigClass;
private final CopyOnWriteList<BapSshHostConfiguration> hostConfigurations = new CopyOnWriteList<BapSshHostConfiguration>();
private final CopyOnWriteList<BapSshHostConfiguration> hostConfigurations = new CopyOnWriteList<>();
private BapSshCommonConfiguration commonConfig;
private SshDefaults defaults;

Expand Down Expand Up @@ -100,14 +107,7 @@ public List<BapSshHostConfiguration> getHostConfigurations() {
retVal.add(current);
}

Collections.sort(retVal, new Comparator<BapSshHostConfiguration>() {

@Override
public int compare(BapSshHostConfiguration p1, BapSshHostConfiguration p2) {
return p1.getName().compareTo(p2.getName());
}

});
Collections.sort(retVal, Comparator.comparing(BPHostConfiguration::getName));

return retVal;
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/resources/jenkins/plugins/publish_over_ssh/BapSshHostConfiguration/config.jelly
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@
<f:entry title="${%proxyUser}" field="proxyUser">
<f:textbox/>
</f:entry>
<f:entry title="${%proxyPassword}" field="proxyPassword">
<f:entry title="${%proxyPassword}" field="secretProxyPassword">
<f:password/>
</f:entry>

Expand Down

0 comments on commit 70b7689

Please sign in to comment.