AVRO 2404: Fix insecure urls. #529

nielsbasjes · 2019-05-27T07:52:11Z

No description provided.

nandorKollar · 2019-05-27T08:13:00Z

I don't think that we should replace each http to https, especially not those which are in license header. We should only use secure URLs for downloading artifacts no?

nielsbasjes · 2019-05-27T09:18:19Z

@nandorKollar We can do it that way.
Yet what I see is that in more and more ways security is becoming and issue.
If we generate a documentation website and people click on a non-secure link it is still a way of injecting malicious code.
So that I why I chose to go 'all the way'.
I see either way as 'valid' yet I prefer to get rid of everything 'non-safe' as much as possible.

nandorKollar · 2019-05-27T10:15:21Z

@nielsbasjes I see, though I'm not strongly agains replacing everything to secure URL, my concern in this case is that it can cause unwanted merge conflicts (especially during backporting). Does Rat check pass with the changed license header too?

I'm not sure how did you replace the protocol to https, but maybe not every URL has a corresponding secure version, pointing to an invalid url after changing (in case you used script for this, then probably this is not a valid concern :) ). In addition, it looks like there are some URLs which still use http, but looks like have a secure endpoint too, for example http://www.exolab.org

lang/csharp/src/apache/ipc.test/GeneratedFiles/org/apache/avro/test/All.cs

nandorKollar · 2019-05-27T10:30:48Z

lang/csharp/src/apache/ipc/Responder.cs

