Add CEP for a URL field for package records - #151
Conversation
|
Hi @jaimergp, I would to please move this to the RFC stage, thanks! |
|
Dear @conda/steering-council, this CEP is officially entering its Request For Comments (RFC) period. This will last two weeks, until May 12th EOD AoE. |
|
|
||
| ## Specification | ||
|
|
||
| Package repodata records MAY contain a `url` field. When present, the value SHALL be set to either an absolute or relative URL. |
There was a problem hiding this comment.
Can relative paths traverse parents with ../ or is that forbidden? If not, should we forbid that for security purposes?
There was a problem hiding this comment.
Hi @jaimergp, thanks for the feedback! I updated the CEP to exclude path traversal to parents. I think it isn't great for security and other methods of declaring base_urls are already flexible enough without it.
|
|
||
| This CEP introduces a new optional `url` field to package records. Since the field is optional, this change is backwards-compatible: | ||
|
|
||
| - Older clients that don't recognize the `url` field will continue to construct download URLs using the existing method (combining `base_url` with the package filename) |
There was a problem hiding this comment.
Won't this mean that these old clients will get 404s for records that define url?
There was a problem hiding this comment.
We will use the backwards compatible repodata update strategy, I made this more clear in the CEP.
|
|
||
| ## Specification | ||
|
|
||
| Package repodata records MAY contain a `url` field. When present, the value SHALL be set to either an absolute or relative URL. |
There was a problem hiding this comment.
Are there any rules where the filename in the url field must match the filename in the repodata key? Should they? Which filename is it used for the local cache if they don't match?
There was a problem hiding this comment.
Thanks, I added language about the cache key needing to be the basename of the file. This will support both current files and future ones like wheels.
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
|
One question I have is what we do with lock-files. Let me explain. Currently, lockfiles store the packages by their url. But now we have two urls. We have the "channel url" ( I think we should still use the "channel url" in lock files to uniquely identify the package in the channel/repodata, and also encode the "download url" separately. I think it would be good to include this information in the CEP as a recommendation. WDYT? |
|
Hi @baszalmstra, this is a great point, thanks for bringing it up! I agree on using the channel URL for packages on the channel. But the recommendation needs to handle the case where a record's url is an absolute external URL, for example how we are using it for wheels linked to a CDN. In that case the artifact doesn't live on the channel at all, so the download URL is the primary artifact identifier. What if I update the CEP to say:
Would that be clear and unambiguous? |
|
I think it doesn't pose a problem as long as we keep both urls in the lockfile. We will never download from the "channel url" if a "download url" is present. So in the current - conda: https://conda.anaconda.org/pypi-test/noarch/foobar-0.1.0-a.whl
url: files.pypihosted.com/myexamplehash/cp310-cp310-macosx_10_9_x86_64.whl
- conda: https://conda.anaconda.org/pypi-test/noarch/foobar-0.1.0-b.whl
url: files.pypihosted.com/myexamplehash/cp310-cp310-macosx_10_9_x86_64.whlNote that the download url is the same but the unique identifier from the channel isnt. |
|
Hey @baszalmstra, I could definitely be wrong and might be missing something, but to your example, |
|
Let me try to clarify with a problem statement: What happens if you have two wheel channels that have different metadata for the same url? Both could end up in the lock-file, but if we use the "download url" we cant uniquely identify them. |
|
The way I read it, Bas wants a unique identifier of a package record entry in the repodata. So far, that happened to be the download URL as well, so that's what we used. This CEP breaks that happy accident and now we don't have a guaranteed unique identifier. Bas wants to keep using that URL as a URI (identifier instead of location). I wonder though if we should elevate PURLs (with the changes proposed in Cheng's #159) as those unique identifiers. |
|
Exactly. 👍 |
|
Hi @baszalmstra and @jaimergp, I added a Lockfiles considerations section, please let me know if you think that captures the concern and the right proposal for how to deal with it going forward. |
| Lockfile implementations SHOULD adopt Package URLs (PURLs), as defined in [conda/ceps#159](https://github.com/conda/ceps/pull/159) (draft), as the canonical unique identifier for package records. A conda PURL encodes a package's identity (channel, name, version, build string, and subdir) in a standardized, location-independent form, for example: | ||
|
|
||
| ```text | ||
| pkg:conda/conda-forge/numpy@2.4.2?build=py314hd4f4903_0&subdir=linux-64 |
There was a problem hiding this comment.
This is not quite enough because it doesnt include the full url of the channel itself. A lockfile can easily contain numpy from conda-forge but from different sources. I dont know what the canonical form for that is?
There was a problem hiding this comment.
Agreed, and would argue that we should either stick to the "conda URL" or do something like:
{conda_url}@{resolved_url}
Otherwise, indirection via conda_url -> PackageRecord (in lockfile) -> URL sounds like it should work fine.
There was a problem hiding this comment.
As I read #159, this should resolve to https://conda.anaconda.org/conda-forge/noarch/repodata.json. There is a repodata in that place, assuming numpy resolves in there with the data provided, this should specify the package OK.
There was a problem hiding this comment.
Yes but if the lockfile contains packages from http://prefix.dev/conda-forge and http://conda.anaconda.org/conda-forge we need to be able to distinguish them.
|
Hi @danyeaw — thanks for driving this CEP. Before the vote, I want to flag a critical security gap that I think CEP #151's absolute Two requests:
Without an explicit opt-out on the client side, our mutual customers will expose their organizations to downloads circumventing their security gates. Thanks in advance, |
|
Hey @adam-browning, thanks for the feedback. I added a new security considerations section. I think a compromised channel maintainer normally could also modify packages on the channel, but I do mention that this adds a narrower attack vector if someone somehow only got access to change the |
|
I want to make sure we track any security implications for url fields added to the index.json blobs at package build time. We don't want folks injecting random urls into artifacts this way. I think these should be stripped, if ever found, and never kept in the indexing process. |
|
I see some alternative possibilities for the mirroring use case:
|
|
If a channel operator wanted to do something malicious today, they already can. They control the whole channel and all the packages. Is there some other threat scenario that I'm missing? If a channel could reverse proxy a package URL today, that seems like it is even worse than if a channel could explicitly set a malicious channel URL with this CEP. At least the explicit case is auditable. The only genuinely new thing security related about this CEP is that a proxy/mirror now needs to understand and rewrite the url field to keep downloads within its perimeter. This is no different than a PyPI mirror has to do today. I agree that packages shouldn't be setting their own URLs, and that should be stripped if they do. |
Checklist for submitter
cep-0000.mdnamedcep-XXXX.mdin the root level.Checklist for CEP approvals
${greatest-number-in-main} + 1.cep-XXXX.mdfile has been renamed accordingly.# CEP XXXX -header has been edited accordingly.pre-commitchecks are passing.