Feature to sign existing rpm file

[mat1e]

Link to rpm-builder#75

This allows to sign an existing rpm file by using RpmFileSignatureProcessor.

Example :

RpmFileSignatureProcessor signatureProcessor = new RpmFileSignatureProcessor();
ByteArrayOutputStream signedPackage = signatureProcessor.perform(your_rpm, the_private_key, pass_phrase);

A complete example is available in units tests -> RpmFileSignatureProcessorTest

The solution has been verified on linux with "rpm --checksig" command.

A next step could be to verify the signature (test initialized) but I prefers to develop it in another feature.

[ctron]

There are a few hoops to jump through, but I will help you to get this done!

First, you need to sign the ECA:

Log into the Eclipse projects forge (you will need to create an account with the Eclipse Foundation if you have not already done so); click on "Eclipse Contributor Agreement"; and Complete the form. Be sure to use the same email address when you register for the account that you intend to use on Git commit records.

I don't think the "signed off" header is required anymore, we will find that out afterwards :)

[mat1e]

I signed the ECA. What do you mean about the "signed off" header ?

[ctron]

Here is quick summary on this: https://docs.pi-hole.net/guides/github/how-to-signoff/ … but, we don't need it anymore. The ECA check passed!

[ctron]

It looks good in general, there are a few things I would kindly ask you to iron out.

There are a few other items that I would like to see changed, but that is me :) And I am not sure you want to be bothered with that. It's about the API and how to use this. I would try to aim for some changes that prefer static methods (when an instance is not required) and which use streaming of data (vs reading in the full RPM into memory).

I am happy to work with you on this, if you are interested. But it is ok for me, if you don't have the time. I might follow up on this later on.

I leave this up to you, but don't feel pressured in any way! Really!

[ctron]

It's ok if the test isn't there yet. But I think in this case, it should be removed.

[mat1e]

It is to keep the test file logic. Read rpm -> sign it -> write signed rpm -> read it -> verify signature -> remove it. If I remove this test, write the rpm after sign it became useless.

[dwalluck]

Or complete the test before accepting the PR...

[mat1e]

I agree for static method. But I hesitate to do a class with an InputStream instance of the file, or static method which can be good too.
I'm thinking about the signature check wich will probably reuse somes parts of the signature process, like the split of the file's parts and makes the digests to compare it.
I think the best way is to make static methods but somes will be split for the signature check.

[mat1e]

I would like to do use streaming of data like you describe but I don't know how to do it. That was what I want to do at the first time but I didn't find a way to stream the files many times and split it (or maybe I have to pass a File object directly to the method?).

[ctron]

I would like to do use streaming of data like you describe but I don't know how to do it. That was what I want to do at the first time but I didn't find a way to stream the files many times and split it (or maybe I have to pass a File object directly to the method?).

Yea, I am not sure either, but let's give it a try and try to iterate a bit on it:

• I think, instead of returning a ByteArrayOutputStream, you could take in an OutputStream, and just write to that. If the user decides to hand you in a ByteArrayOutputStream, that's ok then. But if the user passes in a file based stream, then we could directly write to that. Saving us one copy of the RPM in memory.
• I think that bufBytes could be replaced with ByteBuffer#wrap.
• Instead of clear, flip might be the right call in the ByteBuffers. And calling it once, outside the for loop, should be enough, as only the result of slice is passed in: https://github.com/eclipse/packager/pull/46/files#diff-d58432badc1a45e120b6484d2ff50a0475346a20bf1fa218154fa22de98fb6f4R120-R121
• I guess the getDefaultsSignatureProcessors method could include all processors, as it is already part of this class: https://github.com/eclipse/packager/pull/46/files#diff-d58432badc1a45e120b6484d2ff50a0475346a20bf1fa218154fa22de98fb6f4R117-R118

For the other part, it indeed looks like one needs to iterate over the data twice: 1) for generating the digests, 2) for writing the content (after calculating the digest).

I guess there are two alternatives to that:

1. We could pass in a File/Path instead, and open/read it twice.
2. We could find a way to calculate the final size of the signature header, skip it at first, and later on write to that specific location. That could be done by working with the FileChannel of a FileOutputStream, or but using a RandomAccessFile instead of a stream.

However, I think that's not worth the effort, unless someone asks for it. But I think there should be a note on the API which tells people that the RPM content will be read into memory, for bigger RPMs that might have an impact.

[mat1e]

