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-45841] - Disable JNLP/JNLP2/CLI protocols on new installations #2950

Merged
Show file tree
Hide file tree
Changes from 8 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
7 changes: 6 additions & 1 deletion core/src/main/java/hudson/cli/CliProtocol.java
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,17 @@ public String getName() {
return jenkins.CLI.get().isEnabled() ? "CLI-connect" : null;
}

@Override
public boolean isDeprecated() {
return true;
}

/**
* {@inheritDoc}
*/
@Override
public String getDisplayName() {
return "Jenkins CLI Protocol/1";
return "Jenkins CLI Protocol/1 (deprecated, unencrypted)";
}

@Override
Expand Down
8 changes: 7 additions & 1 deletion core/src/main/java/hudson/cli/CliProtocol2.java
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,18 @@ public boolean isOptIn() {
return false;
}

@Override
public boolean isDeprecated() {
// We do not recommend it though it may be required for Remoting CLI
return true;
}

/**
* {@inheritDoc}
*/
@Override
public String getDisplayName() {
return "Jenkins CLI Protocol/2";
return "Jenkins CLI Protocol/2 (deprecated)";
}

@Override
Expand Down
13 changes: 13 additions & 0 deletions core/src/main/java/jenkins/AgentProtocol.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import java.io.IOException;
import java.net.Socket;
import java.util.Set;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import jenkins.model.Jenkins;

/**
Expand Down Expand Up @@ -53,6 +55,16 @@ public boolean isOptIn() {
public boolean isRequired() {
return false;
}

/**
* Checks if the protocol is deprecated.
*
* @since TODO
*/
public boolean isDeprecated() {
return false;
}

/**
* Protocol name.
*
Expand Down Expand Up @@ -86,6 +98,7 @@ public static ExtensionList<AgentProtocol> all() {
return ExtensionList.lookup(AgentProtocol.class);
}

@CheckForNull
public static AgentProtocol of(String protocolName) {
for (AgentProtocol p : all()) {
String n = p.getName();
Expand Down
10 changes: 10 additions & 0 deletions core/src/main/java/jenkins/install/SetupWizard.java
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@
import java.net.HttpURLConnection;
import java.net.URL;
import java.net.URLConnection;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import jenkins.CLI;
Expand All @@ -62,6 +64,7 @@
import net.sf.json.JSONObject;
import org.apache.commons.io.FileUtils;
import org.apache.commons.io.IOUtils;
import org.jenkinsci.remoting.engine.JnlpProtocol4Handler;
import org.kohsuke.accmod.restrictions.DoNotUse;
import org.kohsuke.stapler.interceptor.RequirePOST;

Expand Down Expand Up @@ -127,6 +130,13 @@ public class SetupWizard extends PageDecorator {
// 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
112 changes: 112 additions & 0 deletions core/src/main/java/jenkins/slaves/DeprecatedAgentProtocolMonitor.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/*
* The MIT License
*
* Copyright (c) 2017 CloudBees, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
package jenkins.slaves;

import hudson.Extension;
import hudson.init.InitMilestone;
import hudson.init.Initializer;
import hudson.model.AdministrativeMonitor;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.CheckForNull;
import jenkins.AgentProtocol;
import jenkins.model.Jenkins;
import org.apache.commons.lang.StringUtils;
import org.jenkinsci.Symbol;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;


/**
* Monitors enabled protocols and warns if a protocol is deprecated.
*
* @author Oleg Nenashev
* @since TODO
*/
@Extension
@Symbol("remotingProtocolVersions")
Copy link
Member

Choose a reason for hiding this comment

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

