NIFI-5346 Introduces new PGP controller service and PGP processors.

[natural]

Introduces new PGP Controller Service and PGP Processors

Enables PGP encryption, decryption, digital signing and verification functionality; fixes bug NIFI-5346.

Also addresses or resolves NIFI-5335, NIFI-6708.

This PR replaces #4032.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

• Is there a JIRA ticket associated with this PR? Is it referenced
  in the commit message?

• Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

• Has your PR been rebased against the latest commit within the target branch (typically master)?

• Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

For code changes:

• Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
• Have you written or updated unit tests to verify your changes?
• Have you verified that the full build is successful on both JDK 8 and JDK 11?
• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
• If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
• If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
• If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

• Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

[alopresto]

I think I was unclear in our previous conversations. I was imagining an (internal) service which provided these high-level functions, separate from the NiFi interface SpecificControllerService and class StandardSpecificControllerService implements SpecificControllerService (see SSLContextService and StandardSSLContextService/RestrictedStandardSSLContextService or KerberosCredentialsService/KeytabCredentialsService for examples).

This would provide the following benefits:

• The PGPService would provide easy-to-consume and safe-to-use PGP operations to NiFi code.
• The PGP-specific logic would only be here, making upgrades/change-of-library/etc. work isolated and contained.
• The PGPKeyMaterialService (example name) would have a StandardPGPKeyMaterialService implementation which defined the necessary PropertyDescriptors and logic for parsing the low-level values out of the NiFi concepts of PD/AllowableValue into Java types. These parameters to the cryptographic operations I believe you have encapsulated in the various EncryptOptions, etc. classes. I would expect these to share a common marker interface like PGPOptions.
• The parsed parameters would be extracted from the controller service implementation within the respective processor implementations in #onTrigger() and passed to the PGPService instance to perform the actual cryptographic operations.

In my view, controller services and processors should both be minimal in the sense that they are wrappers/adaptors for fitting the actual behavior/logic into the NiFi framework, while the logic, especially unrelated to NiFi-specific concepts, should be encapsulated on its own to provide clear separation of concerns. This is demonstrated (incompletely and not as cleanly as I would like) in EncryptContent which delegates the actual operations to OpenPGPKeyBasedEncryptor, OpenPGPPasswordBasedEncryptor, KeyBasedEncryptor, and PasswordBasedEncryptor.

[natural]

This will require a few hours to re-organize, but I don't foresee any real difficulty. The controller class is clearly doing double-duty and the behaviors are cleanly marked and clearly separable. If history is any guide, the packaging and module-making parts will be the most difficult. Do you have any suggested module layout in mind?

[natural]

For now I've refactored these methods into classes/interfaces into a new package org.apache.nifi.security.pgp; please advise on that choice when you can.

[alopresto]

I think that package is appropriate. The pure cryptographic services can go in the nifi-security-utils module. The module already includes BouncyCastle as a dependency so you should not need to modify LICENSE or NOTICE.

[natural]

I've extracted the (internal) service you mentioned to an interface called PGPOperator and a standard implementation called StandardPGPOperator, both in package org.apache.nifi.security.pgp and module nifi-security-utils.

I picked "operator" to avoid confusion with "service" and "controller". "PGPUtil" or similar wasn't considered because we have (at least) two of those already.

NB the package placement of the Controller Service interfaces; this was intentional but I'm not certain if it's optimal.

[alopresto]

I also don't see any changes to the existing EncryptContent processor. We should clearly indicate that the PGP operations in the new processors will be faster and easier to use and configure, update the documentation for both, and explain to the users and developers why this decision was made. We don't want to break BC on a minor release, but users should be encouraged to change now so we can drop the PGP operations from the legacy processor in a major release.

[davidmargolis]

Thank you for doing all this work! I am trying to sign files to be verified by another party (e.g. by pgp). I suppose this is good for generating detached signatures as an attribute. What about modifying the flowfile to be the signed version (non-detached) (i.e. gpg --output doc.sig --sign doc)? Are there plans for a SignContentPGP processor? If not, how could you use this processor to later combine the signature with another file.

[natural]

There are no plans for a SignContentPGPProcessor of which I'm aware, but it sounds like a good addition; could you open a jira ticket for the feature if/when this PR is merged?

[davidmargolis]

I created my ticket NIFI-7322. Do you mind taking a look at it and fixing it if you see any issues, as this is my first time submitting a ticket for this project.

[davidmargolis]

Should there be options for clear text signing (similar to gpg --clearsign)?

[natural]

Perhaps yes, and I think that's the case when the sign+verify processors allow for additional types of encoding. As it stands, the algorithm selection is separate from the (fixed, hex) encoding.

[davidmargolis]

Okay I see thank you

[sasqnz]

When using a ECDSA/ECDH pgp key, the public key is created with ECDSA as primary, and ECDH as a sub key. This method fails to detect the ECDH sub key as it does not contain a user id; which means that the keys hash only contains the signing key, not the encryption key. This then fails at ln 742.
Steps to repeat
gpg --full-gen-key --expert
Please select what kind of key you want:
(1) RSA and RSA (default)
(2) DSA and Elgamal
(3) DSA (sign only)
(4) RSA (sign only)
(7) DSA (set your own capabilities)
(8) RSA (set your own capabilities)
(9) ECC and ECC
(10) ECC (sign only)
(11) ECC (set your own capabilities)
(13) Existing key
(14) Existing key from card
Your selection? 9
Please select which elliptic curve you want:
(1) Curve 25519
(3) NIST P-256
(4) NIST P-384
(5) NIST P-521
(9) secp256k1
Your selection? 4
Please specify how long the key should be valid.
0 = key does not expire
= key expires in n days
w = key expires in n weeks
m = key expires in n months
y = key expires in n years
Key is valid for? (0) 0
Key does not expire at all
Is this correct? (y/N) y

gpg --export --armor --output public-key.asc

[sasqnz]

As with readPublicKeys this is not compatible with ECDSA/ECDH as the secret key for decryption is a sub key of the parent.
The following very hacky solution will detect the correct secret key, but a better solution would be to change the keys hash to also store the type of key along with the user id so that the process is less fragile

        	if (!secretKey.isMasterKey()) {
        		// search for user id from the parent of this sub key
        		try {
					for (Iterator<PGPSignature> it = secretKey.getPublicKey().getSignatures(); it.hasNext(); ) {
						PGPSignature signature = it.next();
						long keyId = signature.getKeyID();
						PGPSecretKey parentKey = rings.getSecretKey(keyId);
						for (final Iterator<String> it2 = parentKey.getUserIDs(); it2.hasNext(); ) {
					        keys.put(it2.next(), secretKey);
					    }
					}
				} catch (PGPException e) {
					// TODO Auto-generated catch block
					e.printStackTrace();
				}
        	}

[alopresto]

Thanks for the additional comments @sasqnz . I believe the original author is not continuing with this effort, so I will be making a new PR for this ticket and will be sure to test the cases you raised.

[github-actions]

We're marking this PR as stale due to lack of updates in the past few months. If after another couple of weeks the stale label has not been removed this PR will be closed. This stale marker and eventual auto close does not indicate a judgement of the PR just lack of reviewer bandwidth and helps us keep the PR queue more manageable. If you would like this PR re-opened you can do so and a committer can remove the stale tag. Or you can open a new PR. Try to help review other PRs to increase PR review bandwidth which in turn helps yours.