#967 use correct fileinfo checksum field
Merged by mikem. Opened by tkopecek.
tkopecek/koji issue966  into  master

Download 967.patch

Fixes: https://pagure.io/koji/issue/966

This code should more explicitly distinguish between the rpm case and the archive case, rather than just relying on the checksum field.

We should also need to check the checksum_type field. Even if we don't want to support types besides md5 here, we should be checking the value.

The comment about rpms seems to need an update. It says, "but check it for everything else", yet we don't seem to check much of anything for rpms here.

Lack of checks for rpms is a blind spot. We do have the sigmd5 value, but we can't rely on having rpmlib inside the windows vm. I'm wondering if we could do that check in kojivmd instead (in localCache()), outside the vm. Granted, maybe this could be a separate PR.

rebased onto 3d8b036485dea8f05ae6eee724e0ecb66031611c

no true - rpm needn't be used at all

This bit has me wondering about a few things. I guess this is needed if the server doesn't return a content length? Is this really the right thing? What if the server uses chunked transfer encoding? Worth an explanatory comment at least.

this isn't actually checking the embedded md5sum. koji.get_rpm_header just pulls out the data, it sets the rpms flags to disable signature and digest checks. If we want to verify here, we need to so something different.

no true - rpm needn't be used at all

I'm not sure I see what you're getting at with this comment. Could you elaborate?

1 new commit added

  • check_sigmd5 for kojivmd
  1. Content-Length - It is coming from older code. I've created separate issue #982
  2. checking md5sum and my comment are about same thing. I've thought, that in all builds will be rpm binary used in some time, so I just checked that header is in line with db. In runtime rpm would fail on wrong sigmd5 vs header. But many builds just put rpm file in zip or similar things and never touch it, so corrupted file wouldn't be detected. I've added new commit which really checks the content.

2 new commits added

  • check_sigmd5 for kojivmd
  • use correct fileinfo checksum field

+def check_sigmd5(filename):

It's certainly not too complicated to do this, but I'm a little concerned about adding even more rpm parsing directly into Koji. The rpm team keep suggesting that future changes may break code like this.

Also, I'm not sure why this is needed. You're using it kojivmd, which has rpmlib available to it.

@mikem Do you know about some way how to do it with rpmlib? I'm not able to find any verify method for that. I need to read binary header and payload and I don't think I'm able to extract it. Reading hedaer in TransactionSet simply doesn't raise any error regardless of setVSFlags

I've also wanted to limit rpm usages due to #953.

Looking at this again.

https://github.com/mikem23/koji-playground/commits/pagure/pr/967

Added a unit test, fixed some small issues, and rebased

Commit 6cce316f fixes this pull-request

Pull-Request has been merged by mikem

Metadata