agentProtocolVersions? Or simply deprecatedAgentProtocols?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not only about Agents :(

@Restricted(NoExternalUse.class)
public class DeprecatedAgentProtocolMonitor extends AdministrativeMonitor {

private static final Logger LOGGER = Logger.getLogger(DeprecatedAgentProtocolMonitor.class.getName());

public DeprecatedAgentProtocolMonitor() {
super(AgentProtocol.class.getName() + "-deprecated");
Copy link
Member

Choose a reason for hiding this comment

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

delete and use the default ID

}

@Override
public String getDisplayName() {
return "Deprecated Remoting protocols";
}

@Override
public boolean isActivated() {
final Set<String> agentProtocols = Jenkins.getInstance().getAgentProtocols();
for (String name : agentProtocols) {
AgentProtocol pr = AgentProtocol.of(name);
if (pr != null && pr.isDeprecated()) {
return true;
}
}
return false;
}

@Restricted(NoExternalUse.class)
Copy link
Member

Choose a reason for hiding this comment

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

Redundant—already present on the whole class.

public String getDeprecatedProtocols() {
String res = getDeprecatedProtocolsString();
return res != null ? res : "N/A";
}

@CheckForNull
public static String getDeprecatedProtocolsString() {
final List<String> deprecatedProtocols = new ArrayList<>();
final Set<String> agentProtocols = Jenkins.getInstance().getAgentProtocols();
for (String name : agentProtocols) {
AgentProtocol pr = AgentProtocol.of(name);
if (pr != null && pr.isDeprecated()) {
deprecatedProtocols.add(name);
}
}
if (deprecatedProtocols.isEmpty()) {
return null;
}
return StringUtils.join(deprecatedProtocols, ',');
}

@Initializer(after = InitMilestone.PLUGINS_STARTED)
@Restricted(NoExternalUse.class)
public static void initializerCheck() {
String protocols = getDeprecatedProtocolsString();
if(protocols != null) {
LOGGER.log(Level.WARNING, "This Jenkins instance uses deprecated Remoting protocols: {0}"
Copy link
Member

Choose a reason for hiding this comment

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

Superfluous logging, delete. Presence of an active AdministrativeMonitor suffices. Will show up in the manager UI, in support bundles, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to keep this log as well. It is important if somebody has remote logging system configured

+ "It may impact stability of the instance. "
+ "If newer protocol versions are supported by all system components "
+ "(agents, CLI and other clients), "
+ "it is highly recommended to disable the deprecated protocols.", protocols);
}
}
}
14 changes: 14 additions & 0 deletions core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol.java
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,15 @@
* is generated once and used forever, which makes this whole scheme
* less secure.
*
* <h2>UI Extensions</h2>
Copy link
Member

Choose a reason for hiding this comment

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

I think you meant to place this Javadoc on AgentProtocol, the extension point, not on this implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed

* <dl>
* <dt>description.jelly</dt>
* <dd>Optional protocol description</dd>
* <dt>deprecationCause.jelly</dt>
* <dd>Optional. If the protocol is marked as {@link #isDeprecated()},
* clarifies the deprecation reason and provides extra documentation links</dd>
* </dl>
*
* @author Kohsuke Kawaguchi
* @since 1.467
*/
Expand Down Expand Up @@ -79,6 +88,11 @@ public boolean isOptIn() {
return OPT_IN;
}

@Override
public boolean isDeprecated() {
return true;
}

@Override
public String getName() {
return handler.isEnabled() ? handler.getName() : null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ public boolean isOptIn() {
return false;
}

@Override
public boolean isDeprecated() {
return true;
}

/**
* {@inheritDoc}
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@ public String getDisplayName() {
return Messages.JnlpSlaveAgentProtocol3_displayName();
}

@Override
public boolean isDeprecated() {
return true;
}

@Override
public void handle(Socket socket) throws IOException, InterruptedException {
handler.handle(socket,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core">
${%message}
</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
message=This protocol is an obsolete protocol, which has been replaced by CLI2-connect. \
It is also not encrypted.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core">
${%message}
</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
message=Remoting-based CLI is deprecated and not recommended due to the security reasons. \
It is recommended to disable this protocol on the instance. \
if you need Remoting CLI on your instance, this protocol has to be enabled.
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ l.layout(norefresh:true, permission:app.ADMINISTER, title:my.displayName, csscla
td(colspan:"2");
td(class:"setting-description"){
st.include(from:p, page: "description", optional:true);
if (p.deprecated) {
br()
text(b(_("Deprecated. ")))
st.include(from:p, page: "deprecationCause", optional:true);
}
}
td();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core">
<div class="warning">
${%blurb(it.deprecatedProtocols)}
<a href="${rootURL}/configureSecurity">${%Protocol Configuration}</a>
</div>
</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
blurb=This Jenkins instance uses deprecated Remoting protocols: {0}. \
Copy link
Member

Choose a reason for hiding this comment

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

AgentProtocol is not restricted to Remoting, though I am unaware of any non-Remoting-based protocols other than the trivial ping.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, "agent" would be definitely worse due to CLI. I will rename it

It may impact stability of the instance. \
If newer protocol versions are supported by all system components (agents, CLI and other clients), \
it is highly recommended to disable the deprecated protocols.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core">
${%message}
</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
message=This protocol is an obsolete protocol, which has been replaced by JNLP2-connect. \
It is also not encrypted.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core">
${%message}
<a href="https://github.com/jenkinsci/remoting/blob/master/docs/protocols.md#jnlp2-connect-errata">${%JNLP2 Protocol Errata}</a>
</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
message=This protocol has known stability issues, and it is replaced by JNLP4. \
It is also not encrypted. \
See more information in the protocol Errata.
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core">
${%This protocol is unstable. See the protocol documentation for more info.}
<!-- TODO: Move/mirror it to jenkins.io -->
<a href="https://github.com/jenkinsci/remoting/blob/master/docs/protocols.md#jnlp3-connect-errata">${%JNLP3 Protocol Errata}</a>
Copy link
Member

Choose a reason for hiding this comment

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

Do not publish a link to the master branch, as this will break when that documentation is moved! Always use a permalink to a specific tag or hash. Better yet, use a jenkins.io redirect.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, will setup redirects for the remoting documentation.
@daniel-beck Do we have guides for it?

Copy link
Member

@daniel-beck daniel-beck Jul 31, 2017

Choose a reason for hiding this comment

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

@oleg-nenashev Just do what has been done before: https://github.com/jenkins-infra/jenkins.io/tree/master/content/redirect

Note that these are typically rather specific URLs, often redirecting to more general pages. This is deliberate, as it makes restructuring documentation much easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

@daniel-beck ack, thanks. I would rather want to implement automatic publishing of Remoting docs to jenkins.io, but AFAIK our infrastructure is not ready to it.

Copy link
Member

Choose a reason for hiding this comment

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

Even then, a redirect URL from Jenkins is the way to go as per @jglick's comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I have the requested links in #2950. Now we just need to agree where to keep them, CC @rtyler

</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@

Original file line number Diff line number Diff line change
@@ -1,4 +1 @@
summary=Extends the version 2 protocol by adding basic encryption but requires a thread per client. \
This protocol falls back to Java Web Start Agent Protocol/2 (unencrypted) when it can't create a secure connection. \
This protocol is not recommended. \
Use Java Web Start Agent Protocol/4 instead.
summary=Extends the version 2 protocol by adding basic encryption but requires a thread per client.
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,4 @@
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.

summary=Erweitert das Protokoll Version 2 um einfache Verschl\u00FCsselung, aber erfordert einen Thread pro Client. \
Dieses Protokoll f\u00E4llt auf Protokoll-Version 2 (unverschl\u00FCsselt) zur\u00FCck, wenn keine sichere Verbindung hergestellt werden kann. \
Dieses Protokoll sollte nicht verwendet werden, stattdessen sollte Protokoll-Version 4 verwendet werden.
summary=Erweitert das Protokoll Version 2 um einfache Verschl\u00fcsselung, aber erfordert einen Thread pro Client.
6 changes: 3 additions & 3 deletions core/src/main/resources/jenkins/slaves/Messages.properties
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.

JnlpSlaveAgentProtocol.displayName=Java Web Start Agent Protocol/1 (unencrypted)
JnlpSlaveAgentProtocol2.displayName=Java Web Start Agent Protocol/2 (unencrypted)
JnlpSlaveAgentProtocol3.displayName=Java Web Start Agent Protocol/3 (basic encryption)
JnlpSlaveAgentProtocol.displayName=Java Web Start Agent Protocol/1 (deprecated, unencrypted)
JnlpSlaveAgentProtocol2.displayName=Java Web Start Agent Protocol/2 (deprecated, unencrypted)
JnlpSlaveAgentProtocol3.displayName=Java Web Start Agent Protocol/3 (deprecated, basic encryption)
JnlpSlaveAgentProtocol4.displayName=Java Web Start Agent Protocol/4 (TLS encryption)
Loading