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.

Sign up for GitHub

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

Jump to bottom

New licences, optional header info, relative path to licence #5

Merged
merged 3 commits into from
Jun 8, 2020
Merged

New licences, optional header info, relative path to licence #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9361f86
New licences, optional header info, relative path to licence
kvch Jun 4, 2020
78580f5
securejoin dependency dir && add more info to readme
kvch Jun 5, 2020
685af18
add unit test for setting LicenceFile
kvch Jun 8, 2020
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
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ A partial list of allowed licences at Elastic is included in `assets/rules.json`
In some cases, the application will not be able to detect the licence type or infer the correct URL for a dependency. When there are issues with licences (no licence file or unknown licence type), the application will fail with an error message instructing the user to add an override to continue. The overrides file is a file containing newline-delimited JSON where each line contains a JSON object bearing the following format:

- `name`: Required. Module name to apply the override to.
- `licenceFile`: Optional. Path to a file containing the licence text for this module under the module directory. It must be relative to the dependency path.
- `licenceType`: Optional. Type of licence (Apache-2.0, ISC etc.). Provide a [SPDX](https://spdx.org/licenses/) identifier.
- `licenceTextOverrideFile`: Optional. Path to a file containing the licence text for this module. Path must be relative to the `overrides.json` file.
- `url`: Optional. URL to the dependency website.
Expand Down
2 changes: 1 addition & 1 deletion dependency/dependency.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ type List struct {
type Info struct {
Name string `json:"name"`
Dir string `json:"-"`
LicenceFile string `json:"-"`
LicenceFile string `json:"licenceFile"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this require providing the full path to the licence file in the module cache directory? The reason for disallowing it was for two reasons:

  • Portability: to prevent OS-specific or user-specific paths from breaking the override rules
  • Security: to prevent someone from accidentally (or intentionally) outputting contents of sensitive files

The workaround for the problem is to define the licenceTextOverrideFile which is handled more securely by chrooting to the project directory (example: https://github.com/elastic/cloud-on-k8s/blob/master/hack/licence-detector/overrides/overrides.json). Will that not work for your use case too?

Copy link
Contributor Author

@kvch kvch Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I did not have to provide a full path, just a relative one to the root of the directory of the dependency. I am fine with complying with the security requirements you have. However, I would rather not store the licences of our dependencies in the beats repository. I know in general those files are rarely changed, but still. We should not maintain those files. How do you keep the licence files in your repository up to date?

As a middle ground, what do you think about adding an extra check to the path in licenceFile to make sure it is located in the folder of the module? This way we can make sure no sensitive files are published.

I am not sure I understand your concerns about portability. How would this break the override rules?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I didn't realise that the relative path would work because my memory was that it required the full path. That takes away my main concern about portability (although it might still break on Windows but nobody develops on Windows right? 😄 )

+1 on using the same securepath methods on the licence file path to ensure that the file is only within the go module cache.

LicenceType string `json:"licenceType"`
URL string `json:"url"`
Version string `json:"version"`
Expand Down
8 changes: 8 additions & 0 deletions detector/detector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"strings"
"time"

securejoin "github.com/cyphar/filepath-securejoin"
"github.com/google/licenseclassifier"
"github.com/karrick/godirwalk"
"github.com/markbates/pkger"
Expand Down Expand Up @@ -157,6 +158,13 @@ func doDetectLicences(licenceRegex *regexp.Regexp, classifier *licenseclassifier
if err != nil && !errors.Is(err, errLicenceNotFound) {
return nil, fmt.Errorf("failed to find licence file for %s in %s: %w", depInfo.Name, depInfo.Dir, err)
}
} else if depInfo.LicenceTextOverrideFile == "" {
// if licence file is given but no overrides, use the selected licence file
licFile, err := securejoin.SecureJoin(depInfo.Dir, depInfo.LicenceFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this doesn't quite work because if the overrides file contains a licenceTextOverrideFile, the licenceFile field is set to that as well:

go-licence-detector/dependency/dependency.go

Lines 83 to 89 in 919d39a

if dep.LicenceTextOverrideFile != "" {
licFile, err := securejoin.SecureJoin(rootDir, dep.LicenceTextOverrideFile)
if err != nil {
return nil, fmt.Errorf("failed to generate secure path to licence text file of %s: %w", dep.Name, err)
}
dep.LicenceFile = licFile
}

So when it comes to this point, it will be trying to join the full path to the overridden licence file with the module cache path and end up with an invalid path.

When you fix the problem, it would be good to add a unit test to capture the scenario as it is something missing from the test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it is fixed now.

if err != nil {
return nil, fmt.Errorf("failed to generate secure path to licence file of %s: %w", depInfo.Name, err)
}
depInfo.LicenceFile = licFile
}

// detect the licence type if the override hasn't provided one
Expand Down
63 changes: 62 additions & 1 deletion detector/detector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ func TestDetect(t *testing.T) {
{
name: "All",
includeIndirect: true,
overrides: map[string]dependency.Info{
"github.com/gorhill/cronexpr": {Name: "github.com/gorhill/cronexpr", LicenceType: "GPL-3.0"},
},
wantDependencies: func() *dependency.List {
return &dependency.List{
Indirect: mkIndirectDeps(),
Expand All @@ -46,6 +49,9 @@ func TestDetect(t *testing.T) {
{
name: "DirectOnly",
includeIndirect: false,
overrides: map[string]dependency.Info{
"github.com/gorhill/cronexpr": {Name: "github.com/gorhill/cronexpr", LicenceType: "GPL-3.0"},
},
wantDependencies: func() *dependency.List {
return &dependency.List{
Direct: mkDirectDeps(),
Expand All @@ -58,6 +64,7 @@ func TestDetect(t *testing.T) {
overrides: map[string]dependency.Info{
"github.com/davecgh/go-spew": {Name: "github.com/davecgh/go-spew", URL: "http://example.com/go-spew"},
"github.com/russross/blackfriday/v2": {Name: "github.com/russross/blackfriday/v2", LicenceType: "MIT"},
"github.com/gorhill/cronexpr": {Name: "github.com/gorhill/cronexpr", LicenceType: "GPL-3.0"},
},
wantDependencies: func() *dependency.List {
deps := &dependency.List{}
Expand All @@ -81,6 +88,19 @@ func TestDetect(t *testing.T) {
return deps
},
},
{
name: "WithValidLicenceFileOverride",
includeIndirect: true,
overrides: map[string]dependency.Info{
"github.com/gorhill/cronexpr": {Name: "github.com/gorhill/cronexpr", LicenceFile: "GPLv3"},
},
wantDependencies: func() *dependency.List {
return &dependency.List{
Indirect: mkIndirectDeps(),
Direct: mkDirectOverridenDeps(),
}
},
},
{
name: "WithInvalidLicenceFileOverride",
includeIndirect: true,
Expand Down Expand Up @@ -113,7 +133,7 @@ func TestDetect(t *testing.T) {
require.NoError(t, err)
defer f.Close()

rules, err := LoadRules("")
rules, err := LoadRules("testdata/rules.json")
require.NoError(t, err)

gotDependencies, err := Detect(f, classifier, rules, tc.overrides, tc.includeIndirect)
Expand Down Expand Up @@ -180,6 +200,47 @@ func mkDirectDeps() []dependency.Info {
LicenceFile: "testdata/github.com/russross/blackfriday/v2@v2.0.1/LICENSE.rst",
URL: "https://github.com/russross/blackfriday",
},
{
Name: "github.com/gorhill/cronexpr",
Version: "v0.0.0-20161205141322-d520615e531a",
VersionTime: "2016-12-05T14:13:22Z",
Dir: "testdata/github.com/gorhill/cronexpr@v0.0.0-20161205141322-d520615e531a",
LicenceType: "GPL-3.0",
LicenceFile: "",
URL: "https://github.com/gorhill/cronexpr",
},
}
}

func mkDirectOverridenDeps() []dependency.Info {
return []dependency.Info{
{
Name: "github.com/ekzhu/minhash-lsh",
Version: "v0.0.0-20171225071031-5c06ee8586a1",
VersionTime: "2017-12-25T07:10:31Z",
Dir: "testdata/github.com/ekzhu/minhash-lsh@v0.0.0-20171225071031-5c06ee8586a1",
LicenceType: "MIT",
LicenceFile: "testdata/github.com/ekzhu/minhash-lsh@v0.0.0-20171225071031-5c06ee8586a1/licence.txt",
URL: "https://github.com/ekzhu/minhash-lsh",
},
{
Name: "github.com/russross/blackfriday/v2",
Version: "v2.0.1",
VersionTime: "2018-09-20T17:16:15Z",
Dir: "testdata/github.com/russross/blackfriday/v2@v2.0.1",
LicenceType: "BSD-2-Clause",
LicenceFile: "testdata/github.com/russross/blackfriday/v2@v2.0.1/LICENSE.rst",
URL: "https://github.com/russross/blackfriday",
},
{
Name: "github.com/gorhill/cronexpr",
Version: "v0.0.0-20161205141322-d520615e531a",
VersionTime: "2016-12-05T14:13:22Z",
Dir: "testdata/github.com/gorhill/cronexpr@v0.0.0-20161205141322-d520615e531a",
LicenceType: "GPL-3.0",
LicenceFile: "testdata/github.com/gorhill/cronexpr@v0.0.0-20161205141322-d520615e531a/GPLv3",
URL: "https://github.com/gorhill/cronexpr",
},
}
}

Expand Down
18 changes: 13 additions & 5 deletions detector/rules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,14 @@ const embeddedRulesFile = "go.elastic.co/go-licence-detector:/assets/rules.json"

// rulesFile represents the structure of the rules file.
type rulesFile struct {
Whitelist []string `json:"whitelist"`
Whitelist []string `json:"whitelist"`
Yellowlist []string `json:"yellowlist"`
}

// Rules holds rules for the detector.
type Rules struct {
WhiteList map[string]struct{}
WhiteList map[string]struct{}
YellowList map[string]struct{}
}

// LoadRules loads rules from the given path. Embedded rules file is loaded if the path is empty.
Expand Down Expand Up @@ -66,18 +68,24 @@ func LoadRules(path string) (*Rules, error) {
}

rules := &Rules{
WhiteList: make(map[string]struct{}, len(rf.Whitelist)),
WhiteList: make(map[string]struct{}, len(rf.Whitelist)),
YellowList: make(map[string]struct{}, len(rf.Yellowlist)),
}

for _, w := range rf.Whitelist {
rules.WhiteList[w] = struct{}{}
}

for _, y := range rf.Yellowlist {
rules.YellowList[y] = struct{}{}
}

return rules, nil
}

// IsAllowed returns true if the given licence is allowed by the rules.
func (r *Rules) IsAllowed(licenceID string) bool {
_, ok := r.WhiteList[licenceID]
return ok
_, isWhiteListed := r.WhiteList[licenceID]
_, isYellowListed := r.YellowList[licenceID]
return isWhiteListed || isYellowListed
}
8 changes: 8 additions & 0 deletions detector/rules_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,11 @@ func TestRulesWhiteList(t *testing.T) {
require.True(t, rules.IsAllowed("Apache-2.0"))
require.False(t, rules.IsAllowed("WTFPL"))
}

func TestRulesYellowList(t *testing.T) {
rules, err := LoadRules("testdata/rules.json")

require.NoError(t, err)
require.True(t, rules.IsAllowed("GPL-3.0"))
require.False(t, rules.IsAllowed("WTFPL"))
}
7 changes: 7 additions & 0 deletions detector/testdata/deps.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,10 @@
"Dir": "testdata/github.com/russross/blackfriday/v2@v2.0.1",
"GoMod": "testdata/github.com/russross/blackfriday/v2@v2.0.1/go.mod"
}
{
"Path": "github.com/gorhill/cronexpr",
"Version": "v0.0.0-20161205141322-d520615e531a",
"Time": "2016-12-05T14:13:22Z",
"Dir": "testdata/github.com/gorhill/cronexpr@v0.0.0-20161205141322-d520615e531a",
"GoMod": "testdata/github.com/gorhill/cronexpr/@v/v0.0.0-20161205141322-d520615e531a.mod"
}
Loading