Deny-list certain file extensions

[ktdreyer]

A user on Fedora's devel list accidentally uploaded an .rpm file to the dist-git lookaside cache.

https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/X3YKUJRZEAM6VKJFZHUSKXVTDOR6TI44/

The upload script should reject files ending in ".rpm", because those are probably mistakes.

The dist-git cache is forever, so this will keep us from needlessly storing these mistakes there.

[clime]

Yes, this can be done and it is probably a good idea. I think we should add a config option like blacklisted_extensions. Thank you for the suggestion.

[xsuchy]

It will be likely to better reject it on rpkg side, where you can emit a nice warning and ask the user for confirmation.
Also, mind the fact that rpkg specifically whitelist 'rpm' as an extension which should be upload to look-aside cache.
Also in general, I see lots of users on stack overflow producing rpm which include rpm. They have various legitime reasons.

[ktdreyer]

Would you mind showing me where rpkg is doing this whitelisting?

[clime]

https://pagure.io/rpkg/blob/master/f/pyrpkg/__init__.py#_89

I admit that doing this client-side has certain advantages. The dist-git's upload script isn't very smart and you would need first to upload the file to find out it has a bad extension.

[ktdreyer]

It looks like UPLOADEXTS is only used in import_srpm(). In particular, it's not used with rpkg upload or rpkg new-files. Do I have that right?

[clime]

It looks like UPLOADEXTS is only used in import_srpm(). In particular, it's not used with rpkg upload or rpkg new-files. Do I have that right?

Yes, it looks like it. And actually i missed that we can use the remote check https://pagure.io/rpkg/blob/master/f/pyrpkg/lookaside.py#_296 to check whether file has a valid extension according to the server (while also checking that the file hasn't been uploaded yet). But then it would deserve a comment change in rpkg: https://pagure.io/rpkg/blob/master/f/pyrpkg/lookaside.py#_202 and the protocol would be a bit off: https://pagure.io/rpkg/blob/master/f/pyrpkg/lookaside.py#_261 but that might not be that big deal.

I think both approaches (server-side check/client-side check) are possible. In case, of client side approach, there could be a configuration option that would say whether or not some "rare" extensions should be supported so user could override the default and still upload the file if he/she knows what he/she is doing. This seems to be a good option to me but it requires some coding and also a new configuration option. But the file extension check could then be a soft constraint.

The server check would be a hard constraint: i.e. you couldn't override allowed extensions by the server, although you could still just use some other extension (so you could just fool the check if you wanted to).

It's also true that rpkg allows the rpm extension although i went through all the Fedora spec files and i haven't found a source with src.rpm (didn't look for just .rpm though).

[clime]

Given how things are in the upload script (that the upload remote check is designed mainly to test whether the file has already been uploaded), i think it might be more suitable to implement the support client-side. At the same time, it shouldn't be hardcoded into rpkg because fedpkg may allow different extensions than let's say centpkg. So there should be some configuration option in /etc/rpkg/fedpkg.conf that would specify allowed extensions for uploading. Would you agree with that? It's just a suggestion.

I think we should also make sure that using source rpms as rpm sources should really not be allowed in Fedora. I am not sure it is actually specified in the packaging documentation anywhere.

[ktdreyer]

IMHO I still think we could do this server-side

[clime]

Ok, I will look at it.

[xsuchy]

FYI related https://pagure.io/rpkg/issue/484 rpkg no longer consider extension when uploading file. It simply upload everything binary.

[clime]

I am no longer a maintainer in this repo. Perhaps somebody else can take a look at this.