Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
> All my concerns seem to have been addressed, thanks for your patience and all > the work on this! Thanks Panu! Really appreciate your help with this! -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-687209427___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
Merged #1203 into master. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#event-3729073410___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai approved this pull request. All my concerns seem to have been addressed, thanks for your patience and all the work on this! -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#pullrequestreview-482537516___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
Alrighty, I think we're good to go here. With 4.16 just branched off this is a good time to bring in new stuff for a little soak time - with stuff like this there's almost always something to tweak that only time will bring up. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-687060986___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request. > @@ -14,6 +14,7 @@ DISTCHECK_CONFIGURE_FLAGS = \ --with-audit \ --with-selinux \ --with-imaevm \ + --with-fsverity \ Hi Panu, No worries, hope you had a good break! I have been swamped with another project the last month, but just made the changes you suggested and pushed them in. I think it worked, at least it looks like it all passed. It's the first time I ever dealt with Docker, so let me know if I broke something. If there is anything else you think is needed, please let me know. Jes -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#discussion_r479377848___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request. > @@ -14,6 +14,7 @@ DISTCHECK_CONFIGURE_FLAGS = \ --with-audit \ --with-selinux \ --with-imaevm \ + --with-fsverity \ Sorry for the silence, I've been on a vacation. We need to get this past the CI to consider merging. I'd suggest taking this out of the initial autofoo commit, and instead add a new "Enable fsverity in CI" commit to the end of the series that adds this *and* fsverity-utils-devel to ci/Dockerfile along with the other build dependencies. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#pullrequestreview-460769176___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
> > RPM doesn't actually need the fsverity utility to be present, but it does > > need libfsverity > > Yup, the library is what I meant by my comment, not the utility. Thanks for > adding the check. > > I'll need to take closer look at the updated version but overall I think its > in fair shape for this point, and the added tags totally reasonable. So I'm > considering the tags reserved, and there are no competing tag additions at > the moment, but if it makes you sleep better we can certainly merge the tag > addition right away to cement the reservation. Hi Panu, any updates on applying these changes? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-664363451___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
fsverity-utils-1.1 which includes fsverity-utils-devel is now available in rawhide and Fedora 32, so it should be possible to build this now. Let me know if you hit any issues. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-654248452___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
> > RPM doesn't actually need the fsverity utility to be present, but it does > > need libfsverity > > Yup, the library is what I meant by my comment, not the utility. Thanks for > adding the check. > > I'll need to take closer look at the updated version but overall I think its > in fair shape for this point, and the added tags totally reasonable. So I'm > considering the tags reserved, and there are no competing tag additions at > the moment, but if it makes you sleep better we can certainly merge the tag > addition right away to cement the reservation. Sounds good, thanks for looking at it! I think we can wait for the library to land. I pushed it into Fedora 32 for testing yesterday, so hopefully it will get out shortly: https://bodhi.fedoraproject.org/updates/FEDORA-2020-2f41b015c0 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-645552077___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
> RPM doesn't actually need the fsverity utility to be present, but it does > need libfsverity Yup, the library is what I meant by my comment, not the utility. Thanks for adding the check. I'll need to take closer look at the updated version but overall I think its in fair shape for this point, and the added tags totally reasonable. So I'm considering the tags reserved, and there are no competing tag additions at the moment, but if it makes you sleep better we can certainly merge the tag addition right away to cement the reservation. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-643925438___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
I have pushed a fix for the configure issue, and configure should fail is one specifies --with-fsverity and it isn't available. Apologies if I messed something up, autoconf/automake and I do not get along. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-643482342___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
> Oh, sorry, I've forgot to update "status" here. > > We can't merge a patch that fails the CI tests - this fails because fsverity > is enabled in the CI but the library doesn't exist in Fedora 32. Hardly > surprising as the library version isn't even released upstream AFAICS. That > can be worked around by not enabling it in the CI, but I'm also not going to > merge a patch I've never seen compile (and I haven't gotten around to build > from upstream yet, although I did notice the library thing has been merged). > I'd prefer to see an upstream release of fsverity library before merging and > optimally, said version in Fedora >= 32 so we could enable it in CI, but I do > realize there could be other incompatibilities preventing the latter from > occurring so that can't be a hard requirement. > > Speaking of enabling it in configure, I just noticed that it doesn't actually > check for fsverity presence in configure, so if enabled but missing it'll > fail in middle of compilation instead of configure time as it should. So > there's a minor tweak that'll be needed. That is is totally fair, I was assuming that. My question was more about whether you are happy with the code as it is, while we wait for the library. we are really keen to start using it internally, so I wanted to be sure to agree on the tag numbers at least, to avoid binary incompatibilities. RPM doesn't actually need the fsverity utility to be present, but it does need libfsverity, and there is a link test for that in configure. We should also check for the presence of libfsverity.h, since that is now going into a fsverity-devel package, while the library goes into the fsverity package. The plugin only needs linux/fsverity.h since it only calls the ioctl() to enable things, and that looks to be covered. I'll look into updating configure to also check for libfsverity.h and push further for an official upstream release of fsverity-utils, so I can push it into Fedora. I have been pushing for this regularly. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-643287491___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
Oh, sorry, I've forgot to update "status" here. We can't merge a patch that fails the CI tests - this fails because fsverity is enabled in the CI but the library doesn't exist in Fedora 32. Hardly surprising as the library version isn't even released upstream AFAICS. That can be worked around by not enabling it in the CI, but I'm also not going to merge a patch I've never seen compile (and I haven't gotten around to build from upstream yet, although I did notice the library thing has been merged). I'd prefer to see an upstream release of fsverity library before merging and optimally, said version in Fedora >= 32 so we could enable it in CI, but I do realize there could be other incompatibilities preventing the latter from occurring so that can't be a hard requirement. Speaking of enabling it in configure, I just noticed that it doesn't actually check for fsverity presence in configure, so if enabled but missing it'll fail in middle of compilation instead of configure time as it should. So there's a minor tweak that'll be needed. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-643195027___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
I rebased the branch to make sure it applies cleanly to master. I also added an additional patch, introducing the --verity-algo argument to rpmsign, allowing the user to specify the algorithm to use for the verity signatures. Is there anything else you would like me to address at this point? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-642125240___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
I have pushed the update - let me know if there's anything else that needs addressing. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-638332707___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
> Okay, this sounds like its headed to the right direction then, I agree this > seems like something where the kernel needs to deal with it because it's the > only thing that can. > > I see block size is an argument passed to the ioctl() that enables this > fsverity for a file, but what does that actually mean? Can individual files > have different block size for their Merkle tree? In the current state of > things it seems more like a hardwired global'ish thing. > > What I'm getting at is that if we make block size a configurable thing in > rpm, then the kernel will really need to support different block sizes for > individual files as rpms come from variety of sources and might be signed > with different options. > > I'm tempted to say lets just go with a hardwired 4K size for now to keep > things simple, we can always add another tag for alternative block size if it > becomes necessary for one reason or another. The block_size argument is used as follows: When fsverity is enabled on a file, the kernel will build a Merkle tree for the file, using the specified block size. In addition the signature signs the root hash of the Merkle tree, so the block size has to match that used to generate the signature, for it to validate. Each file can use a different block size for it's Merkle tree, but I think it's fine for RPM to mandate what block size it's willing to generate. As you say, we can add a tag for the block size later if we find a need for it. I'll update the code to use 4K. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-638308319___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
Okay, this sounds like its headed to the right direction then, I agree this seems like something where the kernel needs to deal with it because it's the only thing that can. I see block size is an argument passed to the ioctl() that enables this fsverity for a file, but what does that actually mean? Can individual files have different block size for their Merkle tree? In the current state of things it seems more like a hardwired global'ish thing. What I'm getting at is that if we make block size a configurable thing in rpm, then the kernel will really need to support different block sizes for individual files as rpms come from variety of sources and might be signed with different options. I'm tempted to say lets just go with a hardwired 4K size for now to keep things simple, we can always add another tag for alternative block size if it becomes necessary for one reason or another. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-638092594___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
> I have been thinking a fair bit about this and I see a couple of options: > > 1. We could in principle generate signatures for every supported page size. > This would require adding more tags, ie. one for each page size. > 2. Do not install signatures if the page size doesn't match the expected page > size of the signature. > 3. Work with the kernel to support 4K Merkle tree block size independent of > the page size. > > Right now fsverity is only supported on ext4 and f2fs, both of these > currently only work with block size == PAGE__SIZE, which is suboptimal. I > raised this issue on the linux-fscrypt list already. > > We are actively working on adding fsverity support to btrfs, and the design > here is to support 4K Merkle tree blocks independently of the page size. > > I think 2) and 3) are the most reasonable approach. The changes to support 4K > blocks in btrfs should handle the generic kernel code that assumes block size > == page size, so it should be doable to fix the other file systems to support > this too. Having discussed this further with Chris Mason who is working on the btrfs support. It seems that rather than mandating 4K Merkle tree, it really is the job of the kernel to support whatever Merkle tree block size it is being presented, not the job of RPM to cater for it. So the other way of looking at it is to carry the Merkle tree block size in a tag, and expect the kernel to support that. It won't work everywhere right now, but that is where it should go. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-637700416___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
> Ok, good. For now I think we need to concentrate on the fundamental problem > of architecture dependency. While most architectures today use 4K pages, > being common doesn't make it arch independent, and then there even are > architectures where this is configurable (eg aarch64). A noarch package > cannot have content that is only valid on some architectures. > > How are we supposed to deal with this? I have been thinking a fair bit about this and I see a couple of options: 1) We could in principle generate signatures for every supported page size. This would require adding more tags, ie. one for each page size. 2) Do not install signatures if the page size doesn't match the expected page size of the signature. 3) Work with the kernel to support 4K Merkle tree block size independent of the page size. Right now fsverity is only supported on ext4 and f2fs, both of these currently only work with block size == PAGE__SIZE, which is suboptimal. I raised this issue on the linux-fscrypt list already. We are actively working on adding fsverity support to btrfs, and the design here is to support 4K Merkle tree blocks independently of the page size. I think 2) and 3) are the most reasonable approach. The changes to support 4K blocks in btrfs should handle the generic kernel code that assumes block size == page size, so it should be doable to fix the other file systems to support this too. Thoughts ? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-637633413___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
Ok, good. For now I think we need to concentrate on the fundamental problem of architecture dependency. While most architectures today use 4K pages, being common doesn't make it arch independent, and then there are architectures where this is configurable (eg aarch64). A noarch package cannot have content that is only valid on some architectures. How are we supposed to deal with this? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-636676219___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
I have pushed an updated patchset into the repo. I think it addresses everything we discussed, including getting rid of the LENGTH and BLKSZ tags, adding the --delfilesign option to rpmsign, and switches to base64 encoding. Let me know if you find anything else that needs addressing or if I messed up and forgot something. The changes to fsverity-utils went into the official tree earlier this week, so just waiting for a new release to be tagged before I can push an update into rawhide. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#issuecomment-636215427___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request. > + rpmlog(RPMLOG_DEBUG, "fsverity not supported by file system for > %s\n", + path); + break; + case EOPNOTSUPP: + rpmlog(RPMLOG_DEBUG, "fsverity not enabled on file system for %s\n", + path); + break; + case ETXTBSY: + rpmlog(RPMLOG_DEBUG, "file is open by other process %s\n", + path); + break; + default: + rpmlog(RPMLOG_DEBUG, "failed to enable verity (errno %i) for %s\n", + errno, path); + break; + } I went through this and changed it to return RPMRC_FAIL for all cases, except for the error cases where fsverity is not enabled or supported by the file system. I think it's fair to fail hard and ugly for the cases where the signature is malformed etc. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#discussion_r432761349___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request.
> @@ -430,6 +438,10 @@ typedef enum rpmSigTag_e {
RPMSIGTAG_SHA256 = RPMTAG_SHA256HEADER,
RPMSIGTAG_FILESIGNATURES = RPMTAG_SIG_BASE + 18,
RPMSIGTAG_FILESIGNATURELENGTH = RPMTAG_SIG_BASE + 19,
+RPMSIGTAG_VERITYSIGNATURES = RPMTAG_SIG_BASE + 20,
+RPMSIGTAG_VERITYSIGNATURELENGTH= RPMTAG_SIG_BASE + 21,
+RPMSIGTAG_VERITYSIGNATUREALGO = RPMTAG_SIG_BASE + 22,
+RPMSIGTAG_VERITYSIGNATUREBLKSZ = RPMTAG_SIG_BASE + 23,
OK I see your point. It isn't unreasonable to count on page size for this and
mandate that it is up to the file system to cope if the file system block size
and page size don't match. I will take out this tag.
I also have the base64 encoding/decoding working now, which allowed me to get
rid of the signature length tag.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r432724168___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request. > + rpmlog(RPMLOG_DEBUG, "fsverity not supported by file system for > %s\n", + path); + break; + case EOPNOTSUPP: + rpmlog(RPMLOG_DEBUG, "fsverity not enabled on file system for %s\n", + path); + break; + case ETXTBSY: + rpmlog(RPMLOG_DEBUG, "file is open by other process %s\n", + path); + break; + default: + rpmlog(RPMLOG_DEBUG, "failed to enable verity (errno %i) for %s\n", + errno, path); + break; + } Returning FAIL here will fail the package install (which isn't pretty, but sometimes necessary). The exact semantics of when its okay to ignore failure are case-dependent and subject to debate and often practical experiences too. I think it's unreasonable to expect getting this 100% right from the go, consider this feedback more of a guideline than specific demands: silencing and/or ignoring an error should always be an exception to the rule. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#discussion_r432348210___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> if (deleting) { /* Nuke all the signature tags. */
deleteSigs(sigh);
+ deleteFileSigs(sigh);
Yeah, having a single option for both types of file signatures should be plenty.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r432294404___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> +}
+
+rpmlog(RPMLOG_DEBUG, _("key: %s\n"), key);
+rpmlog(RPMLOG_DEBUG, _("cert: %s\n"), cert);
+
+compr = headerGetString(h, RPMTAG_PAYLOADCOMPRESSOR);
+rpmio_flags = rstrscat(NULL, "r.", compr ? compr : "gzip", NULL);
+
+gzdi = Fdopen(fdDup(Fileno(fd)), rpmio_flags);
+free(rpmio_flags);
+if (!gzdi)
+ rpmlog(RPMLOG_DEBUG, _("Fdopen() failed\n"));
+
+files = rpmfilesNew(NULL, h, RPMTAG_BASENAMES, RPMFI_FLAGS_QUERY);
+fi = rpmfiNewArchiveReader(gzdi, files,
+ RPMFI_ITER_READ_ARCHIVE_OMIT_HARDLINKS);
Ok, good to know.
Absolutely not expecting any such work to be part of this effort, just musing
about possibilities. It's useful to be at least aware of the
possibilities/consequences from the go.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r432291739___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> @@ -430,6 +438,10 @@ typedef enum rpmSigTag_e {
RPMSIGTAG_SHA256 = RPMTAG_SHA256HEADER,
RPMSIGTAG_FILESIGNATURES = RPMTAG_SIG_BASE + 18,
RPMSIGTAG_FILESIGNATURELENGTH = RPMTAG_SIG_BASE + 19,
+RPMSIGTAG_VERITYSIGNATURES = RPMTAG_SIG_BASE + 20,
+RPMSIGTAG_VERITYSIGNATURELENGTH= RPMTAG_SIG_BASE + 21,
+RPMSIGTAG_VERITYSIGNATUREALGO = RPMTAG_SIG_BASE + 22,
+RPMSIGTAG_VERITYSIGNATUREBLKSZ = RPMTAG_SIG_BASE + 23,
Allowing for options (eg at distro level) is not the issue, the real issue is
that for this to be a generally meaningful feature for rpm at all, these
signatures need to be architecture and filesystem independent. For rpm it's all
the same whether the block size is big or small as long as its consistent
across all architectures, we can't have essentially arch-specific signatures in
a noarch package. So this seems like quite a deal-breaker to me, unless I'm
misunderstanding something (which is certainly possible).
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r432288920___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request. > @@ -3,7 +3,8 @@ include $(top_srcdir)/rpm.am AM_CFLAGS = @RPMCFLAGS@ -AM_CPPFLAGS = -I$(top_builddir) -I$(top_srcdir) -I$(top_builddir)/include/ +AM_CPPFLAGS = -I$(top_builddir) -I$(top_srcdir) -I$(top_builddir)/include/ \ + -I$(includedir) It's gone -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431905464___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request. > + rpmlog(RPMLOG_DEBUG, "fsverity not supported by file system for > %s\n", + path); + break; + case EOPNOTSUPP: + rpmlog(RPMLOG_DEBUG, "fsverity not enabled on file system for %s\n", + path); + break; + case ETXTBSY: + rpmlog(RPMLOG_DEBUG, "file is open by other process %s\n", + path); + break; + default: + rpmlog(RPMLOG_DEBUG, "failed to enable verity (errno %i) for %s\n", + errno, path); + break; + } > Ignoring on unsupported filesystems is totally fine, that's exactly what we > do for SELinux (which is not supported on all filesystems either). There > might well be other specific errors where ignoring is the right thing to do, > but in general errors should be treated as such and fail the install, or at > least issue a warning that something might not be okay. OK, excuse my ignorance here. If I return RPMRC_FAIL for these, will it fail the installation of the package, or just spew warnings? I didn't want to get into a state where I had to check the file system for each file, before I enabled verity on the file. Do you prefer I return RPMRC_OK if it's not supported, but RPMRC_FAIL if it actually fails? I'll have to have another look at all the return values to make sure I have them covered correctly. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431905174___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request.
> if (deleting) { /* Nuke all the signature tags. */
deleteSigs(sigh);
+ deleteFileSigs(sigh);
> The IMA signatures originally were covered by package signature, but that
> breaks some fundamental rpm rules so it was changed in a latter release. So
> these days file signatures are entirely separate items, and can be added and
> removed without affecting others.
Sweet, I was under the assumption that they were covered, so didn't want to go
down that path. I'll have a look at adding this as a separate --delfilesigs
option. I think it's reasonable to delete all file signatures with one option,
IMA and fsverity, but I can also make it two, if you prefer.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431902614___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request.
> +}
+
+rpmlog(RPMLOG_DEBUG, _("key: %s\n"), key);
+rpmlog(RPMLOG_DEBUG, _("cert: %s\n"), cert);
+
+compr = headerGetString(h, RPMTAG_PAYLOADCOMPRESSOR);
+rpmio_flags = rstrscat(NULL, "r.", compr ? compr : "gzip", NULL);
+
+gzdi = Fdopen(fdDup(Fileno(fd)), rpmio_flags);
+free(rpmio_flags);
+if (!gzdi)
+ rpmlog(RPMLOG_DEBUG, _("Fdopen() failed\n"));
+
+files = rpmfilesNew(NULL, h, RPMTAG_BASENAMES, RPMFI_FLAGS_QUERY);
+fi = rpmfiNewArchiveReader(gzdi, files,
+ RPMFI_ITER_READ_ARCHIVE_OMIT_HARDLINKS);
> Right, silly me. I plead ignorance and amnesia on what little I know about
> the Merkle tree stuff... but now that you remind me, it makes me think
> there's quite a bit of mutual interest here.
>
> There are multiple places in rpm that would benefit from gradually verifiable
> content, starting with the file digests themselves. If rpm stored the Merkle
> hashes for the files at build time, I suppose you could then just sign those?
> And when available, rpm could use those instead of the traditional digests
> for its verify operation for quicker identification of modified content.
Yes that should be feasible, the traditional digest can be derived from the
Merkle tree. If we added support for rpm to use a Merkle tree internally that
would be a mutual benefit, since it allows one to verify a data block in linear
time without having to checksum the entire file. Note that the Merkle tree
consumes roughly 1/127 of the file size, so there is a cost with it, but it's
not crazy. It would be a fair amount of work on top of this, but maybe
something to put on the long term goal list?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431900804___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request.
> @@ -430,6 +438,10 @@ typedef enum rpmSigTag_e {
RPMSIGTAG_SHA256 = RPMTAG_SHA256HEADER,
RPMSIGTAG_FILESIGNATURES = RPMTAG_SIG_BASE + 18,
RPMSIGTAG_FILESIGNATURELENGTH = RPMTAG_SIG_BASE + 19,
+RPMSIGTAG_VERITYSIGNATURES = RPMTAG_SIG_BASE + 20,
+RPMSIGTAG_VERITYSIGNATURELENGTH= RPMTAG_SIG_BASE + 21,
+RPMSIGTAG_VERITYSIGNATUREALGO = RPMTAG_SIG_BASE + 22,
+RPMSIGTAG_VERITYSIGNATUREBLKSZ = RPMTAG_SIG_BASE + 23,
> > Block size is a little tricky, as it depends on the file system block size,
> > which depending on the file system is either fixed or page size. I was
> > planning on adding option parameters to rpmsign allowing someone to specify
> > them for the case they want to build rpms on one platform to be installed
> > on another platform.
>
> Eek. This seems terrible from rpm POV.
>
> I mean, rpm's get built on some system somewhere, and signed on another, and
> then installed on wide variety of anything that we have zero control (or
> interest) over, data in rpms simply cannot be filesystem specific. Or
> page-size specific for that matter - while arch specific packages are
> obviously arch specific, a huge part of rpms are noarch where no such
> assumptions can be made.
>
> I get that on the kernel side the filesystem block / page size is the
> smallest unit that will be read in at once, but seems to me the verity block
> size should be hardwired to the lowest common denominator (something like 512
> or 1024) and verification on page-in looks at the data at that denominator
> size.
It's not pretty at all, I totally agree. The problem is that the kernel has to
hold the Merkle tree in memory while files are opened, so the smaller the block
size, the larger the memory footprint. The default should be PAGE_SIZE for
public packages, but I would like to retain this option for those who package
and maintain their own rpms, if that's OK with you?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431898002___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request.
> @@ -430,6 +438,10 @@ typedef enum rpmSigTag_e {
RPMSIGTAG_SHA256 = RPMTAG_SHA256HEADER,
RPMSIGTAG_FILESIGNATURES = RPMTAG_SIG_BASE + 18,
RPMSIGTAG_FILESIGNATURELENGTH = RPMTAG_SIG_BASE + 19,
+RPMSIGTAG_VERITYSIGNATURES = RPMTAG_SIG_BASE + 20,
+RPMSIGTAG_VERITYSIGNATURELENGTH= RPMTAG_SIG_BASE + 21,
+RPMSIGTAG_VERITYSIGNATUREALGO = RPMTAG_SIG_BASE + 22,
+RPMSIGTAG_VERITYSIGNATUREBLKSZ = RPMTAG_SIG_BASE + 23,
> Yes. So you'd have:
>
> ```
> RPMTAG_VERITYSIGNATURES = RPMTAG_SIG_BASE+24, /* s */
> [...]
> RPMSIGTAG_VERITYSIGNATURES = RPMTAG_VERITYSIGNATURES,
> ```
>
> ...and the similarly for the other tags.
>
> > With regard to the different tags, then for the signature length, it
> > depends on the key used and the algorithm. Are you suggesting we calculate
> > the length of the signature from the length of the signature array and
> > divide it by the number of entries?
>
> I'm not sure what I'm suggesting Storage size as such is not an issue, it's
> just that I find the length tag looking superfluous. Isn't it just "strlen()
> / 2" of the hex data - and once using base64, something you'll get from
> rpmBase64Decode(). It's not a value you need upfront, is it?
Oh that's an interesting point, I'll have a look at that with the base64 code.
If I can derive the size from that, that would be a great win.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431895475___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request. > + rpmlog(RPMLOG_DEBUG, "fsverity not supported by file system for > %s\n", + path); + break; + case EOPNOTSUPP: + rpmlog(RPMLOG_DEBUG, "fsverity not enabled on file system for %s\n", + path); + break; + case ETXTBSY: + rpmlog(RPMLOG_DEBUG, "file is open by other process %s\n", + path); + break; + default: + rpmlog(RPMLOG_DEBUG, "failed to enable verity (errno %i) for %s\n", + errno, path); + break; + } Ignoring on unsupported filesystems is totally fine, that's exactly what we do for SELinux (which is not supported on all filesystems either). There might well be other specific errors where ignoring is the right thing to do, but in general errors should be treated as such and fail the install, or at least issue a warning that something might not be okay. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431687458___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> +}
+
+static char *rpmVeritySignFile(rpmfi fi, size_t *sig_size, char *key,
+ char *keypass, char *cert, uint16_t algo,
+ uint32_t block_size)
+{
+struct libfsverity_merkle_tree_params params;
+struct libfsverity_signature_params sig_params;
+struct libfsverity_digest *digest = NULL;
+rpm_loff_t file_size;
+char *digest_hex, *sig_hex = NULL;
+uint8_t *sig = NULL;
+int status;
+
+if (S_ISLNK(rpmfiFMode(fi)))
+ file_size = 0;
Ack.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431679634___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> @@ -430,6 +438,10 @@ typedef enum rpmSigTag_e {
RPMSIGTAG_SHA256 = RPMTAG_SHA256HEADER,
RPMSIGTAG_FILESIGNATURES = RPMTAG_SIG_BASE + 18,
RPMSIGTAG_FILESIGNATURELENGTH = RPMTAG_SIG_BASE + 19,
+RPMSIGTAG_VERITYSIGNATURES = RPMTAG_SIG_BASE + 20,
+RPMSIGTAG_VERITYSIGNATURELENGTH= RPMTAG_SIG_BASE + 21,
+RPMSIGTAG_VERITYSIGNATUREALGO = RPMTAG_SIG_BASE + 22,
+RPMSIGTAG_VERITYSIGNATUREBLKSZ = RPMTAG_SIG_BASE + 23,
> Block size is a little tricky, as it depends on the file system block size,
> which depending on the file system is either fixed or page size. I was
> planning on adding option parameters to rpmsign allowing someone to specify
> them for the case they want to build rpms on one platform to be installed on
> another platform.
Eek. This seems terrible from rpm POV.
I mean, rpm's get built on some system somewhere, and signed on another, and
then installed on wide variety of anything that we have zero control (or
interest) over, data in rpms simply cannot be filesystem specific. Or page-size
specific for that matter - while arch specific packages are obviously arch
specific, a huge part of rpms are noarch where no such assumptions can be made.
I get that on the kernel side the filesystem block / page size is the smallest
unit that will be read in at once, but seems to me the verity block size should
be hardwired to the lowest common denominator (something like 512 or 1024) and
verification on page-in looks at the data at that denominator size.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431678635___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> @@ -430,6 +438,10 @@ typedef enum rpmSigTag_e {
RPMSIGTAG_SHA256 = RPMTAG_SHA256HEADER,
RPMSIGTAG_FILESIGNATURES = RPMTAG_SIG_BASE + 18,
RPMSIGTAG_FILESIGNATURELENGTH = RPMTAG_SIG_BASE + 19,
+RPMSIGTAG_VERITYSIGNATURES = RPMTAG_SIG_BASE + 20,
+RPMSIGTAG_VERITYSIGNATURELENGTH= RPMTAG_SIG_BASE + 21,
+RPMSIGTAG_VERITYSIGNATUREALGO = RPMTAG_SIG_BASE + 22,
+RPMSIGTAG_VERITYSIGNATUREBLKSZ = RPMTAG_SIG_BASE + 23,
> So first question, you are suggesting I move the tags to the range of
> RPMTAG_SIG_BASE like this - just want to make sure I got it right:
RPMTAG_SHA256HEADER = RPMTAG_SIG_BASE+17, /* s */
Yes. So you'd have:
```
RPMTAG_VERITYSIGNATURES = RPMTAG_SIG_BASE+24, /* s */
[...]
RPMSIGTAG_VERITYSIGNATURES = RPMTAG_VERITYSIGNATURES,
```
...and the similarly for the other tags.
> With regard to the different tags, then for the signature length, it depends
> on the key used and the algorithm. Are you suggesting we calculate the length
> of the signature from the length of the signature array and divide it by the
> number of entries?
I'm not sure what I'm suggesting :smile: Storage size as such is not an issue,
it's just that I find the length tag looking superfluous. Isn't it just
"strlen() / 2" of the hex data - and once using base64, something you'll get
from rpmBase64Decode(). It's not a value you need upfront, is it?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431669436___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> +}
+
+rpmlog(RPMLOG_DEBUG, _("key: %s\n"), key);
+rpmlog(RPMLOG_DEBUG, _("cert: %s\n"), cert);
+
+compr = headerGetString(h, RPMTAG_PAYLOADCOMPRESSOR);
+rpmio_flags = rstrscat(NULL, "r.", compr ? compr : "gzip", NULL);
+
+gzdi = Fdopen(fdDup(Fileno(fd)), rpmio_flags);
+free(rpmio_flags);
+if (!gzdi)
+ rpmlog(RPMLOG_DEBUG, _("Fdopen() failed\n"));
+
+files = rpmfilesNew(NULL, h, RPMTAG_BASENAMES, RPMFI_FLAGS_QUERY);
+fi = rpmfiNewArchiveReader(gzdi, files,
+ RPMFI_ITER_READ_ARCHIVE_OMIT_HARDLINKS);
Right, silly me. I plead ignorance and amnesia on what little I know about the
Merkle tree stuff... but now that you remind me, it makes me think there's
quite a bit of mutual interest here.
There are multiple places in rpm that would benefit from gradually verifiable
content, starting with the file digests themselves. If rpm stored the Merkle
hashes for the files at build time, I suppose you could then just sign those?
And when available, rpm could use those instead of the traditional digests for
its verify operation for quicker identification of modified content.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431652462___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> if (deleting) { /* Nuke all the signature tags. */
deleteSigs(sigh);
+ deleteFileSigs(sigh);
The IMA signatures originally were covered by package signature, but that
breaks some fundamental rpm rules so it was changed in a latter release. So
these days file signatures are entirely separate items, and can be added and
removed without affecting others.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431638751___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request. > @@ -3,7 +3,8 @@ include $(top_srcdir)/rpm.am AM_CFLAGS = @RPMCFLAGS@ -AM_CPPFLAGS = -I$(top_builddir) -I$(top_srcdir) -I$(top_builddir)/include/ +AM_CPPFLAGS = -I$(top_builddir) -I$(top_srcdir) -I$(top_builddir)/include/ \ + -I$(includedir) Understood, the right way to do that is to pass CPPFLAGS=-I/some/path as an argument to ./configure. So yeah please drop it. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431634790___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> @@ -494,6 +505,36 @@ static rpmRC includeFileSignatures(Header *sigp, Header
> *hdrp)
#endif
}
+static rpmRC includeVeritySignatures(FD_t fd, Header *sigp, Header *hdrp)
+{
+#ifdef WITH_FSVERITY
+rpmRC rc;
+char *key = rpmExpand("%{?_file_signing_key}", NULL);
+char *keypass = rpmExpand("%{?_file_signing_key_password}", NULL);
+char *cert = rpmExpand("%{?_file_signing_cert}", NULL);
+
+if (rstreq(keypass, "")) {
+ free(keypass);
+ keypass = NULL;
+}
+
+if (key && cert) {
Oh, sorry, my bad. Weary eyes at the end of the day getting things mixed up, do
ignore.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431633575___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request.
> @@ -166,8 +184,9 @@ int main(int argc, char *argv[])
argerror(_("no arguments given"));
}
-#ifdef WITH_IMAEVM
-if (fileSigningKey && !(sargs.signflags & RPMSIGN_FLAG_IMA)) {
+#if defined(WITH_IMAEVM) || defined(WITH_FSVERITY)
+if (fileSigningKey &&
+ !(sargs.signflags & (RPMSIGN_FLAG_IMA | RPMSIGN_FLAG_FSVERITY))) {
argerror(_("--fskpath may only be specified when signing files"));
I like this and introduced a flags_sign_files() helper. It was applicable in
two places, so it made the code a lot easier to read.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431434364___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request.
> @@ -494,6 +505,36 @@ static rpmRC includeFileSignatures(Header *sigp, Header
> *hdrp)
#endif
}
+static rpmRC includeVeritySignatures(FD_t fd, Header *sigp, Header *hdrp)
+{
+#ifdef WITH_FSVERITY
+rpmRC rc;
+char *key = rpmExpand("%{?_file_signing_key}", NULL);
+char *keypass = rpmExpand("%{?_file_signing_key_password}", NULL);
+char *cert = rpmExpand("%{?_file_signing_cert}", NULL);
+
+if (rstreq(keypass, "")) {
+ free(keypass);
+ keypass = NULL;
+}
+
+if (key && cert) {
I am not sure I understand this comment. The above is keypass which is a
pointer to the key password. It is only required if the key is password
protected, so it's optional.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431427233___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request. > @@ -3,7 +3,8 @@ include $(top_srcdir)/rpm.am AM_CFLAGS = @RPMCFLAGS@ -AM_CPPFLAGS = -I$(top_builddir) -I$(top_srcdir) -I$(top_builddir)/include/ +AM_CPPFLAGS = -I$(top_builddir) -I$(top_srcdir) -I$(top_builddir)/include/ \ + -I$(includedir) I was using this to include headers from a package that wasn't installed on my system in order to build rpm against it. In particular before I had a working fsverity-utils package providing libfsverity.h installed. I can drop this if you don't like it. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431360592___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request. > + rpmlog(RPMLOG_DEBUG, "fsverity not supported by file system for > %s\n", + path); + break; + case EOPNOTSUPP: + rpmlog(RPMLOG_DEBUG, "fsverity not enabled on file system for %s\n", + path); + break; + case ETXTBSY: + rpmlog(RPMLOG_DEBUG, "file is open by other process %s\n", + path); + break; + default: + rpmlog(RPMLOG_DEBUG, "failed to enable verity (errno %i) for %s\n", + errno, path); + break; + } I did this on purpose as I didn't want the installation to fail. fsverity support is on a per filesystem level, and I deliberately ignore the failures silently on file systems that do not have the support. If someone builds all their rpms with verity signatures, but not all their systems have fsverity enabled/supported in the file system, I don't think they want to get a large number of errors when trying to install packages. There is also the situation where someone has a setup where one partition has fsverity enabled and one where it isn't. If parts of the rpm are installed on both partitions, this could lead to some tricky failure situations. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431359462___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request.
> +}
+
+static char *rpmVeritySignFile(rpmfi fi, size_t *sig_size, char *key,
+ char *keypass, char *cert, uint16_t algo,
+ uint32_t block_size)
+{
+struct libfsverity_merkle_tree_params params;
+struct libfsverity_signature_params sig_params;
+struct libfsverity_digest *digest = NULL;
+rpm_loff_t file_size;
+char *digest_hex, *sig_hex = NULL;
+uint8_t *sig = NULL;
+int status;
+
+if (S_ISLNK(rpmfiFMode(fi)))
+ file_size = 0;
I totally agree, unfortunately the kernel doesn't currently offer symlink
signature support. It is something I want to address there.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431357516___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request. > + * Copyright (C) 2020 Facebook + * + * Author: Jes Sorensen + */ + +#include "system.h" + +#include /* RPMSIGTAG & related */ +#include /* rpmlog */ +#include +#include /* rpmDigestLength */ +#include "lib/header.h"/* HEADERGET_MINMEM */ +#include "lib/header_internal.h" +#include "lib/rpmtypes.h" /* rpmRC */ +#include +#include Fixed, thanks! -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431356845___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request.
> if (deleting) { /* Nuke all the signature tags. */
deleteSigs(sigh);
+ deleteFileSigs(sigh);
>From my understanding, the package signature covers the file signatures, so we
>cannot remove them without invalidating the package signature?
If I am misunderstanding this, please let me know.
I also added deletion of the IMA signatures to my patch since I figured they
should be deleted.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431355737___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request. > @@ -71,6 +71,18 @@ void headerMergeLegacySigs(Header h, Header sigh) case RPMSIGTAG_FILESIGNATURELENGTH: td.tag = RPMTAG_FILESIGNATURELENGTH; break; + case RPMSIGTAG_VERITYSIGNATURES: + td.tag = RPMTAG_VERITYSIGNATURES; + break; + case RPMSIGTAG_VERITYSIGNATURELENGTH: + td.tag = RPMTAG_VERITYSIGNATURELENGTH; + break; + case RPMSIGTAG_VERITYSIGNATUREALGO: + td.tag = RPMTAG_VERITYSIGNATUREALGO; + break; + case RPMSIGTAG_VERITYSIGNATUREBLKSZ: + td.tag = RPMTAG_VERITYSIGNATUREBLKSZ; + break; Fixed, thanks -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431354661___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request.
> @@ -396,6 +397,16 @@ static void deleteSigs(Header sigh)
headerDel(sigh, RPMSIGTAG_PGP5);
}
+static void deleteFileSigs(Header sigh)
+{
+headerDel(sigh, RPMSIGTAG_FILESIGNATURELENGTH);
+headerDel(sigh, RPMSIGTAG_FILESIGNATURES);
+headerDel(sigh, RPMTAG_VERITYSIGNATURELENGTH);
+headerDel(sigh, RPMTAG_VERITYSIGNATURES);
+headerDel(sigh, RPMTAG_VERITYSIGNATUREALGO);
+headerDel(sigh, RPMTAG_VERITYSIGNATUREBLKSZ);
Thanks, I actually discovered this the hard way and already fixed it in my tree.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431346632___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request.
> @@ -116,8 +116,12 @@ struct rpmfiles_s {
int digestalgo;/*!< File digest algorithm */
int signaturelength; /*!< File signature length */
+int veritysiglength; /*!< Verity signature length */
+uint16_t verityalgo; /*!< Verity signature length */
+uint32_t verityblksz; /*!< Verity signature length */
Whoops, fixed, thanks!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431338448___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request.
> +}
+
+rpmlog(RPMLOG_DEBUG, _("key: %s\n"), key);
+rpmlog(RPMLOG_DEBUG, _("cert: %s\n"), cert);
+
+compr = headerGetString(h, RPMTAG_PAYLOADCOMPRESSOR);
+rpmio_flags = rstrscat(NULL, "r.", compr ? compr : "gzip", NULL);
+
+gzdi = Fdopen(fdDup(Fileno(fd)), rpmio_flags);
+free(rpmio_flags);
+if (!gzdi)
+ rpmlog(RPMLOG_DEBUG, _("Fdopen() failed\n"));
+
+files = rpmfilesNew(NULL, h, RPMTAG_BASENAMES, RPMFI_FLAGS_QUERY);
+fi = rpmfiNewArchiveReader(gzdi, files,
+ RPMFI_ITER_READ_ARCHIVE_OMIT_HARDLINKS);
I wish I could, but unfortunately I don't believe it is possible. fsverity
generates a Merkle tree (basically a tree of digests) and signs the root hash,
and we cannot derive the root sha from the file sha.
This is what I mentioned in here:
https://github.com/rpm-software-management/rpm/issues/1121#issuecomment-621421288
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431335773___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@jessorensen commented on this pull request.
> @@ -430,6 +438,10 @@ typedef enum rpmSigTag_e {
RPMSIGTAG_SHA256 = RPMTAG_SHA256HEADER,
RPMSIGTAG_FILESIGNATURES = RPMTAG_SIG_BASE + 18,
RPMSIGTAG_FILESIGNATURELENGTH = RPMTAG_SIG_BASE + 19,
+RPMSIGTAG_VERITYSIGNATURES = RPMTAG_SIG_BASE + 20,
+RPMSIGTAG_VERITYSIGNATURELENGTH= RPMTAG_SIG_BASE + 21,
+RPMSIGTAG_VERITYSIGNATUREALGO = RPMTAG_SIG_BASE + 22,
+RPMSIGTAG_VERITYSIGNATUREBLKSZ = RPMTAG_SIG_BASE + 23,
Thanks for the feedback, much appreciated!
So first question, you are suggesting I move the tags to the range of
RPMTAG_SIG_BASE like this - just want to make sure I got it right:
RPMTAG_SHA256HEADER = RPMTAG_SIG_BASE+17, /* s */
With regard to the different tags, then for the signature length, it depends on
the key used and the algorithm. Are you suggesting we calculate the length of
the signature from the length of the signature array and divide it by the
number of entries?
Block size is a little tricky, as it depends on the file system block size,
which depending on the file system is either fixed or page size. I was planning
on adding option parameters to rpmsign allowing someone to specify them for the
case they want to build rpms on one platform to be installed on another
platform.
I was operating under the assumption that as long as the tag was an integer, it
wouldn't take up too much space and not be a big deal?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r431332533___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai requested changes on this pull request. Various things to address, to a large part due to unfortunate use of file signing as the example, and hopefully significant simplification is possible, but overall I think we're on the manageable side. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#pullrequestreview-418922424___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> @@ -494,6 +505,36 @@ static rpmRC includeFileSignatures(Header *sigp, Header
> *hdrp)
#endif
}
+static rpmRC includeVeritySignatures(FD_t fd, Header *sigp, Header *hdrp)
+{
+#ifdef WITH_FSVERITY
+rpmRC rc;
+char *key = rpmExpand("%{?_file_signing_key}", NULL);
+char *keypass = rpmExpand("%{?_file_signing_key_password}", NULL);
+char *cert = rpmExpand("%{?_file_signing_cert}", NULL);
+
+if (rstreq(keypass, "")) {
+ free(keypass);
+ keypass = NULL;
+}
+
+if (key && cert) {
I'd go with just `if (key && *key && cert)` and remove the special casing of an
empty string in the above.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#pullrequestreview-418914507___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> @@ -166,8 +184,9 @@ int main(int argc, char *argv[])
argerror(_("no arguments given"));
}
-#ifdef WITH_IMAEVM
-if (fileSigningKey && !(sargs.signflags & RPMSIGN_FLAG_IMA)) {
+#if defined(WITH_IMAEVM) || defined(WITH_FSVERITY)
+if (fileSigningKey &&
+ !(sargs.signflags & (RPMSIGN_FLAG_IMA | RPMSIGN_FLAG_FSVERITY))) {
argerror(_("--fskpath may only be specified when signing files"));
The actual {} block ends on the same indentation level as the condition, which
makes it unnecessarily hard to read. Either:
- use a helper variable to shorten the condition to fit on one line
- further indent the line continuation to make it stand out
- put the opening { on a line of its own
With good naming, the helper variable approach typically yields by far the most
readable code, but it depends.
Similar indentation problems are present elsewhere in the patch(es) too, please
check out for them.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#pullrequestreview-418913170___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request. > @@ -3,7 +3,8 @@ include $(top_srcdir)/rpm.am AM_CFLAGS = @RPMCFLAGS@ -AM_CPPFLAGS = -I$(top_builddir) -I$(top_srcdir) -I$(top_builddir)/include/ +AM_CPPFLAGS = -I$(top_builddir) -I$(top_srcdir) -I$(top_builddir)/include/ \ + -I$(includedir) This isn't right, $(includedir) reflects where includes of this build of rpm is to be installed on "make install". What problem is this change trying to solve? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#pullrequestreview-418909344___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> + rpmlog(RPMLOG_DEBUG, "fsverity not supported by file system for
> %s\n",
+ path);
+ break;
+ case EOPNOTSUPP:
+ rpmlog(RPMLOG_DEBUG, "fsverity not enabled on file system for %s\n",
+ path);
+ break;
+ case ETXTBSY:
+ rpmlog(RPMLOG_DEBUG, "file is open by other process %s\n",
+ path);
+ break;
+ default:
+ rpmlog(RPMLOG_DEBUG, "failed to enable verity (errno %i) for %s\n",
+ errno, path);
+ break;
+ }
AFAICT these failures should result in RPMRC_FAIL return code as the plugin
failed to accomplish its task, accompanied by an actual error message. I'd
suggest something like this to replace the big switch:
```
if (ioctl() ... != 0) {
rpmlog(RPMLOG_ERR, _("enabling verity failed: %s %s\n", path),
strerror(errno));
rc = RPMRC_FAIL;
}
```
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#pullrequestreview-418905497___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> +}
+
+static char *rpmVeritySignFile(rpmfi fi, size_t *sig_size, char *key,
+ char *keypass, char *cert, uint16_t algo,
+ uint32_t block_size)
+{
+struct libfsverity_merkle_tree_params params;
+struct libfsverity_signature_params sig_params;
+struct libfsverity_digest *digest = NULL;
+rpm_loff_t file_size;
+char *digest_hex, *sig_hex = NULL;
+uint8_t *sig = NULL;
+int status;
+
+if (S_ISLNK(rpmfiFMode(fi)))
+ file_size = 0;
No signatures for symlinks? Symlink pointing to an unintended place can have
pretty drastic consequences...
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#pullrequestreview-418310585___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request. > + * Copyright (C) 2020 Facebook + * + * Author: Jes Sorensen + */ + +#include "system.h" + +#include /* RPMSIGTAG & related */ +#include /* rpmlog */ +#include +#include /* rpmDigestLength */ +#include "lib/header.h"/* HEADERGET_MINMEM */ +#include "lib/header_internal.h" +#include "lib/rpmtypes.h" /* rpmRC */ +#include +#include Always use "..." when including internal-only headers, like you're doing with the others. And the libfsverity.h include should be moved outside the group of rpm's own includes to have it stand out a bit. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#pullrequestreview-418308520___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> if (deleting) { /* Nuke all the signature tags. */
deleteSigs(sigh);
+ deleteFileSigs(sigh);
I think deleting file signatures needs to be a separate thing from the main
package signatures, you might want to delete one but not the other. I guess
there's actually no way to remove IMA signatures atm...
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#pullrequestreview-418306319___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> @@ -396,6 +397,16 @@ static void deleteSigs(Header sigh)
headerDel(sigh, RPMSIGTAG_PGP5);
}
+static void deleteFileSigs(Header sigh)
+{
+headerDel(sigh, RPMSIGTAG_FILESIGNATURELENGTH);
+headerDel(sigh, RPMSIGTAG_FILESIGNATURES);
+headerDel(sigh, RPMTAG_VERITYSIGNATURELENGTH);
+headerDel(sigh, RPMTAG_VERITYSIGNATURES);
+headerDel(sigh, RPMTAG_VERITYSIGNATUREALGO);
+headerDel(sigh, RPMTAG_VERITYSIGNATUREBLKSZ);
This deals with a signature header, so RPMSIGTAG_* values should be used (even
if they actually match the RPMTAG_* counterparts)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#pullrequestreview-418303191___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request. > @@ -71,6 +71,18 @@ void headerMergeLegacySigs(Header h, Header sigh) case RPMSIGTAG_FILESIGNATURELENGTH: td.tag = RPMTAG_FILESIGNATURELENGTH; break; + case RPMSIGTAG_VERITYSIGNATURES: + td.tag = RPMTAG_VERITYSIGNATURES; + break; + case RPMSIGTAG_VERITYSIGNATURELENGTH: + td.tag = RPMTAG_VERITYSIGNATURELENGTH; + break; + case RPMSIGTAG_VERITYSIGNATUREALGO: + td.tag = RPMTAG_VERITYSIGNATUREALGO; + break; + case RPMSIGTAG_VERITYSIGNATUREBLKSZ: + td.tag = RPMTAG_VERITYSIGNATUREBLKSZ; + break; RPMSIGTAG_FILESIGNATURELENGTH is the wrong example to follow here, for all new tags the signature and main header tags should be the same value so no translation is needed, ie how SHA1/SHA256/RSA/DSA are handled. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203#pullrequestreview-418299557___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> +digest_hex = pgpHexStr(digest->digest, digest->digest_size);
+rpmlog(RPMLOG_DEBUG, _("file(size %li): %s: digest(%i): %s, idx %i\n"),
+ file_size, rpmfiFN(fi), digest->digest_size, digest_hex,
+ rpmfiFX(fi));
+
+free(digest_hex);
+
+memset(_params, 0, sizeof(struct libfsverity_signature_params));
+sig_params.keyfile = key;
+sig_params.certfile = cert;
+if (libfsverity_sign_digest(digest, _params, , sig_size)) {
+ rpmlog(RPMLOG_DEBUG, _("failed to sign digest\n"));
+ goto out;
+}
+
+sig_hex = pgpHexStr(sig, *sig_size);
Here too, IMA file signatures set a bad example. Use base64 encoded strings
instead of hex, it's much more space efficient (and IMA should be changed to
that as well)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#pullrequestreview-418294777___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> @@ -116,8 +116,12 @@ struct rpmfiles_s {
int digestalgo;/*!< File digest algorithm */
int signaturelength; /*!< File signature length */
+int veritysiglength; /*!< Verity signature length */
+uint16_t verityalgo; /*!< Verity signature length */
+uint32_t verityblksz; /*!< Verity signature length */
Comments seem copy-pasted, I doubt these are all about length :)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#pullrequestreview-418292899___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> +}
+
+rpmlog(RPMLOG_DEBUG, _("key: %s\n"), key);
+rpmlog(RPMLOG_DEBUG, _("cert: %s\n"), cert);
+
+compr = headerGetString(h, RPMTAG_PAYLOADCOMPRESSOR);
+rpmio_flags = rstrscat(NULL, "r.", compr ? compr : "gzip", NULL);
+
+gzdi = Fdopen(fdDup(Fileno(fd)), rpmio_flags);
+free(rpmio_flags);
+if (!gzdi)
+ rpmlog(RPMLOG_DEBUG, _("Fdopen() failed\n"));
+
+files = rpmfilesNew(NULL, h, RPMTAG_BASENAMES, RPMFI_FLAGS_QUERY);
+fi = rpmfiNewArchiveReader(gzdi, files,
+ RPMFI_ITER_READ_ARCHIVE_OMIT_HARDLINKS);
Mmh. Reading through the entire archive unpacking things as we go is expensive
and very much out of the ordinary for signing.
Could you instead use rpm's file hash algorithm for the purpose, ie if rpm's
file digests are sha256 then use that for verity too so you don't need to
recalculate?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#pullrequestreview-418292313___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> @@ -430,6 +438,10 @@ typedef enum rpmSigTag_e {
RPMSIGTAG_SHA256 = RPMTAG_SHA256HEADER,
RPMSIGTAG_FILESIGNATURES = RPMTAG_SIG_BASE + 18,
RPMSIGTAG_FILESIGNATURELENGTH = RPMTAG_SIG_BASE + 19,
+RPMSIGTAG_VERITYSIGNATURES = RPMTAG_SIG_BASE + 20,
+RPMSIGTAG_VERITYSIGNATURELENGTH= RPMTAG_SIG_BASE + 21,
+RPMSIGTAG_VERITYSIGNATUREALGO = RPMTAG_SIG_BASE + 22,
+RPMSIGTAG_VERITYSIGNATUREBLKSZ = RPMTAG_SIG_BASE + 23,
The other thing here is: do we really need all these tags? Isn't length easily
calculable from the actual signature, and does the block size actually matter
for rpm? As in, can we not just decide that we use 4096 as the page size for
rpm's purposes and that's it?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#discussion_r430400205___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
@pmatilai commented on this pull request.
> @@ -430,6 +438,10 @@ typedef enum rpmSigTag_e {
RPMSIGTAG_SHA256 = RPMTAG_SHA256HEADER,
RPMSIGTAG_FILESIGNATURES = RPMTAG_SIG_BASE + 18,
RPMSIGTAG_FILESIGNATURELENGTH = RPMTAG_SIG_BASE + 19,
+RPMSIGTAG_VERITYSIGNATURES = RPMTAG_SIG_BASE + 20,
+RPMSIGTAG_VERITYSIGNATURELENGTH= RPMTAG_SIG_BASE + 21,
+RPMSIGTAG_VERITYSIGNATUREALGO = RPMTAG_SIG_BASE + 22,
+RPMSIGTAG_VERITYSIGNATUREBLKSZ = RPMTAG_SIG_BASE + 23,
RPMSIGTAG_FILESIGNATURES is not a good example on this, that's the *very* old
and bad way. Define the RPMSIGTAG_* values to the corresponding RPMTAG_* values
instead, eg like RPMSIGTAG_SHA256 does.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1203#pullrequestreview-418274610___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)
This patchset changes to enable fsverity support natively in RPM. It requires libfsverity to build, which I have submitted patches for to the fsverity-utils maintainer. I have done my best to not break anything with this patchset, but please let me know if I got something wrong. Further details of the design and reasoning for it can be found here: https://github.com/rpm-software-management/rpm/issues/1121#issuecomment-621421288 Thanks, Jes You can view, comment on, or merge this pull request online at: https://github.com/rpm-software-management/rpm/pull/1203 -- Commit Summary -- * sign/Makefile respect --includedir * rpmfiArchiveRead() use signed return value to handle -1 on error * rpmsign: RPMSIGN_FLAG_IMA is already set * Add basic autoconf and framework for fsverity support * rpmsign: Handle --certpath for signing certificate * Implement rpmSignVerity() * rpmsignverity: Add verity signature headers to the package * rpmsignverity: Move digest and signature generation to helper function * rpmSignVerity: Generate signatures for files not present in archive * Convert RPMSIGTAG_VERITYfoo to RPMTAG_VERITYfoo tags on package read * Process verity tags on package read * Delete IMA and fsverity file signatures upon --delsig * Generate a zero-length signature for symlinks * rpmsignverity.c: Clean up debug logging * plugins/fsverity: Install fsverity signatures * fsverity - add tags for fsverity algorithm and block size * fsverity plugin: Use tags for algorithm and block size * Add fsverity tags to rpmgeneral.at -- File Changes -- M Makefile.am (1) M configure.ac (16) M lib/package.c (12) M lib/rpmarchive.h (4) M lib/rpmfi.c (41) M lib/rpmfi.h (11) M lib/rpmfiles.h (11) M lib/rpmtag.h (12) M macros.in (4) M plugins/Makefile.am (6) A plugins/fsverity.c (168) M rpmsign.c (33) M sign/Makefile.am (8) M sign/rpmgensig.c (47) M sign/rpmsign.h (1) A sign/rpmsignverity.c (234) A sign/rpmsignverity.h (29) M tests/rpmgeneral.at (4) -- Patch Links -- https://github.com/rpm-software-management/rpm/pull/1203.patch https://github.com/rpm-software-management/rpm/pull/1203.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1203 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