-                    {    
-                        // Presumably no match on the exception, throw the original
-                        throw error;
+                    try 


Hm, for some reason Github tells that there was some modification, what? It seems unrelated to insecure urls.

Strange, When doing a git blame on my machine I only see the copyright url change near the top of the file.

Found it: There were trailing spaces on those lines. Although it appears that I introduced them ... strange.

nandorKollar · 2019-05-27T10:31:20Z

lang/csharp/src/apache/ipc/Specific/SpecificRequestor.cs

@@ -87,8 +87,8 @@ public override object ReadResponse(Schema writer, Schema reader, Decoder decode

        public override Exception ReadError(Schema writer, Schema reader, Decoder decoder)
        {
-            var response = new SpecificReader<object>(writer, reader).Read(null, decoder);
-
+            var response = new SpecificReader<object>(writer, reader).Read(null, decoder);


This seems unrelated too.

Strange, When doing a git blame on my machine I only see the copyright url change near the top of the file.

nielsbasjes · 2019-05-27T11:54:22Z

The rat check passes because I added the config to allow the https link.
I also submitted a patch for Rat to support these out of the box: https://issues.apache.org/jira/browse/RAT-212 (which is allowed according to legal)

Fokko · 2019-06-09T07:40:17Z

@nielsbasjes any update on this? Great work so far, but I think we should get this in ASAP.

nielsbasjes · 2019-06-20T10:54:24Z

Cleaned up the changes.

Fixed the issue id in the commit messages (I made a mistake)
Skipped the white space changes (shouldn't be in here).
The https license URL has been checked with legal and has been committed in Apache Rat as "valid" (will be in rat 0.14) https://issues.apache.org/jira/browse/RAT-212

nielsbasjes · 2019-06-20T11:36:49Z

I'm manually checking the documentation links.
Reasons for me to leave urls unchanged:

It is something like a ns or dtd name in an XML
It is a test url in a unit test (linking to localhost)
Sites does not respond on https or give a certficate error.
Domain is completely down (or even for sale) like grepcode.com, codehaus.org,

nielsbasjes · 2019-06-20T11:51:47Z

To quickly find a usable list of remaining urls I used this bash command:

find . -type f | fgrep -v .gem | xargs fgrep http:/ | egrep -v '(http://localhost|http://127.0.0.1|xmlns|xsi|\.dtd|test|NOTICE|LICENSE|INSTALL|README|See http)'

The only remaining this at this point is the same for both py and py3:

nbasjes@ubuntu-vm:~/workspace/Apache/avro$ fgrep -B5 -A5 http:// ./lang/py3/avro/txipc.py ./lang/py/src/avro/txipc.py 
./lang/py3/avro/txipc.py-
./lang/py3/avro/txipc.py-class TwistedHTTPTransceiver(object):
./lang/py3/avro/txipc.py-  """This transceiver uses the Agent class present in Twisted.web >= 9.0
./lang/py3/avro/txipc.py-     for issuing requests to the remote endpoint."""
./lang/py3/avro/txipc.py-  def __init__(self, host, port, remote_name=None, reactor=None):
./lang/py3/avro/txipc.py:    self.url = "http://%s:%d/" % (host, port)
./lang/py3/avro/txipc.py-
./lang/py3/avro/txipc.py-    if remote_name is None:
./lang/py3/avro/txipc.py-      # There's no easy way to get this peer's remote address
./lang/py3/avro/txipc.py-      # in Twisted so I use a random UUID to identify ourselves
./lang/py3/avro/txipc.py-      import uuid
--
./lang/py/src/avro/txipc.py-
./lang/py/src/avro/txipc.py-class TwistedHTTPTransceiver(object):
./lang/py/src/avro/txipc.py-  """This transceiver uses the Agent class present in Twisted.web >= 9.0
./lang/py/src/avro/txipc.py-     for issuing requests to the remote endpoint."""
./lang/py/src/avro/txipc.py-  def __init__(self, host, port, remote_name=None, reactor=None):
./lang/py/src/avro/txipc.py:    self.url = "http://%s:%d/" % (host, port)
./lang/py/src/avro/txipc.py-
./lang/py/src/avro/txipc.py-    if remote_name is None:
./lang/py/src/avro/txipc.py-      # There's no easy way to get this peer's remote address
./lang/py/src/avro/txipc.py-      # in Twisted so I use a random UUID to identify ourselves
./lang/py/src/avro/txipc.py-      import uuid

My python knowledge is to limited to know if this needs to be fixed or not.

At least with this set of changes the entire build uses secured transport and we are now able to quickly find all insecure usecases more quickly.

Please review

lang/c/INSTALL

lang/c/cmake_avrolib.bat

doc/NOTICE

Fokko · 2019-06-21T07:00:09Z

Thanks Niels for taking care of this! 👏

nielsbasjes · 2019-06-21T11:06:40Z

@Fokko : This has been committed on master ONLY.
Should this be on other branches too? Perhaps the 1.9 branch?

Fokko · 2019-06-22T20:32:42Z

Yes, we should cherry-pick -x this onto the 1.9 branch 👍

nielsbasjes · 2019-06-25T13:42:54Z

I'm checking the cherry picked branch-1.9 edition against CI by simply creating a new pull request: #564

probot-autolabeler bot added build website labels May 27, 2019

nandorKollar reviewed May 27, 2019

View reviewed changes

lang/csharp/src/apache/ipc.test/GeneratedFiles/org/apache/avro/test/All.cs Show resolved Hide resolved

nandorKollar reviewed May 27, 2019

View reviewed changes

nielsbasjes force-pushed the AVRO-2401-InsecureUrls branch from 4255501 to 3442c02 Compare June 20, 2019 09:00

nielsbasjes changed the title ~~WORK IN PROGRESS: AVRO 2401: Fix insecure urls.~~ WORK IN PROGRESS: AVRO 2404: Fix insecure urls. Jun 20, 2019

nielsbasjes force-pushed the AVRO-2401-InsecureUrls branch from 3442c02 to 7516358 Compare June 20, 2019 09:04

AVRO-2404: Make jar download secure

72a3cd4

nielsbasjes force-pushed the AVRO-2401-InsecureUrls branch from 7516358 to 8ffef3e Compare June 20, 2019 11:36

nielsbasjes changed the title ~~WORK IN PROGRESS: AVRO 2404: Fix insecure urls.~~ AVRO 2404: Fix insecure urls. Jun 20, 2019

Fokko approved these changes Jun 20, 2019

View reviewed changes

lang/c/INSTALL Show resolved Hide resolved

lang/c/cmake_avrolib.bat Outdated Show resolved Hide resolved

doc/NOTICE Show resolved Hide resolved

nielsbasjes added 2 commits June 20, 2019 15:04

AVRO-2404: Make all references to apache.org https

cfdb432

AVRO-2404: Make all external links https

bda5c1d

nielsbasjes force-pushed the AVRO-2401-InsecureUrls branch from 8ffef3e to bda5c1d Compare June 20, 2019 13:12

nielsbasjes merged commit 2d3b1fe into apache:master Jun 20, 2019

nielsbasjes deleted the AVRO-2401-InsecureUrls branch June 20, 2019 17:39

nielsbasjes restored the AVRO-2401-InsecureUrls branch June 20, 2019 17:43

nielsbasjes mentioned this pull request Jun 25, 2019

AVRO 2404: Fix insecure urls. - branch 1.9 #564

Merged

nielsbasjes deleted the AVRO-2401-InsecureUrls branch June 25, 2019 19:37

blachniet mentioned this pull request Jul 19, 2019

AVRO-2389: C# reflection based serializer/deserializer #587

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AVRO 2404: Fix insecure urls. #529

AVRO 2404: Fix insecure urls. #529

nielsbasjes commented May 27, 2019

nandorKollar commented May 27, 2019

nielsbasjes commented May 27, 2019

nandorKollar commented May 27, 2019

nandorKollar May 27, 2019

nielsbasjes May 27, 2019

nielsbasjes May 27, 2019 •

edited

nandorKollar May 27, 2019

nielsbasjes May 27, 2019

nielsbasjes commented May 27, 2019

Fokko commented Jun 9, 2019

nielsbasjes commented Jun 20, 2019

nielsbasjes commented Jun 20, 2019

nielsbasjes commented Jun 20, 2019

Fokko commented Jun 21, 2019

nielsbasjes commented Jun 21, 2019 •

edited

Fokko commented Jun 22, 2019

nielsbasjes commented Jun 25, 2019

AVRO 2404: Fix insecure urls. #529

AVRO 2404: Fix insecure urls. #529

Conversation

nielsbasjes commented May 27, 2019

nandorKollar commented May 27, 2019

nielsbasjes commented May 27, 2019

nandorKollar commented May 27, 2019

nandorKollar May 27, 2019

Choose a reason for hiding this comment

nielsbasjes May 27, 2019

Choose a reason for hiding this comment

nielsbasjes May 27, 2019 • edited

Choose a reason for hiding this comment

nandorKollar May 27, 2019

Choose a reason for hiding this comment

nielsbasjes May 27, 2019

Choose a reason for hiding this comment

nielsbasjes commented May 27, 2019

Fokko commented Jun 9, 2019

nielsbasjes commented Jun 20, 2019

nielsbasjes commented Jun 20, 2019

nielsbasjes commented Jun 20, 2019

Fokko commented Jun 21, 2019

nielsbasjes commented Jun 21, 2019 • edited

Fokko commented Jun 22, 2019

nielsbasjes commented Jun 25, 2019

nielsbasjes May 27, 2019 •

edited

nielsbasjes commented Jun 21, 2019 •

edited