Re: [Rpm-maint] [rpm-software-management/rpm] RPM fsverity support (#1203)

2020-09-04 Thread jessorensen
> 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)

2020-09-04 Thread Panu Matilainen
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)

2020-09-04 Thread Panu Matilainen
@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)

2020-09-04 Thread Panu Matilainen
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)

2020-08-28 Thread jessorensen
@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)

2020-08-04 Thread Panu Matilainen
@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)

2020-07-27 Thread jessorensen
> > 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)

2020-07-06 Thread jessorensen
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)

2020-06-17 Thread jessorensen
> > 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)

2020-06-15 Thread Panu Matilainen
> 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)

2020-06-12 Thread jessorensen
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)

2020-06-12 Thread jessorensen
> 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)

2020-06-12 Thread Panu Matilainen
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)

2020-06-10 Thread jessorensen
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)

2020-06-03 Thread jessorensen
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)

2020-06-03 Thread jessorensen
> 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)

2020-06-03 Thread Panu Matilainen
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)

2020-06-02 Thread jessorensen
> 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)

2020-06-02 Thread jessorensen
> 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)

2020-06-01 Thread Panu Matilainen
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)

2020-05-29 Thread jessorensen
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)

2020-05-29 Thread jessorensen
@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)

2020-05-29 Thread jessorensen
@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)

2020-05-29 Thread Panu Matilainen
@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)

2020-05-29 Thread Panu Matilainen
@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)

2020-05-29 Thread Panu Matilainen
@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)

2020-05-29 Thread Panu Matilainen
@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)

2020-05-28 Thread jessorensen
@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)

2020-05-28 Thread jessorensen
@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)

2020-05-28 Thread jessorensen
@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)

2020-05-28 Thread jessorensen
@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)

2020-05-28 Thread jessorensen
@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)

2020-05-28 Thread jessorensen
@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)

2020-05-28 Thread Panu Matilainen
@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)

2020-05-28 Thread Panu Matilainen
@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)

2020-05-28 Thread Panu Matilainen
@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)

2020-05-28 Thread Panu Matilainen
@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)

2020-05-28 Thread Panu Matilainen
@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)

2020-05-28 Thread Panu Matilainen
@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)

2020-05-28 Thread Panu Matilainen
@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)

2020-05-28 Thread Panu Matilainen
@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)

2020-05-27 Thread jessorensen
@jessorensen 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);

Good point, I used IMA file signatures as a reference since it's the closest in 
behavior to what I need. Switching to base64 makes a ton of sense though, so 
I'll update the code to use that instead, and push an update when I have that 
working.

-- 
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_r431450635___
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)

2020-05-27 Thread jessorensen
@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)

2020-05-27 Thread jessorensen
@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)

2020-05-27 Thread jessorensen
@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)

2020-05-27 Thread jessorensen
@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)

2020-05-27 Thread jessorensen
@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)

2020-05-27 Thread jessorensen
@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)

2020-05-27 Thread jessorensen
@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)

2020-05-27 Thread jessorensen
@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)

2020-05-27 Thread jessorensen
@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)

2020-05-27 Thread jessorensen
@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)

2020-05-27 Thread jessorensen
@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)

2020-05-27 Thread jessorensen
@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)

2020-05-27 Thread Panu Matilainen
@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)

2020-05-27 Thread Panu Matilainen
@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)

2020-05-27 Thread Panu Matilainen
@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)

2020-05-27 Thread Panu Matilainen
@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)

2020-05-27 Thread Panu Matilainen
@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)

2020-05-26 Thread Panu Matilainen
@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)

2020-05-26 Thread Panu Matilainen
@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)

2020-05-26 Thread Panu Matilainen
@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)

2020-05-26 Thread Panu Matilainen
@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)

2020-05-26 Thread Panu Matilainen
@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)

2020-05-26 Thread Panu Matilainen
@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)

2020-05-26 Thread Panu Matilainen
@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)

2020-05-26 Thread Panu Matilainen
@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)

2020-05-26 Thread Panu Matilainen
@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)

2020-05-26 Thread Panu Matilainen
@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)

2020-04-30 Thread jessorensen
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