[JENKINS-54601] Include security fixes from 1.9.40 in 1.7.24.X #3

varyvol · 2018-11-15T15:42:34Z

Gather security fixes from 1.9.40
Change parent POM
Script and instruction to test/release with Docker (thanks @batmat).

Both PCT and OSS ATH was executed without any detected failure.

@batmat @oleg-nenashev @dwnusbaum @stephenc

…alse

…ncHttpClient#510, close AsyncHttpClient#197

No custom Dockerfile yet was needed for this. But we can easily introduce one if the necessary environment gets more complex/custom.

Add script to make it easier to get a working JDK7 environment

…-client into JENKINS-54601

run-maven-env.sh

fcojfernandez · 2018-11-16T09:34:13Z

run-maven-env.sh

+
+if [ -f "$MAVEN_SETTINGS_SECURITY" ]
+then
+	echo "Using $MAVEN_SETTINGS_SECURITY file found."


Better remove 'Using' or 'found'? It sounds weird to me 'Using settings-security.xml file found'

Agree. Changed.

run-maven-env.sh

fcojfernandez

Code looks to me and also it's picked from https://github.com/AsyncHttpClient/async-http-client/
I have no enough context to review it from a functional PoV

MRamonLeon · 2018-11-16T08:52:02Z

RELEASE.adoc

+= How to release this in Jenkins environment
+
+See the `run-maven.sh` shell script to run a Docker container with the right environment.
+Then, if you already have the right Maven settings.xml files, you should be able to


NIT: Maven settings.xml files-> maven settings*.xml files or maven settings files or maven setting.xml file if you are referring only to this one

MRamonLeon · 2018-11-16T09:57:31Z

src/main/java/com/ning/http/client/AsyncHttpClientConfig.java

