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.

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

Defaults violate spec #4

Closed
stevespringett opened this issue Jul 25, 2018 · 5 comments
Closed

Defaults violate spec #4

stevespringett opened this issue Jul 25, 2018 · 5 comments
Assignees

Comments

@stevespringett
Copy link

The defaults violate the purl specification by omitting the scheme. The scheme has been required per the standard for about 5 months now. The default violates this so every project using it is incompatible with the standard by default. The Dependency-Check project is trying to use it and they're implementing it in a way where they're not compatible with the spec, or any system that relies on PackageURL. I was able to steer them the right way, but i'm sure there are other projects potentially doing the same.

Also, please consider dropping support for the schemeless spec. It's old, its never coming back, and none of the official implementations will be able to parse it.

Also, please consider using the official implementation of the spec. It may not be perfect, so please consider contributing to it for improvement.

https://github.com/package-url

@jdillon
Copy link
Collaborator

jdillon commented Jul 25, 2018

First the pkg scheme change was only merged about 2 months ago:

package-url/purl-spec@8484a29

Our implementation was done before this, though we were aware that eventually/maybe there would be a change to move towards using 'pkg' scheme.

We did not use the "official" implementation, as it seemed inefficient at the time, and later we found some strangeness with things like:

https://github.com/package-url/packageurl-java/blob/master/src/main/java/com/github/packageurl/PackageURL.java#L285

... which lowercases namespace values, which isn't valid, as for example in maven, groupId which would translated to namespace here, is case sensitive. I don't recall what other reasons offhand, as its been a while now since we implemented that and made the decision to implement our own.

As for the spec "may not be perfect" we know. And as for "please consider contributing" we have been, though recently there has been no movement on anything:


I'm fine to change the default to pkg rendering if that is your concern, as the parsing side of the impl will support either so there in no spec violation afaict.

Though i'd probably leave the parsing and/or optional rendering for systems that may have already adopted and are using pkg-less, as was the case at least with our system and probably others that have adopted and used the spec already before the pkg scheme change was made 2 months ago.

@jdillon
Copy link
Collaborator

jdillon commented Jul 26, 2018

@stevespringett btw if you have a chance please look at:

The later really is more concerning, as I believe presently with the purl spec requiring transformation that isn't universal (like type lowercase'ing) it makes the overall spec very fragile and promotes implementations that will probably always be out of compliance. I think if that could be dropped it would further promote the portability of the spec.

Another thing that came up for why we didn't use your impl (initially at least) was we wanted the parsed namespace and subpath elements ready already parsed.

But really IMO at the end of the day if the spec is solid enough, then it shouldn't matter what impl and if the spec has a testsuite/tck then it can be used to ensure compatibility.

I do understand your initial concern was the current default was to use the old-style of this spec, and as I mentioned I can adjust that, though there are existing purl use-cases that already don't use that recent change so I think the impl that copes is more portable, but agree the default should be the latest.

AS for OSS Index, since it uses this library, it supports either form, but prefers the older form as putting pkg: everywhere in displays is just noise.

Personally I don't really agree with the spec change, I don't think its needed, but i'm thinking about it as a more generic coordinate system, and not a drop in URL, where standard schemes are very valid and required for sanity.


BUT, I would like to press upon, if the spec has to know about all the various different formats that exist now, or may exist in the future, and know how to transform them, it means that no implementation could possibly be correct at any given time.

So I STRONGLY suggest those sorts of transformations be dropped from the specification.

@stevespringett
Copy link
Author

Thanks for calling out the issues with the 'official' Java impl I wrote. That's what I was referring to when I said it wasn't perfect. But I did add a case-sensitive Maven example to the test suite along with fixing the Maven issue - and there were other issues as well regarding case sensitivity.

My only real concern with the Sonatype impl was the default. I didn't want people to use a default that varied from the spec.

So Philippe asked me in February if I wanted to be co-admin of the project. At the time I said no because I had too much going on, but now that Dependency-Track is released, CycloneDX BoM spec is published, and both rely on PackageURL heavily, I think it's really important to sort some of these issues out. I pinged Philippe again about co-admin as well as inquiring about project meetings.

I'm also not a big fan of requiring lowercase transformation. I agree with you in that its an input problem. Requirements should not be confused with conventions, and I do believe that, by convention, purl should be lowercase, but that should not be a requirement.

When I originally started talking with Philippe about the security use-cases i was envisioning, he was quite surprised. He had not thought about these cases before. But as of today, it appears that the security needs (commercially, open source, and government) all have a need for PackageURL, and we need it flushed out sooner rather than later.

@jdillon
Copy link
Collaborator

jdillon commented Jul 26, 2018

@stevespringett fyi I did find one problem with our impl, and its again this silly transformation problem specifically for pypi :-(

I do strongly suggest moving towards purl being agnostic for input, except for the various bits of url encoding, and/or requiring type be normalized lower-case. Otherwise I think it should just leave the input as given and if the user gave "pkg:pypi/FOO@1" then it should leave "FOO" asis. Consumers of this could interpret it as case-insensitive or transform, but I do not think its the business of the spec to do this, and as my issue points out it means that at any time any impl of what is otherwise a fairly simple spec could be invalid. As I could easily create a new format "sillypackage" that has some silly requirement that all "S" need to become "S_i_l_l_y" as part of the requirements for the format, but its very unlikely that all package-url impls would be able to keep up with ensuring it had all those transformations of existing formats that may change their best-practices in the future, or for future unknown package formats.

@jdillon jdillon self-assigned this Jul 27, 2018
@jdillon
Copy link
Collaborator

jdillon commented Jul 27, 2018

release-1.0.1 is out which sets the default to follow latest spec defaulting to RenderFlavor.SCHEME and adjusts builder and parse forms to perform evil transformations.

@jdillon jdillon closed this as completed Jul 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants