New feature: Safe repo urls #642

theotherjimmy · 2018-03-15T15:44:27Z

This series of commits adds a sense of "secure" and "insecure" URLs to Mbed CLI.

Security and Liability in Mbed CLI

A user of Mbed CLI does not know about every single repository URL that will be accessed/cloned during mbed import and mbed add. Unlike the git clone <url> command, where end-user is de-facto aware what's being cloned (essentially it's a "consensual clone" as they can see the URL before executing git), Mbed CLI clones many repositories recursively without prior consent or user awareness, except for the starting repository or program URL.

This poses some challenges, including legally, as an end-user could blame Mbed CLI for causing their git or hg to try to access a funky URL/service port.

For example, combining the recursive nature of Mbed CLI with bad intentions, could lead to terrifying results. It's not hard to imagine a malicious program containing 100s .lib files pointing at different ports at b1-rtr0-hsrp.jpl.nasa.gov (as repo URLs), which, once mbed CLI start cracking on it, would look a lot like port scanning. Making many connection attempts on a government monitored network, like NASA's, can get you in real trouble.

Furthermore, in many corporate networks, any connection attempt on arbitrary ports (usually below port 1024), is being flagged, logged and reported - even if it was for all the good reasons.

With everything said above --insecure provides this user consent, effectively acting as a legally binding agreement that the end-user know what they're doing.

Secure URLs

These URLs specify the scheme and either do not specify the port of the remote service, or specify the service to have port 22, 80 or 443.

Insecure URLs

Mbed CLI identifies insecure URLs as URLs that are missing scheme information, or specifie a port other than 22, 80 or 443. When an insecure URL is present on the command line, in a .lib file or in a .bld file, Mbed CLI refuses to use the URL and exits with an error. A user may explicitly ask Mbed CLI to use these URLs by specifying the --insecure option on the command line.

theotherjimmy · 2018-03-15T16:29:39Z

It looks like local paths are not treated specially. They should be considered "secure".

bmcdonnell-ionx · 2018-03-15T16:31:29Z

What do you mean by "local paths"?

theotherjimmy · 2018-03-15T16:34:22Z

mbed add foo/bar is classified as insecure, even if it exists in the same file system.

screamerbg · 2018-03-15T16:35:51Z

The patch wasn't/isn't ready for PR. Could you please close it?

theotherjimmy · 2018-03-15T16:36:14Z

@screamerbg Sure. What does it need?

screamerbg · 2018-03-15T19:18:47Z

@theotherjimmy More work and wider testing. I started writing remote tests for this, that also use py.test too enhance test coverage. It's a very obtrusive rewrite of the url handling.

theotherjimmy · 2018-03-15T19:20:30Z

Thanks.

The current CI did show an issue with handling of local paths with this change.

screamerbg · 2018-03-15T21:50:12Z

@theotherjimmy Reopened and fixed issues.
@bmcdonnell-ionx Please let me know if this PR delivers the functionality you need. Also please test against your repos/workflows. Thanks in advance.

bmcdonnell-ionx · 2018-03-16T14:13:18Z

Thanks @screamerbg.

For testing, should I just build and test directly from this branch? Or should I merge this PR back into master? Or merge into your apparent v1.5.0 WIP (development from your repo)?

theotherjimmy · 2018-03-16T14:36:51Z

@screamerbg I have updated the PR description with the wonderful motivation section you wrote in #621

screamerbg · 2018-03-16T20:19:34Z

@bmcdonnell-ionx I just rebased my branch on top of master (now the 1.5.0 release)

bmcdonnell-ionx · 2018-03-19T16:16:32Z

@screamerbg,

Setup

I just rebased my branch on top of master (now the 1.5.0 release)

OK, I did the same.

me@pc /c/dev_temp/mbed-cli (pr642-rebased-feature_safe_repo_urls)
$ git log --all --graph --decorate --pretty=oneline --abbrev-commit

* e7874ea (HEAD -> pr642-rebased-feature_safe_repo_urls) Correctly handle local URLs
* 02a2650 Code cleanup
* 60cb570 Strip out parasite .git and .hg suffixes in repo URLs
* 5748215 Preserve port in repo URLs when striping out auth strings
* 5209593 Safely convert repo URL to https schema if URL is a public SCM service (github/butbucket), supporting all schemas. This allows anonymous cloning of public repos without having to have ssh keys and associated accounts at github/bitbucket/etc. Without this anonymous users will get clone errors with ssh repository links even if the repository is public. See hhttps://help.github.com/articles/which-remote-url-should-i-use/
* ec6adbf Add insecure URL detection and override switch (--insecure)
* f96b73e Introduce unified Repo.isurl() and update formaturl() use uniformed repo URLs, including for ssh clone links used in Github Add support for host ports Add insecure option to `import`, `add`, `deploy` and `update`
*   53f8b11 (tag: 1.5.0, origin/master, origin/HEAD, master) Merge pull request #641 from screamerbg/development