@@ -467,6 +447,17 @@ public boolean isValid() {
     * @return the {@link HostnameVerifier}
     */
    public HostnameVerifier getHostnameVerifier() {
+        if (hostnameVerifier == null) {
+            synchronized (this) {
+                if (hostnameVerifier == null)


A { will be helpful

IMO, since this repo is to fix security vulnerabilities and we are backporting the fix, I wouldn't change the code to ease future backports

Yes, I prefer not to touch code that is coming from the library directly.

+1 to @varyvol and @fcojfernandez in this case

MRamonLeon · 2018-11-16T10:06:05Z

src/main/java/com/ning/http/client/AsyncHttpClientConfig.java

+        /**
+         * Set the maximum time in millisecond an {@link com.ning.http.client.AsyncHttpClient} can wait when connecting to a remote host
+         *
+         * @param defaultConnectionTimeOutInMs the maximum time in millisecond an {@link com.ning.http.client.AsyncHttpClient} can wait when connecting to a remote host


change the param name to connectTimeout

MRamonLeon · 2018-11-16T10:07:22Z

src/main/java/com/ning/http/client/AsyncHttpClientConfig.java

+        /**
+         * Set the maximum time in millisecond an {@link com.ning.http.client.AsyncHttpClient} can stay idle.
+         *
+         * @param defaultIdleConnectionTimeoutInMs


Ditto for all param javadocs

MRamonLeon · 2018-11-16T10:08:52Z

src/main/java/com/ning/http/client/AsyncHttpClientConfig.java

@@ -1043,6 +1051,11 @@ public Builder setMaxConnectionLifeTimeInMs(int maxConnectionLifeTimeInMs) {
           return this;
        }

+        public Builder setAcceptAnyCertificates(boolean acceptAnyCertificate) {


Add javadoc as other setters?

This is coming from the library directly, that's why.

Co-Authored-By: varyvol <[email protected]>

MRamonLeon

AFAIU

oleg-nenashev

Although it is a formally breaking change, I do not see any usages of changed classes inside the jenkinsci organization. It does not fully guarantee that there will be no impact on Jenkins, because any transitive dependency within GitHub may also include dependency on Asynch HTTP Client. And we cannot be 100% for sure.

Why do I request changes in this pull request?

The README.md in the repository is confusing. The changed is not tested on Travis CI as the README suggests. I either request setting up a flow on Travis/Jenkins or removing the the mention of CI
RELEASE.adoc is not linked from README.adoc. IMO README.adoc is confusing, because it has not been updated after the fork. I recommend to just deleting the existing content and adding a small readme explaining reasons of the fork and what's inside

It would be also great if you could add a summary of how this change was tested (to the PR description).

run-maven-env.sh

oleg-nenashev · 2018-11-20T08:25:08Z

src/main/java/com/ning/http/client/AsyncHttpClientConfig.java

@@ -467,6 +447,17 @@ public boolean isValid() {
     * @return the {@link HostnameVerifier}
     */
    public HostnameVerifier getHostnameVerifier() {
+        if (hostnameVerifier == null) {
+            synchronized (this) {
+                if (hostnameVerifier == null)


+1 to @varyvol and @fcojfernandez in this case

oleg-nenashev · 2018-11-20T08:26:42Z

src/main/java/com/ning/http/client/AsyncHttpClientConfig.java

-         * @param sslEngineFactory the {@link SSLEngineFactory} for secure connection
-         * @return a {@link Builder}
-         */
-        public Builder setSSLEngineFactory(SSLEngineFactory sslEngineFactory) {


Was unable to find usages in Jenkins plugins. I am OK with that though I would recommend to keep a method stub which would be throwing an explicit RuntimeException that the method is removed

oleg-nenashev · 2018-11-20T08:28:12Z

src/main/java/com/ning/http/client/AsyncHttpClientConfigBean.java

@@ -165,11 +166,6 @@ public AsyncHttpClientConfigBean setSslContext(SSLContext sslContext) {
        return this;
    }

-    public AsyncHttpClientConfigBean setSslEngineFactory(SSLEngineFactory sslEngineFactory) {


same as above

oleg-nenashev · 2018-11-20T08:29:07Z

src/main/java/com/ning/http/client/SSLEngineFactory.java

-/**
- * Factory that creates an {@link SSLEngine} to be used for a single SSL connection.
- */
-public interface SSLEngineFactory {


No usages AFAICT

oleg-nenashev · 2018-11-20T08:36:51Z

src/main/java/com/ning/http/util/SslUtils.java

@@ -15,101 +15,43 @@
 */
 package com.ning.http.util;


I was unable to find any usages of this class inside the jenkinsci org. Fine with me

oleg-nenashev · 2018-11-20T08:48:15Z

Maybe we should also rename this repository to lib-async-http-client to follow the common naming pattern for libraries. CC @daniel-beck

varyvol · 2018-11-20T09:16:04Z

@oleg-nenashev I've updated the README file.

pom.xml

oleg-nenashev · 2018-11-20T09:28:44Z

pom.xml

@@ -9,31 +9,18 @@
    <groupId>com.ning</groupId>


Would be nice to update the parent POM to the Jenkins Parent POM to make it follow the jenkins release flow standards. https://github.com/jenkinsci/pom

varyvol · 2018-11-20T10:37:55Z

@oleg-nenashev used https.

fcojfernandez

Better indentation now!

oleg-nenashev

The code itself looks good to me

Did you have a chance to process this comment?

Maybe we should also rename this repository to lib-async-http-client to follow the common naming pattern for libraries.

varyvol · 2018-11-20T15:01:33Z

Yes, but unsure on the answer. I thought that was already contemplated on the HOSTING request. I can raise a ticket if that is the usual naming pattern. Anyway I guess this is independent from the PR.

oleg-nenashev · 2018-11-20T15:16:03Z

Yes, but unsure on the answer. I thought that was already contemplated on the HOSTING request. I can raise a ticket if that is the usual naming pattern. Anyway I guess this is independent from the PR.

Yes, it is independent. Would be nice to discuss it before the release

Rebecca Ysteboe and others added 11 commits November 13, 2018 11:33

Change version for jenkins

fadf57b

[SECURITY-650] Introduce acceptAnyCertificate config, defaulting to f…

2604a2f

…alse

Use a hostname verifier that does hostname verification, backport Asy…

c8e7c7f

…ncHttpClient#510, close AsyncHttpClient#197

Bump netty version

78751a7

Restore necessary compatibility

7c0a9fe

[JENKINS-54601] Fix test failures.

60d3b16

[JENKINS-54601] Correct POM info.

ce27a79

Add script to make it easier to get a working JDK7 environment

3f07407

No custom Dockerfile yet was needed for this. But we can easily introduce one if the necessary environment gets more complex/custom.

Merge pull request #1 from batmat/add-build-Dockerfile

5805e3a

Add script to make it easier to get a working JDK7 environment

[JENKINS-54601] Include proper hostname verifier logic.

5025dfb

Merge branch 'JENKINS-54601' of https://github.com/varyvol/async-http…

98d019d

…-client into JENKINS-54601

varyvol changed the title ~~Jenkins 54601~~ [JENKINS-54601] Include security fixes from 1.9.40 in 1.7.24.X Nov 15, 2018

fcojfernandez reviewed Nov 16, 2018

View reviewed changes

varyvol mentioned this pull request Nov 16, 2018

[JENKINS-54601] Move security fixes to async-http-client library jenkinsci/async-http-client-plugin#9

Merged

fcojfernandez approved these changes Nov 16, 2018

View reviewed changes

MRamonLeon reviewed Nov 16, 2018

View reviewed changes

fcojfernandez and others added 3 commits November 16, 2018 12:09

Update run-maven-env.sh

d93816d

Co-Authored-By: varyvol <[email protected]>

Update run-maven-env.sh

dc88455

Co-Authored-By: varyvol <[email protected]>

[JENKINS-54601] Improve documentation and javadocs.

7d09d9c

MRamonLeon approved these changes Nov 16, 2018

View reviewed changes

oleg-nenashev requested changes Nov 20, 2018

View reviewed changes

Evaristo Gutiérrez added 2 commits November 20, 2018 10:14

[JENKINS-54601] Update README.

fb8cc92

[JENKINS-54601] Update README.

4de09ec

oleg-nenashev reviewed Nov 20, 2018

View reviewed changes

Evaristo Gutiérrez added 2 commits November 20, 2018 10:53

[JENKINS-54601] Unused imports.

d7e53f6

[JENKINS-54601] Use https.

ee69357

fcojfernandez approved these changes Nov 20, 2018

View reviewed changes

[JENKINS-54601] Fix indentation.

f579282

fcojfernandez approved these changes Nov 20, 2018

View reviewed changes

oleg-nenashev approved these changes Nov 20, 2018

View reviewed changes

oleg-nenashev mentioned this pull request Nov 20, 2018

Fix the release destination in the repository #4

Open

varyvol merged commit 20b3885 into jenkinsci:1.7.x Nov 20, 2018

[JENKINS-54601] Include security fixes from 1.9.40 in 1.7.24.X #3

[JENKINS-54601] Include security fixes from 1.9.40 in 1.7.24.X #3

Uh oh!

Conversation

varyvol commented Nov 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fcojfernandez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MRamonLeon left a comment

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev commented Nov 20, 2018

Uh oh!

varyvol commented Nov 20, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varyvol commented Nov 20, 2018

Uh oh!

fcojfernandez left a comment

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

varyvol commented Nov 20, 2018

Uh oh!

oleg-nenashev commented Nov 20, 2018

Uh oh!

Uh oh!

varyvol commented Nov 15, 2018 •

edited

Loading

oleg-nenashev left a comment •

edited

Loading