@ctron, @dwalluck I made a rework taking care of your reviews. I'm not sure of all and I need your opinion...
I'm not seeing any difference with memory usage but maybe I need to try with a bigger file and a smaller JVM.

[ctron]

I think the memory usage will show itself for RPMs which are significantly above the size of the Java heap that the test application normally has. Say a 1 GiB RPM should show a difference.

Unfortunately the changes brought in a few regressions. Using FileChannel#read and #transferTo require to check the result integer. As not all bytes had might have been written. Also see:

packager/rpm/src/main/java/org/eclipse/packager/rpm/build/RpmWriter.java

Lines 265 to 301 in d002b44

	private static long copyFileChannel(final FileChannel fileChannel, final FileChannel file) throws IOException {
	long remaining = fileChannel.size();
	long position = 0;
	while (remaining > 0) {
	// transfer next chunk
	final long rc = fileChannel.transferTo(position, remaining, file);
	// check for negative result
	if (rc < 0) {
	throw new IOException(String.format("Failed to transfer bytes: rc = %s", rc));
	}
	debug("transferTo - position: %s, size: %s => rc: %s", position, remaining, rc);
	// we should never get zero back, but check anyway
	if (rc == 0) {
	break;
	}
	// update state
	position += rc;
	remaining -= rc;
	}
	// final check if we got it all
	if (remaining > 0) {
	throw new IOException("Failed to transfer full content");
	}
	return position;
	}

I am not sure this complexity is worth the effort. I guess this depends on how many RPMs you plan to sign, and if I/O performance might be an issue.

So maybe it is easier to just use something like ByteStreams#copy:

packager/rpm/src/main/java/org/eclipse/packager/rpm/build/RpmWriter.java

Line 246 in d002b44

final long count = ByteStreams.copy(payloadChannel, this.file);

[mat1e]

We don't have many rpm to sign but it can be more than 1.5GB so I prefer to optimize the memory usage.

[mat1e]

I added a method to check if it read/write the good number of bytes. I hope it is enough, actually the test pass and I have the same result than before.

[ctron]

I added a method to check if it read/write the good number of bytes. I hope it is enough, actually the test pass and I have the same result than before.

Yes, that is something that is not really reproducible. It can happen, it can happen only on some platforms. We had an issue like that way back, which only manifested itself on Windows. Still, ignoring the result is a violation of the API, nothing to blame windows for (in this case 😁).

Unfortunately, just failing isn't the right approach IMHO. That is ok if the return code is -1, but in the case of a lower number than expected, it is necessary retry. That is why all the safe* functions exists. I think it makes sense to re-use one of those. Otherwise, the application could just sometimes fail, for no apparent reason, and if you should hit some kind of repeatable scenario, it would just fail, without a way to the user to solve the issue.

Here is what Guava does for example: https://github.com/google/guava/blob/8506c5312bc78552c21612a614aeeb11c3d24f8b/guava/src/com/google/common/io/ByteStreams.java#L139-L150

However, I don't see much value in replicating that. Maybe it is easier to not use FileChannel, and just read using the stream, with the methods (or use a FileChannel, and use Guava's methods):

• https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/io/InputStream.html#readNBytes(byte[],int,int)
• https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/io/InputStream.html#readAllBytes()

That should to the same, but not require you to manually count bytes, or use some of the other "safe" read functions. It requires Java 11, but I think we already require that. And if not, I don't see a reason why we can't change to Java 11.

For the missing, second, unit test: I think an easy solution to this could be to just assume that the rpm tool exists on the path, and run it, checking the result. This will work in GitHub, and if we need to tweak Jenkins, then we can just do this. We do the same for the Maven RPM plugin. We can still replace that later on if there is much benefit from it.

[mat1e]

I checked InputStream.#readNBytes(byte[],int,int) and InputStream#readAllBytes() but it put all bytes into memory... I made a rework using commons-compress methods, I think it is enough for memory optimization.

[ctron]

I checked InputStream.#readNBytes(byte[],int,int) and InputStream#readAllBytes() but it put all bytes into memory... I made a rework using commons-compress methods, I think it is enough for memory optimization.

Bonus points for keeping Java 8 compatibility :D … should anyone need that :)

I think all that remains is to implement the second unit test, IIRC you had a command to verify this, using rpm. I think it is ok to just re-use that.

[dwalluck]

@ctron I know this is an old PR, but I see this isn't really handled properly. I will make a PR.