From f4435b02b012258183e7e81c49a2100d0ca454ff Mon Sep 17 00:00:00 2001 From: Sven Selberg Date: Thu, 5 Nov 2020 11:00:33 +0100 Subject: [PATCH 1/2] Docs: Clarify that 'm' push option sets patch set description Change-Id: Ia8cb481709a6e91567bf5be0dec48c6353c4947b --- Documentation/concept-patch-sets.txt | 2 +- Documentation/user-upload.txt | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Documentation/concept-patch-sets.txt b/Documentation/concept-patch-sets.txt index 8609afd0cd..274fbb0d74 100644 --- a/Documentation/concept-patch-sets.txt +++ b/Documentation/concept-patch-sets.txt @@ -89,7 +89,7 @@ evolves, such as "Added more unit tests." Unlike the change description, a patch set description does not become a part of the project's history. To add a patch set description, click *Add a patch set description*, located in -the file list. +the file list, or provide it link:user-upload.html#patch_set_description[on upload]. GERRIT ------ diff --git a/Documentation/user-upload.txt b/Documentation/user-upload.txt index 56602e27be..c702c767ab 100644 --- a/Documentation/user-upload.txt +++ b/Documentation/user-upload.txt @@ -314,11 +314,11 @@ link:intro-user.html#work-in-progress-by-default[user preference]. If the preference is set so the default behavior is to create `work-in-progress` changes, this can be overridden with the `ready` option. -[[message]] -==== Message +[[patch_set_description]] +==== Patch Set Description -A comment message can be applied to the change by using the `message` (or `m`) -option: +A link:concept-patch-sets.html#_description[patch set description] can be +applied by using the `message` (or `m`) option: ---- git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/experimental%m=This_is_a_rebase_on_master%21 From d0d5fe6566823b42d3ac68808c263d30491a0f10 Mon Sep 17 00:00:00 2001 From: Marija Savtchouk Date: Thu, 5 Nov 2020 13:30:05 +0000 Subject: [PATCH 2/2] Verify hostname when sending emails via SMTP server with SMTPSClient The SMTP server's certificate and hostname must be verified if encryption is enabled with SSL verification in the host settings (sendemail.smtpEncryption and sendemail.sslVerify). SMTPSClient from Apache Commons Net used for SSL processing. It has the following downside: if encryption is not required, SMTPSClient is used in 'explicit' mode with the upgrade to TLS never called. Thus, the client is somewhat misused. However, this helps to avoid code duplication and we could use AuthenticatingSMTPClient instead of custom auth. Tested by running a local gerrit host with different [sendemail] configuration for smtpServer/smtpServerPort/smtpEncryption/sslVerify Traced the email notification to the point where SMTP server fails/passes certificate/hostname verification. Malicious server: pass wrong-mail-host.sectests.net/465/SSL/false fail wrong-mail-host.sectests.net/465/SSL/true Valid SSL server: pass smtp.laposte.net/465/SSL/true|false Valid TLS server: pass smtp.gmail.com/587/TLS/true|false Bug: Issue 12629 Change-Id: I1755749da5006c3b56de010f953a79e25a5b7539 --- .../server/mail/send/SmtpEmailSender.java | 8 +- .../util/ssl/BlindSSLSocketFactory.java | 15 +-- .../gerrit/util/ssl/BlindTrustManager.java | 33 +++++++ .../commons/net/smtp/AuthSMTPClient.java | 98 +++++++++---------- 4 files changed, 84 insertions(+), 70 deletions(-) create mode 100644 java/com/google/gerrit/util/ssl/BlindTrustManager.java diff --git a/java/com/google/gerrit/server/mail/send/SmtpEmailSender.java b/java/com/google/gerrit/server/mail/send/SmtpEmailSender.java index 7207c0099c..4e1ab66ca5 100644 --- a/java/com/google/gerrit/server/mail/send/SmtpEmailSender.java +++ b/java/com/google/gerrit/server/mail/send/SmtpEmailSender.java @@ -392,11 +392,7 @@ public class SmtpEmailSender implements EmailSender { } private SMTPClient open() throws EmailException { - final AuthSMTPClient client = new AuthSMTPClient(UTF_8.name()); - - if (smtpEncryption == Encryption.SSL) { - client.enableSSL(sslVerify); - } + final AuthSMTPClient client = new AuthSMTPClient(smtpEncryption == Encryption.SSL, sslVerify); client.setConnectTimeout(connectTimeout); try { @@ -412,7 +408,7 @@ public class SmtpEmailSender implements EmailSender { } if (smtpEncryption == Encryption.TLS) { - if (!client.startTLS(smtpHost, smtpPort, sslVerify)) { + if (!client.execTLS()) { throw new EmailException("SMTP server does not support TLS"); } if (!client.login()) { diff --git a/java/com/google/gerrit/util/ssl/BlindSSLSocketFactory.java b/java/com/google/gerrit/util/ssl/BlindSSLSocketFactory.java index 6dc10065c0..88845efb24 100644 --- a/java/com/google/gerrit/util/ssl/BlindSSLSocketFactory.java +++ b/java/com/google/gerrit/util/ssl/BlindSSLSocketFactory.java @@ -20,7 +20,6 @@ import java.net.Socket; import java.net.UnknownHostException; import java.security.GeneralSecurityException; import java.security.SecureRandom; -import java.security.cert.X509Certificate; import javax.net.SocketFactory; import javax.net.ssl.SSLContext; import javax.net.ssl.SSLSocketFactory; @@ -32,19 +31,7 @@ public class BlindSSLSocketFactory extends SSLSocketFactory { private static final BlindSSLSocketFactory INSTANCE; static { - final X509TrustManager dummyTrustManager = - new X509TrustManager() { - @Override - public X509Certificate[] getAcceptedIssuers() { - return null; - } - - @Override - public void checkClientTrusted(X509Certificate[] chain, String authType) {} - - @Override - public void checkServerTrusted(X509Certificate[] chain, String authType) {} - }; + final X509TrustManager dummyTrustManager = new BlindTrustManager(); try { final SSLContext context = SSLContext.getInstance("SSL"); diff --git a/java/com/google/gerrit/util/ssl/BlindTrustManager.java b/java/com/google/gerrit/util/ssl/BlindTrustManager.java new file mode 100644 index 0000000000..2db091ab0f --- /dev/null +++ b/java/com/google/gerrit/util/ssl/BlindTrustManager.java @@ -0,0 +1,33 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.util.ssl; + +import java.security.cert.X509Certificate; +import javax.net.ssl.X509TrustManager; + +/** TrustManager implementation that accepts all certificates without validation. */ +public class BlindTrustManager implements X509TrustManager { + + @Override + public X509Certificate[] getAcceptedIssuers() { + return null; + } + + @Override + public void checkClientTrusted(X509Certificate[] chain, String authType) {} + + @Override + public void checkServerTrusted(X509Certificate[] chain, String authType) {} +} diff --git a/java/org/apache/commons/net/smtp/AuthSMTPClient.java b/java/org/apache/commons/net/smtp/AuthSMTPClient.java index 33dd609080..4fe230e1bc 100644 --- a/java/org/apache/commons/net/smtp/AuthSMTPClient.java +++ b/java/org/apache/commons/net/smtp/AuthSMTPClient.java @@ -16,69 +16,67 @@ package org.apache.commons.net.smtp; import static java.nio.charset.StandardCharsets.UTF_8; -import com.google.gerrit.util.ssl.BlindSSLSocketFactory; -import java.io.BufferedReader; -import java.io.BufferedWriter; +import com.google.gerrit.util.ssl.BlindTrustManager; import java.io.IOException; -import java.io.InputStreamReader; -import java.io.OutputStreamWriter; import java.io.UnsupportedEncodingException; -import java.net.SocketException; import java.security.InvalidKeyException; import java.security.NoSuchAlgorithmException; import java.util.Arrays; import java.util.List; import javax.crypto.Mac; import javax.crypto.spec.SecretKeySpec; -import javax.net.ssl.SSLParameters; -import javax.net.ssl.SSLSocket; -import javax.net.ssl.SSLSocketFactory; import org.apache.commons.codec.binary.Base64; -public class AuthSMTPClient extends SMTPClient { +/** + * SMTP Client with authentication support and optional SSL processing and verification. {@link + * org.apache.commons.net.smtp.SMTPSClient} is used for the SSL handshake and hostname verification. + * + *

If shouldHandshakeOnConnect mode is selected, SSL/TLS negotiation starts right after the + * connection has been established. Otherwise SSL/TLS negotiation will only occur if {@link + * AuthSMTPClient#execTLS} is explicitly called and the server accepts the command. + * + *

Examples: + * + *