New licences, optional header info, relative path to licence

[kvch]

As I am introducing this tool to elastic/beats repo, the following changes are required:

1. Yellowlist is introduced for the appropriate licenses: GPL and EPL
2. Overridable path to licence file, as a few repositories are not complying with the licence name conventions
3. Optional header text by licence type. It is required as we need to include a fixed text about the location of the repository in case of EPL licences.

[charith-elastic]

LGTM 👍
I am not too sure about allowing the licence file path to be specified though. My comments are inline.

[charith-elastic]

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?

[kvch]

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?

[charith-elastic]

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.

[kvch]

Thank you for the review. I have just updated the PR.

[anyasabo]

I think we probably want a flag to opt in to the yellow list, correct? I don't think for example that ECK could use GPL code as it is not compatible with the Elastic license. I think the same goes for any golang project that imports a GPL package, so I wonder if my understanding is incorrect.

[charith-elastic]

I found a bug and commented inline. Also, @anyasabo raises a good point about the yellow list. I think the simplest solution is to not include anything controversial in the defaults bundled with the application (in the assets directory). Instead, each project that wants to use the licence-detector should have their own configuration files local to that project as we do in ECK.

[charith-elastic]

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.

[kvch]

Indeed, it is fixed now.

[charith-elastic]

LGTM