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-48480] Switch deprecated protocols to opt-in. #3188

Merged
merged 6 commits into from
Jun 17, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 1 addition & 11 deletions core/src/main/java/hudson/cli/CliProtocol.java
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public class CliProtocol extends AgentProtocol {
*/
@Override
public boolean isOptIn() {
return OPT_IN;
return true;
}

@Override
Expand Down Expand Up @@ -109,14 +109,4 @@ protected void runCli(Connection c) throws IOException, InterruptedException {
channel.join();
}
}

/**
* A/B test turning off this protocol by default.
*/
private static final boolean OPT_IN;

static {
byte hash = Util.fromHexString(Jenkins.getInstance().getLegacyInstanceId())[0];
OPT_IN = (hash % 10) == 0;
}
}
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/cli/CliProtocol2.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public String getName() {
*/
@Override
public boolean isOptIn() {
return false;
return true;
}

@Override
Expand Down
7 changes: 0 additions & 7 deletions core/src/main/java/jenkins/install/SetupWizard.java
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,6 @@ public SetupWizard() {

// Disable CLI over Remoting
CLI.get().setEnabled(false);

// Disable old Non-Encrypted protocols ()
HashSet<String> newProtocols = new HashSet<>(jenkins.getAgentProtocols());
newProtocols.removeAll(Arrays.asList(
"JNLP2-connect", "JNLP-connect", "CLI-connect"
));
jenkins.setAgentProtocols(newProtocols);

// require a crumb issuer
jenkins.setCrumbIssuer(new DefaultCrumbIssuer(false));
Expand Down
12 changes: 1 addition & 11 deletions core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol.java
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public void setHub(NioChannelSelector hub) {
*/
@Override
public boolean isOptIn() {
return OPT_IN;
return true;
}

@Override
Expand Down Expand Up @@ -104,14 +104,4 @@ public void handle(Socket socket) throws IOException, InterruptedException {
ExtensionList.lookup(JnlpAgentReceiver.class));
}


/**
* A/B test turning off this protocol by default.
*/
private static final boolean OPT_IN;

static {
byte hash = Util.fromHexString(Jenkins.getInstance().getLegacyInstanceId())[0];
OPT_IN = (hash % 10) == 0;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public String getName() {
*/
@Override
public boolean isOptIn() {
return false;
return true;
}

@Override
Expand Down
29 changes: 2 additions & 27 deletions core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol3.java
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,12 @@ public void setHub(NioChannelSelector hub) {
*/
@Override
public boolean isOptIn() {
return !ENABLED;
return true ;
}

@Override
public String getName() {
// we only want to force the protocol off for users that have explicitly banned it via system property
// everyone on the A/B test will just have the opt-in flag toggled
// TODO strip all this out and hardcode OptIn==TRUE once JENKINS-36871 is merged
Copy link
Member

Choose a reason for hiding this comment

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

#2492 FTR

return forceEnabled != Boolean.FALSE ? handler.getName() : null;
return handler.isEnabled() ? handler.getName() : null;
}

/**
Expand All @@ -79,26 +76,4 @@ public void handle(Socket socket) throws IOException, InterruptedException {
ExtensionList.lookup(JnlpAgentReceiver.class));
}

/**
* Flag to control the activation of JNLP3 protocol.
*
* <p>
* Once this will be on by default, the flag and this field will disappear. The system property is
* an escape hatch for those who hit any issues and those who are trying this out.
*/
@Restricted(NoExternalUse.class)
@SuppressFBWarnings(value = "MS_SHOULD_BE_REFACTORED_TO_BE_FINAL",
justification = "Part of the administrative API for System Groovy scripts.")
public static boolean ENABLED;
private static final Boolean forceEnabled;

static {
forceEnabled = SystemProperties.optBoolean(JnlpSlaveAgentProtocol3.class.getName() + ".enabled");
if (forceEnabled != null) {
ENABLED = forceEnabled;
} else {
byte hash = Util.fromHexString(Jenkins.getActiveInstance().getLegacyInstanceId())[0];
ENABLED = (hash % 10) == 0;
}
}
}
14 changes: 14 additions & 0 deletions test/src/test/java/hudson/cli/CLIActionTest.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package hudson.cli;

import com.google.common.collect.Lists;

import hudson.ExtensionList;
import hudson.Functions;
import hudson.Launcher;
import hudson.Proc;
Expand All @@ -21,7 +23,9 @@
import java.net.URL;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
Expand All @@ -35,6 +39,8 @@
import org.codehaus.groovy.runtime.Security218;
import static org.hamcrest.Matchers.containsString;
import static org.junit.Assert.*;

import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
Expand All @@ -61,6 +67,14 @@ public class CLIActionTest {

private ExecutorService pool;

@Before
public void setUp() {
jenkins.CLI.get().setEnabled(true);
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary; it is still enabled by default in core, disabled in wizard.

Set<String> agentProtocols = new HashSet<>(j.jenkins.getAgentProtocols());
agentProtocols.add(ExtensionList.lookupSingleton(CliProtocol2.class).getName());
j.jenkins.setAgentProtocols(agentProtocols);
}

/**
* Makes sure that the /cli endpoint is functioning.
*/
Expand Down
14 changes: 14 additions & 0 deletions test/src/test/java/hudson/cli/ClientAuthenticationCacheTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,26 @@
package hudson.cli;

import com.google.common.collect.Lists;

import hudson.ExtensionList;
import hudson.Launcher;
import hudson.security.FullControlOnceLoggedInAuthorizationStrategy;
import hudson.util.Secret;
import hudson.util.StreamTaskListener;
import java.io.ByteArrayOutputStream;
import java.io.File;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.logging.Level;
import javax.annotation.CheckForNull;
import jenkins.model.JenkinsLocationConfiguration;
import org.apache.commons.io.FileUtils;
import org.apache.commons.io.output.TeeOutputStream;
import static org.hamcrest.Matchers.containsString;

import org.junit.Before;
import org.junit.Test;
import static org.junit.Assert.*;
import org.junit.Ignore;
Expand All @@ -59,6 +65,14 @@ public class ClientAuthenticationCacheTest {
@Rule
public LoggerRule logging = new LoggerRule().record(ClientAuthenticationCache.class, Level.FINER);

@Before
public void setUp() {
jenkins.CLI.get().setEnabled(true);
Set<String> agentProtocols = new HashSet<>(r.jenkins.getAgentProtocols());
agentProtocols.add(ExtensionList.lookupSingleton(CliProtocol2.class).getName());
r.jenkins.setAgentProtocols(agentProtocols);
}

@Issue("SECURITY-466")
@Test
public void login() throws Exception {
Expand Down
13 changes: 13 additions & 0 deletions test/src/test/java/hudson/security/CliAuthenticationTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,14 @@
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;

import hudson.ExtensionList;
import hudson.cli.CLI;
import hudson.cli.CLICommand;
import hudson.cli.CliProtocol2;
import jenkins.model.Jenkins;
import org.acegisecurity.Authentication;
import org.apache.commons.io.input.NullInputStream;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.For;
Expand All @@ -18,6 +21,8 @@

import java.io.ByteArrayOutputStream;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Set;

/**
* @author Kohsuke Kawaguchi
Expand All @@ -28,6 +33,14 @@ public class CliAuthenticationTest {
@Rule
public JenkinsRule j = new JenkinsRule();

@Before
public void setUp() {
jenkins.CLI.get().setEnabled(true);
Set<String> agentProtocols = new HashSet<>(j.jenkins.getAgentProtocols());
agentProtocols.add(ExtensionList.lookupSingleton(CliProtocol2.class).getName());
j.jenkins.setAgentProtocols(agentProtocols);
}

@Test
public void test() throws Exception {
// dummy security realm that authenticates when username==password
Expand Down