me@pc /c/dev_temp/mbed-cli (pr642-rebased-feature_safe_repo_urls)
$ python setup.py install
[snip]

Installed c:\python27\lib\site-packages\mbed_cli-1.5.0-py2.7.egg
Processing dependencies for mbed-cli==1.5.0
Finished processing dependencies for mbed-cli==1.5.0

me@pc /c/dev_temp/mbed-cli (pr642-rebased-feature_safe_repo_urls)
$ mbed --version
1.5.0

Results

I came across some unexpected results, so I didn't run every conceivable test. Here's what I have so far...

Attempting to mbed import without the --insecure option fails, as it should. However, can you update the error message to be more informative? i.e. Give a hint that the user may want to consider using the --insecure option.

me@pc /c/dev_temp/test02
$ mbed import http://server01:1234/path/to/my-mbed-os-proj
[mbed] Importing program "my-mbed-os-proj" from "http://server01:1234/path/to/my-mbed-os-proj" at latest revision in the current branch
[mbed] ERROR: Unable to clone repository (http://server01:1234/path/to/my-mbed-os-proj)
---

me@pc /c/dev_temp/test02

mbed import --insecure appears to succeed, but with an unexpected error message. What went wrong here?

me@pc /c/dev_temp/test02
$ mbed import --insecure http://server01:1234/path/to/my-mbed-os-proj
[mbed] Importing program "my-mbed-os-proj" from "http://server01:1234/path/to/my-mbed-os-proj" at latest revision in the current branch
[mbed] Adding library "mbed-os" from "http://server01:1234/path/to/mbed-os" at rev #ad284b28064b
[mbed] WARNING: Unable to cache "C:\dev_temp\test02\my-mbed-os-proj\mbed-os" to "C:\Users\me\.mbed\mbed-cache\server01:1234\path/to/mbed-os"
---

I changed into the newly-created project directory, and tried to mbed add another library. I expected it to fail, since I had omitted the --insecure option. I was surprised to see it (mostly?) succeed, but with the same cache error message as with mbed add --insecure.

Did the --insecure option from the mbed add "stick" in the project working copy, applying to all newly-added libraries, such that I didn't need to --insecure to mbed add the new library? If so, is that intentional?

me@pc /c/dev_temp/test02
$ cd my-mbed-os-proj

me@pc /c/dev_temp/test02/my-mbed-os-proj (master)
$ mbed add http://server01:1234/path/to/another-lib
warning: LF will be replaced by CRLF in another-lib.lib.
The file will have its original line endings in your working directory.
[mbed] Adding library "another-lib" from "http://server01:1234/path/to/another-lib" at latest revision in the current branch
[mbed] Updating reference "another-lib" -> "http://server01:1234/path/to/another-lib/#eda6adca1f0c71dded3f983ae69efe7e6208707b"
[mbed] WARNING: Unable to cache "C:\dev_temp\test02\my-mbed-os-proj\another-lib" to "C:\Users\me\.mbed\mbed-cache\server01:1234\path/to/another-lib"
---

theotherjimmy · 2018-03-19T17:10:20Z

mbed/mbed.py

+            # Safely convert repo URL to https schema if this is a public SCM service (github/butbucket), supporting all schemas.
+            # This allows anonymous cloning of public repos without having to have ssh keys and associated accounts at github/bitbucket/etc.
+            # Without this anonymous users will get clone errors with ssh repository links even if the repository is public.
+            # See hhttps://help.github.com/articles/which-remote-url-should-i-use/


typo "hhttps" -> "https"

Typo "hhttps" -> "https"

theotherjimmy · 2018-03-19T17:11:40Z

Did the --insecure option from the mbed add "stick" in the project working copy

Basically, yes.

bmcdonnell-ionx · 2018-03-21T15:56:04Z

@theotherjimmy Is that desired behavior?

theotherjimmy · 2018-03-21T15:59:38Z

@bmcdonnell-ionx That's a question for @screamerbg.

cmonr · 2018-03-23T02:36:13Z

mbed/mbed.py

@@ -328,7 +325,7 @@ def cleanup():
                    shutil.rmtree(fl)

    def clone(url, path=None, depth=None, protocol=None):
-        m = Bld.isurl(url)
+        m = Bld.isvalidurl(url)


Does this mean that cloning a repo from the filesystem is not supported?

Bld is emulation of SCM. It only exists as a function of the mbed Website.

Personally, I would say that Bld is not SCM, but is supported by Mbed CLI. just my 2c.

cmonr · 2018-03-23T02:37:58Z

mbed/mbed.py

-            return re.match(regex_build_url, m_url.group(1))
-        else:
-            return False
-
    def init(path):
        if not os.path.exists(path):
            os.mkdir(path)


Silly question. Are we assuming that this will always work (say, in the case where a user attempts to create a directory where they don't have permissions to)?

Bld.init() is called within try: statement. See Bld.clone() below.

cmonr · 2018-03-23T02:40:36Z

mbed/mbed.py

+    @classmethod
+    def isinsecure(cls, url):
+        up = urlparse(url)
+        return not up or (up.scheme and up.scheme not in ['http', 'https', 'ssh', 'git']) or (up.port and int(up.port) not in [22, 80, 443])


Is this suppose to read as (not up) or ... or not (up or ...)?

screamerbg · 2018-03-23T17:51:16Z

@theotherjimmy Is that desired behavior?

@bmcdonnell-ionx Yes. The point is not to nag you with --insecure once it gets your consent.

On unrelated note, I just pushed a commit to escape cache paths based on repo url. Could you please try with the latest commit on the branch and see whether the cache warnings are now fixed.

bmcdonnell-ionx · 2018-03-23T21:23:45Z

Could you please try with the latest commit on the branch and see whether the cache warnings are now fixed.

They are not.

[mbed] WARNING: Unable to cache "C:\dev\testproj\mbed-os" to "C:\Users\me\.mbed\mbed-cache\server01:1234\tfs/OurCollection/myproject/_git/mbed-os"

screamerbg · 2018-03-26T13:13:32Z

@bmcdonnell-ionx I've introduced further changes to handle the cache paths with special characters that usually appear on Windows. Could you please retest?

bmcdonnell-ionx · 2018-03-26T17:40:39Z

@screamerbg did you see my PR on your repo?

screamerbg#1

screamerbg · 2018-03-26T18:26:43Z

@bmcdonnell-ionx Just did. Thank you! Now merged.

theotherjimmy

Some minor comments.

theotherjimmy · 2018-03-27T15:01:03Z

mbed/mbed.py

-
-        for _, scm in sorted_scms:
+        info("Trying to guess source control management tool. Supported SCMs: %s" % ', '.join([s.name for s in scms.values()]))
+        for _, scm in scms.items():


This could be written as:

for scm in scms.values()

this is okay if you don't want to change it.

theotherjimmy · 2018-03-27T15:02:51Z

mbed/mbed.py

+            # Safely convert repo URL to https schema if this is a public SCM service (github/butbucket), supporting all schemas.
+            # This allows anonymous cloning of public repos without having to have ssh keys and associated accounts at github/bitbucket/etc.
+            # Without this anonymous users will get clone errors with ssh repository links even if the repository is public.
+            # See hhttps://help.github.com/articles/which-remote-url-should-i-use/


Typo "hhttps" -> "https"

theotherjimmy · 2018-03-27T15:04:50Z

@screamerbg Technically, this is my PR, so you can review it!

bmcdonnell-ionx · 2018-04-11T23:32:50Z

@theotherjimmy

@screamerbg Technically, this is my PR, so you can review it!

Since @screamerbg made the changes, I would think you and/or someone else should review, whether or not that satisfies whatever automated checks are in place.

Or were you implying that you already did review, and you're satisfied?

theotherjimmy · 2018-04-11T23:56:17Z

Yeah, I was implying that I had already reviewed this

…

On Wed, Apr 11, 2018, 18:32 bmcdonnell-ionx ***@***.***> wrote: @theotherjimmy <https://github.com/theotherjimmy> @screamerbg <https://github.com/screamerbg> Technically, this is my PR, so you can review it! Since @screamerbg <https://github.com/screamerbg> made the changes, I would think you and/or someone else should review, whether or not that satisfies whatever automated checks are in place. Or were you implying that you already did review, and you're satisfied? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#642 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAp1YaIcACrDDZgiOnTp4F1fgZL0ZDhJks5tnpKjgaJpZM4SsZSC> .

bmcdonnell-ionx · 2018-04-12T00:12:51Z

@screamerbg, @theotherjimmy, @cmonr - Are further reviews necessary, or is it mergin' time?

theotherjimmy · 2018-04-12T16:19:57Z

@bmcdonnell-ionx Someone, probably @screamerbg, still needs to add a token github review. Then we have all of the automated checks and can merge this.

bmcdonnell-ionx · 2018-04-23T15:33:09Z

@screamerbg / @cmonr, per @theotherjimmy, can one of you add a "token review" and merge this?

…epo URLs, including for ssh clone links used in Github Add support for host ports Add insecure option to `import`, `add`, `deploy` and `update`

…e (github/butbucket), supporting all schemas. This allows anonymous cloning of public repos without having to have ssh keys and associated accounts at github/bitbucket/etc. Without this anonymous users will get clone errors with ssh repository links even if the repository is public. See hhttps://help.github.com/articles/which-remote-url-should-i-use/

screamerbg · 2018-04-27T16:19:37Z

I've just rebased this on top of master (1.5.1) and addressed some of the comments @theotherjimmy made.

@theotherjimmy @cmonr Please review and thumbs up if you're happy with the PR

cmonr

LGTM!

theotherjimmy · 2018-05-07T19:00:40Z

@screamerbg Looks good. Merge at will.

screamerbg · 2018-05-09T08:33:42Z

Thanks!

theotherjimmy mentioned this pull request Mar 15, 2018

mbed add breaks certain URLs #621

Closed

theotherjimmy closed this Mar 15, 2018

screamerbg reopened this Mar 15, 2018

screamerbg force-pushed the feature_safe_repo_urls branch from 188a4eb to c6a08a4 Compare March 15, 2018 21:17

screamerbg requested review from cmonr and screamerbg March 15, 2018 21:50

screamerbg force-pushed the feature_safe_repo_urls branch from ff7b971 to 0039628 Compare March 16, 2018 20:18

theotherjimmy commented Mar 19, 2018

View reviewed changes

cmonr reviewed Mar 23, 2018

View reviewed changes

bmcdonnell-ionx mentioned this pull request Mar 24, 2018

crash_log_parser crashes ARMmbed/mbed-os#6444

Closed

theotherjimmy commented Mar 27, 2018

View reviewed changes

screamerbg changed the title ~~Safe repo urls~~ New feature: Safe repo urls Mar 31, 2018

bmcdonnell-ionx mentioned this pull request Apr 6, 2018

HardFault on return from main() since mbed-os-5.8.0 ARMmbed/mbed-os#6560

Closed

screamerbg approved these changes Apr 27, 2018

View reviewed changes

screamerbg and others added 12 commits April 27, 2018 17:14

Introduce unified Repo.isurl() and update formaturl() use uniformed r…

aa0ff81

…epo URLs, including for ssh clone links used in Github Add support for host ports Add insecure option to `import`, `add`, `deploy` and `update`

Add insecure URL detection and override switch (--insecure)

4ea6eb7

Preserve port in repo URLs when striping out auth strings

b4270f2

Strip out parasite .git and .hg suffixes in repo URLs

54a779f

Code cleanup

ddb2889

Correctly handle local URLs

9d3052f

Code cleanup

1bc1628

Fix isinsecure() ambiguity

5205c87

Escape url elements for cache location

4ab8356

phrasing/punctuation

42737de

Fix cache path issues with special characters

2f47b91

screamerbg force-pushed the feature_safe_repo_urls branch from f9a4c13 to 2f47b91 Compare April 27, 2018 16:15

Minor improvements based on PR feedback

cd3d410

bridadan mentioned this pull request Apr 30, 2018

mbed import and mbed add broken on cygwin #578

Closed

cmonr approved these changes May 7, 2018

View reviewed changes

screamerbg merged commit 16ce2d6 into ARMmbed:master May 9, 2018

New feature: Safe repo urls #642

New feature: Safe repo urls #642

Uh oh!

Conversation

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

Uh oh!

Security and Liability in Mbed CLI

Secure URLs

Insecure URLs

Uh oh!

theotherjimmy commented Mar 15, 2018

Uh oh!

bmcdonnell-ionx commented Mar 15, 2018

Uh oh!

theotherjimmy commented Mar 15, 2018

Uh oh!

screamerbg commented Mar 15, 2018

Uh oh!

theotherjimmy commented Mar 15, 2018

Uh oh!

screamerbg commented Mar 15, 2018

Uh oh!

theotherjimmy commented Mar 15, 2018

Uh oh!

screamerbg commented Mar 15, 2018

Uh oh!

bmcdonnell-ionx commented Mar 16, 2018

Uh oh!

theotherjimmy commented Mar 16, 2018

Uh oh!

screamerbg commented Mar 16, 2018

Uh oh!

bmcdonnell-ionx commented Mar 19, 2018

Setup

Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Mar 19, 2018

Uh oh!

bmcdonnell-ionx commented Mar 21, 2018

Uh oh!

theotherjimmy commented Mar 21, 2018

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!

screamerbg commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmcdonnell-ionx commented Mar 23, 2018

Uh oh!

screamerbg commented Mar 26, 2018

Uh oh!

bmcdonnell-ionx commented Mar 26, 2018

Uh oh!

screamerbg commented Mar 26, 2018

Uh oh!

theotherjimmy 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!

theotherjimmy commented Mar 27, 2018

Uh oh!

bmcdonnell-ionx commented Apr 11, 2018

Uh oh!

theotherjimmy commented Mar 15, 2018 •

edited

Loading

screamerbg commented Mar 23, 2018 •

edited

Loading